2024-01-08 08:18:08

by Ricardo Ribalda

[permalink] [raw]
Subject: [PATCH v4] media: ucvideo: Add quirk for Logitech Rally Bar

Logitech Rally Bar devices, despite behaving as UVC cameras, have a
different power management system that the other cameras from Logitech.

USB_QUIRK_RESET_RESUME is applied to all the UVC cameras from Logitech
at the usb core. Unfortunately, USB_QUIRK_RESET_RESUME causes undesired
USB disconnects, that make them completely unusable.

Instead of creating a list for this family of devices in the core, let's
create a quirk in the UVC driver.

Fixes: e387ef5c47dd ("usb: Add USB_QUIRK_RESET_RESUME for all Logitech UVC webcams")
Cc: [email protected]
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Alan Stern <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Ricardo Ribalda <[email protected]>
---
Tested with a Rallybar Mini with an Acer Chromebook Spin 513
---
Changes in v4:
- Include Logi Rally Bar Huddle (Thanks Kyle!)
- Link to v3: https://lore.kernel.org/r/[email protected]

Changes in v3:
- Move quirk to uvc driver
- Link to v2: https://lore.kernel.org/r/[email protected]

Changes in v2:
- Add Fixes tag
- Add UVC maintainer as Cc
- Link to v1: https://lore.kernel.org/r/[email protected]
---
drivers/media/usb/uvc/uvc_driver.c | 30 ++++++++++++++++++++++++++++++
drivers/media/usb/uvc/uvcvideo.h | 1 +
2 files changed, 31 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 08fcd2ffa727..9663bcac6843 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -14,6 +14,7 @@
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/usb.h>
+#include <linux/usb/quirks.h>
#include <linux/usb/uvc.h>
#include <linux/videodev2.h>
#include <linux/vmalloc.h>
@@ -2233,6 +2234,8 @@ static int uvc_probe(struct usb_interface *intf,
}

uvc_dbg(dev, PROBE, "UVC device initialized\n");
+ if (dev->quirks & UVC_QUIRK_FORCE_RESUME)
+ udev->quirks &= ~USB_QUIRK_RESET_RESUME;
usb_enable_autosuspend(udev);
return 0;

@@ -2574,6 +2577,33 @@ static const struct usb_device_id uvc_ids[] = {
.bInterfaceSubClass = 1,
.bInterfaceProtocol = 0,
.driver_info = UVC_INFO_QUIRK(UVC_QUIRK_RESTORE_CTRLS_ON_INIT) },
+ /* Logitech Rally Bar Huddle */
+ { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
+ | USB_DEVICE_ID_MATCH_INT_INFO,
+ .idVendor = 0x046d,
+ .idProduct = 0x087c,
+ .bInterfaceClass = USB_CLASS_VIDEO,
+ .bInterfaceSubClass = 1,
+ .bInterfaceProtocol = 0,
+ .driver_info = UVC_INFO_QUIRK(UVC_QUIRK_FORCE_RESUME) },
+ /* Logitech Rally Bar */
+ { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
+ | USB_DEVICE_ID_MATCH_INT_INFO,
+ .idVendor = 0x046d,
+ .idProduct = 0x089b,
+ .bInterfaceClass = USB_CLASS_VIDEO,
+ .bInterfaceSubClass = 1,
+ .bInterfaceProtocol = 0,
+ .driver_info = UVC_INFO_QUIRK(UVC_QUIRK_FORCE_RESUME) },
+ /* Logitech Rally Bar Mini */
+ { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
+ | USB_DEVICE_ID_MATCH_INT_INFO,
+ .idVendor = 0x046d,
+ .idProduct = 0x08d3,
+ .bInterfaceClass = USB_CLASS_VIDEO,
+ .bInterfaceSubClass = 1,
+ .bInterfaceProtocol = 0,
+ .driver_info = UVC_INFO_QUIRK(UVC_QUIRK_FORCE_RESUME) },
/* Chicony CNF7129 (Asus EEE 100HE) */
{ .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 6fb0a78b1b00..fa59a21d2a28 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -73,6 +73,7 @@
#define UVC_QUIRK_FORCE_Y8 0x00000800
#define UVC_QUIRK_FORCE_BPP 0x00001000
#define UVC_QUIRK_WAKE_AUTOSUSPEND 0x00002000
+#define UVC_QUIRK_FORCE_RESUME 0x00004000

/* Format flags */
#define UVC_FMT_FLAG_COMPRESSED 0x00000001

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

Best regards,
--
Ricardo Ribalda <[email protected]>



2024-01-09 12:49:03

by Sergey Senozhatsky

[permalink] [raw]
Subject: Re: [PATCH v4] media: ucvideo: Add quirk for Logitech Rally Bar

On (24/01/08 08:17), Ricardo Ribalda wrote:
> Logitech Rally Bar devices, despite behaving as UVC cameras, have a
> different power management system that the other cameras from Logitech.
>
> USB_QUIRK_RESET_RESUME is applied to all the UVC cameras from Logitech
> at the usb core. Unfortunately, USB_QUIRK_RESET_RESUME causes undesired
> USB disconnects, that make them completely unusable.
>
> Instead of creating a list for this family of devices in the core, let's
> create a quirk in the UVC driver.
>
> Fixes: e387ef5c47dd ("usb: Add USB_QUIRK_RESET_RESUME for all Logitech UVC webcams")
> Cc: [email protected]
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Alan Stern <[email protected]>
> Acked-by: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Ricardo Ribalda <[email protected]>

Reviewed-by: Sergey Senozhatsky <[email protected]>

2024-02-04 10:53:28

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v4] media: ucvideo: Add quirk for Logitech Rally Bar

