[Bionic] [PATCH 0/3] Backport USB core quirks

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[Bionic] [PATCH 0/3] Backport USB core quirks

Kai-Heng Feng
BugLink: https://bugs.launchpad.net/bugs/1762695

[Impact]
Currently we need to recompile the entire kernel if we want try quirks
in usbcore.

[Test]
Use kernel parameter "usbcore.quirks=" in conjunction with dynamic debug
for usbcore, we can see that usb quirks successfully enabled for USB
devices dynamically.

[Fix]
Introduce a new module parameter, "usbcore.quirks=". Users now can try
different quirks without compiling anything.

[Regression Potential]
Low. It doesn't change any behavior of usb driver per se, it simply adds
new logic to detect quirks.

Kai-Heng Feng (3):
  usb: core: Add "quirks" parameter for usbcore
  usb: core: Copy parameter string correctly and remove superfluous null
    check
  usb: core: Add USB_QUIRK_DELAY_CTRL_MSG to usbcore quirks

 Documentation/admin-guide/kernel-parameters.txt |  58 ++++++++
 drivers/usb/core/quirks.c                       | 186 +++++++++++++++++++++++-
 drivers/usb/core/usb.c                          |   1 +
 drivers/usb/core/usb.h                          |   1 +
 4 files changed, 241 insertions(+), 5 deletions(-)

--
2.15.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[Bionic] [PATCH 1/3] usb: core: Add "quirks" parameter for usbcore

Kai-Heng Feng
BugLink: https://bugs.launchpad.net/bugs/1762695

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. Also, the quirk parameter can XOR the builtin
quirks for debugging purpose.

This is inspired by usbhid and usb-storage.

Signed-off-by: Kai-Heng Feng <[hidden email]>
Signed-off-by: Greg Kroah-Hartman <[hidden email]>
(cherry picked from commit 027bd6cafd9a1e3a109b5e5682c85ac84e804a8d)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 Documentation/admin-guide/kernel-parameters.txt |  56 ++++++++
 drivers/usb/core/quirks.c                       | 178 +++++++++++++++++++++++-
 drivers/usb/core/usb.c                          |   1 +
 drivers/usb/core/usb.h                          |   1 +
 4 files changed, 231 insertions(+), 5 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 27ca3fbc47aa..969f48683313 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4354,6 +4354,62 @@
 
  usbcore.nousb [USB] Disable the USB subsystem
 
+ usbcore.quirks=
+ [USB] A list of quirk entries to augment the built-in
+ usb core quirk list. List entries are separated by
+ commas. Each entry has the form
+ VendorID:ProductID:Flags. The IDs are 4-digit hex
+ numbers and Flags is a set of letters. Each letter
+ will change the built-in quirk; setting it if it is
+ clear and clearing it if it is set. The letters have
+ the following meanings:
+ 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 f4a548471f0f..11d943a6b7cb 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -6,11 +6,149 @@
  * Copyright (c) 2007 Greg Kroah-Hartman <[hidden email]>
  */
 
+#include <linux/moduleparam.h>
 #include <linux/usb.h>
 #include <linux/usb/quirks.h>
 #include <linux/usb/hcd.h>
 #include "usb.h"
 
