Oops in pwc v4l driver

Oops in pwc v4l driver

[Alex Smith]

Hi,

I found an old Philips Askey VC010 webcam and attempted to get it
working on Linux (latest git, x86_64). It worked fine, I could view
the camera in mplayer. But, however, when I went to move the camera, I
unplugged it and forgot to close mplayer. I didn't notice, moved it,
plugged it back in and immediately Mplayer died and dropped a genaral
protection fault oops + stacktrace in the kernel log. I unplugged the
camera again, and the entire system froze, most likely a kernel panic.
I've reproduced this twice, however I haven't been able to get it to
occur when using mplayer from the command line (so that I can see what
happens when it freezes). I've uploaded the 2 dmesg logs here:

http://www.alex-smith.me.uk/files/dmesg.log
http://www.alex-smith.me.uk/files/dmesg2.log

and also my .config here:

http://www.alex-smith.me.uk/files/config-2.6.23-rc5.txt

The mplayer command I used:

mplayer -cache 128 -tv
driver=v4l:width=640:height=480:outfmt=i420:device=/dev/video1 -vc
rawi420 -vo xv tv://

Thanks,
Alex
-- 
Alex Smith - http://www.alex-smith.me.uk

Re: Oops in pwc v4l driver

[Oliver Neukum]

Am Sonntag 02 September 2007 schrieb Alex Smith:
> Hi,
> 
> I found an old Philips Askey VC010 webcam and attempted to get it
> working on Linux (latest git, x86_64). It worked fine, I could view
> the camera in mplayer. But, however, when I went to move the camera, I
> unplugged it and forgot to close mplayer. I didn't notice, moved it,

Yes,

this is a known issue. This patch should fix it. Does it work for you?

	Regards
		Oliver
---

--- a/drivers/media/video/pwc/pwc-if.c	2007-08-24 10:16:38.000000000 +0200
+++ b/drivers/media/video/pwc/pwc-if.c	2007-08-24 11:01:26.000000000 +0200
@@ -908,31 +908,49 @@ int pwc_isoc_init(struct pwc_device *pde
 	return 0;
 }
 