Hi Ricardo,

Thank you for the patch.

On Mon, Jan 08, 2024 at 08:17:46AM +0000, Ricardo Ribalda wrote:
> Logitech Rally Bar devices, despite behaving as UVC cameras, have a
> different power management system that the other cameras from Logitech.
>
> USB_QUIRK_RESET_RESUME is applied to all the UVC cameras from Logitech
> at the usb core. Unfortunately, USB_QUIRK_RESET_RESUME causes undesired
> USB disconnects, that make them completely unusable.
>
> Instead of creating a list for this family of devices in the core, let's
> create a quirk in the UVC driver.
>
> Fixes: e387ef5c47dd ("usb: Add USB_QUIRK_RESET_RESUME for all Logitech UVC webcams")
> Cc: [email protected]
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Alan Stern <[email protected]>
> Acked-by: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Ricardo Ribalda <[email protected]>
> ---
> Tested with a Rallybar Mini with an Acer Chromebook Spin 513
> ---
> Changes in v4:
> - Include Logi Rally Bar Huddle (Thanks Kyle!)
> - Link to v3: https://lore.kernel.org/r/[email protected]
>
> Changes in v3:
> - Move quirk to uvc driver
> - Link to v2: https://lore.kernel.org/r/[email protected]
>
> Changes in v2:
> - Add Fixes tag
> - Add UVC maintainer as Cc
> - Link to v1: https://lore.kernel.org/r/[email protected]
> ---
> drivers/media/usb/uvc/uvc_driver.c | 30 ++++++++++++++++++++++++++++++
> drivers/media/usb/uvc/uvcvideo.h | 1 +
> 2 files changed, 31 insertions(+)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 08fcd2ffa727..9663bcac6843 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -14,6 +14,7 @@
> #include <linux/module.h>
> #include <linux/slab.h>
> #include <linux/usb.h>
> +#include <linux/usb/quirks.h>
> #include <linux/usb/uvc.h>
> #include <linux/videodev2.h>
> #include <linux/vmalloc.h>
> @@ -2233,6 +2234,8 @@ static int uvc_probe(struct usb_interface *intf,
> }
>
> uvc_dbg(dev, PROBE, "UVC device initialized\n");
> + if (dev->quirks & UVC_QUIRK_FORCE_RESUME)
> + udev->quirks &= ~USB_QUIRK_RESET_RESUME;

If you don't mind, I'll move this above the debug message.

