proposal for deletion of drivers/hid/hid-ids.h

proposal for deletion of drivers/hid/hid-ids.h

[Oliver Neukum]

Hi,

I would like to kill drivers/hid/hid-ids.h and replace it
with numerical IDs in the files using it.

There are two reasons for that.

1. It is a layering violation. There should not be a private
data base for USB IDs in HID.

2. It serves no purpose and adds work. Anyone who adds a quirk
or a special case for devices needs to operate on the numbers,
as those are what he gets from the descriptors. Looking up or
adding a symbolic name for a device is just more work without
a benefit. These numbers have no intrinsic meaning beyond
being unique and it rarely matters (and should not matter)
for which vendor a particular fix is intended.

In the rare cases it does matter when it does matter searching
the official list of USB IDs is less work.

So let's kill this utterly useless step of indirection.

	Regards
		Oliver

Re: proposal for deletion of drivers/hid/hid-ids.h

[Benjamin Tissoires]

On Thu, Mar 26, 2015 at 7:44 AM, Oliver Neukum <oneukum@suse.de> wrote:
> Hi,
>
> I would like to kill drivers/hid/hid-ids.h and replace it
> with numerical IDs in the files using it.
>
> There are two reasons for that.
>
> 1. It is a layering violation. There should not be a private
> data base for USB IDs in HID.

Technically, this DB is not only for USB devices. We also have
Bluetooth and I2C devices here.

>
> 2. It serves no purpose and adds work. Anyone who adds a quirk
> or a special case for devices needs to operate on the numbers,
> as those are what he gets from the descriptors. Looking up or
> adding a symbolic name for a device is just more work without
> a benefit. These numbers have no intrinsic meaning beyond
> being unique and it rarely matters (and should not matter)
> for which vendor a particular fix is intended.

I disagree. This list might not be useful for the
drivers/hid/usbhid/hid-quirks.c by itself in most cases.
However, we mainly rely on this list to add the device in
hid_have_special_driver and hid_ignore in hid-core and in the
submodule that should handle it.

Many times, already having the VID/PID registered in hid-ids.h saved
some time when debugging and adding a subdriver for a special device
because if the VID/PID is already in hid-ids.h, that means that
someone already dealt with it, and it gives us a way to clean it when
the quirk was not appropriate. For instance, many multitouch devices
were added before the creation of hid-multitouch and were registered
with the quirk MULTI_INPUT.

Cheers,
Benjamin

>
> In the rare cases it does matter when it does matter searching
> the official list of USB IDs is less work.
>
> So let's kill this utterly useless step of indirection.
>
>         Regards
>                 Oliver
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: proposal for deletion of drivers/hid/hid-ids.h

[Oliver Neukum]

On Thu, 2015-03-26 at 10:06 -0400, Benjamin Tissoires wrote:
> On Thu, Mar 26, 2015 at 7:44 AM, Oliver Neukum <oneukum@suse.de> wrote:
> > Hi,
> >
> > I would like to kill drivers/hid/hid-ids.h and replace it
> > with numerical IDs in the files using it.
> >
> > There are two reasons for that.
> >
> > 1. It is a layering violation. There should not be a private
> > data base for USB IDs in HID.
> 
> Technically, this DB is not only for USB devices. We also have
> Bluetooth and I2C devices here.

Well, the token IDs ;)

> > 2. It serves no purpose and adds work. Anyone who adds a quirk
> > or a special case for devices needs to operate on the numbers,
> > as those are what he gets from the descriptors. Looking up or
> > adding a symbolic name for a device is just more work without
> > a benefit. These numbers have no intrinsic meaning beyond
> > being unique and it rarely matters (and should not matter)
> > for which vendor a particular fix is intended.
> 
> I disagree. This list might not be useful for the
> drivers/hid/usbhid/hid-quirks.c by itself in most cases.
> However, we mainly rely on this list to add the device in
> hid_have_special_driver and hid_ignore in hid-core and in the
> submodule that should handle it.

Can you explain how we depend on it? We certainly use it, but
how do we depend on it? I don't see how just the numbers would
be worse. In fact they would be better as you again save a lookup.

> Many times, already having the VID/PID registered in hid-ids.h saved
> some time when debugging and adding a subdriver for a special device
> because if the VID/PID is already in hid-ids.h, that means that

Again, I see how having the VID/PID pair is an advantage. I don't
see why having symbolic names for that pair is an advantage.
Just having the numbers in a list of quirky devices would serve
the same purpose.

