Tree dumb questions from an occasional

Tree dumb questions from an occasional

[Marco Morandini]

First of all, please bear with me for writing this.

Should this appear email to be a criticism toward any of you,
be assured that this is not the intention. I'm
really grateful to all you guys, who keep improving the kernel
and allows us to use a free operating system.

My questions here below are basically along the lines of
"would it make sense to write this and that?". This is not
because I'd like someone else to do the work for me, but
because I really don't know whether it makes sense or not.
Should be wrong to make such type of questions without  
accompanying proofs of concept, then please ignore me, and
I'll go back into my cave.

Some background: I've recently bought a laser digital pointer
from HP. It connects through Bluetooth.
Since it did not work, I made a point to have it working,
even if I'm not a kernel developer, and do not plan to become a
kernel developer.
At the end, this turned out to be a two-line patch, adding
the HID_QUIRK_MULTI_INPUT for such device. Something that
would likely require less than 5 minutes 
of a not-so proficient kernel developer.

However, the process for me was much more cumbersome:
I had to navigate a lot o wrong or misleading documentation
in different forums, try to make sense of the kernel documentation,
understand what a HID descriptor is,
understand how to parse it, try to make sense of some 
unknown kernel code (mostly unsuccessfully), 
try with ebpf, try this and that... you get the idea.

Now, I'm writing because I _think_ I've learned something 
in the process, and perhaps it could be useful to share it.

Thus the questions:

1) do you think it would make sense to have some basic documentation
describing what a hid descriptor is, where to download the documents 
defining it (https://www.usb.org/ is linked from the docs, 
but this is not enough to get started, at least for someone like me), 
how to actually read it from the hardware, how to parse it... ? 
Very basic things, that, if I'm not wrong, are not currently 
covered by the kernel documentation, and that could allow 
someone else like me to get started more quickly?
If yes, I can try to write a skeleton for that, but I'm not sure
there will not be errors and/or omissions, thus it would likely need
to be fixed by someone more knowledgeable than me.

2) if I got it right, one can add a quirk like HID_QUIRK_MULTI_INPUT
while loading the usb_hid module, but not while loading the usb_generic
one (that turned out to be the module that manages my HP pointer), 
even if the statically defined quirks were moved into their own file. 
Would it make sense to add the possibility to
add quirks while loading hid_generic? Is this the right place for 
such code? If yes, I can try in my spare time to do this, 
even if I'm not sure I'll be able to get it right.

3) always if I get it right, currently it is not possible to inject quirks
using ebpf, but only to modify the HID descriptor. 
Is this correct? If yes, do you think it would be feasible and reasonable
to add such a  possibility? If yes, I can try in my spare time to do this, 
even if I'm not sure I'll be able to get it right.

Apologize again for this long email:
I understand it's full of good intentions but without any
significant contribution :(

Marco

Re: Tree dumb questions from an occasional

[Benjamin Tissoires]

Hi Marco,


On Mon, Jun 12, 2023 at 12:29 PM Marco Morandini
<marco.morandini@polimi.it> wrote:
>
> First of all, please bear with me for writing this.
>
> Should this appear email to be a criticism toward any of you,
> be assured that this is not the intention. I'm
> really grateful to all you guys, who keep improving the kernel
> and allows us to use a free operating system.
>
> My questions here below are basically along the lines of
> "would it make sense to write this and that?". This is not
> because I'd like someone else to do the work for me, but
> because I really don't know whether it makes sense or not.
> Should be wrong to make such type of questions without
> accompanying proofs of concept, then please ignore me, and
> I'll go back into my cave.

Asking questions is always fine. And given that you already did a lot
of homework, not answering would be rude ;)

>
> Some background: I've recently bought a laser digital pointer
> from HP. It connects through Bluetooth.
> Since it did not work, I made a point to have it working,
> even if I'm not a kernel developer, and do not plan to become a
> kernel developer.
> At the end, this turned out to be a two-line patch, adding
> the HID_QUIRK_MULTI_INPUT for such device. Something that
> would likely require less than 5 minutes
> of a not-so proficient kernel developer.

Yeah, right. This is the kind of situation where it's usually easy
enough to detect with hid-tools[0]. We can record the device on your
machine, then we can replay it locally on ours, and make several
attempts.

>
> However, the process for me was much more cumbersome:
> I had to navigate a lot o wrong or misleading documentation
> in different forums, try to make sense of the kernel documentation,
> understand what a HID descriptor is,
> understand how to parse it, try to make sense of some
> unknown kernel code (mostly unsuccessfully),
> try with ebpf, try this and that... you get the idea.

