* [PATCH v2] usb: core: Add "quirks" parameter for usbcore
@ 2017-12-06 10:26 ` Kai-Heng Feng
0 siblings, 0 replies; 6+ messages in thread
From: Kai-Heng Feng @ 2017-12-06 10:26 UTC (permalink / raw)
To: gregkh; +Cc: corbet, linux-usb, linux-doc, linux-kernel, Kai-Heng Feng
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@canonical.com>
---
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);
+MODULE_PARM_DESC(quirks, "Add/modify USB quirks by specifying"
+ "quirks=vendorID:productID:quirks");
+
/* 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]);
+
+ 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);
/*
* Pixart-based mice would trigger remote wakeup issue on AMD
* Yangtze chipset, so set them as RESET_RESUME flag.
*/
if (usb_amd_resume_quirk(udev))
- udev->quirks |= __usb_detect_quirks(udev,
+ udev->quirks |= usb_detect_static_quirks(udev,
usb_amd_resume_quirk_list);
if (udev->quirks)
- dev_dbg(&udev->dev, "USB quirks for this device: %x\n",
+ dev_dbg(&udev->dev, "Static USB quirks for this device: %x\n",
udev->quirks);
#ifdef CONFIG_USB_DEFAULT_PERSIST
@@ -362,7 +450,7 @@ void usb_detect_interface_quirks(struct usb_device *udev)
{
u32 quirks;
- quirks = __usb_detect_quirks(udev, usb_interface_quirk_list);
+ quirks = usb_detect_static_quirks(udev, usb_interface_quirk_list);
if (quirks == 0)
return;
diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h
index f1fcec2fd5f8..40254f2fafb2 100644
--- 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
+
/* string descriptors must not be fetched using a 255-byte read */
#define USB_QUIRK_STRING_FETCH_255 BIT(0)
--
2.14.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [v2] usb: core: Add "quirks" parameter for usbcore
@ 2017-12-06 10:26 ` Kai-Heng Feng
0 siblings, 0 replies; 6+ messages in thread
From: Kai-Heng Feng @ 2017-12-06 10:26 UTC (permalink / raw)
To: gregkh; +Cc: corbet, linux-usb, linux-doc, linux-kernel, Kai-Heng Feng
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@canonical.com>
---
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);
+MODULE_PARM_DESC(quirks, "Add/modify USB quirks by specifying"
+ "quirks=vendorID:productID:quirks");
+
/* 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]);
+
+ 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);
/*
* Pixart-based mice would trigger remote wakeup issue on AMD
* Yangtze chipset, so set them as RESET_RESUME flag.
*/
if (usb_amd_resume_quirk(udev))
- udev->quirks |= __usb_detect_quirks(udev,
+ udev->quirks |= usb_detect_static_quirks(udev,
usb_amd_resume_quirk_list);
if (udev->quirks)
- dev_dbg(&udev->dev, "USB quirks for this device: %x\n",
+ dev_dbg(&udev->dev, "Static USB quirks for this device: %x\n",
udev->quirks);
#ifdef CONFIG_USB_DEFAULT_PERSIST
@@ -362,7 +450,7 @@ void usb_detect_interface_quirks(struct usb_device *udev)
{
u32 quirks;
- quirks = __usb_detect_quirks(udev, usb_interface_quirk_list);
+ quirks = usb_detect_static_quirks(udev, usb_interface_quirk_list);
if (quirks == 0)
return;
diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h
index f1fcec2fd5f8..40254f2fafb2 100644
--- 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
+
/* string descriptors must not be fetched using a 255-byte read */
#define USB_QUIRK_STRING_FETCH_255 BIT(0)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] usb: core: Add "quirks" parameter for usbcore
@ 2017-12-06 14:10 ` Greg Kroah-Hartman
0 siblings, 0 replies; 6+ messages in thread
From: Greg KH @ 2017-12-06 14:10 UTC (permalink / raw)
To: Kai-Heng Feng; +Cc: corbet, linux-usb, linux-doc, linux-kernel
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@canonical.com>
> ---
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [v2] usb: core: Add "quirks" parameter for usbcore
@ 2017-12-06 14:10 ` Greg Kroah-Hartman
0 siblings, 0 replies; 6+ messages in thread
From: Greg Kroah-Hartman @ 2017-12-06 14:10 UTC (permalink / raw)
To: Kai-Heng Feng; +Cc: corbet, linux-usb, linux-doc, linux-kernel
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@canonical.com>
> ---
> 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
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] usb: core: Add "quirks" parameter for usbcore
@ 2017-12-07 7:06 ` Kai-Heng Feng
0 siblings, 0 replies; 6+ messages in thread
From: Kai Heng Feng @ 2017-12-07 7:06 UTC (permalink / raw)
To: Greg KH; +Cc: corbet, linux-usb, linux-doc, linux-kernel
> On 6 Dec 2017, at 10:10 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> 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@canonical.com>
>> ---
>> 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?
Sure, I think make it runtime tunable is a good idea.
>
>> +MODULE_PARM_DESC(quirks, "Add/modify USB quirks by specifying"
>> + "quirks=vendorID:productID:quirks");
>
> Don't break strings over multiple lines.
Will fix in next version.
>
>> +
>> /* 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.)
Makes sense. Will improve this in next version.
>
>> +
>> + 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 :(
That’s my original intention. I want to let users can override quirk, in
case the quirk somehow regressed their device.
>
>> --- 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?
It’s from usbhid. I think I’ll make it 16 next version.
I’ll put it in the .c file.
>
> Also, Oliver's request is good, xor is a nice way to do things if at all
> possible.
Yea, I think Oliver’s suggestion is great. Will do in next version.
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* [v2] usb: core: Add "quirks" parameter for usbcore
@ 2017-12-07 7:06 ` Kai-Heng Feng
0 siblings, 0 replies; 6+ messages in thread
From: Kai-Heng Feng @ 2017-12-07 7:06 UTC (permalink / raw)
To: Greg KH; +Cc: corbet, linux-usb, linux-doc, linux-kernel
> On 6 Dec 2017, at 10:10 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> 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@canonical.com>
>> ---
>> 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?
Sure, I think make it runtime tunable is a good idea.
>
>> +MODULE_PARM_DESC(quirks, "Add/modify USB quirks by specifying"
>> + "quirks=vendorID:productID:quirks");
>
> Don't break strings over multiple lines.
Will fix in next version.
>
>> +
>> /* 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.)
Makes sense. Will improve this in next version.
>
>> +
>> + 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 :(
That’s my original intention. I want to let users can override quirk, in
case the quirk somehow regressed their device.
>
>> --- 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?
It’s from usbhid. I think I’ll make it 16 next version.
I’ll put it in the .c file.
>
> Also, Oliver's request is good, xor is a nice way to do things if at all
> possible.
Yea, I think Oliver’s suggestion is great. Will do in next version.
>
> thanks,
>
> greg k-h
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-12-07 7:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-06 10:26 [PATCH v2] usb: core: Add "quirks" parameter for usbcore Kai-Heng Feng
2017-12-06 10:26 ` [v2] " Kai-Heng Feng
2017-12-06 14:10 ` [PATCH v2] " Greg KH
2017-12-06 14:10 ` [v2] " Greg Kroah-Hartman
2017-12-07 7:06 ` [PATCH v2] " Kai Heng Feng
2017-12-07 7:06 ` [v2] " Kai-Heng Feng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.