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

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

[Mark Cooke]

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

Hi Alan, all,

I added an extra check to the videodev.c's video_ioctl, to check vfl is
valid before dereferencing. (See attached patch).

The sequence to trigger the oops appears to be:

1. Open philips usb webcam device (any usb video device with ioctls ?).
2. Wait for (fairly rare) USB EMI issue to be flagged by hub.c.
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.

I added a check to video_icotl to ensure video_device[] isn't NULL
before trying to call the ioctl method, and it returns EINVAL in that
case.

I'm not very familiar with the locking of usb/usb-uhci in this EMI case,
but both host drivers oops in the same way.  It would seem the open file
handle needs to be disassociated too somehow - or at least 'defered'
until the video camera is re-enabled after the EMI event.

Best regards,

Mark

PS. I've recently changed the physical layout of the machine, and it
seems the new cabling layout causes these occasional EMI issues.

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

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

--- linux-2.4.21/drivers/media/video/videodev.c.orig	2003-06-13 15:51:34.000000000 +0100
+++ linux-2.4.21/drivers/media/video/videodev.c	2003-07-10 23:00:49.000000000 +0100
@@ -209,7 +209,12 @@
 	unsigned int cmd, unsigned long arg)
 {
 	struct video_device *vfl = video_devdata(file);
-	int err=vfl->ioctl(vfl, cmd, (void *)arg);
+	int err;
+
+	if (!vfl)
+		return -EINVAL;
+	
+	err = vfl->ioctl(vfl, cmd, (void *)arg);
 
 	if(err!=-ENOIOCTLCMD)
 		return err;

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

[Greg KH]

On Sat, Jul 12, 2003 at 04:42:23PM +0100, Mark Cooke wrote:
> 
> PS. I've recently changed the physical layout of the machine, and it
> seems the new cabling layout causes these occasional EMI issues.

Then change it back!
EMI issues are electrical issues.  The kernel can't really do anything
about them.

But it can not oops, that's not very nice...

thanks,

greg k-h

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

[Alan Cox]

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

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

[Mark Cooke]

[-- 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 =
 {