diff mbox series

[RESEND] media: uvcvideo: Disable autosuspend for Insta360 Link

Message ID 20221101-instal-v1-0-d13d1331c4b5@chromium.org
State New
Headers show
Series [RESEND] media: uvcvideo: Disable autosuspend for Insta360 Link | expand

Commit Message

Ricardo Ribalda Dec. 2, 2022, 4:48 p.m. UTC 
When the device suspends, it keeps power-cycling.

The user notices it because the LED constanct oscillate between
blue (ready) and no LED (off).

<6>[95202.128542] usb 3-3-port4: attempt power cycle
<6>[95206.070120] usb 3-3.4: new high-speed USB device number 49 using xhci_hcd
<6>[95206.212027] usb 3-3.4: New USB device found, idVendor=2e1a, idProduct=4c01, bcdDevice= 2.00
<6>[95206.212044] usb 3-3.4: New USB device strings: Mfr=1, Product=2, SerialNumber=<Serial: 1>
<6>[95206.212050] usb 3-3.4: Product: Insta360 Link
<6>[95206.212075] usb 3-3.4: Manufacturer: Amba
<7>[95206.214862] usb 3-3.4: GPIO lookup for consumer privacy
<7>[95206.214866] usb 3-3.4: using lookup tables for GPIO lookup
<7>[95206.214869] usb 3-3.4: No GPIO consumer privacy found
<6>[95206.214871] usb 3-3.4: Found UVC 1.10 device Insta360 Link (2e1a:4c01)
<3>[95206.217113] usb 3-3.4: Failed to query (GET_INFO) UVC control 14 on unit 1: -32 (exp. 1).
<3>[95206.217733] usb 3-3.4: Failed to query (GET_INFO) UVC control 16 on unit 1: -32 (exp. 1).
<4>[95206.223544] usb 3-3.4: Warning! Unlikely big volume range (=32767), cval->res is probably wrong.
<4>[95206.223554] usb 3-3.4: [9] FU [Mic Capture Volume] ch = 1, val = -32768/-1/1
<6>[95210.698990] usb 3-3.4: USB disconnect, device number 49
<6>[95211.963090] usb 3-3.4: new high-speed USB device number 50 using xhci_hcd
<6>[95212.657061] usb 3-3.4: new full-speed USB device number 51 using xhci_hcd
<3>[95212.783119] usb 3-3.4: device descriptor read/64, error -32
<3>[95213.015076] usb 3-3.4: device descriptor read/64, error -32
<6>[95213.120358] usb 3-3-port4: attempt power cycle

Bus 001 Device 009: ID 2e1a:4c01 Amba Insta360 Link
Device Descriptor:
  bLength                18
  bDescriptorType         1
  bcdUSB               2.00
  bDeviceClass          239 Miscellaneous Device
  bDeviceSubClass         2
  bDeviceProtocol         1 Interface Association
  bMaxPacketSize0        64
  idVendor           0x2e1a
  idProduct          0x4c01
  bcdDevice            2.00
  iManufacturer           1 Amba
  iProduct                2 Insta360 Link
  iSerial                 0
  bNumConfigurations      1

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
media: uvcvideo: Disable autosuspend for Insta360

The device does not handle properly the USB suspend and makes it barely usable.

To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: linux-media@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/media/usb/uvc/uvc_driver.c | 13 ++++++++++++-
 drivers/media/usb/uvc/uvcvideo.h   |  1 +
 2 files changed, 13 insertions(+), 1 deletion(-)


---
base-commit: 23758867219c8d84c8363316e6dd2f9fd7ae3049
change-id: 20221101-instal-9a77ba1cc448

Best regards,

Comments

Laurent Pinchart Dec. 29, 2022, 2:22 a.m. UTC | #1 
Hi Ricardo,

Thank you for the patch.