> usb_enable_autosuspend(udev);
> return 0;
>
> @@ -2574,6 +2577,33 @@ static const struct usb_device_id uvc_ids[] = {
> .bInterfaceSubClass = 1,
> .bInterfaceProtocol = 0,
> .driver_info = UVC_INFO_QUIRK(UVC_QUIRK_RESTORE_CTRLS_ON_INIT) },
> + /* Logitech Rally Bar Huddle */
> + { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
> + | USB_DEVICE_ID_MATCH_INT_INFO,
> + .idVendor = 0x046d,
> + .idProduct = 0x087c,
> + .bInterfaceClass = USB_CLASS_VIDEO,
> + .bInterfaceSubClass = 1,
> + .bInterfaceProtocol = 0,
> + .driver_info = UVC_INFO_QUIRK(UVC_QUIRK_FORCE_RESUME) },
> + /* Logitech Rally Bar */
> + { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
> + | USB_DEVICE_ID_MATCH_INT_INFO,
> + .idVendor = 0x046d,
> + .idProduct = 0x089b,
> + .bInterfaceClass = USB_CLASS_VIDEO,
> + .bInterfaceSubClass = 1,
> + .bInterfaceProtocol = 0,
> + .driver_info = UVC_INFO_QUIRK(UVC_QUIRK_FORCE_RESUME) },
> + /* Logitech Rally Bar Mini */
> + { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
> + | USB_DEVICE_ID_MATCH_INT_INFO,
> + .idVendor = 0x046d,
> + .idProduct = 0x08d3,
> + .bInterfaceClass = USB_CLASS_VIDEO,
> + .bInterfaceSubClass = 1,
> + .bInterfaceProtocol = 0,
> + .driver_info = UVC_INFO_QUIRK(UVC_QUIRK_FORCE_RESUME) },

In an ideal world we would get a list of all devices that require the
reset resume quirk from Logitech, and restrict the quirk to those
devices only in the USB core. In the real world, this seems fine for
now, we'll worry about this approach not scaling when we'll need to make
it scale.

> /* Chicony CNF7129 (Asus EEE 100HE) */
> { .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 6fb0a78b1b00..fa59a21d2a28 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -73,6 +73,7 @@
> #define UVC_QUIRK_FORCE_Y8 0x00000800
> #define UVC_QUIRK_FORCE_BPP 0x00001000
> #define UVC_QUIRK_WAKE_AUTOSUSPEND 0x00002000
> +#define UVC_QUIRK_FORCE_RESUME 0x00004000

Let's name this UVC_QUICK_NO_RESET_RESUME, as that's what the quirk
does.

I'll make these small changes when applying, no need to resend.

Reviewed-by: Laurent Pinchart <[email protected]>

>
> /* Format flags */
> #define UVC_FMT_FLAG_COMPRESSED 0x00000001
>

--
Regards,

Laurent Pinchart

2024-02-12 12:23:04

by Oliver Neukum

[permalink] [raw]
Subject: Re: [PATCH v4] media: ucvideo: Add quirk for Logitech Rally Bar

On 04.02.24 11:52, Laurent Pinchart wrote:
> Hi Ricardo,
>
> Thank you for the patch.

Hi,

sorry for commenting on this late, but this patch has
a fundamental issue. In fact this issue is the reason the
handling for quirks is in usbcore at all.

If you leave the setting/clearing of this flag to a driver you
are introducing a race condition. The driver may or may not be
present at the time a device is enumerated. And you have
no idea how long the autosuspend delay is on a system
and what its default policy is regarding suspending
devices.
That means that a device can have been suspended and
resumed before it is probed. On a device that needs
RESET_RESUME, we are in trouble.
The inverse issue will arise if a device does not react
well to RESET_RESUME. You cannot rule out that a device
that must not be reset will be reset.

I am sorry, but it seems to me that the exceptions need
to go into usbcore.

Regards
Oliver

2024-02-12 19:05:03

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCH v4] media: ucvideo: Add quirk for Logitech Rally Bar

On Mon, Feb 12, 2024 at 01:22:42PM +0100, Oliver Neukum wrote:
> On 04.02.24 11:52, Laurent Pinchart wrote:
> > Hi Ricardo,
> >
> > Thank you for the patch.
>
> Hi,
>
> sorry for commenting on this late, but this patch has
> a fundamental issue. In fact this issue is the reason the
> handling for quirks is in usbcore at all.
>
> If you leave the setting/clearing of this flag to a driver you
> are introducing a race condition. The driver may or may not be
> present at the time a device is enumerated. And you have
> no idea how long the autosuspend delay is on a system
> and what its default policy is regarding suspending
> devices.
> That means that a device can have been suspended and
> resumed before it is probed. On a device that needs
> RESET_RESUME, we are in trouble.

Not necessarily. If the driver knows that one of these devices may
already have been suspend and resumed, it can issue its own preemptive
reset at probe time.

