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

2018-02-28 Thread kbuild test robot 
Hi Kai-Heng,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on v4.16-rc3 next-20180228]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Kai-Heng-Feng/usb-core-Add-quirks-parameter-for-usbcore/20180227-211635
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git 
usb-testing
config: mips-sb1250_swarm_defconfig (attached as .config)
compiler: mips64-linux-gnuabi64-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=mips 

All errors (new ones prefixed by >>):

   drivers/usb/core/quirks.c:15:43: error: expected ')' before 'sizeof'
module_param_string(quirks, quirks_param, sizeof(quirks_param), 0644);
  ^~
>> drivers/usb/core/quirks.c:16:26: error: expected ')' before string constant
MODULE_PARM_DESC(quirks, "Add/modify USB quirks by specifying 
quirks=vendorID:productID:quirks");
 
^~

vim +16 drivers/usb/core/quirks.c

13  
14  static char quirks_param[128];
  > 15  module_param_string(quirks, quirks_param, sizeof(quirks_param), 0644);
  > 16  MODULE_PARM_DESC(quirks, "Add/modify USB quirks by specifying 
quirks=vendorID:productID:quirks");
17  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

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

2018-02-28 Thread kbuild test robot 
Hi Kai-Heng,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on v4.16-rc3 next-20180228]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Kai-Heng-Feng/usb-core-Add-quirks-parameter-for-usbcore/20180227-211635
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git 
usb-testing
config: tile-tilegx_defconfig (attached as .config)
compiler: tilegx-linux-gcc (GCC) 7.2.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=tile 

All errors (new ones prefixed by >>):

>> drivers/usb/core/quirks.c:15:43: error: expected ')' before 'sizeof'
module_param_string(quirks, quirks_param, sizeof(quirks_param), 0644);
  ^~
   drivers/usb/core/quirks.c:16:26: error: expected ')' before string constant
MODULE_PARM_DESC(quirks, "Add/modify USB quirks by specifying 
quirks=vendorID:productID:quirks");
 
^~

vim +15 drivers/usb/core/quirks.c

13  
14  static char quirks_param[128];
  > 15  module_param_string(quirks, quirks_param, sizeof(quirks_param), 0644);
16  MODULE_PARM_DESC(quirks, "Add/modify USB quirks by specifying 
quirks=vendorID:productID:quirks");
17  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

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

2018-02-28 Thread Matthew Wilcox 
On Thu, Mar 01, 2018 at 12:01:40AM +0800, Kai Heng Feng wrote:
> > Also, you won't need to use a linked list for this; you can just allocate
> > an array of quirks.
> 
> I use linked list because the total quirks number is known after the entire
> string gets parsed.
> Do you suggest that I should just alloc a predefined number (like 16) quirk
> entries, instead of doing it dynamically?

I'd just do:

unsigned int nr = 1;
for (i = 0; i < len; i++)
if (str[i] == ',')
nr++;
kcalloc(nr, sizeof(struct), GFP_KERNEL);

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2018-02-28 Thread Kai Heng Feng 




On 28 Feb 2018, at 10:47 PM, Matthew Wilcox  wrote:

On Mon, Feb 26, 2018 at 11:04:57PM +0800, Kai-Heng Feng wrote:

+static char quirks_param[128];
+module_param_string(quirks, quirks_param, sizeof(quirks_param), 0644);
+MODULE_PARM_DESC(quirks, "Add/modify USB quirks by specifying  
quirks=vendorID:productID:quirks");

+
+static char quirks_param_orig[128];



+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);
+   struct quirk_entry *quirk;
+
+   mutex_lock(_mutex);
+   if (strcmp(quirks_param, quirks_param_orig) != 0) {
+   strcpy(quirks_param_orig, quirks_param);


What happens if the user is writing to quirks_param at the same time
that you memcpy it?

I think you're going about this wrong by trying to use the
module_param_string machinery.  You should be using module_param_cb()
to build the quirks list when the user writes it (and then translate
back into a string when the user wants to read from it.


Thanks! module_param_cb() is exactly what I want.
I’ll use it in next version.


Also, you won't need to use a linked list for this; you can just allocate
an array of quirks.



I use linked list because the total quirks number is known after the entire  
string gets parsed.
Do you suggest that I should just alloc a predefined number (like 16) quirk  
entries, instead of doing it dynamically?


Kai-Heng
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2018-02-28 Thread Matthew Wilcox 
On Mon, Feb 26, 2018 at 11:04:57PM +0800, Kai-Heng Feng wrote:
> +static char quirks_param[128];
> +module_param_string(quirks, quirks_param, sizeof(quirks_param), 0644);
> +MODULE_PARM_DESC(quirks, "Add/modify USB quirks by specifying 
> quirks=vendorID:productID:quirks");
> +
> +static char quirks_param_orig[128];

> +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);
> + struct quirk_entry *quirk;
> +
> + mutex_lock(_mutex);
> + if (strcmp(quirks_param, quirks_param_orig) != 0) {
> + strcpy(quirks_param_orig, quirks_param);

What happens if the user is writing to quirks_param at the same time
that you memcpy it?

I think you're going about this wrong by trying to use the
module_param_string machinery.  You should be using module_param_cb()
to build the quirks list when the user writes it (and then translate
back into a string when the user wants to read from it.
Also, you won't need to use a linked list for this; you can just allocate
an array of quirks.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2018-02-26 Thread 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. 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 
---
v4: Use kmalloc() to reduce memory usage.
Remove tolower() so we can use capital character in the future.

v3: Stop overridding static quirks.
Use XOR for dynamic quirks.
Save parsed quirk as a list to avoid parsing quirk table everytime.

v2: Use in-kernel tolower() function.

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

diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt
index 1d1d53f85ddd..70a7398c20e2 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4368,6 +4368,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
+