On Mon, Jun 12, 2023 at 9:33 PM Marco Morandini
<marco.morandini@xxxxxxxxx> 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
>