+struct quirk_entry {
+ u16 vid;
+ u16 pid;
+ u32 flags;
+};
+
+static DEFINE_MUTEX(quirk_mutex);
+
+static struct quirk_entry *quirk_list;
+static unsigned int quirk_count;
+
+static char quirks_param[128];
+
+static int quirks_param_set(const char *val, const struct kernel_param *kp)
+{
+ char *p, *field;
+ u16 vid, pid;
+ u32 flags;
+ size_t i;
+
+ mutex_lock(&quirk_mutex);
+
+ if (!val || !*val) {
+ quirk_count = 0;
+ kfree(quirk_list);
+ quirk_list = NULL;
+ goto unlock;
+ }
+
+ for (quirk_count = 1, i = 0; val[i]; i++)
+ if (val[i] == ',')
+ quirk_count++;
+
+ if (quirk_list) {
+ kfree(quirk_list);
+ quirk_list = NULL;
+ }
+
+ quirk_list = kcalloc(quirk_count, sizeof(struct quirk_entry),
+     GFP_KERNEL);
+ if (!quirk_list) {
+ mutex_unlock(&quirk_mutex);
+ return -ENOMEM;
+ }
+
+ for (i = 0, p = (char *)val; p && *p;) {
+ /* Each entry consists of VID:PID:flags */
+ field = strsep(&p, ":");
+ if (!field)
+ break;
+
+ if (kstrtou16(field, 16, &vid))
+ break;
+
+ field = strsep(&p, ":");
+ if (!field)
+ break;
+
+ if (kstrtou16(field, 16, &pid))
+ break;
+
+ field = strsep(&p, ",");
+ if (!field || !*field)
+ break;
+
+ /* Collect the flags */
+ for (flags = 0; *field; field++) {
+ switch (*field) {
+ 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 */
+ }
+ }
+
+ quirk_list[i++] = (struct quirk_entry)
+ { .vid = vid, .pid = pid, .flags = flags };
+ }
+
+ if (i < quirk_count)
+ quirk_count = i;
+
+unlock:
+ mutex_unlock(&quirk_mutex);
+
+ return param_set_copystring(val, kp);
+}
+
+static const struct kernel_param_ops quirks_param_ops = {
+ .set = quirks_param_set,
+ .get = param_get_string,
+};
+
+static struct kparam_string quirks_param_string = {
+ .maxlen = sizeof(quirks_param),
+ .string = quirks_param,
+};
+
+module_param_cb(quirks, &quirks_param_ops, &quirks_param_string, 0644);
+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
@@ -320,8 +458,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;
 
@@ -339,21 +477,43 @@ static u32 __usb_detect_quirks(struct usb_device *udev,
  return quirks;
 }
 
+static u32 usb_detect_dynamic_quirks(struct usb_device *udev)
+{
+ u16 vid = le16_to_cpu(udev->descriptor.idVendor);
+ u16 pid = le16_to_cpu(udev->descriptor.idProduct);
+ int i, flags = 0;
+
+ mutex_lock(&quirk_mutex);
+
+ for (i = 0; i < quirk_count; i++) {
+ if (vid == quirk_list[i].vid && pid == quirk_list[i].pid) {
+ flags = quirk_list[i].flags;
+ break;
+ }
+ }
+
+ mutex_unlock(&quirk_mutex);
+
+ 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_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);
 
+ udev->quirks ^= usb_detect_dynamic_quirks(udev);
+
  if (udev->quirks)
  dev_dbg(&udev->dev, "USB quirks for this device: %x\n",
  udev->quirks);
@@ -372,7 +532,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;
 
@@ -380,3 +540,11 @@ void usb_detect_interface_quirks(struct usb_device *udev)
  quirks);
  udev->quirks |= quirks;
 }
+
+void usb_release_quirk_list(void)
+{
+ mutex_lock(&quirk_mutex);
+ kfree(quirk_list);
+ quirk_list = NULL;
+ mutex_unlock(&quirk_mutex);
+}
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 845286f08ab0..193ad94797dc 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -1260,6 +1260,7 @@ static void __exit usb_exit(void)
  if (usb_disabled())
  return;
 
+ usb_release_quirk_list();
  usb_deregister_device_driver(&usb_generic_driver);
  usb_major_cleanup();
  usb_deregister(&usbfs_driver);
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index 2bee08d084ae..bb95f98fe48c 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -36,6 +36,7 @@ extern void usb_deauthorize_interface(struct usb_interface *);
 extern void usb_authorize_interface(struct usb_interface *);
 extern void usb_detect_quirks(struct usb_device *udev);
 extern void usb_detect_interface_quirks(struct usb_device *udev);
