[PATCH 0/7] usbhid: quirks cleanup, add dynamic quirks, ConfigFS interface

[PATCH 0/7] usbhid: quirks cleanup, add dynamic quirks, ConfigFS interface

[Paul Walmsley]

Hello,

As requested, I respun the runtime USB HID quirks patches that I posted a 
few weeks ago.  My application for this code is to switch quirks at 
runtime for a data acquisition device.  This device has at least two 
drivers written for it: one kernel module driver requiring 
HID_QUIRK_IGNORE; one userspace driver based on hiddev requiring 
HID_QUIRK_HIDDEV.  Maybe others will find this code useful.

Patches 1-3 are cleanup and reorganization.  The primary changes move USB 
HID quirk handling into its own files, hid-quirks.[ch].  There should be 
no behavioral changes after 1-3 are applied.  These patches touch the 
Bluetooth HID code lightly, since it references HID quirk #defines.

Patches 4-6 implement dynamic quirk handling ("equirks" for extra quirks) 
and support changing these quirks with a module parameter, 'quirks'. 
Jiri, this uses a separate list as we discussed.  Documentation is in 
kernel-parameters.txt.

Patch 7 implements a ConfigFS-based interface to add/modify/remove dynamic 
quirks while the usbhid module is loaded.  We'd discussed using sysfs for 
this, but upon further consideration, blacklists don't seem to fit into 
the sysfs model very well.  ConfigFS seemed like a better approach.  Not 
that I'm entirely happy with it - it requires a lot of code/data.  oh 
well.  Documentation is included as part of the Kconfig option.

Much of this code could probably be shared with the Bluetooth HID 
subsystem.  I guess there's been some discussion about this recently.  If 
this code seems more appropriate to drivers/hid/, I'd be happy to
reshuffle it.

Jiri, I think I've cleaned up most of the other coding style issues that 
you mentioned.  Please let me know if there's more that needs to be done.

Patches against 2.6.21-rc6.


- Paul

Re: [PATCH 0/7] usbhid: quirks cleanup, add dynamic quirks, ConfigFS interface

[Jiri Kosina]

On Wed, 11 Apr 2007, Paul Walmsley wrote:

> Much of this code could probably be shared with the Bluetooth HID 
> subsystem. I guess there's been some discussion about this recently.  
> If this code seems more appropriate to drivers/hid/, I'd be happy to 
> reshuffle it.

Hi Paul,

thanks a lot for taking care of this. I am really willing to have this 
functionality, so as soon as we settle it down, I am going to merge this.

Currently the handling of quirky HID devices in Bluetooth is rather 
trivial - I have added some time ago handling for bluetooth version of 
mighty mouse into bluetooth hid code (commit cb3fecc in Linus' tree). It 
is not clear now whether there will be a big overlap for usbhid and hidp 
quirk handling, or they are going to be different. So it's probably fine 
to leave it usbhid-specific for now. Later we will see what is the better 
option to reflect real world.

> Jiri, I think I've cleaned up most of the other coding style issues that you
> mentioned.  Please let me know if there's more that needs to be done.

I have a few rather simple notes to your patches. The most disturbing one 
for you probably is a query to do one more round of respinning, against 
current hid tree - there have been lots of code changes in the area you 
touch, which are queued for 2.6.22 merge window to open, so I would get 
huge reject when I just apply your patches. Sorry for that.

I could redo this myself, but I am not sure if I'll manage to find time 
for it by the end of the week. Thanks a lot.

- [2/7] it would help if you could respin against current hid.git tree. I 
  have already performed some time ago some of the tasks you did (like 
  consolidating the defines scattered around the hid-core.c (I also made 
  the blacklist sorted again), etc.). Also new quirks have been queued, 
  etc. I can do this myself, but it will take some time, before I process 
  other things in my queue :)
- [3/7] the same as above - there have been changes in this area which I 
  have queued for mainline merge in my tree, so just simple respin against 
  hid tree would help a lot. I'd also probably rename 
  usbhid_lookup_any_quirk() just to usbhid_lookup_quirk()  but that's not 
  a big deal