On Fri, Dec 02, 2022 at 05:48:52PM +0100, Ricardo Ribalda wrote:
> When the device suspends, it keeps power-cycling.
> 
> The user notices it because the LED constanct oscillate between
> blue (ready) and no LED (off).
> 
> <6>[95202.128542] usb 3-3-port4: attempt power cycle
> <6>[95206.070120] usb 3-3.4: new high-speed USB device number 49 using xhci_hcd
> <6>[95206.212027] usb 3-3.4: New USB device found, idVendor=2e1a, idProduct=4c01, bcdDevice= 2.00
> <6>[95206.212044] usb 3-3.4: New USB device strings: Mfr=1, Product=2, SerialNumber=<Serial: 1>
> <6>[95206.212050] usb 3-3.4: Product: Insta360 Link
> <6>[95206.212075] usb 3-3.4: Manufacturer: Amba
> <7>[95206.214862] usb 3-3.4: GPIO lookup for consumer privacy
> <7>[95206.214866] usb 3-3.4: using lookup tables for GPIO lookup
> <7>[95206.214869] usb 3-3.4: No GPIO consumer privacy found
> <6>[95206.214871] usb 3-3.4: Found UVC 1.10 device Insta360 Link (2e1a:4c01)
> <3>[95206.217113] usb 3-3.4: Failed to query (GET_INFO) UVC control 14 on unit 1: -32 (exp. 1).
> <3>[95206.217733] usb 3-3.4: Failed to query (GET_INFO) UVC control 16 on unit 1: -32 (exp. 1).
> <4>[95206.223544] usb 3-3.4: Warning! Unlikely big volume range (=32767), cval->res is probably wrong.
> <4>[95206.223554] usb 3-3.4: [9] FU [Mic Capture Volume] ch = 1, val = -32768/-1/1
> <6>[95210.698990] usb 3-3.4: USB disconnect, device number 49
> <6>[95211.963090] usb 3-3.4: new high-speed USB device number 50 using xhci_hcd
> <6>[95212.657061] usb 3-3.4: new full-speed USB device number 51 using xhci_hcd
> <3>[95212.783119] usb 3-3.4: device descriptor read/64, error -32
> <3>[95213.015076] usb 3-3.4: device descriptor read/64, error -32
> <6>[95213.120358] usb 3-3-port4: attempt power cycle
> 
> Bus 001 Device 009: ID 2e1a:4c01 Amba Insta360 Link
> Device Descriptor:
>   bLength                18
>   bDescriptorType         1
>   bcdUSB               2.00
>   bDeviceClass          239 Miscellaneous Device
>   bDeviceSubClass         2
>   bDeviceProtocol         1 Interface Association
>   bMaxPacketSize0        64
>   idVendor           0x2e1a
>   idProduct          0x4c01
>   bcdDevice            2.00
>   iManufacturer           1 Amba
>   iProduct                2 Insta360 Link
>   iSerial                 0
>   bNumConfigurations      1
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> media: uvcvideo: Disable autosuspend for Insta360
> 
> The device does not handle properly the USB suspend and makes it barely usable.

Isn't this best handled with a quirk in the USB core ? Autosuspend is a
device feature, not an interface feature, so if the USB sound driver is
loaded but uvcvideo isn't, the kernel may still attempt to autosuspend
the device.

> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> To: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: linux-media@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 13 ++++++++++++-
>  drivers/media/usb/uvc/uvcvideo.h   |  1 +
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 215fb483efb0..ad95c7599863 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -2223,7 +2223,9 @@ static int uvc_probe(struct usb_interface *intf,
>  	}
>  
>  	uvc_dbg(dev, PROBE, "UVC device initialized\n");
> -	usb_enable_autosuspend(udev);
> +	if (!(dev->quirks & UVC_QUIRK_DISABLE_AUTOSUSPEND))
> +		usb_enable_autosuspend(udev);
> +
>  	return 0;
>  
>  error:
> @@ -2967,6 +2969,15 @@ static const struct usb_device_id uvc_ids[] = {
>  	  .bInterfaceSubClass	= 1,
>  	  .bInterfaceProtocol	= 0,
>  	  .driver_info		= (kernel_ulong_t)&uvc_quirk_force_y8 },
> +	/* Insta360 Link */
> +	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> +				| USB_DEVICE_ID_MATCH_INT_INFO,
> +	  .idVendor		= 0x2e1a,
> +	  .idProduct		= 0x4c01,
> +	  .bInterfaceClass	= USB_CLASS_VIDEO,
> +	  .bInterfaceSubClass	= 1,
> +	  .bInterfaceProtocol	= 0,
> +	  .driver_info		= UVC_INFO_QUIRK(UVC_QUIRK_DISABLE_AUTOSUSPEND) },
>  	/* GEO Semiconductor GC6500 */
>  	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
>  				| USB_DEVICE_ID_MATCH_INT_INFO,
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index df93db259312..47c86c7c6346 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -74,6 +74,7 @@
>  #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT	0x00000400
>  #define UVC_QUIRK_FORCE_Y8		0x00000800
>  #define UVC_QUIRK_FORCE_BPP		0x00001000
> +#define UVC_QUIRK_DISABLE_AUTOSUSPEND	0x00002000
>  
>  /* Format flags */
>  #define UVC_FMT_FLAG_COMPRESSED		0x00000001
> 
> ---
> base-commit: 23758867219c8d84c8363316e6dd2f9fd7ae3049
> change-id: 20221101-instal-9a77ba1cc448
Ricardo Ribalda March 8, 2023, 10:43 p.m. UTC | #2 
On Wed, 1 Mar 2023 at 10:04, Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> Hi Laurent
>
> We are back to this issue.
>
>
> On Thu, 29 Dec 2022 at 03:22, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Ricardo,
> >
> > Thank you for the patch.
> >
> > On Fri, Dec 02, 2022 at 05:48:52PM +0100, Ricardo Ribalda wrote:
> > > When the device suspends, it keeps power-cycling.
> > >
> > > The user notices it because the LED constanct oscillate between
> > > blue (ready) and no LED (off).
> > >
> > > <6>[95202.128542] usb 3-3-port4: attempt power cycle
> > > <6>[95206.070120] usb 3-3.4: new high-speed USB device number 49 using xhci_hcd
> > > <6>[95206.212027] usb 3-3.4: New USB device found, idVendor=2e1a, idProduct=4c01, bcdDevice= 2.00
> > > <6>[95206.212044] usb 3-3.4: New USB device strings: Mfr=1, Product=2, SerialNumber=<Serial: 1>
> > > <6>[95206.212050] usb 3-3.4: Product: Insta360 Link
> > > <6>[95206.212075] usb 3-3.4: Manufacturer: Amba
> > > <7>[95206.214862] usb 3-3.4: GPIO lookup for consumer privacy
> > > <7>[95206.214866] usb 3-3.4: using lookup tables for GPIO lookup
> > > <7>[95206.214869] usb 3-3.4: No GPIO consumer privacy found
> > > <6>[95206.214871] usb 3-3.4: Found UVC 1.10 device Insta360 Link (2e1a:4c01)
> > > <3>[95206.217113] usb 3-3.4: Failed to query (GET_INFO) UVC control 14 on unit 1: -32 (exp. 1).
> > > <3>[95206.217733] usb 3-3.4: Failed to query (GET_INFO) UVC control 16 on unit 1: -32 (exp. 1).
> > > <4>[95206.223544] usb 3-3.4: Warning! Unlikely big volume range (=32767), cval->res is probably wrong.
> > > <4>[95206.223554] usb 3-3.4: [9] FU [Mic Capture Volume] ch = 1, val = -32768/-1/1
> > > <6>[95210.698990] usb 3-3.4: USB disconnect, device number 49
> > > <6>[95211.963090] usb 3-3.4: new high-speed USB device number 50 using xhci_hcd
> > > <6>[95212.657061] usb 3-3.4: new full-speed USB device number 51 using xhci_hcd
> > > <3>[95212.783119] usb 3-3.4: device descriptor read/64, error -32
> > > <3>[95213.015076] usb 3-3.4: device descriptor read/64, error -32
> > > <6>[95213.120358] usb 3-3-port4: attempt power cycle
> > >
> > > Bus 001 Device 009: ID 2e1a:4c01 Amba Insta360 Link
> > > Device Descriptor:
> > >   bLength                18
> > >   bDescriptorType         1
> > >   bcdUSB               2.00
> > >   bDeviceClass          239 Miscellaneous Device
> > >   bDeviceSubClass         2
> > >   bDeviceProtocol         1 Interface Association
> > >   bMaxPacketSize0        64
> > >   idVendor           0x2e1a
> > >   idProduct          0x4c01
> > >   bcdDevice            2.00
> > >   iManufacturer           1 Amba
> > >   iProduct                2 Insta360 Link
> > >   iSerial                 0
> > >   bNumConfigurations      1
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > > media: uvcvideo: Disable autosuspend for Insta360
> > >
> > > The device does not handle properly the USB suspend and makes it barely usable.
> >
> > Isn't this best handled with a quirk in the USB core ? Autosuspend is a
> > device feature, not an interface feature, so if the USB sound driver is
> > loaded but uvcvideo isn't, the kernel may still attempt to autosuspend
> > the device.
> >
>
> Seems like USB_QUIRK_NO_AUTOSUSPEND was gone for a long time
>
> https://lore.kernel.org/lkml/20071115064457.GU19218@kroah.com/
>
> under the assumption that autosuspend was off by default and user
> space should only enable autosuspend on the devices that support it
> (if I understand it correctly).
>
> There are two other quirks still available: USB_QUIRK_RESET_RESUME and
> USB_QUIRK_DISCONNECT_SUSPEND, but they do not seem to work for this
> device (Yunke, thanks for looking into this)
>
> If we are explicitly enabling autosuspend on the driver, shouldn't we
> make sure that the device supports it?
>