> The inverse issue will arise if a device does not react
> well to RESET_RESUME. You cannot rule out that a device
> that must not be reset will be reset.

That's a separate issue, with its own list of potential problems.

> I am sorry, but it seems to me that the exceptions need
> to go into usbcore.

If we do then we may want to come up with a better scheme for seeing
which devices need to have a quirk flag set. A static listing probably
won't be good enough; the decision may have to be made dynamically.

Alan Stern

2024-02-13 10:47:40

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v4] media: ucvideo: Add quirk for Logitech Rally Bar

On Mon, Feb 12, 2024 at 02:04:31PM -0500, Alan Stern wrote:
> On Mon, Feb 12, 2024 at 01:22:42PM +0100, Oliver Neukum wrote:
> > On 04.02.24 11:52, Laurent Pinchart wrote:
> > > Hi Ricardo,
> > >
> > > Thank you for the patch.
> >
> > Hi,
> >
> > sorry for commenting on this late, but this patch has
> > a fundamental issue. In fact this issue is the reason the
> > handling for quirks is in usbcore at all.
> >
> > If you leave the setting/clearing of this flag to a driver you
> > are introducing a race condition. The driver may or may not be
> > present at the time a device is enumerated. And you have
> > no idea how long the autosuspend delay is on a system
> > and what its default policy is regarding suspending
> > devices.
> > That means that a device can have been suspended and
> > resumed before it is probed. On a device that needs
> > RESET_RESUME, we are in trouble.
>
> Not necessarily. If the driver knows that one of these devices may
> already have been suspend and resumed, it can issue its own preemptive
> reset at probe time.
>
> > The inverse issue will arise if a device does not react
> > well to RESET_RESUME. You cannot rule out that a device
> > that must not be reset will be reset.
>
> That's a separate issue, with its own list of potential problems.
>
> > I am sorry, but it seems to me that the exceptions need
> > to go into usbcore.
>
> If we do then we may want to come up with a better scheme for seeing
> which devices need to have a quirk flag set. A static listing probably
> won't be good enough; the decision may have to be made dynamically.

I don't mind either way personally. Oliver, could you try to find a good
solution with Ricardo ? I'll merge the outcome.

--
Regards,

Laurent Pinchart

2024-02-19 15:14:13

by Ricardo Ribalda

[permalink] [raw]
Subject: Re: [PATCH v4] media: ucvideo: Add quirk for Logitech Rally Bar

Hi Oliver

Would you prefer a version like this?

https://lore.kernel.org/all/[email protected]/

If so I can re-submit a version with the 3 vid/pids. Alan, would you
be happy with that?

Regards!

On Tue, 13 Feb 2024 at 11:47, Laurent Pinchart
<[email protected]> wrote:
>
> On Mon, Feb 12, 2024 at 02:04:31PM -0500, Alan Stern wrote:
> > On Mon, Feb 12, 2024 at 01:22:42PM +0100, Oliver Neukum wrote:
> > > On 04.02.24 11:52, Laurent Pinchart wrote:
> > > > Hi Ricardo,
> > > >
> > > > Thank you for the patch.
> > >
> > > Hi,
> > >
> > > sorry for commenting on this late, but this patch has
> > > a fundamental issue. In fact this issue is the reason the
> > > handling for quirks is in usbcore at all.
> > >
> > > If you leave the setting/clearing of this flag to a driver you
> > > are introducing a race condition. The driver may or may not be
> > > present at the time a device is enumerated. And you have
> > > no idea how long the autosuspend delay is on a system
> > > and what its default policy is regarding suspending
> > > devices.
> > > That means that a device can have been suspended and
> > > resumed before it is probed. On a device that needs
> > > RESET_RESUME, we are in trouble.
> >
> > Not necessarily. If the driver knows that one of these devices may
> > already have been suspend and resumed, it can issue its own preemptive
> > reset at probe time.
> >
> > > The inverse issue will arise if a device does not react
> > > well to RESET_RESUME. You cannot rule out that a device
> > > that must not be reset will be reset.
> >
> > That's a separate issue, with its own list of potential problems.
> >
> > > I am sorry, but it seems to me that the exceptions need
> > > to go into usbcore.
> >
> > If we do then we may want to come up with a better scheme for seeing
> > which devices need to have a quirk flag set. A static listing probably
> > won't be good enough; the decision may have to be made dynamically.
>
> I don't mind either way personally. Oliver, could you try to find a good
> solution with Ricardo ? I'll merge the outcome.
>
> --
> Regards,
>
> Laurent Pinchart