- at least [5/7] and [6/7] seem to have mangled whitespaces on some 
  certain places on a first sight, could you fix that please in the next 
  round?
- [7/7] I agree with using configfs. What is quite unfortunate is that 
  adding a new quirk requires now another place that it is necessary to 
  modify - forgetting to do it and keeping configfs values out-of-sync 
  seems rather easy mistake to do. But I am not able to come with an idea 
  immediately how to make it better.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 0/7] usbhid: quirks cleanup, add dynamic quirks, ConfigFS interface

[Jiri Kosina]

On Wed, 11 Apr 2007, Jiri Kosina wrote:

> - [7/7] I agree with using configfs. What is quite unfortunate is that 
>   adding a new quirk requires now another place that it is necessary to 
>   modify - forgetting to do it and keeping configfs values out-of-sync 
>   seems rather easy mistake to do. But I am not able to come with an idea 
>   immediately how to make it better.

Maybe just reusing hid_blacklist and extending it with 'name' column for 
the purposes of configfs would suffice? (ca_mode and ca_owner could be 
made default implicitly I guess).

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 0/7] usbhid: quirks cleanup, add dynamic quirks, ConfigFS interface

[Paul Walmsley]

Hello Jiri,

On Wed, 11 Apr 2007, Jiri Kosina wrote:

> I have a few rather simple notes to your patches.

Thanks for the review - I've fixed all of the issues that you noted and 
rebased the patches against hid.git - will post them shortly.

I made one other cosmetic change, 'equirks' is now 'dquirks' -- for 
dynamic quirks -- it occurred to me that it makes more sense than 'extra 
quirks.'


- Paul

Re: [PATCH 0/7] usbhid: quirks cleanup, add dynamic quirks, ConfigFS interface

[Paul Walmsley]

On Wed, 11 Apr 2007, Jiri Kosina wrote:

> On Wed, 11 Apr 2007, Jiri Kosina wrote:
>
>> - [7/7] I agree with using configfs. What is quite unfortunate is that
>>   adding a new quirk requires now another place that it is necessary to
>>   modify - forgetting to do it and keeping configfs values out-of-sync
>>   seems rather easy mistake to do. But I am not able to come with an idea
>>   immediately how to make it better.
>
> Maybe just reusing hid_blacklist and extending it with 'name' column for
> the purposes of configfs would suffice? (ca_mode and ca_owner could be
> made default implicitly I guess).

Hi Jiri,

You're talking about using another static struct hid_blacklist array in 
place of the struct hid_quirk_types array, right?  We could do that, but I 
don't see how it would solve the problem of having two places to update 
when a new quirk type is added - both the #defines and the new 
hid_blacklist array would still need to be changed.  But maybe I'm 
misunderstanding what you're proposing...

One other way to do it would be to remove the HID_QUIRK* #defines and to 
call find_quirk_bits_by_name() instead, which iterates over struct 
hid_quirk_types.  Comparatively slower than a preprocessor define, but 
then again this code isn't in a fast path anyway...


- Paul

Re: [PATCH 0/7] usbhid: quirks cleanup, add dynamic quirks, ConfigFS interface

[Jiri Kosina]

On Wed, 11 Apr 2007, Paul Walmsley wrote:

> You're talking about using another static struct hid_blacklist array in 
> place of the struct hid_quirk_types array, right?  

Hi Paul,

no, in fact I was just thinking about extending the hid_blacklist[] with 
additional field 'name', which will be used for the purpose of configfs. 

This way we would not have to duplicate the entries anywhere. All the 
other fields you are currently keeping in hid_quirk_types[] could be made 
implicitly default, only 'name' is the one that matters, right?

Thanks,

-- 
Jiri Kosina

Re: [PATCH 0/7] usbhid: quirks cleanup, add dynamic quirks, ConfigFS interface

