| Message ID | 20190321145119.14530-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: core: Call request_module before doing device_add | expand |
On Thu, Mar 21, 2019 at 3:51 PM Hans de Goede <hdegoede@redhat.com> 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@redhat.com> > --- 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 >
Hi, On 25-03-19 11:54, Benjamin Tissoires wrote: > On Thu, Mar 21, 2019 at 3:51 PM Hans de Goede <hdegoede@redhat.com> 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@redhat.com> >> --- > > 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. 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 >>
On Mon, Mar 25, 2019 at 11:57 AM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 25-03-19 11:54, Benjamin Tissoires wrote: > > On Thu, Mar 21, 2019 at 3:51 PM Hans de Goede <hdegoede@redhat.com> 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@redhat.com> > >> --- > > > > 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 > >>
Hi, On 25-03-19 16:12, Benjamin Tissoires wrote: > On Mon, Mar 25, 2019 at 11:57 AM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 25-03-19 11:54, Benjamin Tissoires wrote: >>> On Thu, Mar 21, 2019 at 3:51 PM Hans de Goede <hdegoede@redhat.com> 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@redhat.com> >>>> --- >>> >>> 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? First hunch: the creation of hid_device-s from the usbhid driver does not go through hid_add_device ? Just a hunch I did not check. Maybe add a pr_err before the request_module to check this? Regards, Hans > > 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 >>>>
Hi, On 25-03-19 16:17, Hans de Goede wrote: > Hi, > > On 25-03-19 16:12, Benjamin Tissoires wrote: >> On Mon, Mar 25, 2019 at 11:57 AM Hans de Goede <hdegoede@redhat.com> wrote: >>> >>> Hi, >>> >>> On 25-03-19 11:54, Benjamin Tissoires wrote: >>>> On Thu, Mar 21, 2019 at 3:51 PM Hans de Goede <hdegoede@redhat.com> 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@redhat.com> >>>>> --- >>>> >>>> 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? > > First hunch: the creation of hid_device-s from the usbhid driver does > not go through hid_add_device ? Just a hunch I did not check. Ok, so that clearly is not the cause. I can reproduce the problem and after 1.5 hours of debugging I've this fixed now. The problem is we should not add a '\n' at the end of the string we pass to request_module. I copied the code building the string from modalias_show() where the \n does belong and did not notice this :| The reasons things were working with hid-logitech-dj.ko is because it contains the following modalias: "hid:b0003g0102v0000046Dp*" where the ending '*' happily matches the superfluous '\n' at the end. I'll send a v2 fixing this. Regards, Hans
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)
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@redhat.com> --- drivers/hid/hid-core.c | 8 ++++++++ 1 file changed, 8 insertions(+)