Re: 2.4.22pre3 / pwc / emi disconnect == oops, workaround

[Mark Cooke <mpc@star.sr.bham.ac.uk>]

[-- Attachment #1: Type: text/plain, Size: 1052 bytes --]

How about something like the attached patch ? (based on the se401
driver)

Caution: I'm not in front of the test machine currently, so it's only
had a compile test so far (and the EMI error only happens about once a
day)

Also, while I was grepping around, it seems ov511 suffers from the same
unregister in the disconnect callback.  Someone with an ov511 camera
might want to check into that.

Cheers,

Mark

On Sun, 2003-07-13 at 09:34, Alan Cox wrote:
> On Sad, 2003-07-12 at 16:42, Mark Cooke wrote:
> > 3. usb.c appears to force a disconnect immediately after #2.
> > 4. pwc module warns about a disconnect while open.
> > 5. Next call to video_ioctl with handle from #1 causes oops.
> 
> pwc driver bug. It must defer its unregister until it closes
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
-- 
Mark Cooke <mpc@star.sr.bham.ac.uk>

[-- Attachment #2: patch-2.4.22-pre3-ac1-videodev --]
[-- Type: text/plain, Size: 4038 bytes --]

--- linux-2.4.21/drivers/usb/pwc-if.c.orig	2003-03-04 22:43:01.000000000 +0000
+++ linux-2.4.21/drivers/usb/pwc-if.c	2003-07-13 10:55:10.000000000 +0100
@@ -1077,6 +1077,7 @@
 /* Note that all cleanup is done in the reverse order as in _open */
 static void pwc_video_close(struct video_device *vdev)
 {
+        /* Called with BKL held */
 	struct pwc_device *pdev;
 	int i;
 
@@ -1107,17 +1108,16 @@
 				Err("Failed to power down camera (%d)\n", i);
 		}
 	}
-
+	
 	pdev->vopen = 0;
-	if (pdev->decompressor != NULL) {
-		pdev->decompressor->exit();
-		pdev->decompressor->unlock();
-	}
-	pwc_free_buffers(pdev);
-
-	/* wake up _disconnect() routine */
-	if (pdev->unplugged)
-		wake_up(&pdev->remove_ok);
+	
+	if (pdev->unplugged) {
+	  /* Camera was unplugged during use.  Now the user is dead, we can
+	     unregister */
+	  video_unregister_device(pdev->vdev); 
+	  usb_pwc_remove_disconnected(pdev);
+	}
+	
 	Trace(TRACE_OPEN, "<< video_close()\n");
 }
 
@@ -1885,11 +1885,8 @@
 static void usb_pwc_disconnect(struct usb_device *udev, void *ptr)
 {
 	struct pwc_device *pdev;
-	int hint;
-	DECLARE_WAITQUEUE(wait, current);
 
 	lock_kernel();
-	free_mem_leak();
 
 	pdev = (struct pwc_device *)ptr;
 	if (pdev == NULL) {
@@ -1910,53 +1907,41 @@
 		return;
 	}
 #endif	
-	
-	pdev->unplugged = 1;
-	if (pdev->vdev != NULL) {
-		Trace(TRACE_PROBE, "Unregistering video device.\n");
-		video_unregister_device(pdev->vdev); 
-		if (pdev->vopen) {
-			Info("Disconnected while device/video is open!\n");
-			
-			/* Wake up any processes that might be waiting for
-			   a frame, let them return an error condition
-			 */
-			wake_up(&pdev->frameq);
-			
-			/* Wait until we get a 'go' from _close(). This used
-			   to have a gigantic race condition, since we kfree()
-			   stuff here, but we have to wait until close() 
-			   is finished. 
-			 */
-			   
-			Trace(TRACE_PROBE, "Sleeping on remove_ok.\n");
-			add_wait_queue(&pdev->remove_ok, &wait);
-			set_current_state(TASK_UNINTERRUPTIBLE);
-			/* ... wait ... */
-			schedule();
-			remove_wait_queue(&pdev->remove_ok, &wait);
-			set_current_state(TASK_RUNNING);
-			Trace(TRACE_PROBE, "Done sleeping.\n");
-			set_mem_leak(pdev->vdev);
-			pdev->vdev = NULL;
-		}
-		else {
-			/* Normal disconnect; remove from available devices */
-			kfree(pdev->vdev);
-			pdev->vdev = NULL;
-		}
-	}
 
-	/* search device_hint[] table if we occupy a slot, by any chance */
-	for (hint = 0; hint < MAX_DEV_HINTS; hint++)
-		if (device_hint[hint].pdev == pdev)
-			device_hint[hint].pdev = NULL;
-
-	pdev->udev = NULL;
+	if (!pdev->user) {
+	  Trace(TRACE_PROBE, "Unregistering video device.\n");
+	  video_unregister_device(pdev->vdev); 
+	  usb_pwc_remove_disconnected(pdev);
+	} else {
+	  Trace(TRACE_PROBE, "Disconnect while open - defer until close.\n");
+	  pdev->unplugged = 1;
+	}
+	
 	unlock_kernel();
-	kfree(pdev);
 }
 
+static inline void usb_pwc_remove_disconnected(struct pwc_device *pdev)
+{
+  int hint;
+  
+  /* search device_hint[] table if we occupy a slot, by any chance */
+  for (hint = 0; hint < MAX_DEV_HINTS; hint++)
+    if (device_hint[hint].pdev == pdev)
+      device_hint[hint].pdev = NULL;
+  
+  if (pdev->decompressor != NULL) {
+    pdev->decompressor->exit();
+    pdev->decompressor->unlock();
+  }
+  pwc_free_buffers(pdev);
+  
+  /* Normal disconnect; remove from available devices */
+  free_mem_leak();
+  kfree(pdev->vdev);
+  pdev->vdev = NULL;
+  pdev->udev = NULL;
+  kfree(pdev);
+}
 
 /* *grunt* We have to do atoi ourselves :-( */
 static int pwc_atoi(const char *s)
--- linux-2.4.21/drivers/usb/pwc-if.c.orig	2003-07-13 10:58:24.000000000 +0100
+++ linux-2.4.21/drivers/usb/pwc-if.c	2003-07-13 11:01:49.000000000 +0100
@@ -94,6 +94,7 @@
 
 static void *usb_pwc_probe(struct usb_device *udev, unsigned int ifnum, const struct usb_device_id *id);
 static void usb_pwc_disconnect(struct usb_device *udev, void *ptr);
+static inline void usb_pwc_remove_disconnected(struct pwc_device *pdev);
 
 static struct usb_driver pwc_driver =
 {