diff mbox series

[v2] usb: core: Add quirk for Logitech Rallybar

Message ID 20231222-rallybar-v2-1-5849d62a9514@chromium.org (mailing list archive)
State New
Headers show
Series [v2] usb: core: Add quirk for Logitech Rallybar | expand

Commit Message

Ricardo Ribalda Dec. 22, 2023, 10:55 p.m. UTC 
Logitech Rallybar devices, despite behaving as UVC camera, they have a
different power management system than the rest of the other Logitech
cameras.

USB_QUIRK_RESET_RESUME causes undesired USB disconnects, that make the
device unusable.

These are the only two devices that have this behavior, and we do not
have the list of devices that require USB_QUIRK_RESET_RESUME, so lets
create a new lit for them that un-apply the USB_QUIRK_RESET_RESUME
quirk.

Fixes: e387ef5c47dd ("usb: Add USB_QUIRK_RESET_RESUME for all Logitech UVC webcams")
Cc: stable@vger.kernel.org
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Tested with a Rallybar Mini with an Acer Chromebook Spin 513
---
Changes in v2:
- Add Fixes tag
- Add UVC maintainer as Cc
- Link to v1: https://lore.kernel.org/r/20231222-rallybar-v1-1-82b2a4d3106f@chromium.org
---
 drivers/usb/core/quirks.c | 10 ++++++++++
 1 file changed, 10 insertions(+)


---
base-commit: c0f65a7c112b3cfa691cead54bcf24d6cc2182b5
change-id: 20231222-rallybar-19ce0c64d5e6

Best regards,

Comments

Alan Stern Dec. 23, 2023, 8:01 p.m. UTC | #1 
On Fri, Dec 22, 2023 at 10:55:49PM +0000, Ricardo Ribalda wrote:
> Logitech Rallybar devices, despite behaving as UVC camera, they have a
> different power management system than the rest of the other Logitech
> cameras.
> 
> USB_QUIRK_RESET_RESUME causes undesired USB disconnects, that make the
> device unusable.
> 
> These are the only two devices that have this behavior, and we do not
> have the list of devices that require USB_QUIRK_RESET_RESUME, so lets
> create a new lit for them that un-apply the USB_QUIRK_RESET_RESUME
> quirk.
> 
> Fixes: e387ef5c47dd ("usb: Add USB_QUIRK_RESET_RESUME for all Logitech UVC webcams")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---

Would it make more sense to do this inside the uvc driver instead of 
creating a new single-purpose list in the core?

Alan Stern

