Re: Oops in pwc v4l driver
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
Re: Oops in pwc v4l driver
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
Re: Oops in pwc v4l driver
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;
> }
>
Re: Oops in pwc v4l driver
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
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
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
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;
+
Oops in pwc v4l driver
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 - 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/