> someone already dealt with it, and it gives us a way to clean it when
> the quirk was not appropriate. For instance, many multitouch devices
> were added before the creation of hid-multitouch and were registered
> with the quirk MULTI_INPUT.

Well, yes, so you needed to grep for MULTI_INPUT. The entries would
still have been present, just with nummerical IDs.

	Regards
		Oliver

Re: proposal for deletion of drivers/hid/hid-ids.h

[Oliver Neukum]

On Fri, 2015-03-27 at 09:49 +0100, Oliver Neukum wrote:
> On Thu, 2015-03-26 at 10:06 -0400, Benjamin Tissoires wrote:

> Well, yes, so you needed to grep for MULTI_INPUT. The entries would
> still have been present, just with nummerical IDs.

Especially as I see stuff like this:

0c3910c255 (Stephane Chatty               2010-04-10 16:43:08 +0200  282) #define USB_VENDOR_ID_DWAV            0x0eef
fe6065dc30 (Peter Hutterer                2010-02-02 13:40:40 +1000  283) #define USB_DEVICE_ID_EGALAX_TOUCHCONTROLLER  0x0001
729b814ace (Forest Bond                   2012-11-06 13:41:22 -0500  284) #define USB_DEVICE_ID_DWAV_TOUCHCONTROLLER    0x0002
e36f690b37 (Benjamin Tissoires            2011-11-23 10:54:31 +0100  285) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_480D     0x480d
e36f690b37 (Benjamin Tissoires            2011-11-23 10:54:31 +0100  286) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_480E     0x480e
fd1d152583 (Benjamin Tissoires            2012-03-06 10:53:47 +0100  287) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7207     0x7207
e36f690b37 (Benjamin Tissoires            2011-11-23 10:54:31 +0100  288) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_720C     0x720c
fd1d152583 (Benjamin Tissoires            2012-03-06 10:53:47 +0100  289) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7224     0x7224
2ce09df47b (Benjamin Tissoires            2012-03-06 17:57:02 +0100  290) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_722A     0x722A
fd1d152583 (Benjamin Tissoires            2012-03-06 10:53:47 +0100  291) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_725E     0x725e
fd1d152583 (Benjamin Tissoires            2012-03-06 10:53:47 +0100  292) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7262     0x7262
e36f690b37 (Benjamin Tissoires            2011-11-23 10:54:31 +0100  293) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_726B     0x726b
e36f690b37 (Benjamin Tissoires            2011-11-23 10:54:31 +0100  294) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_72A1     0x72a1
aa672da1b0 (Andy Shevchenko               2013-05-17 14:34:48 +0300  295) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_72AA     0x72aa
aa672da1b0 (Andy Shevchenko               2013-05-17 14:34:48 +0300  296) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_72C4     0x72c4
aa672da1b0 (Andy Shevchenko               2013-05-17 14:34:48 +0300  297) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_72D0     0x72d0
66f06127f3 (Benjamin Tissoires            2011-11-23 10:54:33 +0100  298) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_72FA     0x72fa
bb9ff21072 (Marek Vasut                   2011-11-23 10:54:32 +0100  299) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7302     0x7302
fd1d152583 (Benjamin Tissoires            2012-03-06 10:53:47 +0100  300) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7349     0x7349
ae01c9e53f (Thierry Reding                2012-08-09 08:34:48 +0200  301) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_73F7     0x73f7
e36f690b37 (Benjamin Tissoires            2011-11-23 10:54:31 +0100  302) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_A001     0xa001

I mean these entries are screaming: I make no sense.
Including the number in the name is so close to the nadir
to make no difference.

	Regards
		Oliver
 

Re: proposal for deletion of drivers/hid/hid-ids.h

[Benjamin Tissoires]

On Fri, Mar 27, 2015 at 4:49 AM, Oliver Neukum <oneukum@suse.de> wrote:
> On Thu, 2015-03-26 at 10:06 -0400, Benjamin Tissoires wrote:
>> On Thu, Mar 26, 2015 at 7:44 AM, Oliver Neukum <oneukum@suse.de> wrote:
>> > 2. It serves no purpose and adds work. Anyone who adds a quirk
>> > or a special case for devices needs to operate on the numbers,
>> > as those are what he gets from the descriptors. Looking up or
>> > adding a symbolic name for a device is just more work without
>> > a benefit. These numbers have no intrinsic meaning beyond
>> > being unique and it rarely matters (and should not matter)
>> > for which vendor a particular fix is intended.
>>
>> I disagree. This list might not be useful for the
>> drivers/hid/usbhid/hid-quirks.c by itself in most cases.
>> However, we mainly rely on this list to add the device in
>> hid_have_special_driver and hid_ignore in hid-core and in the
>> submodule that should handle it.
>
> Can you explain how we depend on it? We certainly use it, but
> how do we depend on it? I don't see how just the numbers would
> be worse. In fact they would be better as you again save a lookup.