> Tested with a Rallybar Mini with an Acer Chromebook Spin 513
> ---
> Changes in v2:
> - Add Fixes tag
> - Add UVC maintainer as Cc
> - Link to v1: https://lore.kernel.org/r/20231222-rallybar-v1-1-82b2a4d3106f@chromium.org
> ---
>  drivers/usb/core/quirks.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
> index 15e9bd180a1d..8fa8de50e7f0 100644
> --- a/drivers/usb/core/quirks.c
> +++ b/drivers/usb/core/quirks.c
> @@ -553,6 +553,14 @@ static const struct usb_device_id usb_interface_quirk_list[] = {
>  	{ }  /* terminating entry must be last */
>  };
>  
> +static const struct usb_device_id usb_interface_unsupported_quirk_list[] = {
> +	/* Logitech Rallybar VC systems*/
> +	{ USB_DEVICE(0x046d, 0x089b), .driver_info = USB_QUIRK_RESET_RESUME },
> +	{ USB_DEVICE(0x046d, 0x08d3), .driver_info = USB_QUIRK_RESET_RESUME },
> +
> +	{ }  /* terminating entry must be last */
> +};
> +
>  static const struct usb_device_id usb_amd_resume_quirk_list[] = {
>  	/* Lenovo Mouse with Pixart controller */
>  	{ USB_DEVICE(0x17ef, 0x602e), .driver_info = USB_QUIRK_RESET_RESUME },
> @@ -718,6 +726,8 @@ void usb_detect_interface_quirks(struct usb_device *udev)
>  	u32 quirks;
>  
>  	quirks = usb_detect_static_quirks(udev, usb_interface_quirk_list);
> +	quirks &= ~usb_detect_static_quirks(udev,
> +					usb_interface_unsupported_quirk_list);
>  	if (quirks == 0)
>  		return;
>  
> 
> ---
> base-commit: c0f65a7c112b3cfa691cead54bcf24d6cc2182b5
> change-id: 20231222-rallybar-19ce0c64d5e6
> 
> Best regards,
> -- 
> Ricardo Ribalda <ribalda@chromium.org>
>
Ricardo Ribalda Jan. 2, 2024, 11:31 a.m. UTC | #2 
Hi Alan

On Sat, 23 Dec 2023 at 21:01, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Fri, Dec 22, 2023 at 10:55:49PM +0000, Ricardo Ribalda wrote:
> > Logitech Rallybar devices, despite behaving as UVC camera, they have a
> > different power management system than the rest of the other Logitech
> > cameras.
> >
> > USB_QUIRK_RESET_RESUME causes undesired USB disconnects, that make the
> > device unusable.
> >
> > These are the only two devices that have this behavior, and we do not
> > have the list of devices that require USB_QUIRK_RESET_RESUME, so lets
> > create a new lit for them that un-apply the USB_QUIRK_RESET_RESUME
> > quirk.
> >
> > Fixes: e387ef5c47dd ("usb: Add USB_QUIRK_RESET_RESUME for all Logitech UVC webcams")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
>
> Would it make more sense to do this inside the uvc driver instead of
> creating a new single-purpose list in the core?

I can try to move it to the uvc driver. But maybe it is better to keep it here:

The same vid:pid also has other functionality, not only uvc: Sync
agent interface, UPD Interface, ADB interface.
If we apply the quirk to the uvc driver, and the uvc driver is not
loaded, the other functionality will still be broken....

I expect to see more devices from Logitech not needing the
RESET_RESUME quirk... so this list will eventually grow.

Setting/useting RESET_RESUME in two different locations, can make the
code difficult to follow.

What do you think?

Regards!
Greg KH Jan. 2, 2024, 1:34 p.m. UTC | #3 
On Tue, Jan 02, 2024 at 12:31:53PM +0100, Ricardo Ribalda wrote:
> Hi Alan
> 
> On Sat, 23 Dec 2023 at 21:01, Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Fri, Dec 22, 2023 at 10:55:49PM +0000, Ricardo Ribalda wrote:
> > > Logitech Rallybar devices, despite behaving as UVC camera, they have a
> > > different power management system than the rest of the other Logitech
> > > cameras.
> > >
> > > USB_QUIRK_RESET_RESUME causes undesired USB disconnects, that make the
> > > device unusable.
> > >
> > > These are the only two devices that have this behavior, and we do not
> > > have the list of devices that require USB_QUIRK_RESET_RESUME, so lets
> > > create a new lit for them that un-apply the USB_QUIRK_RESET_RESUME
> > > quirk.
> > >
> > > Fixes: e387ef5c47dd ("usb: Add USB_QUIRK_RESET_RESUME for all Logitech UVC webcams")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> >
> > Would it make more sense to do this inside the uvc driver instead of
> > creating a new single-purpose list in the core?
> 
> I can try to move it to the uvc driver. But maybe it is better to keep it here:
> 
> The same vid:pid also has other functionality, not only uvc: Sync
> agent interface, UPD Interface, ADB interface.
> If we apply the quirk to the uvc driver, and the uvc driver is not
> loaded, the other functionality will still be broken....
> 
> I expect to see more devices from Logitech not needing the
> RESET_RESUME quirk... so this list will eventually grow.
> 
> Setting/useting RESET_RESUME in two different locations, can make the
> code difficult to follow.
> 
> What do you think?

Try it in the specific driver first please.

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 15e9bd180a1d..8fa8de50e7f0 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -553,6 +553,14 @@  static const struct usb_device_id usb_interface_quirk_list[] = {
 	{ }  /* terminating entry must be last */
 };
 
+static const struct usb_device_id usb_interface_unsupported_quirk_list[] = {
+	/* Logitech Rallybar VC systems*/
+	{ USB_DEVICE(0x046d, 0x089b), .driver_info = USB_QUIRK_RESET_RESUME },
+	{ USB_DEVICE(0x046d, 0x08d3), .driver_info = USB_QUIRK_RESET_RESUME },
+
+	{ }  /* terminating entry must be last */
+};
+
 static const struct usb_device_id usb_amd_resume_quirk_list[] = {
 	/* Lenovo Mouse with Pixart controller */
 	{ USB_DEVICE(0x17ef, 0x602e), .driver_info = USB_QUIRK_RESET_RESUME },
@@ -718,6 +726,8 @@  void usb_detect_interface_quirks(struct usb_device *udev)
 	u32 quirks;
 
 	quirks = usb_detect_static_quirks(udev, usb_interface_quirk_list);
+	quirks &= ~usb_detect_static_quirks(udev,
+					usb_interface_unsupported_quirk_list);
 	if (quirks == 0)
 		return;