On Mon, Mar 25, 2019 at 11:57 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi,
>
> On 25-03-19 11:54, Benjamin Tissoires wrote:
> > On Thu, Mar 21, 2019 at 3:51 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >>
> >> Recent kernels allow the generic-hid driver to be used as fallback for
> >> devices with a specialized driver, when the hiddev is not listed in
> >> hid_have_special_driver. Over time we are removing more and more
> >> devices from the hid_have_special_driver table as devices get tested
> >> to support this setup.
> >>
> >> Before this commit the following happens when a HID device which has a
> >> special-driver and is no longer listed in hid_have_special_driver, gets
> >> enumerated:
> >>
> >> 1) device_add() gets called
> >> 2) bus_add_device() looks for a matching already registered hid driver,
> >> and bind hid-generic to the new device
> >> 3) kobject_uevent(&dev->kobj, KOBJ_ADD) gets called notifying userspace of
> >> the new hid_dev. udev calls modprobe based on the modalias in the uevent
> >> 4) The special driver gets loaded by modprobe
> >> 5) __hid_bus_reprobe_drivers() unbinds hid-generic and binds the new driver
> >>
> >> There are a couple of downsides to this:
> >>
> >> a) The probing messages printend when a HID driver bounds show up twice in
> >> dmesg, which is confusing for the user
> >>
> >> b) The (un)binding typically causes one or more evdev device-nodes to get
> >> (un)registed firing of udev events to which e.g. the xserver responds by
> >> (un)registering xinput devices and reporting this to interested clients.
> >> IOW the i. bind generic, ii. unbind generic, iii. bind special driver dance
> >> sets in motion a whole chain of events each step, while we really only want
> >> the events from step iii. to be reported to userspace.
> >>
> >> This commits introduces a request_module call before the device_add()
> >> call, so that the special-driver is loaded when step 2) looks for a
> >> matching driver and we directly bind the specialized driver.
> >>
> >> Note the request_module call translates to an execve("/sbin/modprobe", ...)
> >> and we now do this for each HID device added. So this is not entirely free,
> >> but adding HID devices is not something which happens 100s of times a
> >> second, so this should be fine.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> >> ---
> >
> > This seems good, but I do wonder if there is a catch or not.
> > IIRC (I might be wrong), calling request_module() would try to load a
> > module synchronously. So what happens if you are in the initramfs
> > where you do not have any extra modules?
> > Would this result in a timeout and add a delay or will this
> > "immediately" return?
>
> You are right that the /sbin/modprobe call is synchronously, that
> is request_module will wait for the modprobe process to exit, note
> it waits for modprobe to exit, not for the module to show up.
>
> > I am just worried this will not add some delays during boot.
>
> So AFAIK (and in my testing, I did specifically measure my systems
> boot time before and after), this will not cause additional delays,
> if the module is not present in the initrd, modprobe will simply fail
> to load it and exit immediately.
>
OK, good.
However, when testing the commit with a Logitech Unifying, hid-generic
still loads prior to hid-logitech-dj. But when creating the DJ
devices, hid-logitech-hidpp is loaded and we do not see hid-generic
for those devices.
Any ideas what could explain this?
Cheers,
Benjamin
> Regards,
>
> Hans
>
>
>
> >> drivers/hid/hid-core.c | 8 ++++++++
> >> 1 file changed, 8 insertions(+)
> >>
> >> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> >> index da7231f2944a..ee5fb8a1dd50 100644
> >> --- a/drivers/hid/hid-core.c
> >> +++ b/drivers/hid/hid-core.c
> >> @@ -2351,6 +2351,14 @@ int hid_add_device(struct hid_device *hdev)
> >> dev_set_name(&hdev->dev, "%04X:%04X:%04X.%04X", hdev->bus,
> >> hdev->vendor, hdev->product, atomic_inc_return(&id));
> >>
> >> + /*
> >> + * Try loading the module for the device before the add, so that we do
> >> + * not first have hid-generic binding only to have it replaced
> >> + * immediately afterwards with a specialized driver.
> >> + */
> >> + request_module("hid:b%04Xg%04Xv%08Xp%08X\n",
> >> + hdev->bus, hdev->group, hdev->vendor, hdev->product);
> >> +
> >> hid_debug_register(hdev, dev_name(&hdev->dev));
> >> ret = device_add(&hdev->dev);
> >> if (!ret)
> >> --
> >> 2.21.0
> >>