Heh, you tried hid-bpf :) thanks!

>
> Now, I'm writing because I _think_ I've learned something
> in the process, and perhaps it could be useful to share it.
>
> Thus the questions:
>
> 1) do you think it would make sense to have some basic documentation
> describing what a hid descriptor is, where to download the documents
> defining it (https://www.usb.org/ is linked from the docs,
> but this is not enough to get started, at least for someone like me),
> how to actually read it from the hardware, how to parse it... ?

Yes, very much yes. At least having pointers to various projects that
can read HID descriptors and parse them would be already better.

> Very basic things, that, if I'm not wrong, are not currently
> covered by the kernel documentation, and that could allow
> someone else like me to get started more quickly?
> If yes, I can try to write a skeleton for that, but I'm not sure
> there will not be errors and/or omissions, thus it would likely need
> to be fixed by someone more knowledgeable than me.

Sure. Please write (if you want) your first draft, we can review it
and we can iterate from there. Do not forget to add the linux doc
mailing list in CC in case some people from there want to also add
things.

>
> 2) if I got it right, one can add a quirk like HID_QUIRK_MULTI_INPUT
> while loading the usb_hid module, but not while loading the usb_generic
> one (that turned out to be the module that manages my HP pointer),
> even if the statically defined quirks were moved into their own file.
> Would it make sense to add the possibility to
> add quirks while loading hid_generic? Is this the right place for
> such code? If yes, I can try in my spare time to do this,
> even if I'm not sure I'll be able to get it right.

I'm not 100% sure of what you mean, but currently dynamic quirks can
be added to the *usbhid* module (not usb_hid or usb_generic), which is
the transport layer driver for HID.
This module is responsible for creating a HID device, which can be
handled by HID drivers, hid_generic being one of them.

As the name says, hid_generic is supposed to be generic, and I do not
want to see special cases handled there, because it would not be
generic anymore.

However, other drivers, (hid_multitouch for instance) can have a
.driver_data dynamically set too, which allows for quirking a device.
But the quirk is local to the driver.

Given that HID_QUIRK_MULTI_INPUT is a global HID quirk, it makes sense
IMO to add it at the usbhid level because you are just using the
hid-generic implementation.

Furthermore, if you submit your patch to the LKML, the quirk will
likely end up in driver/hid/hid-quirks.c which is exactly the static
equivalent of the dynamic one from usbhid.

So I don't think having such a new quirk mechanism makes sense.
Furthermore, that quirk mechanism allows for quick and dirty testing
of the impact on the kernel, but a proper submission as a kernel patch
ensures that everybody gets the same fix, so I'd rather not have a
forum that explains in details what to do for a given HID product when
we can just quirk it in the tree and forget.

>
> 3) always if I get it right, currently it is not possible to inject quirks
> using ebpf, but only to modify the HID descriptor.
> Is this correct?

You can also change the events and filter them out if you want, but
yes that's pretty much the extent of HID_BPF nowadays.

> If yes, do you think it would be feasible and reasonable
> to add such a  possibility? If yes, I can try in my spare time to do this,
> even if I'm not sure I'll be able to get it right.

Long story short: I would rather not have such bpf call.

The fundamental of HID-BPF is that you take a HID thingy, and you
output another HID thingy. Messing up with the kernel drivers is out
of the scope.

But furthermore, what would be the benefit compared to the already
available dynamic quirks the kernel allows to have? You can simply add
a configuration file to your system to locally have the dynamic quirk
set, and then it gets re-applied on every reboot.

As much as I'd like bpf to be the universal answer, this doesn't seem
to be a good replacement to what we currently have.

HID-BPF seems to be a good answer to replace (some) drivers, but I
don't think it should replace the generic kernel processing. So that's
why I don't think this is a good idea.

>
> Apologize again for this long email:
> I understand it's full of good intentions but without any
> significant contribution :(

No need to apologize. You are actually proposing ideas and your help
to make things better for end-users, which is extremely valuable in
itself :)

Cheers,
Benjamin

[0] https://gitlab.freedesktop.org/libevdev/hid-tools

Re: Tree dumb questions from an occasional

[Marco Morandini]

> Yeah, right. This is the kind of situation where it's usually easy
> enough to detect with hid-tools[0]. We can record the device on your
> machine, then we can replay it locally on ours, and make several
> attempts.

I was not aware of hid-tools, I will mention it in my doc attempt!