--
Ricardo Ribalda

2024-02-29 17:02:21

by Ricardo Ribalda

[permalink] [raw]
Subject: Re: [PATCH v4] media: ucvideo: Add quirk for Logitech Rally Bar

Oliver, friendly ping

On Mon, 19 Feb 2024 at 16:13, Ricardo Ribalda <[email protected]> wrote:
>
> Hi Oliver
>
> Would you prefer a version like this?
>
> https://lore.kernel.org/all/[email protected]/
>
> If so I can re-submit a version with the 3 vid/pids. Alan, would you
> be happy with that?
>
> Regards!
>
> On Tue, 13 Feb 2024 at 11:47, Laurent Pinchart
> <[email protected]> wrote:
> >
> > On Mon, Feb 12, 2024 at 02:04:31PM -0500, Alan Stern wrote:
> > > On Mon, Feb 12, 2024 at 01:22:42PM +0100, Oliver Neukum wrote:
> > > > On 04.02.24 11:52, Laurent Pinchart wrote:
> > > > > Hi Ricardo,
> > > > >
> > > > > Thank you for the patch.
> > > >
> > > > Hi,
> > > >
> > > > sorry for commenting on this late, but this patch has
> > > > a fundamental issue. In fact this issue is the reason the
> > > > handling for quirks is in usbcore at all.
> > > >
> > > > If you leave the setting/clearing of this flag to a driver you
> > > > are introducing a race condition. The driver may or may not be
> > > > present at the time a device is enumerated. And you have
> > > > no idea how long the autosuspend delay is on a system
> > > > and what its default policy is regarding suspending
> > > > devices.
> > > > That means that a device can have been suspended and
> > > > resumed before it is probed. On a device that needs
> > > > RESET_RESUME, we are in trouble.
> > >
> > > Not necessarily. If the driver knows that one of these devices may
> > > already have been suspend and resumed, it can issue its own preemptive
> > > reset at probe time.
> > >
> > > > The inverse issue will arise if a device does not react
> > > > well to RESET_RESUME. You cannot rule out that a device
> > > > that must not be reset will be reset.
> > >
> > > That's a separate issue, with its own list of potential problems.
> > >
> > > > I am sorry, but it seems to me that the exceptions need
> > > > to go into usbcore.
> > >
> > > If we do then we may want to come up with a better scheme for seeing
> > > which devices need to have a quirk flag set. A static listing probably
> > > won't be good enough; the decision may have to be made dynamically.
> >
> > I don't mind either way personally. Oliver, could you try to find a good
> > solution with Ricardo ? I'll merge the outcome.
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>
>
>
> --
> Ricardo Ribalda



--
Ricardo Ribalda

2024-03-22 12:02:21

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v4] media: ucvideo: Add quirk for Logitech Rally Bar

On Thu, Feb 29, 2024 at 05:57:38PM +0100, Ricardo Ribalda wrote:
> Oliver, friendly ping

Seconded :-) We can help with the implementation, but we would like your
guidance on the direction you think this should take.

> On Mon, 19 Feb 2024 at 16:13, Ricardo Ribalda wrote:
> >
> > Hi Oliver
> >
> > Would you prefer a version like this?
> >
> > https://lore.kernel.org/all/[email protected]/
> >
> > If so I can re-submit a version with the 3 vid/pids. Alan, would you
> > be happy with that?
> >
> > Regards!
> >
> > On Tue, 13 Feb 2024 at 11:47, Laurent Pinchart wrote:
> > > On Mon, Feb 12, 2024 at 02:04:31PM -0500, Alan Stern wrote:
> > > > On Mon, Feb 12, 2024 at 01:22:42PM +0100, Oliver Neukum wrote:
> > > > > On 04.02.24 11:52, Laurent Pinchart wrote:
> > > > > > Hi Ricardo,
> > > > > >
> > > > > > Thank you for the patch.
> > > > >
> > > > > Hi,
> > > > >
> > > > > sorry for commenting on this late, but this patch has
> > > > > a fundamental issue. In fact this issue is the reason the
> > > > > handling for quirks is in usbcore at all.
> > > > >
> > > > > If you leave the setting/clearing of this flag to a driver you
> > > > > are introducing a race condition. The driver may or may not be
> > > > > present at the time a device is enumerated. And you have
> > > > > no idea how long the autosuspend delay is on a system
> > > > > and what its default policy is regarding suspending
> > > > > devices.
> > > > > That means that a device can have been suspended and
> > > > > resumed before it is probed. On a device that needs
> > > > > RESET_RESUME, we are in trouble.
> > > >
> > > > Not necessarily. If the driver knows that one of these devices may
> > > > already have been suspend and resumed, it can issue its own preemptive
> > > > reset at probe time.
> > > >
> > > > > The inverse issue will arise if a device does not react
> > > > > well to RESET_RESUME. You cannot rule out that a device
> > > > > that must not be reset will be reset.
> > > >
> > > > That's a separate issue, with its own list of potential problems.
> > > >
> > > > > I am sorry, but it seems to me that the exceptions need
> > > > > to go into usbcore.
> > > >
> > > > If we do then we may want to come up with a better scheme for seeing
> > > > which devices need to have a quirk flag set. A static listing probably
> > > > won't be good enough; the decision may have to be made dynamically.
> > >
> > > I don't mind either way personally. Oliver, could you try to find a good
> > > solution with Ricardo ? I'll merge the outcome.

