Re: Oops in pwc v4l driver

2007-09-26 Thread Oliver Neukum
The pwc driver is defficient in locking, which can trigger an oops
when disconnecting.

Signed-off-by: Oliver Neukum <[EMAIL PROTECTED]>

Regards
Oliver
---

--- a/drivers/media/video/pwc/pwc-if.c  2007-08-24 10:16:38.0 +0200
+++ b/drivers/media/video/pwc/pwc-if.c  2007-08-24 11:01:26.0 +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(>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(>frameq, );
set_current_state(TASK_RUNNING);
-   return -pdev->error_status ;
+   rv = -pdev->error_status ;
+   goto err_out;
}
if (noblock) {
remove_wait_queue(>frameq, );
set_current_state(TASK_RUNNING);
-   return -EWOULDBLOCK;
+   rv = -EWOULDBLOCK;
+   goto err_out;
}
if (signal_pending(current)) {
remove_wait_queue(>frameq, );
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);
 

Re: Oops in pwc v4l driver

2007-09-26 Thread Oliver Neukum
The pwc driver is defficient in locking, which can trigger an oops
when disconnecting.

Signed-off-by: Oliver Neukum <[EMAIL PROTECTED]>

Regards
Oliver
---

--- a/drivers/media/video/pwc/pwc-if.c  2007-08-24 10:16:38.0 +0200
+++ b/drivers/media/video/pwc/pwc-if.c  2007-08-24 11:01:26.0 +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(>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(>frameq, );
set_current_state(TASK_RUNNING);
-   return -pdev->error_status ;
+   rv = -pdev->error_status ;
+   goto err_out;
}
if (noblock) {
remove_wait_queue(>frameq, );
set_current_state(TASK_RUNNING);
-   return -EWOULDBLOCK;
+   rv = -EWOULDBLOCK;
+   goto err_out;
}
if (signal_pending(current)) {
remove_wait_queue(>frameq, );
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);
 

Re: Oops in pwc v4l driver

2007-09-26 Thread 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.c2007-08-24 10:16:38.0 
> +0200
> +++ b/drivers/media/video/pwc/pwc-if.c2007-08-24 11:01:26.0 
> +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(>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(>frameq, );
>   set_current_state(TASK_RUNNING);
> - return -pdev->error_status ;
> + rv = -pdev->error_status ;
> + goto err_out;
>   }
>   if (noblock) {
>   remove_wait_queue(>frameq, );
>   set_current_state(TASK_RUNNING);
> - return -EWOULDBLOCK;
> + rv = -EWOULDBLOCK;
> + goto err_out;
>   }
>   if 

Re: Oops in pwc v4l driver

2007-09-26 Thread 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.c2007-08-24 10:16:38.0 
 +0200
 +++ b/drivers/media/video/pwc/pwc-if.c2007-08-24 11:01:26.0 
 +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);
 -

Re: Oops in pwc v4l driver

2007-09-26 Thread Oliver Neukum
The pwc driver is defficient in locking, which can trigger an oops
when disconnecting.

Signed-off-by: Oliver Neukum [EMAIL PROTECTED]

Regards
Oliver
---

--- a/drivers/media/video/pwc/pwc-if.c  2007-08-24 10:16:38.0 +0200
+++ b/drivers/media/video/pwc/pwc-if.c  2007-08-24 11:01:26.0 +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);
 
/* 

Re: Oops in pwc v4l driver

2007-09-26 Thread Oliver Neukum
The pwc driver is defficient in locking, which can trigger an oops
when disconnecting.

Signed-off-by: Oliver Neukum [EMAIL PROTECTED]

Regards
Oliver
---

--- a/drivers/media/video/pwc/pwc-if.c  2007-08-24 10:16:38.0 +0200
+++ b/drivers/media/video/pwc/pwc-if.c  2007-08-24 11:01:26.0 +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);
 
/* 

Re: Oops in pwc v4l driver

2007-09-08 Thread Alex Smith
On 03/09/2007, Michal Piotrowski <[EMAIL PROTECTED]> wrote:
> Hi Alex,
>
> On 02/09/07, Alex Smith <[EMAIL PROTECTED]> 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: Oops in pwc v4l driver

2007-09-08 Thread Alex Smith
On 03/09/2007, Michal Piotrowski [EMAIL PROTECTED] wrote:
 Hi Alex,

 On 02/09/07, Alex Smith [EMAIL PROTECTED] 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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: Oops in pwc v4l driver

2007-09-03 Thread Oliver Neukum
Am Montag 03 September 2007 schrieb Michal Piotrowski:
> Hi Alex,
> 
> On 02/09/07, Alex Smith <[EMAIL PROTECTED]> 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



-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: Oops in pwc v4l driver

2007-09-03 Thread Oliver Neukum
Am Montag 03 September 2007 schrieb Michal Piotrowski:
 Hi Alex,
 
 On 02/09/07, Alex Smith [EMAIL PROTECTED] 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



-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: Oops in pwc v4l driver

2007-09-02 Thread Michal Piotrowski
Hi Alex,

On 02/09/07, Alex Smith <[EMAIL PROTECTED]> 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/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: Oops in pwc v4l driver

2007-09-02 Thread 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.0 +0200
+++ b/drivers/media/video/pwc/pwc-if.c  2007-08-24 11:01:26.0 +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(>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(>frameq, );
set_current_state(TASK_RUNNING);
-   return -pdev->error_status ;
+   rv = -pdev->error_status ;
+   goto err_out;
}
if (noblock) {
remove_wait_queue(>frameq, );
set_current_state(TASK_RUNNING);
-   return -EWOULDBLOCK;
+   rv = -EWOULDBLOCK;
+   goto err_out;
}
if (signal_pending(current)) {
remove_wait_queue(>frameq, );
set_current_state(TASK_RUNNING);
-   return -ERESTARTSYS;
+   rv = -ERESTARTSYS;
+

Re: Oops in pwc v4l driver

2007-09-02 Thread 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.0 +0200
+++ b/drivers/media/video/pwc/pwc-if.c  2007-08-24 11:01:26.0 +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;
+  

Re: Oops in pwc v4l driver

2007-09-02 Thread Michal Piotrowski
Hi Alex,

On 02/09/07, Alex Smith [EMAIL PROTECTED] 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/
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/