Alan, Greg, any idea about what is the best way to proceed here from a
USB perspective?

Thanks!

>
> > > To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > To: Mauro Carvalho Chehab <mchehab@kernel.org>
> > > Cc: linux-media@vger.kernel.org
> > > Cc: linux-kernel@vger.kernel.org
> > > ---
> > >  drivers/media/usb/uvc/uvc_driver.c | 13 ++++++++++++-
> > >  drivers/media/usb/uvc/uvcvideo.h   |  1 +
> > >  2 files changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > index 215fb483efb0..ad95c7599863 100644
> > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > @@ -2223,7 +2223,9 @@ static int uvc_probe(struct usb_interface *intf,
> > >       }
> > >
> > >       uvc_dbg(dev, PROBE, "UVC device initialized\n");
> > > -     usb_enable_autosuspend(udev);
> > > +     if (!(dev->quirks & UVC_QUIRK_DISABLE_AUTOSUSPEND))
> > > +             usb_enable_autosuspend(udev);
> > > +
> > >       return 0;
> > >
> > >  error:
> > > @@ -2967,6 +2969,15 @@ static const struct usb_device_id uvc_ids[] = {
> > >         .bInterfaceSubClass   = 1,
> > >         .bInterfaceProtocol   = 0,
> > >         .driver_info          = (kernel_ulong_t)&uvc_quirk_force_y8 },
> > > +     /* Insta360 Link */
> > > +     { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> > > +                             | USB_DEVICE_ID_MATCH_INT_INFO,
> > > +       .idVendor             = 0x2e1a,
> > > +       .idProduct            = 0x4c01,
> > > +       .bInterfaceClass      = USB_CLASS_VIDEO,
> > > +       .bInterfaceSubClass   = 1,
> > > +       .bInterfaceProtocol   = 0,
> > > +       .driver_info          = UVC_INFO_QUIRK(UVC_QUIRK_DISABLE_AUTOSUSPEND) },
> > >       /* GEO Semiconductor GC6500 */
> > >       { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> > >                               | USB_DEVICE_ID_MATCH_INT_INFO,
> > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > index df93db259312..47c86c7c6346 100644
> > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > @@ -74,6 +74,7 @@
> > >  #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT      0x00000400
> > >  #define UVC_QUIRK_FORCE_Y8           0x00000800
> > >  #define UVC_QUIRK_FORCE_BPP          0x00001000
> > > +#define UVC_QUIRK_DISABLE_AUTOSUSPEND        0x00002000
> > >
> > >  /* Format flags */
> > >  #define UVC_FMT_FLAG_COMPRESSED              0x00000001
> > >
> > > ---
> > > base-commit: 23758867219c8d84c8363316e6dd2f9fd7ae3049
> > > change-id: 20221101-instal-9a77ba1cc448
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>
>
>
> --
> Ricardo Ribalda
Alan Stern March 8, 2023, 11:02 p.m. UTC | #3 
On Wed, Mar 08, 2023 at 11:43:09PM +0100, Ricardo Ribalda wrote:
> On Wed, 1 Mar 2023 at 10:04, Ricardo Ribalda <ribalda@chromium.org> wrote:
> >
> > Hi Laurent
> >
> > We are back to this issue.
> >
> >
> > On Thu, 29 Dec 2022 at 03:22, Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > >
> > > Hi Ricardo,
> > >
> > > Thank you for the patch.
> > >
> > > On Fri, Dec 02, 2022 at 05:48:52PM +0100, Ricardo Ribalda wrote:
> > > > When the device suspends, it keeps power-cycling.
> > > >
> > > > The user notices it because the LED constanct oscillate between
> > > > blue (ready) and no LED (off).

> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > > media: uvcvideo: Disable autosuspend for Insta360
> > > >
> > > > The device does not handle properly the USB suspend and makes it barely usable.
> > >
> > > Isn't this best handled with a quirk in the USB core ? Autosuspend is a
> > > device feature, not an interface feature, so if the USB sound driver is
> > > loaded but uvcvideo isn't, the kernel may still attempt to autosuspend
> > > the device.
> > >
> >
> > Seems like USB_QUIRK_NO_AUTOSUSPEND was gone for a long time
> >
> > https://lore.kernel.org/lkml/20071115064457.GU19218@kroah.com/
> >
> > under the assumption that autosuspend was off by default and user
> > space should only enable autosuspend on the devices that support it
> > (if I understand it correctly).
> >
> > There are two other quirks still available: USB_QUIRK_RESET_RESUME and
> > USB_QUIRK_DISCONNECT_SUSPEND, but they do not seem to work for this
> > device (Yunke, thanks for looking into this)
> >
> > If we are explicitly enabling autosuspend on the driver, shouldn't we
> > make sure that the device supports it?
> >
> 
> Alan, Greg, any idea about what is the best way to proceed here from a
> USB perspective?

How is autosuspend getting enabled for this device?  It is disabled by 
default for non-hub USB devices.

If the uvcvideo or USB sound driver is enabling autosuspend, the driver 
should be fixed.  Perhaps by adding a quirk bit for this purpose.

If userspace is enabling autosuspend, then any misbehavior isn't the 
kernel's fault.  :-)

Alan Stern
Ricardo Ribalda March 8, 2023, 11:12 p.m. UTC | #4 
Hi Alan

On Thu, 9 Mar 2023 at 00:02, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, Mar 08, 2023 at 11:43:09PM +0100, Ricardo Ribalda wrote:
> > On Wed, 1 Mar 2023 at 10:04, Ricardo Ribalda <ribalda@chromium.org> wrote:
> > >
> > > Hi Laurent
> > >
> > > We are back to this issue.
> > >
> > >
> > > On Thu, 29 Dec 2022 at 03:22, Laurent Pinchart
> > > <laurent.pinchart@ideasonboard.com> wrote:
> > > >
> > > > Hi Ricardo,
> > > >
> > > > Thank you for the patch.
> > > >
> > > > On Fri, Dec 02, 2022 at 05:48:52PM +0100, Ricardo Ribalda wrote:
> > > > > When the device suspends, it keeps power-cycling.
> > > > >
> > > > > The user notices it because the LED constanct oscillate between
> > > > > blue (ready) and no LED (off).
>
> > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > ---
> > > > > media: uvcvideo: Disable autosuspend for Insta360
> > > > >
> > > > > The device does not handle properly the USB suspend and makes it barely usable.
> > > >
> > > > Isn't this best handled with a quirk in the USB core ? Autosuspend is a
> > > > device feature, not an interface feature, so if the USB sound driver is
> > > > loaded but uvcvideo isn't, the kernel may still attempt to autosuspend
> > > > the device.
> > > >
> > >
> > > Seems like USB_QUIRK_NO_AUTOSUSPEND was gone for a long time
> > >
> > > https://lore.kernel.org/lkml/20071115064457.GU19218@kroah.com/
> > >
> > > under the assumption that autosuspend was off by default and user
> > > space should only enable autosuspend on the devices that support it
> > > (if I understand it correctly).
> > >
> > > There are two other quirks still available: USB_QUIRK_RESET_RESUME and
> > > USB_QUIRK_DISCONNECT_SUSPEND, but they do not seem to work for this
> > > device (Yunke, thanks for looking into this)
> > >
> > > If we are explicitly enabling autosuspend on the driver, shouldn't we
> > > make sure that the device supports it?
> > >
> >
> > Alan, Greg, any idea about what is the best way to proceed here from a
> > USB perspective?
>
> How is autosuspend getting enabled for this device?  It is disabled by
> default for non-hub USB devices.

It is enabled on the driver via usb_enable_autosuspend()
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/usb/uvc/uvc_driver.c#n2211

>
> If the uvcvideo or USB sound driver is enabling autosuspend, the driver
> should be fixed.  Perhaps by adding a quirk bit for this purpose.

This is what I tried with this patch :). Laurent, could you please
take a second look to it?
Thanks!