We simply depend on it because we reuse the symbolic names all over
the place in the HID tree (not for every defined symbol, but a good
part of it).
If you consider the ones used in usbhid/hid-quirks.c and
hid-multitouch.c, yes I agree, it's a pain and they add nothing beside
a little bit of documentation saying "hey, we already handled this
PID".

>
>> Many times, already having the VID/PID registered in hid-ids.h saved
>> some time when debugging and adding a subdriver for a special device
>> because if the VID/PID is already in hid-ids.h, that means that
>
> Again, I see how having the VID/PID pair is an advantage. I don't
> see why having symbolic names for that pair is an advantage.
> Just having the numbers in a list of quirky devices would serve
> the same purpose.

No. Having the number only means you have to also make a check on the
vendor ID, because 0x0001 can be reused by any vendor. OTOH, having
the symbolic name where there is the VENDOR_NAME_PID makes lookups
much easier.

I think you are a little biaised by your recent patches in hid-quirks.c.

To show you my point, I made a quick grep in drivers/hid:

$> grep -r -E "USB_DEVICE_ID(\\w*)" * -o -h | sort | uniq -c
 -> 631 USB PID returned

And then extracting the uniq defines by appearance count:
1 -> 25  -> OK, these could be cleaned up
2 -> 326 -> Those are used only once, so we could remove them
3 -> 165
4 -> 79
5 -> 19
6 -> 7
7 -> 6
8 -> 0
9 -> 2
10 -> 1
11 -> 1

So yes, 55 % of these defines are used only once at most and are useless.
The other 45 % need to be defined somewhere. And given that they are
used in more than one file (at least hid-core.c and the subdriver),
well, hid-ids.h seems to be the place.
Having them defined once prevents accidental typos where we blacklist
a device without adding it to the correct subdriver.

I am not arguing against being more relaxed or removing the unused
once (though I can see them as a benefit somehow), just that it makes
no sense screaming that the list is not used and should be deleted
blindly.

>
>> someone already dealt with it, and it gives us a way to clean it when
>> the quirk was not appropriate. For instance, many multitouch devices
>> were added before the creation of hid-multitouch and were registered
>> with the quirk MULTI_INPUT.
>
> Well, yes, so you needed to grep for MULTI_INPUT. The entries would
> still have been present, just with nummerical IDs.

Don't be silly. That's not the way it works. You have a bug report,
you get the VID/PID, and then you start from it to know what is
happening.
You never know in advanced which quirk has been added and makes your
device behave not correctly (MULTI_INPUT is easy to deduce from the
user space, NO_GET is an other story for instance).

I usually check in hid-ids.h if it the device is known already. Then I
check for the symbolic name in the hid tree to get the exact places
where it is used. So for me having hid-ids.h makes sense.

Note that I already started infringing this rule in some drivers where
it makes no sense: in hid-logitech-hidpp, I used only the numerical
value for the DJ devices because I know that they won't be used
anywhere else.

Cheers,
Benjamin

Re: proposal for deletion of drivers/hid/hid-ids.h

[Benjamin Tissoires]

