[PATCH 0/7] usbhid: add load-time and run-time USB HID quirks configuration

[PATCH 0/7] usbhid: add load-time and run-time USB HID quirks configuration

[Paul Walmsley]

The USB HID quirk list ('hid_blacklist') is a compile-time fixed array
in the current kernel.  Any modifications require editing the module
source and recompiling.  Some distributions compile the usbhid code
into the kernel, thus requiring that the entire kernel be rebuilt.

For most HID devices, this is not a major problem, since quirks are
unlikely to change often.  Some devices, however, have multiple
drivers available, requiring different HID quirks.  For example, the
LabJack U12 data acquisition device requires HID_QUIRK_IGNORE for one
driver and HID_QUIRK_HIDDEV for another.  With a static hid_blacklist
array, switching between the two is quite difficult.

This series of patches fixes this problem by adding a new module
parameter 'hid_quirks' which allows quirks to be modified at boot or
module load-time.  These quirks can also be reviewed and configured at
runtime via a procfs file, /proc/usbhid/device_quirks.  (This latter
ability is controlled by the Kconfig option CONFIG_USBHID_PROC_FS.)  The 
patches also add a new field 'idProductMask' to the hid_blacklist, so some 
of the multiple-device vendor quirks can be converted into list entries 
efficiently.

Against 2.6.21-rc4.  Comments welcome,


- Paul

Re: [PATCH 0/7] usbhid: add load-time and run-time USB HID quirks configuration

[Jiri Kosina]

On Sun, 18 Mar 2007, Paul Walmsley wrote:

> The USB HID quirk list ('hid_blacklist') is a compile-time fixed array 
> in the current kernel.  Any modifications require editing the module 
> source and recompiling.  Some distributions compile the usbhid code into 
> the kernel, thus requiring that the entire kernel be rebuilt. For most 
> HID devices, this is not a major problem, since quirks are unlikely to 
> change often.  Some devices, however, have multiple drivers available, 
> requiring different HID quirks.  For example, the LabJack U12 data 
> acquisition device requires HID_QUIRK_IGNORE for one driver and 
> HID_QUIRK_HIDDEV for another.  With a static hid_blacklist array, 
> switching between the two is quite difficult.

Hi Paul,

I agree that current quirk handling in usbhid is not the best thing on 
earth and needs improving - this should be acomplished in future by 
converting HID layer to bus.

However, I am not sure whether your solution is not over-designed.

You can currently "force" HID_QUIRK_IGNORE for the device that has been 
already bound to usbhid driver through 'unbind' file in sysfs. This lets 
you to ask usbhid driver to release the device. So you can just have 
HID_QUIRK_HIDDEV quirk set, and bind/unbind the device depending whether 
you want to use hiddev driver or a separate driver. I agree that with 
other quirks it's not so easy, but I am not aware of any device/driver 
which would need to change other quirks in runtime.

Also your solution cosumes significantly more memory (adding 16 bits to 
every hid_blacklist entry, duplicating the information into list that in 
most cases will stay intact and just mirror the contents of 
hid_blacklist). Not that it's a big issue, but still worth noting.

There are also some CodingStyle issues (like inconsistent usage of "return 
something" and "return(something);" and over-commenting on some places (no 
need to say that we are going to allocate memory before calling kmalloc(), 
etc.), but these are not that important now.

To summarize: do you see any other application of your aproach, for any 
currently existing devices/drivers, that can't be accomplished by 
unbinding the usbhid driver from the device?

> This series of patches fixes this problem by adding a new module
> parameter 'hid_quirks' which allows quirks to be modified at boot or
> module load-time.  These quirks can also be reviewed and configured at
> runtime via a procfs file, /proc/usbhid/device_quirks. 

This is unfortunately also not going to fly - the general rule of thumb is 
that no new entries should be added into /proc. You could acomplish the 
very same functionality through sysfs, right?

Thanks,

-- 
Jiri Kosina

Re: [PATCH 0/7] usbhid: add load-time and run-time USB HID quirks configuration

[Paul Walmsley]

Hello Jiri,

thanks for the review.

On Mon, 19 Mar 2007, Jiri Kosina wrote:

> You can currently "force" HID_QUIRK_IGNORE for the device that has been
> already bound to usbhid driver through 'unbind' file in sysfs. This lets
> you to ask usbhid driver to release the device. So you can just have
> HID_QUIRK_HIDDEV quirk set, and bind/unbind the device depending whether
> you want to use hiddev driver or a separate driver.

Thanks, I was unaware of that 'unbind' feature.  Defining HID_QUIRK_HIDDEV 
by default seems sort of kludgy, but it would probably work for my 
purposes.  Certainly my preference would still be for some sort of boot or 
runtime configuration for this, so the device is associated with the 
proper driver upon initial plugin.

> Also your solution cosumes significantly more memory (adding 16 bits to
> every hid_blacklist entry, duplicating the information into list that in
> most cases will stay intact and just mirror the contents of
> hid_blacklist). Not that it's a big issue, but still worth noting.

Right now there are about 160 entries in the hid_blacklist, so adding the 
mask costs about 320 bytes extra.  If that's a concern, a bigger issue is 
the list_head added to each hid_blacklist entry - two pointers per entry - 
so, 1280 bytes on a 32-bit machine (not counting the list head itself)

If you'd rather not to have the mask code in the patch, I'll drop it. 
It's only in there to remove the runtime-immutable special-case code for 
Wacom and Codemercs devices, so it's not really useful for my specific 
concern.

> There are also some CodingStyle issues (like inconsistent usage of "return
> something" and "return(something);" and over-commenting on some places (no
> need to say that we are going to allocate memory before calling kmalloc(),
> etc.), but these are not that important now.

If you ultimately decide you want the patchset, I'll respin it with these 
issues resolved to your satisfaction.

> To summarize: do you see any other application of your aproach, for any
> currently existing devices/drivers, that can't be accomplished by
> unbinding the usbhid driver from the device?

Aside from other devices that have a similar HIDDEV/IGNORE situation, 
there are two other possible uses that occur to me - but I don't know if 
either one is compelling enough to justify the extra code involved in the 
patchset.  I suspect that runtime quirks configuration would probably be 
useful for testing new HID devices.  I can also foresee a situation where 
a manufacturer revises a device, and an existing quirk is no longer 
applicable for some users, or a new quirk needs to be added for particular 
users.  But, these are both theoretical use cases.  Since the patches have 
been useful for me, I wouldn't mind seeing them in mainline, but I don't 
have any stake in it either way.

(re procfs usage)
> This is unfortunately also not going to fly - the general rule of thumb is
> that no new entries should be added into /proc. You could acomplish the
> very same functionality through sysfs, right?

Yes - if you decide that you want the patchset, I'll convert it to sysfs.


regards,

- Paul

Re: [PATCH 0/7] usbhid: add load-time and run-time USB HID quirks configuration

[Jiri Kosina]

On Mon, 19 Mar 2007, Paul Walmsley wrote:

> > Also your solution cosumes significantly more memory (adding 16 bits to
> > every hid_blacklist entry, duplicating the information into list that in
> > most cases will stay intact and just mirror the contents of
> > hid_blacklist). Not that it's a big issue, but still worth noting.
> Right now there are about 160 entries in the hid_blacklist, so adding the mask
> costs about 320 bytes extra.  If that's a concern, a bigger issue is the
> list_head added to each hid_blacklist entry - two pointers per entry - so,
> 1280 bytes on a 32-bit machine (not counting the list head itself)

Hi Paul,

What is your opinion on an alternative aproach - the hid_blacklist[] could 
stay intact, and the linked list in your patch will be used in a slightly 
different way - it will be used to define *exceptions* (which will be 
possible to be tuned in runtime) from hid_blacklist, rather than 
duplicating the information which is already present there?

> If you'd rather not to have the mask code in the patch, I'll drop it. It's
> only in there to remove the runtime-immutable special-case code for Wacom and
> Codemercs devices, so it's not really useful for my specific concern.

My opinion is that two exceptions (wacom and codemercs) can be easily 
handled by few lines of code, and extending the hid_blacklist[] just 
because of these two users looks maybe a little bit over-generalized to 
me.

