Hi Brandon,
On Friday 30 May 2008, Brandon Philips wrote:
> Bug report: https://bugzilla.novell.com/show_bug.cgi?id=387876
>
> If probe fails the UVC driver may still be bound to a streaming
> interface. This interface must be released with
> usb_driver_release_interface() otherwise the interface will continue to
> be bound causing suspend operations to fail.
Thanks for the report.
> Tested using this little hack:
> > +++ uvc_video.c
> > @@ -870,6 +870,8 @@ int uvc_video_init(st
> > if ((ret = uvc_get_video_ctrl(video, probe, 1, GET_DEF)) < 0 &&
> > (ret = uvc_get_video_ctrl(video, probe, 1, GET_CUR)) < 0)
> > return ret;
> > +
> > + return -EIO;
>
> Signed-off-by: Brandon Philips <[EMAIL PROTECTED]>
>
> ---
> uvc_driver.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> Index: uvc/uvc_driver.c
> ===================================================================
> --- uvc.orig/uvc_driver.c
> +++ uvc/uvc_driver.c
> @@ -865,6 +865,8 @@ static int uvc_parse_standard_control(st
>
> if (uvc_parse_streaming(dev, streaming) < 0) {
> usb_put_intf(intf);
> + usb_driver_release_interface(&uvc_driver.driver,
> + intf);
> kfree(streaming->format);
> kfree(streaming->header.bmaControls);
> kfree(streaming);
> @@ -1524,6 +1526,8 @@ void uvc_delete(struct kref *kref)
> struct uvc_streaming *streaming;
> streaming = list_entry(p, struct uvc_streaming, list);
> usb_put_intf(streaming->intf);
> + usb_driver_release_interface(&uvc_driver.driver,
> + streaming->intf);
> kfree(streaming->format);
> kfree(streaming->header.bmaControls);
> kfree(streaming);
Shouldn't usb_driver_release_interface be called before usb_put_intf ? It
looks like the interface can be freed by usb_put_intf otherwise.
You are also missing an error path (kzalloc returning NULL right after
claiming the interface). Here is a modified version of your patch that cleans
up the driver code a little as well. It works fine here, could you please
test it ?
Best regards,
Laurent Pinchart
Index: uvc_driver.c
===================================================================
--- uvc_driver.c (revision 211)
+++ uvc_driver.c (working copy)
@@ -517,11 +517,11 @@
}
static int uvc_parse_streaming(struct uvc_device *dev,
- struct uvc_streaming *streaming)
+ struct usb_interface *intf)
{
+ struct uvc_streaming *streaming = NULL;
struct uvc_format *format;
struct uvc_frame *frame;
- struct usb_interface *intf = streaming->intf;
struct usb_host_interface *alts = &intf->altsetting[0];
unsigned char *_buffer, *buffer = alts->extra;
int _buflen, buflen = alts->extralen;
@@ -531,6 +531,31 @@
__u16 psize;
int ret;
+ if (intf->cur_altsetting->desc.bInterfaceSubClass
+ != SC_VIDEOSTREAMING) {
+ uvc_trace(UVC_TRACE_DESCR, "device %d interface %d isn't a "
+ "video streaming interface\n", dev->udev->devnum,
+ intf->altsetting[0].desc.bInterfaceNumber);
+ return -EINVAL;
+ }
+
+ if (usb_driver_claim_interface(&uvc_driver.driver, intf, dev)) {
+ uvc_trace(UVC_TRACE_DESCR, "device %d interface %d is already "
+ "claimed\n", dev->udev->devnum,
+ intf->altsetting[0].desc.bInterfaceNumber);
+ return -EINVAL;
+ }
+
+ streaming = kzalloc(sizeof *streaming, GFP_KERNEL);
+ if (streaming == NULL) {
+ usb_driver_release_interface(&uvc_driver.driver, intf);
+ return -EINVAL;
+ }
+
+ mutex_init(&streaming->mutex);
+ streaming->intf = usb_get_intf(intf);
+ streaming->intfnum = intf->cur_altsetting->desc.bInterfaceNumber;
+
/* The Pico iMage webcam has its class-specific interface descriptors
* after the endpoint descriptors.
*/
@@ -561,7 +586,7 @@
if (buflen <= 2) {
uvc_trace(UVC_TRACE_DESCR, "no class-specific streaming "
"interface descriptors found.\n");
- return -EINVAL;
+ goto error;
}
/* Parse the header descriptor. */
@@ -569,7 +594,7 @@
uvc_trace(UVC_TRACE_DESCR, "device %d videostreaming interface "
"%d OUTPUT HEADER descriptor is not supported.\n",
dev->udev->devnum, alts->desc.bInterfaceNumber);
- return -EINVAL;
+ goto error;
} else if (buffer[2] == VS_INPUT_HEADER) {
p = buflen >= 5 ? buffer[3] : 0;
n = buflen >= 12 ? buffer[12] : 0;
@@ -579,7 +604,7 @@
"interface %d INPUT HEADER descriptor is "
"invalid.\n", dev->udev->devnum,
alts->desc.bInterfaceNumber);
- return -EINVAL;
+ goto error;
}
streaming->header.bNumFormats = p;
@@ -592,15 +617,17 @@
streaming->header.bControlSize = n;
streaming->header.bmaControls = kmalloc(p*n, GFP_KERNEL);
- if (streaming->header.bmaControls == NULL)
- return -ENOMEM;
+ if (streaming->header.bmaControls == NULL) {
+ ret = -ENOMEM;
+ goto error;
+ }
memcpy(streaming->header.bmaControls, &buffer[13], p*n);
} else {
uvc_trace(UVC_TRACE_DESCR, "device %d videostreaming interface "
"%d HEADER descriptor not found.\n", dev->udev->devnum,
alts->desc.bInterfaceNumber);
- return -EINVAL;
+ goto error;
}
buflen -= buffer[0];
@@ -657,14 +684,16 @@
uvc_trace(UVC_TRACE_DESCR, "device %d videostreaming interface "
"%d has no supported formats defined.\n",
dev->udev->devnum, alts->desc.bInterfaceNumber);
- return -EINVAL;
+ goto error;
}
size = nformats * sizeof *format + nframes * sizeof *frame
+ nintervals * sizeof *interval;
format = kzalloc(size, GFP_KERNEL);
- if (format == NULL)
- return -ENOMEM;
+ if (format == NULL) {
+ ret = -ENOMEM;
+ goto error;
+ }
frame = (struct uvc_frame *)&format[nformats];
interval = (__u32 *)&frame[nframes];
@@ -683,7 +712,7 @@
ret = uvc_parse_format(dev, streaming, format,
&interval, buffer, buflen);
if (ret < 0)
- return ret;
+ goto error;
frame += format->nframes;
format++;
@@ -715,7 +744,16 @@
streaming->maxpsize = psize;
}
+ list_add_tail(&streaming->list, &dev->streaming);
return 0;
+
+error:
+ usb_driver_release_interface(&uvc_driver.driver, intf);
+ usb_put_intf(intf);
+ kfree(streaming->format);
+ kfree(streaming->header.bmaControls);
+ kfree(streaming);
+ return ret;
}
/* Parse vendor-specific extensions. */
@@ -805,7 +843,6 @@
const unsigned char *buffer, int buflen)
{
struct usb_device *udev = dev->udev;
- struct uvc_streaming *streaming;
struct uvc_entity *unit, *term;
struct usb_interface *intf;
struct usb_host_interface *alts = dev->intf->cur_altsetting;
@@ -836,42 +873,7 @@
continue;
}
- if (intf->cur_altsetting->desc.bInterfaceSubClass
- != SC_VIDEOSTREAMING) {
- uvc_trace(UVC_TRACE_DESCR, "device %d "
- "interface %d isn't a video "
- "streaming interface\n",
- udev->devnum, i);
- continue;
- }
-
- if (usb_interface_claimed(intf)) {
- uvc_trace(UVC_TRACE_DESCR, "device %d "
- "interface %d is already claimed\n",
- udev->devnum, i);
- continue;
- }
-
- usb_driver_claim_interface(&uvc_driver.driver, intf,
- dev);
-
- streaming = kzalloc(sizeof *streaming, GFP_KERNEL);
- if (streaming == NULL)
- continue;
- mutex_init(&streaming->mutex);
- streaming->intf = usb_get_intf(intf);
- streaming->intfnum =
- intf->cur_altsetting->desc.bInterfaceNumber;
-
- if (uvc_parse_streaming(dev, streaming) < 0) {
- usb_put_intf(intf);
- kfree(streaming->format);
- kfree(streaming->header.bmaControls);
- kfree(streaming);
- continue;
- }
-
- list_add_tail(&streaming->list, &dev->streaming);
+ uvc_parse_streaming(dev, intf);
}
break;
@@ -1523,6 +1525,7 @@
list_for_each_safe(p, n, &dev->streaming) {
struct uvc_streaming *streaming;
streaming = list_entry(p, struct uvc_streaming, list);
+ usb_driver_release_interface(&uvc_driver.driver, streaming->intf);
usb_put_intf(streaming->intf);
kfree(streaming->format);
kfree(streaming->header.bmaControls);
_______________________________________________
Linux-uvc-devel mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/linux-uvc-devel