On Fri, Mar 27, 2015 at 5:39 AM, Oliver Neukum <oliver@neukum.org> wrote:
> On Fri, 2015-03-27 at 09:49 +0100, Oliver Neukum wrote:
>> On Thu, 2015-03-26 at 10:06 -0400, Benjamin Tissoires wrote:
>
>> Well, yes, so you needed to grep for MULTI_INPUT. The entries would
>> still have been present, just with nummerical IDs.
>
> Especially as I see stuff like this:
>
> 0c3910c255 (Stephane Chatty               2010-04-10 16:43:08 +0200  282) #define USB_VENDOR_ID_DWAV            0x0eef
> fe6065dc30 (Peter Hutterer                2010-02-02 13:40:40 +1000  283) #define USB_DEVICE_ID_EGALAX_TOUCHCONTROLLER  0x0001
> 729b814ace (Forest Bond                   2012-11-06 13:41:22 -0500  284) #define USB_DEVICE_ID_DWAV_TOUCHCONTROLLER    0x0002
> e36f690b37 (Benjamin Tissoires            2011-11-23 10:54:31 +0100  285) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_480D     0x480d
> e36f690b37 (Benjamin Tissoires            2011-11-23 10:54:31 +0100  286) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_480E     0x480e
> fd1d152583 (Benjamin Tissoires            2012-03-06 10:53:47 +0100  287) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7207     0x7207
> e36f690b37 (Benjamin Tissoires            2011-11-23 10:54:31 +0100  288) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_720C     0x720c
> fd1d152583 (Benjamin Tissoires            2012-03-06 10:53:47 +0100  289) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7224     0x7224
> 2ce09df47b (Benjamin Tissoires            2012-03-06 17:57:02 +0100  290) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_722A     0x722A
> fd1d152583 (Benjamin Tissoires            2012-03-06 10:53:47 +0100  291) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_725E     0x725e
> fd1d152583 (Benjamin Tissoires            2012-03-06 10:53:47 +0100  292) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7262     0x7262
> e36f690b37 (Benjamin Tissoires            2011-11-23 10:54:31 +0100  293) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_726B     0x726b
> e36f690b37 (Benjamin Tissoires            2011-11-23 10:54:31 +0100  294) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_72A1     0x72a1
> aa672da1b0 (Andy Shevchenko               2013-05-17 14:34:48 +0300  295) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_72AA     0x72aa
> aa672da1b0 (Andy Shevchenko               2013-05-17 14:34:48 +0300  296) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_72C4     0x72c4
> aa672da1b0 (Andy Shevchenko               2013-05-17 14:34:48 +0300  297) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_72D0     0x72d0
> 66f06127f3 (Benjamin Tissoires            2011-11-23 10:54:33 +0100  298) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_72FA     0x72fa
> bb9ff21072 (Marek Vasut                   2011-11-23 10:54:32 +0100  299) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7302     0x7302
> fd1d152583 (Benjamin Tissoires            2012-03-06 10:53:47 +0100  300) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7349     0x7349
> ae01c9e53f (Thierry Reding                2012-08-09 08:34:48 +0200  301) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_73F7     0x73f7
> e36f690b37 (Benjamin Tissoires            2011-11-23 10:54:31 +0100  302) #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_A001     0xa001
>
> I mean these entries are screaming: I make no sense.
> Including the number in the name is so close to the nadir
> to make no difference.

Well, first, these entries are now used only once in the hid tree, and
it's in hid-multitouch. So it's a rather bad example IMO and I agree,
we could simply clean them up.

Second, I take that personally (even if there were no intent behind,
but git blame kinds of forces me to take it personally). There is a
story behind those defines and I only tried to keep them manageable.
If you look at the parent commit, they were called
"USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_X", X being 1 to N. It make even
less sense to have those. But at the time, they were needed because
hid-core.c needed them in hid_have_special_driver so they were used in
2 different files.

My point is:
- if you want to clean up the entries that are not used in several
places, fine by me
- but please keep at least the defines for the vendor_ID and the
defines were we are re-using them in hid-core.c and someplace else.

Cheers,
Benjamin

Re: proposal for deletion of drivers/hid/hid-ids.h

[Jiri Kosina]

On Thu, 26 Mar 2015, Oliver Neukum wrote:

> I would like to kill drivers/hid/hid-ids.h and replace it
> with numerical IDs in the files using it.
> 
> There are two reasons for that.
> 
> 1. It is a layering violation. There should not be a private
> data base for USB IDs in HID.

Not really; it's not limited to USB IDs.

> 2. It serves no purpose and adds work. Anyone who adds a quirk or a 
> special case for devices needs to operate on the numbers, as those are 
> what he gets from the descriptors. Looking up or adding a symbolic name 
> for a device is just more work without a benefit. These numbers have no 
> intrinsic meaning beyond being unique and it rarely matters (and should 
> not matter) for which vendor a particular fix is intended.
> 
> In the rare cases it does matter when it does matter searching the 
> official list of USB IDs is less work.
> 
> So let's kill this utterly useless step of indirection.

I know that there are some really useless cases (e.g. the ones which don't 
add any extra information, and just do "VENDOR_<PID>" to construct the 
string). But I personally like to see at least the vendor IDs as a string, 
so that I am immediately identify behavior for particular vendors when 
looking at the code.

I am not *forcing* everybody to add entry to hid-ids.h for each and every 
quirk he is adding, but I guess I'd rather insist to have at least vendor 
IDs there.

Thanks,

-- 
Jiri Kosina
SUSE Labs