[Paul Walmsley]

On Thu, 12 Apr 2007, Jiri Kosina wrote:

> On Wed, 11 Apr 2007, Paul Walmsley wrote:
>
>> You're talking about using another static struct hid_blacklist array in
>> place of the struct hid_quirk_types array, right?
>
> no, in fact I was just thinking about extending the hid_blacklist[] with
> additional field 'name', which will be used for the purpose of configfs.
>
> This way we would not have to duplicate the entries anywhere. All the
> other fields you are currently keeping in hid_quirk_types[] could be made
> implicitly default, only 'name' is the one that matters, right?

Your last statement is correct.  The problem is that I don't see how the 
rest of what you describe would work in the current arrangement.  Maybe 
you could send an example of how your idea would work in terms of 
configfs.

Right now, there's only one configfs attribute (= filename) per quirk type 
- 23 of these.  Even if multiple dynamic device quirks are added, no new 
configfs attributes are created for each file; the old ones are simply 
reused.  The patch's current configfs directory structure is:

  /usbhid/quirks_runtime/<vendor_id>:<product_id>/<quirk type name>

...

Now, another implementation approach would be to get rid of the quirk type 
names entirely, to use a path such as:

  /usbhid/quirks_runtime/<vendor_id>:<product_id>

with the last component being a r/w configfs attribute containing the 
quirks value.  Is that what you are proposing?  I considered it when I did 
the initial implementation.  The problem with it is that there seems to be 
no way for userspace to create new configfs files/attributes -- only 
configfs directories.


- Paul

Re: [PATCH 0/7] usbhid: quirks cleanup, add dynamic quirks, ConfigFS interface

[Jiri Kosina]

On Thu, 12 Apr 2007, Paul Walmsley wrote:

> Now, another implementation approach would be to get rid of the quirk 
> type names entirely, to use a path such as:
> 
>  /usbhid/quirks_runtime/<vendor_id>:<product_id>
> 
> with the last component being a r/w configfs attribute containing the quirks
> value.  Is that what you are proposing?  I considered it when I did the
> initial implementation.  The problem with it is that there seems to be no way
> for userspace to create new configfs files/attributes -- only configfs
> directories.

Yes, this was my very original thought - I really somehow don't think that 
having to duplicate every new quirk is a nice solution. Joel, is there any 
specific reason why configfs doesn't allow this please?

Or we could reconsider going back to the idea of using sysfs.

Thanks a lot,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 0/7] usbhid: quirks cleanup, add dynamic quirks, ConfigFS interface

[Paul Walmsley]

On Thu, 12 Apr 2007, Jiri Kosina wrote:

> On Thu, 12 Apr 2007, Paul Walmsley wrote:
>
>> Now, another implementation approach would be to get rid of the quirk
>> type names entirely, to use a path such as:
>>
>>  /usbhid/quirks_runtime/<vendor_id>:<product_id>
>>
>> with the last component being a r/w configfs attribute containing the quirks
>> value.  Is that what you are proposing?  I considered it when I did the
>> initial implementation.  The problem with it is that there seems to be no way
>> for userspace to create new configfs files/attributes -- only configfs
>> directories.
>
> Yes, this was my very original thought - I really somehow don't think that
> having to duplicate every new quirk is a nice solution. Joel, is there any
> specific reason why configfs doesn't allow this please?

well, another approach would be to go to a layout like:

/usbhid/blacklist/<vendor_id>:<product_id>/quirks

in such a model, individual files would not have to be created or removed.

Personally I think that having the quirk names broken out as individual 
files is more in line with the idea of 'one file per config attribute' 
that comes up in both the sysfs and configfs documentation.  Besides, the 
worst thing that can happen if someone updates the HID_QUIRK #defines, but 
not hid_quirk_types[], is that the quirk won't be readable or modifiable 
from userspace, until someone fixes the mismatch.

But ultimately, I don't think the choice of approach matters all that 
much, really.  It's pretty marginal functionality.


- Paul