Re: [PATCH v2] usb: core: Add "quirks" parameter for usbcore

[Kai Heng Feng <kai.heng.feng@canonical.com>]

> 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