>
> If userspace is enabling autosuspend, then any misbehavior isn't the
> kernel's fault.  :-)
>
> Alan Stern
Laurent Pinchart March 22, 2024, 12:08 p.m. UTC | #5 
Hi Ricardo,

On Thu, Mar 09, 2023 at 12:12:27AM +0100, Ricardo Ribalda wrote:
> On Thu, 9 Mar 2023 at 00:02, Alan Stern wrote:
> > On Wed, Mar 08, 2023 at 11:43:09PM +0100, Ricardo Ribalda wrote:
> > > On Wed, 1 Mar 2023 at 10:04, Ricardo Ribalda wrote:
> > > > On Thu, 29 Dec 2022 at 03:22, Laurent Pinchart wrote:
> > > > > On Fri, Dec 02, 2022 at 05:48:52PM +0100, Ricardo Ribalda wrote:
> > > > > > When the device suspends, it keeps power-cycling.
> > > > > >
> > > > > > The user notices it because the LED constanct oscillate between
> > > > > > blue (ready) and no LED (off).
> >
> > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > > ---
> > > > > > media: uvcvideo: Disable autosuspend for Insta360
> > > > > >
> > > > > > The device does not handle properly the USB suspend and makes it barely usable.
> > > > >
> > > > > Isn't this best handled with a quirk in the USB core ? Autosuspend is a
> > > > > device feature, not an interface feature, so if the USB sound driver is
> > > > > loaded but uvcvideo isn't, the kernel may still attempt to autosuspend
> > > > > the device.
> > > > >
> > > >
> > > > Seems like USB_QUIRK_NO_AUTOSUSPEND was gone for a long time
> > > >
> > > > https://lore.kernel.org/lkml/20071115064457.GU19218@kroah.com/
> > > >
> > > > under the assumption that autosuspend was off by default and user
> > > > space should only enable autosuspend on the devices that support it
> > > > (if I understand it correctly).
> > > >
> > > > There are two other quirks still available: USB_QUIRK_RESET_RESUME and
> > > > USB_QUIRK_DISCONNECT_SUSPEND, but they do not seem to work for this
> > > > device (Yunke, thanks for looking into this)
> > > >
> > > > If we are explicitly enabling autosuspend on the driver, shouldn't we
> > > > make sure that the device supports it?
> > > >
> > >
> > > Alan, Greg, any idea about what is the best way to proceed here from a
> > > USB perspective?
> >
> > How is autosuspend getting enabled for this device?  It is disabled by
> > default for non-hub USB devices.
> 
> It is enabled on the driver via usb_enable_autosuspend()
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/usb/uvc/uvc_driver.c#n2211
> 
> > If the uvcvideo or USB sound driver is enabling autosuspend, the driver
> > should be fixed.  Perhaps by adding a quirk bit for this purpose.
> 
> This is what I tried with this patch :). Laurent, could you please
> take a second look to it?
> Thanks!