Anyway, I think that the possibility to modify the hid_blakclist[] in 
runtime could be useful. If you agree with the above and would care to 
redo the pathces in such way, I would be inclined to merge them.

Thanks,

-- 
Jiri Kosina

Re: [PATCH 0/7] usbhid: add load-time and run-time USB HID quirks configuration

[Li Yu]

On Mon, 19 Mar 2007, Jiri Kosina wrote:

>I agree that current quirk handling in usbhid is not the best thing on
>earth and needs improving - this should be acomplished in future by
>converting HID layer to bus.

Hi, Jiri,

I find this mail list just now :D

I am try to make HID busize. Here is the design overview and progress:

--------------------------------------
    HID bus design overview.
--------------------------------------

A. Terms.

    The device of an driver: this mean the device that this driver matched.

B. Design.

    As we discussed before, The entire HID subsystem is divided into
three layers:
   
    1. HID Driver Layer (HIDDL).
       
        This layer is composite of all HID drivers, however, these
drivers are not equal each other, there are divided into three kinds:
       
        1.1 Fundamental driver.
               
        This is the most natural driver for us, they control hardware or
other something that can effect hardware directly. In most cases, each
of this kind driver is one standard and extendible implementation of HID
specification for specific HID transport layer. For example, usbhid.ko
for HID/USB or hidp.ko for Bluetooth/HID. This should alway input-able
driver, i.e, it is able to allow HIDAL register input device for its device.
                   
        1.2 Shadow driver.
       
        These driver is created for those buggy/strange/extended HID
device which fundamental driver can't handle rightly/completely. this
driver does not share HID information with related fundamental driver,
but the HID information of their devices is derived from other device of
fundamental driver. These may not be input-able driver. The shadow
driver does not share the input device with the related fundamental
driver, even shadow driver is input-able, it have new input device in
such case.
       
        1.3 HIDDEV drvier.
       
        In fact, This isn't one public kind of driver. it just is one
optionally flag parameter while register driver (also include shadow
driver). When turn on this feature, the HIDAL will create one hiddev for
each device of driver, so the every such device also have one hiddev buddy.
   
    2. HID Abstract Layer (HIDAL).       
   
        This layer maintain HID bus, HID driver, HID device, HID hiddev
such logical feature, and interact with input subsystem.

    3. HID Transport Layer (HIDTL).

        This layer may be rather thin. Up to now, we should have two
implementation of transport layer, one for USB, one for Bluetooth.

C. Progress.

Status: USB Fundamental driver works for new HID API now.
Working: Shadow driver support.
TODO:
    1. Shadow driver support.
    2. Port some drivers to new HID API, includes:
        1) All HID ff drivers.
        2) iBook/PowerBook special keys driver.
    3. Bluetooth fundamental driver.
    4. Bluetooth transport layer.
    5. Port Other drivers.
    6. HIDDEV/USB driver support.
    7. HIDDEV/Bluetooth driver support.

I think that post this text earlier is better decision, well, I guess
the new controversy about HID may begin ...
 
PS: In my words, the hiddev is same with rawdev, is it right in your words?
    I cann't acquire the support for this from my corporation,  :(  So, I
    must complete this in my spare time, it may take longer time.

Good luck.

- Li Yu

Re: [PATCH 0/7] usbhid: add load-time and run-time USB HID quirks configuration

[Paul Walmsley]

Hi Jiri,

On Tue, 20 Mar 2007, Jiri Kosina wrote:

> What is your opinion on an alternative aproach - the hid_blacklist[] could
> stay intact, and the linked list in your patch will be used in a slightly
> different way - it will be used to define *exceptions* (which will be
> possible to be tuned in runtime) from hid_blacklist, rather than
> duplicating the information which is already present there?

That approach makes a lot of sense.  It provides runtime configurability, 
without the added memory costs involved with a big linked list or the 
product mask.  The best of both worlds.

> Anyway, I think that the possibility to modify the hid_blakclist[] in
> runtime could be useful. If you agree with the above and would care to
> redo the pathces in such way, I would be inclined to merge them.

Sounds good, I'll redo the patches.


- Paul