> Sure. Please write (if you want) your first draft, we can review it
> and we can iterate from there. Do not forget to add the linux doc
> mailing list in CC in case some people from there want to also add
> things.

Ok, will try.

>> 2) if I got it right, one can add a quirk like HID_QUIRK_MULTI_INPUT
>> while loading the usb_hid module, but not while loading the usb_generic
>> one (that turned out to be the module that manages my HP pointer),
>> even if the statically defined quirks were moved into their own file.
>> Would it make sense to add the possibility to
>> add quirks while loading hid_generic? Is this the right place for
>> such code? If yes, I can try in my spare time to do this,
>> even if I'm not sure I'll be able to get it right.
> 
> I'm not 100% sure of what you mean, but currently dynamic quirks can
> be added to the *usbhid* module (not usb_hid or usb_generic), which is
> the transport layer driver for HID.
> This module is responsible for creating a HID device, which can be
> handled by HID drivers, hid_generic being one of them.

You are right, I should have written usbhid.
I was convinced that hid_generic
did not get the quirks that were set at loading time by usbhid, but only those
defined in quirks.c .

What I really meant was that the quirk I ended up adding is

{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_HP, 0x464a), HID_QUIRK_MULTI_INPUT }

If I got it right usbhid can add only quirks with

BUS_USB (and not BUS_BLUETOOTH, like the above)

because of the code in usbhid/hid-core.c 

(

retval = hid_quirks_init(quirks_param, BUS_USB, MAX_USBHID_BOOT_QUIRKS); 

is this the right line in hid_init ?

)

and the fact that one cannot specify the
bus that should be used (whatever this "bus" means in the kernel, I
still need to get it):

MODULE_PARM_DESC(quirks, "Add/modify USB HID quirks by specifying "
                " quirks=vendorID:productID:quirks"
                " where vendorID, productID, and quirks are all in"
                " 0x-prefixed hex");

Long story short: if I

- either boot with 

usbhid.quirks=0x3f0:0x464a:0x40

- or, alternatively try to

sun:/home/marco/READMEs/HPElitePresenterMouse # rmmod usbhid 
sun:/home/marco/READMEs/HPElitePresenterMouse # modprobe -v usbhid "quirks=0x03f0:0x464a:0x40"
insmod /lib/modules/6.3.6-1-default/kernel/drivers/hid/usbhid/usbhid.ko.zst quirks=0x03f0:0x464a:0x40

my device does not work correctly, but with the added 

{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_HP, 0x464a), HID_QUIRK_MULTI_INPUT }

it does work. 

Hoping that I got it right and HID_QUIRK_MULTI_INPUT corresponds to
0x40, otherwise all what I've written make no sense, and I should go back
and re-do my homework....

At any rate: if there is a way to specify the correct quirk for a device like the one
I stumbled upon, while waiting for the correct upstream fix to percolate down
to the distributions, then of course my questions 2) and 3) (add the options to
specify quirks while loading hid-generic (question 2) and by resorting to ebpf (question 3))
do not make sense.

> As the name says, hid_generic is supposed to be generic, and I do not
> want to see special cases handled there, because it would not be
> generic anymore.

Understood, thank you.

 
> Furthermore, if you submit your patch to the LKML, the quirk will
> likely end up in driver/hid/hid-quirks.c which is exactly the static
> equivalent of the dynamic one from usbhid.

Not exactly, because of the bus issue (again, assuming I got it right).

> No need to apologize. You are actually proposing ideas and your help
> to make things better for end-users, which is extremely valuable in
> itself :)

Thank you, you are very kind. I only hope I've not written too much
nonsense here above.

Marco

Re: Tree dumb questions from an occasional

[Benjamin Tissoires]

On Mon, Jun 12, 2023 at 9:33 PM Marco Morandini
<marco.morandini@polimi.it> wrote:
>
> > Yeah, right. This is the kind of situation where it's usually easy
> > enough to detect with hid-tools[0]. We can record the device on your
> > machine, then we can replay it locally on ours, and make several
> > attempts.
>
> I was not aware of hid-tools, I will mention it in my doc attempt!
>
> > Sure. Please write (if you want) your first draft, we can review it
> > and we can iterate from there. Do not forget to add the linux doc
> > mailing list in CC in case some people from there want to also add
> > things.
>
> Ok, will try.

\o/