Done, and

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > If userspace is enabling autosuspend, then any misbehavior isn't the
> > kernel's fault.  :-)
diff mbox series

Patch

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 215fb483efb0..ad95c7599863 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2223,7 +2223,9 @@  static int uvc_probe(struct usb_interface *intf,
 	}
 
 	uvc_dbg(dev, PROBE, "UVC device initialized\n");
-	usb_enable_autosuspend(udev);
+	if (!(dev->quirks & UVC_QUIRK_DISABLE_AUTOSUSPEND))
+		usb_enable_autosuspend(udev);
+
 	return 0;
 
 error:
@@ -2967,6 +2969,15 @@  static const struct usb_device_id uvc_ids[] = {
 	  .bInterfaceSubClass	= 1,
 	  .bInterfaceProtocol	= 0,
 	  .driver_info		= (kernel_ulong_t)&uvc_quirk_force_y8 },
+	/* Insta360 Link */
+	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
+				| USB_DEVICE_ID_MATCH_INT_INFO,
+	  .idVendor		= 0x2e1a,
+	  .idProduct		= 0x4c01,
+	  .bInterfaceClass	= USB_CLASS_VIDEO,
+	  .bInterfaceSubClass	= 1,
+	  .bInterfaceProtocol	= 0,
+	  .driver_info		= UVC_INFO_QUIRK(UVC_QUIRK_DISABLE_AUTOSUSPEND) },
 	/* GEO Semiconductor GC6500 */
 	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
 				| USB_DEVICE_ID_MATCH_INT_INFO,
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index df93db259312..47c86c7c6346 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -74,6 +74,7 @@ 
 #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT	0x00000400
 #define UVC_QUIRK_FORCE_Y8		0x00000800
 #define UVC_QUIRK_FORCE_BPP		0x00001000
+#define UVC_QUIRK_DISABLE_AUTOSUSPEND	0x00002000
 
 /* Format flags */
 #define UVC_FMT_FLAG_COMPRESSED		0x00000001