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?
I am just worried this will not add some delays during boot.
Cheers,
Benjamin
> 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
>