+extern void usb_release_quirk_list(void);
 extern int usb_remove_device(struct usb_device *udev);
 
 extern int usb_get_device_descriptor(struct usb_device *dev,
--
2.15.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[Bionic] [PATCH 2/3] usb: core: Copy parameter string correctly and remove superfluous null check

Kai-Heng Feng
In reply to this post by Kai-Heng Feng
BugLink: https://bugs.launchpad.net/bugs/1762695

strsep() slices string, so the string gets copied by
param_set_copystring() at the end of quirks_param_set() is not the
original value.
Fix that by calling param_set_copystring() earlier.

The null check for val is unnecessary, the caller of quirks_param_set()
does not pass null string.
Remove the superfluous null check. This is found by Smatch.

Fixes: 027bd6cafd9a ("usb: core: Add "quirks" parameter for usbcore")
Cc: Dan Carpenter <[hidden email]>
Signed-off-by: Kai-Heng Feng <[hidden email]>
Signed-off-by: Greg Kroah-Hartman <[hidden email]>
(cherry picked from commit a030501499b032bd218e1d01c07677bab6a0d53f)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/usb/core/quirks.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 11d943a6b7cb..a4f53e1f1ce1 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -31,10 +31,15 @@ static int quirks_param_set(const char *val, const struct kernel_param *kp)
  u16 vid, pid;
  u32 flags;
  size_t i;
+ int err;
+
+ err = param_set_copystring(val, kp);
+ if (err)
+ return err;
 
  mutex_lock(&quirk_mutex);
 
- if (!val || !*val) {
+ if (!*val) {
  quirk_count = 0;
  kfree(quirk_list);
  quirk_list = NULL;
@@ -133,7 +138,7 @@ static int quirks_param_set(const char *val, const struct kernel_param *kp)
 unlock:
  mutex_unlock(&quirk_mutex);
 
- return param_set_copystring(val, kp);
+ return 0;
 }
 
 static const struct kernel_param_ops quirks_param_ops = {
--
2.15.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[Bionic] [PATCH 3/3] usb: core: Add USB_QUIRK_DELAY_CTRL_MSG to usbcore quirks

Kai-Heng Feng
In reply to this post by Kai-Heng Feng
BugLink: https://bugs.launchpad.net/bugs/1762695

There's a new quirk, USB_QUIRK_DELAY_CTRL_MSG. Add it to usbcore quirks
for completeness.

Signed-off-by: Kai-Heng Feng <[hidden email]>
Signed-off-by: Greg Kroah-Hartman <[hidden email]>
(cherry picked from commit 4d8d5a392ae110d9b5889afd2b4beef9a09e712d)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 Documentation/admin-guide/kernel-parameters.txt | 4 +++-
 drivers/usb/core/quirks.c                       | 3 +++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 969f48683313..6091973f844a 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4407,7 +4407,9 @@
  calculation);
  m = USB_QUIRK_DISCONNECT_SUSPEND (Device needs
  to be disconnected before suspend to
- prevent spurious wakeup)
+ prevent spurious wakeup);
+ n = USB_QUIRK_DELAY_CTRL_MSG (Device needs a
+ pause after every control message);
  Example: quirks=0781:5580:bk,0a5c:5834:gij
 
  usbhid.mousepoll=
diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index a4f53e1f1ce1..84ccbb0424c2 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -124,6 +124,9 @@ static int quirks_param_set(const char *val, const struct kernel_param *kp)
  case 'm':
  flags |= USB_QUIRK_DISCONNECT_SUSPEND;
  break;
+ case 'n':
+ flags |= USB_QUIRK_DELAY_CTRL_MSG;
+ break;
  /* Ignore unrecognized flag characters */
  }
  }
--
2.15.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED: [Bionic] [PATCH 0/3] Backport USB core quirks

Seth Forshee
In reply to this post by Kai-Heng Feng
On Tue, Apr 10, 2018 at 05:49:28PM +0800, Kai-Heng Feng wrote:

> BugLink: https://bugs.launchpad.net/bugs/1762695
>
> [Impact]
> Currently we need to recompile the entire kernel if we want try quirks
> in usbcore.
>
> [Test]
> Use kernel parameter "usbcore.quirks=" in conjunction with dynamic debug
> for usbcore, we can see that usb quirks successfully enabled for USB
> devices dynamically.
>
> [Fix]
> Introduce a new module parameter, "usbcore.quirks=". Users now can try
> different quirks without compiling anything.
>
> [Regression Potential]
> Low. It doesn't change any behavior of usb driver per se, it simply adds
> new logic to detect quirks.

Applied to bionic/master-next, thanks!

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team