--
Regards,

Laurent Pinchart

2024-03-22 14:41:04

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCH v4] media: ucvideo: Add quirk for Logitech Rally Bar

On Fri, Mar 22, 2024 at 01:57:34PM +0200, Laurent Pinchart wrote:
> On Thu, Feb 29, 2024 at 05:57:38PM +0100, Ricardo Ribalda wrote:
> > Oliver, friendly ping
>
> Seconded :-) We can help with the implementation, but we would like your
> guidance on the direction you think this should take.

Thirded :-) I want to hear what Oliver thinks about the original
patch.

Alan Stern

> > On Mon, 19 Feb 2024 at 16:13, Ricardo Ribalda wrote:
> > >
> > > Hi Oliver
> > >
> > > Would you prefer a version like this?
> > >
> > > https://lore.kernel.org/all/[email protected]/
> > >
> > > If so I can re-submit a version with the 3 vid/pids. Alan, would you
> > > be happy with that?
> > >
> > > Regards!
> > >
> > > On Tue, 13 Feb 2024 at 11:47, Laurent Pinchart wrote:
> > > > On Mon, Feb 12, 2024 at 02:04:31PM -0500, Alan Stern wrote:
> > > > > On Mon, Feb 12, 2024 at 01:22:42PM +0100, Oliver Neukum wrote:
> > > > > > On 04.02.24 11:52, Laurent Pinchart wrote:
> > > > > > > Hi Ricardo,
> > > > > > >
> > > > > > > Thank you for the patch.
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > sorry for commenting on this late, but this patch has
> > > > > > a fundamental issue. In fact this issue is the reason the
> > > > > > handling for quirks is in usbcore at all.
> > > > > >
> > > > > > If you leave the setting/clearing of this flag to a driver you
> > > > > > are introducing a race condition. The driver may or may not be
> > > > > > present at the time a device is enumerated. And you have
> > > > > > no idea how long the autosuspend delay is on a system
> > > > > > and what its default policy is regarding suspending
> > > > > > devices.
> > > > > > That means that a device can have been suspended and
> > > > > > resumed before it is probed. On a device that needs
> > > > > > RESET_RESUME, we are in trouble.
> > > > >
> > > > > Not necessarily. If the driver knows that one of these devices may
> > > > > already have been suspend and resumed, it can issue its own preemptive
> > > > > reset at probe time.
> > > > >
> > > > > > The inverse issue will arise if a device does not react
> > > > > > well to RESET_RESUME. You cannot rule out that a device
> > > > > > that must not be reset will be reset.
> > > > >
> > > > > That's a separate issue, with its own list of potential problems.
> > > > >
> > > > > > I am sorry, but it seems to me that the exceptions need
> > > > > > to go into usbcore.
> > > > >
> > > > > If we do then we may want to come up with a better scheme for seeing
> > > > > which devices need to have a quirk flag set. A static listing probably
> > > > > won't be good enough; the decision may have to be made dynamically.
> > > >
> > > > I don't mind either way personally. Oliver, could you try to find a good
> > > > solution with Ricardo ? I'll merge the outcome.
>
> --
> Regards,
>
> Laurent Pinchart