>
> >> 2) if I got it right, one can add a quirk like HID_QUIRK_MULTI_INPUT
> >> while loading the usb_hid module, but not while loading the usb_generic
> >> one (that turned out to be the module that manages my HP pointer),
> >> even if the statically defined quirks were moved into their own file.
> >> Would it make sense to add the possibility to
> >> add quirks while loading hid_generic? Is this the right place for
> >> such code? If yes, I can try in my spare time to do this,
> >> even if I'm not sure I'll be able to get it right.
> >
> > I'm not 100% sure of what you mean, but currently dynamic quirks can
> > be added to the *usbhid* module (not usb_hid or usb_generic), which is
> > the transport layer driver for HID.
> > This module is responsible for creating a HID device, which can be
> > handled by HID drivers, hid_generic being one of them.
>
> You are right, I should have written usbhid.
> I was convinced that hid_generic
> did not get the quirks that were set at loading time by usbhid, but only those
> defined in quirks.c .
>
> What I really meant was that the quirk I ended up adding is
>
> { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_HP, 0x464a), HID_QUIRK_MULTI_INPUT }
>
> If I got it right usbhid can add only quirks with
>
> BUS_USB (and not BUS_BLUETOOTH, like the above)

Yes, that is correct

>
> because of the code in usbhid/hid-core.c
>
> (
>
> retval = hid_quirks_init(quirks_param, BUS_USB, MAX_USBHID_BOOT_QUIRKS);
>
> is this the right line in hid_init ?
>
> )
>
> and the fact that one cannot specify the
> bus that should be used (whatever this "bus" means in the kernel, I
> still need to get it):

The bus is the transport layer that exposes the HID device.

If you plug in your mouse over USB, then the bus is BUS_USB. If the
mouse is wirelessly connected through Bluetooth, then it's
BUS_BLUETOOTH, and if your touchpad is integrated in the laptop and is
using I2C to communicate, the bus is BUS_I2C :)

Each bus has its own transport driver (usbhid, i2c_hid, hidp
(Bluetooth classic), uhid (BLE)) and they all translate the data from
the original bus into HID.

>
> MODULE_PARM_DESC(quirks, "Add/modify USB HID quirks by specifying "
>                 " quirks=vendorID:productID:quirks"
>                 " where vendorID, productID, and quirks are all in"
>                 " 0x-prefixed hex");
>
> Long story short: if I
>
> - either boot with
>
> usbhid.quirks=0x3f0:0x464a:0x40
>
> - or, alternatively try to
>
> sun:/home/marco/READMEs/HPElitePresenterMouse # rmmod usbhid
> sun:/home/marco/READMEs/HPElitePresenterMouse # modprobe -v usbhid "quirks=0x03f0:0x464a:0x40"
> insmod /lib/modules/6.3.6-1-default/kernel/drivers/hid/usbhid/usbhid.ko.zst quirks=0x03f0:0x464a:0x40
>
> my device does not work correctly, but with the added
>
> { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_HP, 0x464a), HID_QUIRK_MULTI_INPUT }
>
> it does work.

That is correct, because usbhid is only responsible for USB devices
(which is the vast majority of devices), and hidp (nor uhid) doesn't
have such dynamic quirks.

>
> Hoping that I got it right and HID_QUIRK_MULTI_INPUT corresponds to
> 0x40, otherwise all what I've written make no sense, and I should go back
> and re-do my homework....
>
> At any rate: if there is a way to specify the correct quirk for a device like the one
> I stumbled upon, while waiting for the correct upstream fix to percolate down
> to the distributions, then of course my questions 2) and 3) (add the options to
> specify quirks while loading hid-generic (question 2) and by resorting to ebpf (question 3))
> do not make sense.

OK. The missing point was that you were using a Bluetooth device, and
not a USB one. And that makes a big difference, because yes, you can
not dynamically quirk devices for anything but USB.

I still stand by the fact that hid_generic is not the correct place.
These kinds of quirks are global to the device, and putting them in
hid_generic would make it the wrong place IMO.

Actually, the one place where it would make sense to have such dynamic
quirks is in the hid-core (hid.ko) module itself. It would make sense
to have a BUS:VID:PID:QUIRKS parameter.
But having such a parameter is not without constraints, because it's
not really "dynamic", and we can only set a limited number of quirks.

In your particular case, we might as well use an HID-BPF program that
tweaks the report descriptor which would force the kernel to "use" the
multi-input quirk.

Would you mind attaching the output of hid-recorder while you do some
HID events and where you show the bug?

