Re: [PATCH v2] usb: core: Add "quirks" parameter for usbcore
From: Greg KH
Date: Wed Dec 06 2017 - 09:10:01 EST
On Wed, Dec 06, 2017 at 06:26:21PM +0800, Kai-Heng Feng wrote:
> Trying quirks in usbcore needs to rebuild the driver or the entire
> kernel if it's builtin. It can save a lot of time if usbcore has similar
> ability like "usbhid.quirks=" and "usb-storage.quirks=".
>
> Rename the original quirk detection function to "static" as we introduce
> this new "dynamic" function.
>
> Now users can use "usbcore.quirks=" as short term workaround before the
> next kernel release.
>
> This is inspired by usbhid and usb-storage.
>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
> ---
> v2: use in-kernel tolower() function.
>
> Documentation/admin-guide/kernel-parameters.txt | 55 +++++++++++++
> drivers/usb/core/quirks.c | 100 ++++++++++++++++++++++--
> include/linux/usb/quirks.h | 2 +
> 3 files changed, 151 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 6571fbfdb2a1..d42fb278320b 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4302,6 +4302,61 @@
>
> usbcore.nousb [USB] Disable the USB subsystem
>
> + usbcore.quirks=
> + [USB] A list of quirks entries to supplement or
> + override the built-in usb core quirk list. List
> + entries are separated by commas. Each entry has
> + the form VID:PID:Flags where VID and PID are Vendor
> + and Product ID values (4-digit hex numbers) and
> + Flags is a set of characters, each corresponding
> + to a common usb core quirk flag as follows:
> + a = USB_QUIRK_STRING_FETCH_255 (string
> + descriptors must not be fetched using
> + a 255-byte read);
> + b = USB_QUIRK_RESET_RESUME (device can't resume
> + correctly so reset it instead);
> + c = USB_QUIRK_NO_SET_INTF (device can't handle
> + Set-Interface requests);
> + d = USB_QUIRK_CONFIG_INTF_STRINGS (device can't
> + handle its Configuration or Interface
> + strings);
> + e = USB_QUIRK_RESET (device can't be reset
> + (e.g morph devices), don't use reset);
> + f = USB_QUIRK_HONOR_BNUMINTERFACES (device has
> + more interface descriptions than the
> + bNumInterfaces count, and can't handle
> + talking to these interfaces);
> + g = USB_QUIRK_DELAY_INIT (device needs a pause
> + during initialization, after we read
> + the device descriptor);
> + h = USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL (For
> + high speed and super speed interrupt
> + endpoints, the USB 2.0 and USB 3.0 spec
> + require the interval in microframes (1
> + microframe = 125 microseconds) to be
> + calculated as interval = 2 ^
> + (bInterval-1).
> + Devices with this quirk report their
> + bInterval as the result of this
> + calculation instead of the exponent
> + variable used in the calculation);
> + i = USB_QUIRK_DEVICE_QUALIFIER (device can't
> + handle device_qualifier descriptor
> + requests);
> + j = USB_QUIRK_IGNORE_REMOTE_WAKEUP (device
> + generates spurious wakeup, ignore
> + remote wakeup capability);
> + k = USB_QUIRK_NO_LPM (device can't handle Link
> + Power Management);
> + l = USB_QUIRK_LINEAR_FRAME_INTR_BINTERVAL
> + (Device reports its bInterval as linear
> + frames instead of the USB 2.0
> + calculation);
> + m = USB_QUIRK_DISCONNECT_SUSPEND (Device needs
> + to be disconnected before suspend to
> + prevent spurious wakeup)
> + Example: quirks=0781:5580:bk,0a5c:5834:gij
> +
> usbhid.mousepoll=
> [USBHID] The interval which mice are to be polled at.
>
> diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
> index f1dbab6f798f..5471765765be 100644
> --- a/drivers/usb/core/quirks.c
> +++ b/drivers/usb/core/quirks.c
> @@ -11,6 +11,12 @@
> #include <linux/usb/hcd.h>
> #include "usb.h"
>
> +/* Quirks specified at module load time */
> +static char *quirks_param[MAX_USB_BOOT_QUIRKS];
> +module_param_array_named(quirks, quirks_param, charp, NULL, 0444);
So you can't modify this at runtime? Why not?
> +MODULE_PARM_DESC(quirks, "Add/modify USB quirks by specifying"
> + "quirks=vendorID:productID:quirks");
Don't break strings over multiple lines.
> +
> /* Lists of quirky USB devices, split in device quirks and interface quirks.
> * Device quirks are applied at the very beginning of the enumeration process,
> * right after reading the device descriptor. They can thus only match on device
> @@ -310,8 +316,8 @@ static int usb_amd_resume_quirk(struct usb_device *udev)
> return 0;
> }
>
> -static u32 __usb_detect_quirks(struct usb_device *udev,
> - const struct usb_device_id *id)
> +static u32 usb_detect_static_quirks(struct usb_device *udev,
> + const struct usb_device_id *id)
> {
> u32 quirks = 0;
>
> @@ -329,23 +335,105 @@ static u32 __usb_detect_quirks(struct usb_device *udev,
> return quirks;
> }
>
> +static u32 usb_detect_dynamic_quirks(struct usb_device *udev)
> +{
> + u16 dev_vid = le16_to_cpu(udev->descriptor.idVendor);
> + u16 dev_pid = le16_to_cpu(udev->descriptor.idProduct);
> + u16 quirk_vid, quirk_pid;
> + char quirks[32];
> + char *p;
> + u32 flags = 0;
> + int n, m;
> +
> + for (n = 0; n < MAX_USB_BOOT_QUIRKS && quirks_param[n]; n++) {
> + m = sscanf(quirks_param[n], "%hx:%hx:%s",
> + &quirk_vid, &quirk_pid, quirks);
> + if (m != 3)
> + pr_warn("Could not parse USB quirk module param %s\n",
> + quirks_param[n]);
dev_warn().
Also, you are going to complain about the inability to parse the quirks
for _EVERY_ USB device in the system? That feels really wrong. Parse
it once, and then use the parsed table when matching things up. Don't
parse the string each and every time a device is added, that's a
horrible waste of time (not like USB enumeration is fast, but really,
let's not make it worse for no reason.)
> +
> + if (quirk_vid == dev_vid && quirk_pid == dev_pid)
> + break;
> + }
> + if (n == MAX_USB_BOOT_QUIRKS || !quirks_param[n])
> + return 0;
> +
> + p = quirks;
> +
> + /* Collect the flags */
> + for (; *p; p++) {
> + switch (tolower(*p)) {
> + case 'a':
> + flags |= USB_QUIRK_STRING_FETCH_255;
> + break;
> + case 'b':
> + flags |= USB_QUIRK_RESET_RESUME;
> + break;
> + case 'c':
> + flags |= USB_QUIRK_NO_SET_INTF;
> + break;
> + case 'd':
> + flags |= USB_QUIRK_CONFIG_INTF_STRINGS;
> + break;
> + case 'e':
> + flags |= USB_QUIRK_RESET;
> + break;
> + case 'f':
> + flags |= USB_QUIRK_HONOR_BNUMINTERFACES;
> + break;
> + case 'g':
> + flags |= USB_QUIRK_DELAY_INIT;
> + break;
> + case 'h':
> + flags |= USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL;
> + break;
> + case 'i':
> + flags |= USB_QUIRK_DEVICE_QUALIFIER;
> + break;
> + case 'j':
> + flags |= USB_QUIRK_IGNORE_REMOTE_WAKEUP;
> + break;
> + case 'k':
> + flags |= USB_QUIRK_NO_LPM;
> + break;
> + case 'l':
> + flags |= USB_QUIRK_LINEAR_FRAME_INTR_BINTERVAL;
> + break;
> + case 'm':
> + flags |= USB_QUIRK_DISCONNECT_SUSPEND;
> + break;
> + /* Ignore unrecognized flag characters */
> + }
> + }
> +
> + return flags;
> +}
> +
> /*
> * Detect any quirks the device has, and do any housekeeping for it if needed.
> */
> void usb_detect_quirks(struct usb_device *udev)
> {
> - udev->quirks = __usb_detect_quirks(udev, usb_quirk_list);
> + udev->quirks = usb_detect_dynamic_quirks(udev);
> +
> + if (udev->quirks) {
> + dev_dbg(&udev->dev, "Dynamic USB quirks for this device: %x\n",
> + udev->quirks);
> + return;
> + }
> +
> + udev->quirks = usb_detect_static_quirks(udev, usb_quirk_list);
Did you just overwrite the dynamic with the static ones? It feels like
you just did :(
> --- a/include/linux/usb/quirks.h
> +++ b/include/linux/usb/quirks.h
> @@ -8,6 +8,8 @@
> #ifndef __LINUX_USB_QUIRKS_H
> #define __LINUX_USB_QUIRKS_H
>
> +#define MAX_USB_BOOT_QUIRKS 4
Why 4? And why in this .h file?
Also, Oliver's request is good, xor is a nice way to do things if at all
possible.
thanks,
greg k-h