-void pwc_isoc_cleanup(struct pwc_device *pdev)
+static void pwc_iso_stop(struct pwc_device *pdev)
 {
 	int i;
 
-	PWC_DEBUG_OPEN(">> pwc_isoc_cleanup()\n");
-	if (pdev == NULL)
-		return;
-	if (pdev->iso_init == 0)
-		return;
-
 	/* Unlinking ISOC buffers one by one */
 	for (i = 0; i < MAX_ISO_BUFS; i++) {
 		struct urb *urb;
 
 		urb = pdev->sbuf[i].urb;
 		if (urb != 0) {
-			if (pdev->iso_init) {
-				PWC_DEBUG_MEMORY("Unlinking URB %p\n", urb);
-				usb_kill_urb(urb);
-			}
+			PWC_DEBUG_MEMORY("Unlinking URB %p\n", urb);
+			usb_kill_urb(urb);
+		}
+	}
+}
+
+static void pwc_iso_free(struct pwc_device *pdev)
+{
+	int i;
+
+	/* Freeing ISOC buffers one by one */
+	for (i = 0; i < MAX_ISO_BUFS; i++) {
+		struct urb *urb;
+
+		urb = pdev->sbuf[i].urb;
+		if (urb != 0) {
 			PWC_DEBUG_MEMORY("Freeing URB\n");
 			usb_free_urb(urb);
 			pdev->sbuf[i].urb = NULL;
 		}
 	}
+}
+
+void pwc_isoc_cleanup(struct pwc_device *pdev)
+{
+	PWC_DEBUG_OPEN(">> pwc_isoc_cleanup()\n");
+	if (pdev == NULL)
+		return;
+	if (pdev->iso_init == 0)
+		return;
+
+	pwc_iso_stop(pdev);
+	pwc_iso_free(pdev);
 
 	/* Stop camera, but only if we are sure the camera is still there (unplug
 	   is signalled by EPIPE)
@@ -1212,6 +1230,7 @@ static int pwc_video_close(struct inode 
 
 	PWC_DEBUG_OPEN(">> video_close called(vdev = 0x%p).\n", vdev);
 
+	lock_kernel();
 	pdev = (struct pwc_device *)vdev->priv;
 	if (pdev->vopen == 0)
 		PWC_DEBUG_MODULE("video_close() called on closed device?\n");
@@ -1231,7 +1250,6 @@ static int pwc_video_close(struct inode 
 	pwc_isoc_cleanup(pdev);
 	pwc_free_buffers(pdev);
 
-	lock_kernel();
 	/* Turn off LEDS and power down camera, but only when not unplugged */
 	if (!pdev->unplugged) {
 		/* Turn LEDs off */
@@ -1277,7 +1295,7 @@ static ssize_t pwc_video_read(struct fil
 	struct pwc_device *pdev;
 	int noblock = file->f_flags & O_NONBLOCK;
 	DECLARE_WAITQUEUE(wait, current);
-	int bytes_to_read;
+	int bytes_to_read, rv = 0;
 	void *image_buffer_addr;
 
 	PWC_DEBUG_READ("pwc_video_read(vdev=0x%p, buf=%p, count=%zd) called.\n",
@@ -1287,8 +1305,12 @@ static ssize_t pwc_video_read(struct fil
 	pdev = vdev->priv;
 	if (pdev == NULL)
 		return -EFAULT;
-	if (pdev->error_status)
-		return -pdev->error_status; /* Something happened, report what. */
+
+	mutex_lock(&pdev->modlock);
+	if (pdev->error_status) {
+		rv = -pdev->error_status; /* Something happened, report what. */
+		goto err_out;
+	}
 
 	/* In case we're doing partial reads, we don't have to wait for a frame */
 	if (pdev->image_read_pos == 0) {
@@ -1299,17 +1321,20 @@ static ssize_t pwc_video_read(struct fil
 			if (pdev->error_status) {
 				remove_wait_queue(&pdev->frameq, &wait);
 				set_current_state(TASK_RUNNING);
-				return -pdev->error_status ;
+				rv = -pdev->error_status ;
+				goto err_out;
 			}
 			if (noblock) {
 				remove_wait_queue(&pdev->frameq, &wait);
 				set_current_state(TASK_RUNNING);
-				return -EWOULDBLOCK;
+				rv = -EWOULDBLOCK;
+				goto err_out;
 			}
 			if (signal_pending(current)) {
 				remove_wait_queue(&pdev->frameq, &wait);
 				set_current_state(TASK_RUNNING);
-				return -ERESTARTSYS;
+				rv = -ERESTARTSYS;
+				goto err_out;
 			}
 			schedule();
 			set_current_state(TASK_INTERRUPTIBLE);
@@ -1318,8 +1343,10 @@ static ssize_t pwc_video_read(struct fil
 		set_current_state(TASK_RUNNING);
 
 		/* Decompress and release frame */
-		if (pwc_handle_frame(pdev))
-			return -EFAULT;
+		if (pwc_handle_frame(pdev)) {
+			rv = -EFAULT;
+			goto err_out;
+		}
 	}
 
 	PWC_DEBUG_READ("Copying data to user space.\n");
@@ -1334,14 +1361,20 @@ static ssize_t pwc_video_read(struct fil
 	image_buffer_addr = pdev->image_data;
 	image_buffer_addr += pdev->images[pdev->fill_image].offset;
 	image_buffer_addr += pdev->image_read_pos;
-	if (copy_to_user(buf, image_buffer_addr, count))
-		return -EFAULT;
+	if (copy_to_user(buf, image_buffer_addr, count)) {
+		rv = -EFAULT;
+		goto err_out;
+	}
 	pdev->image_read_pos += count;
 	if (pdev->image_read_pos >= bytes_to_read) { /* All data has been read */
 		pdev->image_read_pos = 0;
 		pwc_next_image(pdev);
 	}
+	mutex_unlock(&pdev->modlock);
 	return count;
+err_out:
+	mutex_unlock(&pdev->modlock);
+	return rv;
 }
 
 static unsigned int pwc_video_poll(struct file *file, poll_table *wait)
@@ -1367,7 +1400,20 @@ static unsigned int pwc_video_poll(struc
 static int pwc_video_ioctl(struct inode *inode, struct file *file,
 			   unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, pwc_video_do_ioctl);
+	struct video_device *vdev = file->private_data;
+	struct pwc_device *pdev; 
+	int r = -ENODEV;
+
+	if (!vdev)
+		goto out;
+	pdev = vdev->priv;
+
+	mutex_lock(&pdev->modlock);
+	if (!pdev->unplugged)
+		r = video_usercopy(inode, file, cmd, arg, pwc_video_do_ioctl);
+	mutex_unlock(&pdev->modlock);
+out:
+	return r;
 }
 
 static int pwc_video_mmap(struct file *file, struct vm_area_struct *vma)
@@ -1810,7 +1856,10 @@ static void usb_pwc_disconnect(struct us
 	wake_up_interruptible(&pdev->frameq);
 	/* Wait until device is closed */
 	if(pdev->vopen) {
+		mutex_lock(&pdev->modlock);
 		pdev->unplugged = 1;
+		mutex_unlock(&pdev->modlock);
+		pwc_iso_stop(pdev);
 	} else {
 		/* Device is closed, so we can safely unregister it */
 		PWC_DEBUG_PROBE("Unregistering video device in disconnect().\n");
@@ -1828,7 +1877,6 @@ disconnect_out:
 	unlock_kernel();
 }
 
-
 /* *grunt* We have to do atoi ourselves :-( */
 static int pwc_atoi(const char *s)
 {

Re: Oops in pwc v4l driver

[Michal Piotrowski]

Hi Alex,

On 02/09/07, Alex Smith <lkml@alex-smith.me.uk> wrote:
> Hi,
>
> I found an old Philips Askey VC010 webcam and attempted to get it
> working on Linux (latest git, x86_64).

Is this a regression? Does 2.6.22 work fine?

Regards,
Michal

-- 
LOG
http://www.stardust.webpages.pl/log/

Re: Oops in pwc v4l driver

[Oliver Neukum]

Am Montag 03 September 2007 schrieb Michal Piotrowski:
> Hi Alex,
> 
> On 02/09/07, Alex Smith <lkml@alex-smith.me.uk> wrote:
> > Hi,
> >
> > I found an old Philips Askey VC010 webcam and attempted to get it
> > working on Linux (latest git, x86_64).
> 
> Is this a regression? Does 2.6.22 work fine?

2.6.22 would fail in a different way. This is a question of definition.
A patch is available in any case.

	Regards
		Oliver

Re: Oops in pwc v4l driver

[Alex Smith]

On 03/09/2007, Michal Piotrowski <michal.k.k.piotrowski@gmail.com> wrote:
> Hi Alex,
>
> On 02/09/07, Alex Smith <lkml@alex-smith.me.uk> wrote:
> > Hi,
> >
> > I found an old Philips Askey VC010 webcam and attempted to get it
> > working on Linux (latest git, x86_64).
>
> Is this a regression? Does 2.6.22 work fine?

Not sure, I haven't tested with .22. But that patch definitely fixes
the issue - I can't cause it no matter what I do.

Thanks,
Alex

(P.S. Sorry for the late reply - I don't check my lkml email that often)
-- 
Alex Smith - http://www.alex-smith.me.uk

Re: Oops in pwc v4l driver

[Mauro Carvalho Chehab]

Hi Oliver,

May you send your Signed-off-by for the patch?

Cheers,
Mauro.

Em Dom, 2007-09-02 às 15:02 +0200, Oliver Neukum escreveu:
> Am Sonntag 02 September 2007 schrieb Alex Smith:
> > Hi,
> > 
> > I found an old Philips Askey VC010 webcam and attempted to get it
> > working on Linux (latest git, x86_64). It worked fine, I could view
> > the camera in mplayer. But, however, when I went to move the camera, I
> > unplugged it and forgot to close mplayer. I didn't notice, moved it,
> 
> Yes,
> 
> this is a known issue. This patch should fix it. Does it work for you?
> 
> 	Regards
> 		Oliver
> ---
> 
> --- a/drivers/media/video/pwc/pwc-if.c	2007-08-24 10:16:38.000000000 +0200
> +++ b/drivers/media/video/pwc/pwc-if.c	2007-08-24 11:01:26.000000000 +0200
> @@ -908,31 +908,49 @@ int pwc_isoc_init(struct pwc_device *pde
>  	return 0;
>  }
>  
> -void pwc_isoc_cleanup(struct pwc_device *pdev)
> +static void pwc_iso_stop(struct pwc_device *pdev)
>  {
>  	int i;
>  
> -	PWC_DEBUG_OPEN(">> pwc_isoc_cleanup()\n");
> -	if (pdev == NULL)
> -		return;
> -	if (pdev->iso_init == 0)
> -		return;
> -
>  	/* Unlinking ISOC buffers one by one */
>  	for (i = 0; i < MAX_ISO_BUFS; i++) {
>  		struct urb *urb;
>  
>  		urb = pdev->sbuf[i].urb;
>  		if (urb != 0) {
> -			if (pdev->iso_init) {
> -				PWC_DEBUG_MEMORY("Unlinking URB %p\n", urb);
> -				usb_kill_urb(urb);
> -			}
> +			PWC_DEBUG_MEMORY("Unlinking URB %p\n", urb);
> +			usb_kill_urb(urb);
> +		}
> +	}
> +}
> +
> +static void pwc_iso_free(struct pwc_device *pdev)
> +{
> +	int i;
> +
> +	/* Freeing ISOC buffers one by one */
> +	for (i = 0; i < MAX_ISO_BUFS; i++) {
> +		struct urb *urb;
> +
> +		urb = pdev->sbuf[i].urb;
> +		if (urb != 0) {
>  			PWC_DEBUG_MEMORY("Freeing URB\n");
>  			usb_free_urb(urb);
>  			pdev->sbuf[i].urb = NULL;
>  		}
>  	}
> +}
> +
> +void pwc_isoc_cleanup(struct pwc_device *pdev)
> +{
> +	PWC_DEBUG_OPEN(">> pwc_isoc_cleanup()\n");
> +	if (pdev == NULL)
> +		return;
> +	if (pdev->iso_init == 0)
> +		return;
> +
> +	pwc_iso_stop(pdev);
> +	pwc_iso_free(pdev);
>  
>  	/* Stop camera, but only if we are sure the camera is still there (unplug
>  	   is signalled by EPIPE)
> @@ -1212,6 +1230,7 @@ static int pwc_video_close(struct inode 
>  
>  	PWC_DEBUG_OPEN(">> video_close called(vdev = 0x%p).\n", vdev);
>  
> +	lock_kernel();
>  	pdev = (struct pwc_device *)vdev->priv;
>  	if (pdev->vopen == 0)
>  		PWC_DEBUG_MODULE("video_close() called on closed device?\n");
> @@ -1231,7 +1250,6 @@ static int pwc_video_close(struct inode 
>  	pwc_isoc_cleanup(pdev);
>  	pwc_free_buffers(pdev);
>  
> -	lock_kernel();
>  	/* Turn off LEDS and power down camera, but only when not unplugged */
>  	if (!pdev->unplugged) {
>  		/* Turn LEDs off */
> @@ -1277,7 +1295,7 @@ static ssize_t pwc_video_read(struct fil
>  	struct pwc_device *pdev;
>  	int noblock = file->f_flags & O_NONBLOCK;
>  	DECLARE_WAITQUEUE(wait, current);
> -	int bytes_to_read;
> +	int bytes_to_read, rv = 0;
>  	void *image_buffer_addr;
>  
>  	PWC_DEBUG_READ("pwc_video_read(vdev=0x%p, buf=%p, count=%zd) called.\n",
> @@ -1287,8 +1305,12 @@ static ssize_t pwc_video_read(struct fil
>  	pdev = vdev->priv;
>  	if (pdev == NULL)
>  		return -EFAULT;
> -	if (pdev->error_status)
> -		return -pdev->error_status; /* Something happened, report what. */
> +
> +	mutex_lock(&pdev->modlock);
> +	if (pdev->error_status) {
> +		rv = -pdev->error_status; /* Something happened, report what. */
> +		goto err_out;
> +	}
>  
>  	/* In case we're doing partial reads, we don't have to wait for a frame */
>  	if (pdev->image_read_pos == 0) {
> @@ -1299,17 +1321,20 @@ static ssize_t pwc_video_read(struct fil
>  			if (pdev->error_status) {
>  				remove_wait_queue(&pdev->frameq, &wait);
>  				set_current_state(TASK_RUNNING);
> -				return -pdev->error_status ;
> +				rv = -pdev->error_status ;
> +				goto err_out;
>  			}
>  			if (noblock) {
>  				remove_wait_queue(&pdev->frameq, &wait);
>  				set_current_state(TASK_RUNNING);
> -				return -EWOULDBLOCK;
> +				rv = -EWOULDBLOCK;
> +				goto err_out;
>  			}
>  			if (signal_pending(current)) {
>  				remove_wait_queue(&pdev->frameq, &wait);
>  				set_current_state(TASK_RUNNING);
> -				return -ERESTARTSYS;
> +				rv = -ERESTARTSYS;
> +				goto err_out;
>  			}
>  			schedule();
>  			set_current_state(TASK_INTERRUPTIBLE);
> @@ -1318,8 +1343,10 @@ static ssize_t pwc_video_read(struct fil
>  		set_current_state(TASK_RUNNING);
>  
>  		/* Decompress and release frame */
> -		if (pwc_handle_frame(pdev))
> -			return -EFAULT;
> +		if (pwc_handle_frame(pdev)) {
> +			rv = -EFAULT;
> +			goto err_out;
> +		}
>  	}
>  
>  	PWC_DEBUG_READ("Copying data to user space.\n");
> @@ -1334,14 +1361,20 @@ static ssize_t pwc_video_read(struct fil
>  	image_buffer_addr = pdev->image_data;
>  	image_buffer_addr += pdev->images[pdev->fill_image].offset;
>  	image_buffer_addr += pdev->image_read_pos;
> -	if (copy_to_user(buf, image_buffer_addr, count))
> -		return -EFAULT;
> +	if (copy_to_user(buf, image_buffer_addr, count)) {
> +		rv = -EFAULT;
> +		goto err_out;
> +	}
>  	pdev->image_read_pos += count;
>  	if (pdev->image_read_pos >= bytes_to_read) { /* All data has been read */
>  		pdev->image_read_pos = 0;
>  		pwc_next_image(pdev);
>  	}
> +	mutex_unlock(&pdev->modlock);
>  	return count;
> +err_out:
> +	mutex_unlock(&pdev->modlock);
> +	return rv;
>  }
>  
>  static unsigned int pwc_video_poll(struct file *file, poll_table *wait)
> @@ -1367,7 +1400,20 @@ static unsigned int pwc_video_poll(struc
>  static int pwc_video_ioctl(struct inode *inode, struct file *file,
>  			   unsigned int cmd, unsigned long arg)
>  {
> -	return video_usercopy(inode, file, cmd, arg, pwc_video_do_ioctl);
> +	struct video_device *vdev = file->private_data;
> +	struct pwc_device *pdev; 
> +	int r = -ENODEV;
> +
> +	if (!vdev)
> +		goto out;
> +	pdev = vdev->priv;
> +
> +	mutex_lock(&pdev->modlock);
> +	if (!pdev->unplugged)
> +		r = video_usercopy(inode, file, cmd, arg, pwc_video_do_ioctl);
> +	mutex_unlock(&pdev->modlock);
> +out:
> +	return r;
>  }
>  
>  static int pwc_video_mmap(struct file *file, struct vm_area_struct *vma)
> @@ -1810,7 +1856,10 @@ static void usb_pwc_disconnect(struct us
>  	wake_up_interruptible(&pdev->frameq);
>  	/* Wait until device is closed */
>  	if(pdev->vopen) {
> +		mutex_lock(&pdev->modlock);
>  		pdev->unplugged = 1;
> +		mutex_unlock(&pdev->modlock);
> +		pwc_iso_stop(pdev);
>  	} else {
>  		/* Device is closed, so we can safely unregister it */
>  		PWC_DEBUG_PROBE("Unregistering video device in disconnect().\n");
> @@ -1828,7 +1877,6 @@ disconnect_out:
>  	unlock_kernel();
>  }
>  
> -
>  /* *grunt* We have to do atoi ourselves :-( */
>  static int pwc_atoi(const char *s)
>  {
> -
> 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/
-- 
Cheers,
Mauro

Re: Oops in pwc v4l driver

[Oliver Neukum]

The pwc driver is defficient in locking, which can trigger an oops
when disconnecting.

Signed-off-by: Oliver Neukum <oneukum@suse.de>

	Regards
		Oliver
---

--- a/drivers/media/video/pwc/pwc-if.c	2007-08-24 10:16:38.000000000 +0200
+++ b/drivers/media/video/pwc/pwc-if.c	2007-08-24 11:01:26.000000000 +0200
@@ -908,31 +908,49 @@ int pwc_isoc_init(struct pwc_device *pde
 	return 0;
 }
 
-void pwc_isoc_cleanup(struct pwc_device *pdev)
+static void pwc_iso_stop(struct pwc_device *pdev)
 {
 	int i;
 
-	PWC_DEBUG_OPEN(">> pwc_isoc_cleanup()\n");
-	if (pdev == NULL)
-		return;
-	if (pdev->iso_init == 0)
-		return;
-
 	/* Unlinking ISOC buffers one by one */
 	for (i = 0; i < MAX_ISO_BUFS; i++) {
 		struct urb *urb;
 
 		urb = pdev->sbuf[i].urb;
 		if (urb != 0) {
-			if (pdev->iso_init) {
-				PWC_DEBUG_MEMORY("Unlinking URB %p\n", urb);
-				usb_kill_urb(urb);
-			}
+			PWC_DEBUG_MEMORY("Unlinking URB %p\n", urb);
+			usb_kill_urb(urb);
+		}
+	}
+}
+
+static void pwc_iso_free(struct pwc_device *pdev)
+{
+	int i;
+
+	/* Freeing ISOC buffers one by one */
+	for (i = 0; i < MAX_ISO_BUFS; i++) {
+		struct urb *urb;
+
+		urb = pdev->sbuf[i].urb;
+		if (urb != 0) {
 			PWC_DEBUG_MEMORY("Freeing URB\n");
 			usb_free_urb(urb);
 			pdev->sbuf[i].urb = NULL;
 		}
 	}
+}
+
+void pwc_isoc_cleanup(struct pwc_device *pdev)
+{
+	PWC_DEBUG_OPEN(">> pwc_isoc_cleanup()\n");
+	if (pdev == NULL)
+		return;
+	if (pdev->iso_init == 0)
+		return;
+
+	pwc_iso_stop(pdev);
+	pwc_iso_free(pdev);
 
 	/* Stop camera, but only if we are sure the camera is still there (unplug
 	   is signalled by EPIPE)
@@ -1212,6 +1230,7 @@ static int pwc_video_close(struct inode 
 
 	PWC_DEBUG_OPEN(">> video_close called(vdev = 0x%p).\n", vdev);
 
+	lock_kernel();
 	pdev = (struct pwc_device *)vdev->priv;
 	if (pdev->vopen == 0)
 		PWC_DEBUG_MODULE("video_close() called on closed device?\n");
@@ -1231,7 +1250,6 @@ static int pwc_video_close(struct inode 
 	pwc_isoc_cleanup(pdev);
 	pwc_free_buffers(pdev);
 
-	lock_kernel();
 	/* Turn off LEDS and power down camera, but only when not unplugged */
 	if (!pdev->unplugged) {
 		/* Turn LEDs off */
@@ -1277,7 +1295,7 @@ static ssize_t pwc_video_read(struct fil
 	struct pwc_device *pdev;
 	int noblock = file->f_flags & O_NONBLOCK;
 	DECLARE_WAITQUEUE(wait, current);
-	int bytes_to_read;
+	int bytes_to_read, rv = 0;
 	void *image_buffer_addr;
 
 	PWC_DEBUG_READ("pwc_video_read(vdev=0x%p, buf=%p, count=%zd) called.\n",
@@ -1287,8 +1305,12 @@ static ssize_t pwc_video_read(struct fil
 	pdev = vdev->priv;
 	if (pdev == NULL)
 		return -EFAULT;
-	if (pdev->error_status)
-		return -pdev->error_status; /* Something happened, report what. */
+
+	mutex_lock(&pdev->modlock);
+	if (pdev->error_status) {
+		rv = -pdev->error_status; /* Something happened, report what. */
+		goto err_out;
+	}
 
 	/* In case we're doing partial reads, we don't have to wait for a frame */
 	if (pdev->image_read_pos == 0) {
@@ -1299,17 +1321,20 @@ static ssize_t pwc_video_read(struct fil
 			if (pdev->error_status) {
 				remove_wait_queue(&pdev->frameq, &wait);
 				set_current_state(TASK_RUNNING);
-				return -pdev->error_status ;
+				rv = -pdev->error_status ;
+				goto err_out;
 			}
 			if (noblock) {
 				remove_wait_queue(&pdev->frameq, &wait);
 				set_current_state(TASK_RUNNING);
-				return -EWOULDBLOCK;
+				rv = -EWOULDBLOCK;
+				goto err_out;
 			}
 			if (signal_pending(current)) {
 				remove_wait_queue(&pdev->frameq, &wait);
 				set_current_state(TASK_RUNNING);
-				return -ERESTARTSYS;
+				rv = -ERESTARTSYS;
+				goto err_out;
 			}
 			schedule();
 			set_current_state(TASK_INTERRUPTIBLE);
@@ -1318,8 +1343,10 @@ static ssize_t pwc_video_read(struct fil
 		set_current_state(TASK_RUNNING);
 
 		/* Decompress and release frame */
-		if (pwc_handle_frame(pdev))
-			return -EFAULT;
+		if (pwc_handle_frame(pdev)) {
+			rv = -EFAULT;
+			goto err_out;
+		}
 	}
 
 	PWC_DEBUG_READ("Copying data to user space.\n");
@@ -1334,14 +1361,20 @@ static ssize_t pwc_video_read(struct fil
 	image_buffer_addr = pdev->image_data;
 	image_buffer_addr += pdev->images[pdev->fill_image].offset;
 	image_buffer_addr += pdev->image_read_pos;
-	if (copy_to_user(buf, image_buffer_addr, count))
-		return -EFAULT;
+	if (copy_to_user(buf, image_buffer_addr, count)) {
+		rv = -EFAULT;
+		goto err_out;
+	}
 	pdev->image_read_pos += count;
 	if (pdev->image_read_pos >= bytes_to_read) { /* All data has been read */
 		pdev->image_read_pos = 0;
 		pwc_next_image(pdev);
 	}
+	mutex_unlock(&pdev->modlock);
 	return count;
+err_out:
+	mutex_unlock(&pdev->modlock);
+	return rv;
 }
 
 static unsigned int pwc_video_poll(struct file *file, poll_table *wait)
@@ -1367,7 +1400,20 @@ static unsigned int pwc_video_poll(struc
 static int pwc_video_ioctl(struct inode *inode, struct file *file,
 			   unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, pwc_video_do_ioctl);
+	struct video_device *vdev = file->private_data;
+	struct pwc_device *pdev; 
+	int r = -ENODEV;
+
+	if (!vdev)
+		goto out;
+	pdev = vdev->priv;
+
+	mutex_lock(&pdev->modlock);
+	if (!pdev->unplugged)
+		r = video_usercopy(inode, file, cmd, arg, pwc_video_do_ioctl);
+	mutex_unlock(&pdev->modlock);
+out:
+	return r;
 }
 
 static int pwc_video_mmap(struct file *file, struct vm_area_struct *vma)
@@ -1810,7 +1856,10 @@ static void usb_pwc_disconnect(struct us
 	wake_up_interruptible(&pdev->frameq);
 	/* Wait until device is closed */
 	if(pdev->vopen) {
+		mutex_lock(&pdev->modlock);
 		pdev->unplugged = 1;
+		mutex_unlock(&pdev->modlock);
+		pwc_iso_stop(pdev);
 	} else {
 		/* Device is closed, so we can safely unregister it */
 		PWC_DEBUG_PROBE("Unregistering video device in disconnect().\n");
@@ -1828,7 +1877,6 @@ disconnect_out:
 	unlock_kernel();
 }
 
-
 /* *grunt* We have to do atoi ourselves :-( */
 static int pwc_atoi(const char *s)
 {

Re: Oops in pwc v4l driver

[Oliver Neukum]

The pwc driver is defficient in locking, which can trigger an oops
when disconnecting.

Signed-off-by: Oliver Neukum <oneukum@suse.de>

	Regards
		Oliver
---

--- a/drivers/media/video/pwc/pwc-if.c	2007-08-24 10:16:38.000000000 +0200
+++ b/drivers/media/video/pwc/pwc-if.c	2007-08-24 11:01:26.000000000 +0200
@@ -908,31 +908,49 @@ int pwc_isoc_init(struct pwc_device *pde
 	return 0;
 }
 
-void pwc_isoc_cleanup(struct pwc_device *pdev)
+static void pwc_iso_stop(struct pwc_device *pdev)
 {
 	int i;
 
-	PWC_DEBUG_OPEN(">> pwc_isoc_cleanup()\n");
-	if (pdev == NULL)
-		return;
-	if (pdev->iso_init == 0)
-		return;
-
 	/* Unlinking ISOC buffers one by one */
 	for (i = 0; i < MAX_ISO_BUFS; i++) {
 		struct urb *urb;
 
 		urb = pdev->sbuf[i].urb;
 		if (urb != 0) {
-			if (pdev->iso_init) {
-				PWC_DEBUG_MEMORY("Unlinking URB %p\n", urb);
-				usb_kill_urb(urb);
-			}
+			PWC_DEBUG_MEMORY("Unlinking URB %p\n", urb);
+			usb_kill_urb(urb);
+		}
+	}
+}
+
+static void pwc_iso_free(struct pwc_device *pdev)
+{
+	int i;
+
+	/* Freeing ISOC buffers one by one */
+	for (i = 0; i < MAX_ISO_BUFS; i++) {
+		struct urb *urb;
+
+		urb = pdev->sbuf[i].urb;
+		if (urb != 0) {
 			PWC_DEBUG_MEMORY("Freeing URB\n");
 			usb_free_urb(urb);
 			pdev->sbuf[i].urb = NULL;
 		}
 	}
+}
+
+void pwc_isoc_cleanup(struct pwc_device *pdev)
+{
+	PWC_DEBUG_OPEN(">> pwc_isoc_cleanup()\n");
+	if (pdev == NULL)
+		return;
+	if (pdev->iso_init == 0)
+		return;
+
+	pwc_iso_stop(pdev);
+	pwc_iso_free(pdev);
 
 	/* Stop camera, but only if we are sure the camera is still there (unplug
 	   is signalled by EPIPE)
@@ -1212,6 +1230,7 @@ static int pwc_video_close(struct inode 
 
 	PWC_DEBUG_OPEN(">> video_close called(vdev = 0x%p).\n", vdev);
 
+	lock_kernel();
 	pdev = (struct pwc_device *)vdev->priv;
 	if (pdev->vopen == 0)
 		PWC_DEBUG_MODULE("video_close() called on closed device?\n");
@@ -1231,7 +1250,6 @@ static int pwc_video_close(struct inode 
 	pwc_isoc_cleanup(pdev);
 	pwc_free_buffers(pdev);
 
-	lock_kernel();
 	/* Turn off LEDS and power down camera, but only when not unplugged */
 	if (!pdev->unplugged) {
 		/* Turn LEDs off */
@@ -1277,7 +1295,7 @@ static ssize_t pwc_video_read(struct fil
 	struct pwc_device *pdev;
 	int noblock = file->f_flags & O_NONBLOCK;
 	DECLARE_WAITQUEUE(wait, current);
-	int bytes_to_read;
+	int bytes_to_read, rv = 0;
 	void *image_buffer_addr;
 
 	PWC_DEBUG_READ("pwc_video_read(vdev=0x%p, buf=%p, count=%zd) called.\n",
@@ -1287,8 +1305,12 @@ static ssize_t pwc_video_read(struct fil
 	pdev = vdev->priv;
 	if (pdev == NULL)
 		return -EFAULT;
-	if (pdev->error_status)
-		return -pdev->error_status; /* Something happened, report what. */
+
+	mutex_lock(&pdev->modlock);
+	if (pdev->error_status) {
+		rv = -pdev->error_status; /* Something happened, report what. */
+		goto err_out;
+	}
 
 	/* In case we're doing partial reads, we don't have to wait for a frame */
 	if (pdev->image_read_pos == 0) {
@@ -1299,17 +1321,20 @@ static ssize_t pwc_video_read(struct fil
 			if (pdev->error_status) {
 				remove_wait_queue(&pdev->frameq, &wait);
 				set_current_state(TASK_RUNNING);
-				return -pdev->error_status ;
+				rv = -pdev->error_status ;
+				goto err_out;
 			}
 			if (noblock) {
 				remove_wait_queue(&pdev->frameq, &wait);
 				set_current_state(TASK_RUNNING);
-				return -EWOULDBLOCK;
+				rv = -EWOULDBLOCK;
+				goto err_out;
 			}
 			if (signal_pending(current)) {
 				remove_wait_queue(&pdev->frameq, &wait);
 				set_current_state(TASK_RUNNING);
-				return -ERESTARTSYS;
+				rv = -ERESTARTSYS;
+				goto err_out;
 			}
 			schedule();
 			set_current_state(TASK_INTERRUPTIBLE);
@@ -1318,8 +1343,10 @@ static ssize_t pwc_video_read(struct fil
 		set_current_state(TASK_RUNNING);
 
 		/* Decompress and release frame */
-		if (pwc_handle_frame(pdev))
-			return -EFAULT;
+		if (pwc_handle_frame(pdev)) {
+			rv = -EFAULT;
+			goto err_out;
+		}
 	}
 
 	PWC_DEBUG_READ("Copying data to user space.\n");
@@ -1334,14 +1361,20 @@ static ssize_t pwc_video_read(struct fil
 	image_buffer_addr = pdev->image_data;
 	image_buffer_addr += pdev->images[pdev->fill_image].offset;
 	image_buffer_addr += pdev->image_read_pos;
-	if (copy_to_user(buf, image_buffer_addr, count))
-		return -EFAULT;
+	if (copy_to_user(buf, image_buffer_addr, count)) {
+		rv = -EFAULT;
+		goto err_out;
+	}
 	pdev->image_read_pos += count;
 	if (pdev->image_read_pos >= bytes_to_read) { /* All data has been read */
 		pdev->image_read_pos = 0;
 		pwc_next_image(pdev);
 	}
+	mutex_unlock(&pdev->modlock);
 	return count;
+err_out:
+	mutex_unlock(&pdev->modlock);
+	return rv;
 }
 
 static unsigned int pwc_video_poll(struct file *file, poll_table *wait)
@@ -1367,7 +1400,20 @@ static unsigned int pwc_video_poll(struc
 static int pwc_video_ioctl(struct inode *inode, struct file *file,
 			   unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, pwc_video_do_ioctl);
+	struct video_device *vdev = file->private_data;
+	struct pwc_device *pdev; 
+	int r = -ENODEV;
+
+	if (!vdev)
+		goto out;
+	pdev = vdev->priv;
+
+	mutex_lock(&pdev->modlock);
+	if (!pdev->unplugged)
+		r = video_usercopy(inode, file, cmd, arg, pwc_video_do_ioctl);
+	mutex_unlock(&pdev->modlock);
+out:
+	return r;
 }
 
 static int pwc_video_mmap(struct file *file, struct vm_area_struct *vma)
@@ -1810,7 +1856,10 @@ static void usb_pwc_disconnect(struct us
 	wake_up_interruptible(&pdev->frameq);
 	/* Wait until device is closed */
 	if(pdev->vopen) {
+		mutex_lock(&pdev->modlock);
 		pdev->unplugged = 1;
+		mutex_unlock(&pdev->modlock);
+		pwc_iso_stop(pdev);
 	} else {
 		/* Device is closed, so we can safely unregister it */
 		PWC_DEBUG_PROBE("Unregistering video device in disconnect().\n");
@@ -1828,7 +1877,6 @@ disconnect_out:
 	unlock_kernel();
 }
 
-
 /* *grunt* We have to do atoi ourselves :-( */
 static int pwc_atoi(const char *s)
 {