Also, FWIW, the number of MULTI_INPUT quirk required in the kernel is
probably a sign that we are not using the best default implementation,
and I've already been pinged to change that. I couldn't find the time
to get back to this, but your device might also help me in having a
broader range of use cases so that we can ditch that quirk once and
for all.

Cheers.
Benjamin

>
> > As the name says, hid_generic is supposed to be generic, and I do not
> > want to see special cases handled there, because it would not be
> > generic anymore.
>
> Understood, thank you.
>
>
> > Furthermore, if you submit your patch to the LKML, the quirk will
> > likely end up in driver/hid/hid-quirks.c which is exactly the static
> > equivalent of the dynamic one from usbhid.
>
> Not exactly, because of the bus issue (again, assuming I got it right).
>
> > No need to apologize. You are actually proposing ideas and your help
> > to make things better for end-users, which is extremely valuable in
> > itself :)
>
> Thank you, you are very kind. I only hope I've not written too much
> nonsense here above.
>
> Marco
>

Re: Tree dumb questions from an occasional

[Benjamin Tissoires]

On Wed, Jun 14, 2023 at 12:18 PM Marco Morandini <marco.morandini@polimi.it> wrote:
>
> > Actually, the one place where it would make sense to have such dynamic
> > quirks is in the hid-core (hid.ko) module itself. It would make sense
> > to have a BUS:VID:PID:QUIRKS parameter.
> > But having such a parameter is not without constraints, because it's
> > not really "dynamic", and we can only set a limited number of quirks.
>
> Ok
>
> >
> > In your particular case, we might as well use an HID-BPF program that
> > tweaks the report descriptor which would force the kernel to "use" the
> > multi-input quirk.
>
> If this means that it could be a nice example (something you would put in
> samples/hid) for HID-BPF, this would be great
> (and I would be curious to understand how to do it).
> But don't waste time for me: the patch is already in your
> for-next and for-6.4/upstream-fixes branches, and for sure I can wait
> and deal with what I have right now.

The program is actually simple: knowing that the kernel splits by
application collection, we can replace the second collection from being
exported as a mouse into a pointer:

See the branch hp_elite_presenter of https://gitlab.freedesktop.org/bentiss/udev-hid-bpf/-/tree/hp_elite_presenter

The program just replaces the byte 79 from 0x02 (mouse) to 0x01
(presenter):

---
SEC("fmod_ret/hid_bpf_rdesc_fixup")
int BPF_PROG(hid_fix_rdesc, struct hid_bpf_ctx *hctx)
{
	__u8 *data = hid_bpf_get_data(hctx, 0 /* offset */, 4096 /* size */);

	if (!data)
		return 0; /* EPERM check */

	/* replace application mouse by application pointer on the second collection */
	if (data[79] == 0x02)
		data[79] = 0x01;

	return 0;
}
---

>
> > Would you mind attaching the output of hid-recorder while you do some
> > HID events and where you show the bug?
>
> Attached; I've tried to use all the mouses/buttons. Do you need the same
> kind of recording taken with the patched kernel?

Nope, no need. hid-recorder is what comes from the device, so the kernel
processing depends on my current tree, which explains how I can test the
various quirks :)

>
> You'll notice that the HID descriptor advertises two mouses and
> two consumer controls, each with different Report IDs.
> This is because this device can be used in two different
> configurations: one is a traditional mouse, that you use on your desk.
> If you turn the device, then you access the second one,
> where you are dealing with a virtual pointer:
> two gyroscopes sense the change of attitude of the device,
> and you can control the cursor by waving around the pointer.
> The buttons that you use in the two different configurations
> are different as well.
>
> > Also, FWIW, the number of MULTI_INPUT quirk required in the kernel is
> > probably a sign that we are not using the best default implementation,
> > and I've already been pinged to change that. I couldn't find the time
> > to get back to this, but your device might also help me in having a
> > broader range of use cases so that we can ditch that quirk once and
> > for all.
>
> I was clumsy looking around trying to understand why it's better
> not to have it as a default, but for sure there are good reason
> for the actual behavior. Perhaps this has to do with the fact that
> you don't want to have duplicated OUTPUTs (if there are any)?
> e.g. https://lkml.iu.edu/hypermail/linux/kernel/0701.1/1132.html ?
>

IIRC it was mostly because some devices were having separate
declarations for their input/output and features, and they were not
especially grouped together. The multi_input quirk splits features/input
/output in different devices, making it harder to configure (in case of
the multitouch ones, where to need to set an operating mode), and the
default was just having one input node for the whole HID device.

Cheers,
Benjamin