| Message ID | 20241104164824.1213529-1-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: hyperv: streamline driver probe to avoid devres issues | expand |
On Mon, Nov 04, 2024 at 05:48:24PM +0100, Vitaly Kuznetsov wrote: > It was found that unloading 'hid_hyperv' module results in a devres > complaint: > > ... > hv_vmbus: unregistering driver hid_hyperv > ------------[ cut here ]------------ > WARNING: CPU: 2 PID: 3983 at drivers/base/devres.c:691 devres_release_group+0x1f2/0x2c0 > ... > Call Trace: > <TASK> > ? devres_release_group+0x1f2/0x2c0 > ? __warn+0xd1/0x1c0 > ? devres_release_group+0x1f2/0x2c0 > ? report_bug+0x32a/0x3c0 > ? handle_bug+0x53/0xa0 > ? exc_invalid_op+0x18/0x50 > ? asm_exc_invalid_op+0x1a/0x20 > ? devres_release_group+0x1f2/0x2c0 > ? devres_release_group+0x90/0x2c0 > ? rcu_is_watching+0x15/0xb0 > ? __pfx_devres_release_group+0x10/0x10 > hid_device_remove+0xf5/0x220 > device_release_driver_internal+0x371/0x540 > ? klist_put+0xf3/0x170 > bus_remove_device+0x1f1/0x3f0 > device_del+0x33f/0x8c0 > ? __pfx_device_del+0x10/0x10 > ? cleanup_srcu_struct+0x337/0x500 > hid_destroy_device+0xc8/0x130 > mousevsc_remove+0xd2/0x1d0 [hid_hyperv] > device_release_driver_internal+0x371/0x540 > driver_detach+0xc5/0x180 > bus_remove_driver+0x11e/0x2a0 > ? __mutex_unlock_slowpath+0x160/0x5e0 > vmbus_driver_unregister+0x62/0x2b0 [hv_vmbus] > ... > > And the issue seems to be that the corresponding devres group is not > allocated. Normally, devres_open_group() is called from > __hid_device_probe() but Hyper-V HID driver overrides 'hid_dev->driver' > with 'mousevsc_hid_driver' stub and basically re-implements > __hid_device_probe() by calling hid_parse() and hid_hw_start() but not > devres_open_group(). hid_device_probe() does not call __hid_device_probe() > for it. Later, when the driver is removed, hid_device_remove() calls > devres_release_group() as it doesn't check whether hdev->driver was > initially overridden or not. > > The issue seems to be related to the commit 62c68e7cee33 ("HID: ensure > timely release of driver-allocated resources") but the commit itself seems > to be correct. > > Fix the issue by dropping the 'hid_dev->driver' override and the > now unneeded hid_parse()/hid_hw_start() calls. One notable difference of > the change is hid_hw_start() is now called with HID_CONNECT_DEFAULT which > implies HID_CONNECT_HIDRAW. This doesn't seem to cause any immediate issues > but 'HID_CONNECT_HIDINPUT | HID_CONNECT_HIDDEV' combo was used in the > driver for a long time and it is unclear whether hidraw was excluded on > purpose or not. > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> A fixme tag would be helpful. > --- > drivers/hid/hid-hyperv.c | 17 ----------------- > 1 file changed, 17 deletions(-) > > diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c > index f33485d83d24..1609a56ffa7c 100644 > --- a/drivers/hid/hid-hyperv.c > +++ b/drivers/hid/hid-hyperv.c > @@ -431,8 +431,6 @@ static const struct hid_ll_driver mousevsc_ll_driver = { > .raw_request = mousevsc_hid_raw_request, > }; > > -static struct hid_driver mousevsc_hid_driver; > - > static int mousevsc_probe(struct hv_device *device, > const struct hv_vmbus_device_id *dev_id) > { > @@ -473,7 +471,6 @@ static int mousevsc_probe(struct hv_device *device, > } > > hid_dev->ll_driver = &mousevsc_ll_driver; > - hid_dev->driver = &mousevsc_hid_driver; > hid_dev->bus = BUS_VIRTUAL; > hid_dev->vendor = input_dev->hid_dev_info.vendor; > hid_dev->product = input_dev->hid_dev_info.product; > @@ -488,20 +485,6 @@ static int mousevsc_probe(struct hv_device *device, > if (ret) > goto probe_err2; > > - > - ret = hid_parse(hid_dev); > - if (ret) { > - hid_err(hid_dev, "parse failed\n"); > - goto probe_err2; > - } > - > - ret = hid_hw_start(hid_dev, HID_CONNECT_HIDINPUT | HID_CONNECT_HIDDEV); > - > - if (ret) { > - hid_err(hid_dev, "hw start failed\n"); > - goto probe_err2; > - } > - > device_init_wakeup(&device->device, true); > > input_dev->connected = true; > -- > 2.47.0 > I have tested this patch, but the original issue reported in commit message is not observed in latest kernel due to an other error reported by Michael here: https://lore.kernel.org/linux-hyperv/SN6PR02MB41573CDE3E478455D17837DED4502@SN6PR02MB4157.namprd02.prod.outlook.com/ The error addressed by this patch is observed before the commit "8b7fd6a15f8c: HID: bpf: move HID-BPF report descriptor fixup earlier", and is resolved by this patch. In fact, this patch appears to fix both issues. Tested-by: Saurabh Sengar <ssengar@linux.microsoft.com> - Saurabh
Saurabh Singh Sengar <ssengar@linux.microsoft.com> writes: > On Mon, Nov 04, 2024 at 05:48:24PM +0100, Vitaly Kuznetsov wrote: >> It was found that unloading 'hid_hyperv' module results in a devres >> complaint: >> >> ... >> hv_vmbus: unregistering driver hid_hyperv >> ------------[ cut here ]------------ >> WARNING: CPU: 2 PID: 3983 at drivers/base/devres.c:691 devres_release_group+0x1f2/0x2c0 >> ... >> Call Trace: >> <TASK> >> ? devres_release_group+0x1f2/0x2c0 >> ? __warn+0xd1/0x1c0 >> ? devres_release_group+0x1f2/0x2c0 >> ? report_bug+0x32a/0x3c0 >> ? handle_bug+0x53/0xa0 >> ? exc_invalid_op+0x18/0x50 >> ? asm_exc_invalid_op+0x1a/0x20 >> ? devres_release_group+0x1f2/0x2c0 >> ? devres_release_group+0x90/0x2c0 >> ? rcu_is_watching+0x15/0xb0 >> ? __pfx_devres_release_group+0x10/0x10 >> hid_device_remove+0xf5/0x220 >> device_release_driver_internal+0x371/0x540 >> ? klist_put+0xf3/0x170 >> bus_remove_device+0x1f1/0x3f0 >> device_del+0x33f/0x8c0 >> ? __pfx_device_del+0x10/0x10 >> ? cleanup_srcu_struct+0x337/0x500 >> hid_destroy_device+0xc8/0x130 >> mousevsc_remove+0xd2/0x1d0 [hid_hyperv] >> device_release_driver_internal+0x371/0x540 >> driver_detach+0xc5/0x180 >> bus_remove_driver+0x11e/0x2a0 >> ? __mutex_unlock_slowpath+0x160/0x5e0 >> vmbus_driver_unregister+0x62/0x2b0 [hv_vmbus] >> ... >> >> And the issue seems to be that the corresponding devres group is not >> allocated. Normally, devres_open_group() is called from >> __hid_device_probe() but Hyper-V HID driver overrides 'hid_dev->driver' >> with 'mousevsc_hid_driver' stub and basically re-implements >> __hid_device_probe() by calling hid_parse() and hid_hw_start() but not >> devres_open_group(). hid_device_probe() does not call __hid_device_probe() >> for it. Later, when the driver is removed, hid_device_remove() calls >> devres_release_group() as it doesn't check whether hdev->driver was >> initially overridden or not. >> >> The issue seems to be related to the commit 62c68e7cee33 ("HID: ensure >> timely release of driver-allocated resources") but the commit itself seems >> to be correct. >> >> Fix the issue by dropping the 'hid_dev->driver' override and the >> now unneeded hid_parse()/hid_hw_start() calls. One notable difference of >> the change is hid_hw_start() is now called with HID_CONNECT_DEFAULT which >> implies HID_CONNECT_HIDRAW. This doesn't seem to cause any immediate issues >> but 'HID_CONNECT_HIDINPUT | HID_CONNECT_HIDDEV' combo was used in the >> driver for a long time and it is unclear whether hidraw was excluded on >> purpose or not. >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > A fixme tag would be helpful. I concluded that it's the unusual 'hid_dev->driver' override in hid-hyperv to blame and not the 62c68e7cee33 ("HID: ensure timely release of driver-allocated resources") but the override was there since the inception of the driver so I'm not sure, mentioning 62c68e7cee33 probably makes more sense... > >> --- >> drivers/hid/hid-hyperv.c | 17 ----------------- >> 1 file changed, 17 deletions(-) >> >> diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c >> index f33485d83d24..1609a56ffa7c 100644 >> --- a/drivers/hid/hid-hyperv.c >> +++ b/drivers/hid/hid-hyperv.c >> @@ -431,8 +431,6 @@ static const struct hid_ll_driver mousevsc_ll_driver = { >> .raw_request = mousevsc_hid_raw_request, >> }; >> >> -static struct hid_driver mousevsc_hid_driver; >> - >> static int mousevsc_probe(struct hv_device *device, >> const struct hv_vmbus_device_id *dev_id) >> { >> @@ -473,7 +471,6 @@ static int mousevsc_probe(struct hv_device *device, >> } >> >> hid_dev->ll_driver = &mousevsc_ll_driver; >> - hid_dev->driver = &mousevsc_hid_driver; >> hid_dev->bus = BUS_VIRTUAL; >> hid_dev->vendor = input_dev->hid_dev_info.vendor; >> hid_dev->product = input_dev->hid_dev_info.product; >> @@ -488,20 +485,6 @@ static int mousevsc_probe(struct hv_device *device, >> if (ret) >> goto probe_err2; >> >> - >> - ret = hid_parse(hid_dev); >> - if (ret) { >> - hid_err(hid_dev, "parse failed\n"); >> - goto probe_err2; >> - } >> - >> - ret = hid_hw_start(hid_dev, HID_CONNECT_HIDINPUT | HID_CONNECT_HIDDEV); >> - >> - if (ret) { >> - hid_err(hid_dev, "hw start failed\n"); >> - goto probe_err2; >> - } >> - >> device_init_wakeup(&device->device, true); >> >> input_dev->connected = true; >> -- >> 2.47.0 >> > > I have tested this patch, but the original issue reported in commit message is > not observed in latest kernel due to an other error reported by Michael here: > https://lore.kernel.org/linux-hyperv/SN6PR02MB41573CDE3E478455D17837DED4502@SN6PR02MB4157.namprd02.prod.outlook.com/ > Let's Cc: Michael then! > The error addressed by this patch is observed before the commit > "8b7fd6a15f8c: HID: bpf: move HID-BPF report descriptor fixup earlier", > and is resolved by this patch. In fact, this patch appears to fix both issues. > > Tested-by: Saurabh Sengar <ssengar@linux.microsoft.com> Thanks! I was reproducing the issue on 6.12-rc6 by just doing 'rmmod hid_hyperv' and tested my patch there.
From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Tuesday, November 5, 2024 9:45 AM > > Saurabh Singh Sengar <ssengar@linux.microsoft.com> writes: > > > On Mon, Nov 04, 2024 at 05:48:24PM +0100, Vitaly Kuznetsov wrote: > >> It was found that unloading 'hid_hyperv' module results in a devres > >> complaint: > >> > >> ... > >> hv_vmbus: unregistering driver hid_hyperv > >> ------------[ cut here ]------------ > >> WARNING: CPU: 2 PID: 3983 at drivers/base/devres.c:691 devres_release_group+0x1f2/0x2c0 > >> ... > >> Call Trace: > >> <TASK> > >> ? devres_release_group+0x1f2/0x2c0 > >> ? __warn+0xd1/0x1c0 > >> ? devres_release_group+0x1f2/0x2c0 > >> ? report_bug+0x32a/0x3c0 > >> ? handle_bug+0x53/0xa0 > >> ? exc_invalid_op+0x18/0x50 > >> ? asm_exc_invalid_op+0x1a/0x20 > >> ? devres_release_group+0x1f2/0x2c0 > >> ? devres_release_group+0x90/0x2c0 > >> ? rcu_is_watching+0x15/0xb0 > >> ? __pfx_devres_release_group+0x10/0x10 > >> hid_device_remove+0xf5/0x220 > >> device_release_driver_internal+0x371/0x540 > >> ? klist_put+0xf3/0x170 > >> bus_remove_device+0x1f1/0x3f0 > >> device_del+0x33f/0x8c0 > >> ? __pfx_device_del+0x10/0x10 > >> ? cleanup_srcu_struct+0x337/0x500 > >> hid_destroy_device+0xc8/0x130 > >> mousevsc_remove+0xd2/0x1d0 [hid_hyperv] > >> device_release_driver_internal+0x371/0x540 > >> driver_detach+0xc5/0x180 > >> bus_remove_driver+0x11e/0x2a0 > >> ? __mutex_unlock_slowpath+0x160/0x5e0 > >> vmbus_driver_unregister+0x62/0x2b0 [hv_vmbus] > >> ... > >> > >> And the issue seems to be that the corresponding devres group is not > >> allocated. Normally, devres_open_group() is called from > >> __hid_device_probe() but Hyper-V HID driver overrides 'hid_dev->driver' > >> with 'mousevsc_hid_driver' stub and basically re-implements > >> __hid_device_probe() by calling hid_parse() and hid_hw_start() but not > >> devres_open_group(). hid_device_probe() does not call __hid_device_probe() > >> for it. Later, when the driver is removed, hid_device_remove() calls > >> devres_release_group() as it doesn't check whether hdev->driver was > >> initially overridden or not. > >> > >> The issue seems to be related to the commit 62c68e7cee33 ("HID: ensure > >> timely release of driver-allocated resources") but the commit itself seems > >> to be correct. > >> > >> Fix the issue by dropping the 'hid_dev->driver' override and the > >> now unneeded hid_parse()/hid_hw_start() calls. One notable difference of > >> the change is hid_hw_start() is now called with HID_CONNECT_DEFAULT which > >> implies HID_CONNECT_HIDRAW. This doesn't seem to cause any immediate issues > >> but 'HID_CONNECT_HIDINPUT | HID_CONNECT_HIDDEV' combo was used in the > >> driver for a long time and it is unclear whether hidraw was excluded on > >> purpose or not. > >> > >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > > > A fixme tag would be helpful. > > I concluded that it's the unusual 'hid_dev->driver' override in > hid-hyperv to blame and not the 62c68e7cee33 ("HID: ensure timely > release of driver-allocated resources") but the override was there since > the inception of the driver so I'm not sure, mentioning 62c68e7cee33 > probably makes more sense... I've finished looking at the linux-next issue in detail, and I agree that the hid_dev->driver override is the underlying cause. I was commenting out that line Monday night, but had not gotten as far as removing the the hid_parse() and hid_hw_start(). Then your patch came out, Vitaly, which is great! > > > > >> --- > >> drivers/hid/hid-hyperv.c | 17 ----------------- > >> 1 file changed, 17 deletions(-) > >> > >> diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c > >> index f33485d83d24..1609a56ffa7c 100644 > >> --- a/drivers/hid/hid-hyperv.c > >> +++ b/drivers/hid/hid-hyperv.c > >> @@ -431,8 +431,6 @@ static const struct hid_ll_driver mousevsc_ll_driver = { > >> .raw_request = mousevsc_hid_raw_request, > >> }; > >> > >> -static struct hid_driver mousevsc_hid_driver; > >> - > >> static int mousevsc_probe(struct hv_device *device, > >> const struct hv_vmbus_device_id *dev_id) > >> { > >> @@ -473,7 +471,6 @@ static int mousevsc_probe(struct hv_device *device, > >> } > >> > >> hid_dev->ll_driver = &mousevsc_ll_driver; > >> - hid_dev->driver = &mousevsc_hid_driver; > >> hid_dev->bus = BUS_VIRTUAL; > >> hid_dev->vendor = input_dev->hid_dev_info.vendor; > >> hid_dev->product = input_dev->hid_dev_info.product; > >> @@ -488,20 +485,6 @@ static int mousevsc_probe(struct hv_device *device, > >> if (ret) > >> goto probe_err2; > >> > >> - > >> - ret = hid_parse(hid_dev); > >> - if (ret) { > >> - hid_err(hid_dev, "parse failed\n"); > >> - goto probe_err2; > >> - } > >> - > >> - ret = hid_hw_start(hid_dev, HID_CONNECT_HIDINPUT | HID_CONNECT_HIDDEV); As you noted, using HID_CONNECT_DEFAULT in the default probe function __hid_device_probe() ends up adding HID_CONNECT_HIDRAW and HID_CONNECT_FF. The latter is benign as it only affects devices that support force-feedback. As best I can tell, HID_CNNECT_HIDRAW causes /dev/hidraw0 to appear, which provides a raw interface to the mouse device. See https://docs.kernel.org/hid/hidraw.html. It doesn't seem like making this interface visible hurts anything, but I'm not 100% sure. The alternative is to keep the "struct hid_driver mousevsc_hid_driver;" line and to populate it with a name, id_table, and an HID probe function specific to the Hyper-V mouse. Then instead of the incorrect assignment to hid_dev->driver, add a module_hid_driver(mousevsc_hid_driver); statement, which registers the driver. The new HID probe function does the hid_parse() and hid_hw_start() which have been removed from mousevsc_probe() as your patch does. With this arrangement, the hid_hw_start() can be done with the desired HID_CONNECT_* options so that /dev/hidraw0 won't appear. It's only a few lines of code. I can try to code up this approach if it is preferred. But I'm likely tied up with some personal things for the next few days, so might not get to it for a little while. Feel free to try it yourself if you want. Question: Have you tested your change with an app that consumes mouse data in the Hyper-V console window? I was trying to figure out a convenient way to do that. I usually install the server version of Linux distros and use only the command line, so didn't have a mouse-consuming app easily available. > >> - > >> - if (ret) { > >> - hid_err(hid_dev, "hw start failed\n"); > >> - goto probe_err2; > >> - } > >> - > >> device_init_wakeup(&device->device, true); > >> > >> input_dev->connected = true; > >> -- > >> 2.47.0 > >> > > > > I have tested this patch, but the original issue reported in commit message is > > not observed in latest kernel due to an other error reported by Michael here: > > https://lore.kernel.org/linux-hyperv/SN6PR02MB41573CDE3E478455D17837DED4502@SN6PR02MB4157.namprd02.prod.outlook.com/ > > > > Let's Cc: Michael then! > > > The error addressed by this patch is observed before the commit > > "8b7fd6a15f8c: HID: bpf: move HID-BPF report descriptor fixup earlier", > > and is resolved by this patch. In fact, this patch appears to fix both issues. > > > > Tested-by: Saurabh Sengar <ssengar@linux.microsoft.com> Yes, I agree that Vitaly's patch fixes the problem I observed in linux-next. I've looked at the details enough to believe that the fix is the correct fix. > > Thanks! I was reproducing the issue on 6.12-rc6 by just doing 'rmmod > hid_hyperv' and tested my patch there. FWIW, I also tested device unbind/bind of the VMBus mousevsc device, and its companion HID device that is created in mousevsc_probe(). Doing unbind/bind for the VMBus mousevsc device works correctly, and the companion HID device goes away and comes back as expected. Doing unbind/bind for just the HID device also works, and the VMBus mousevsc device is not affected, again as expected. Michael
From: Michael Kelley Sent: Wednesday, November 6, 2024 10:36 AM > From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Tuesday, November 5, 2024 > 9:45 AM > > > > Saurabh Singh Sengar <ssengar@linux.microsoft.com> writes: > > > > > On Mon, Nov 04, 2024 at 05:48:24PM +0100, Vitaly Kuznetsov wrote: > > >> It was found that unloading 'hid_hyperv' module results in a devres > > >> complaint: > > >> > > >> ... > > >> hv_vmbus: unregistering driver hid_hyperv > > >> ------------[ cut here ]------------ > > >> WARNING: CPU: 2 PID: 3983 at drivers/base/devres.c:691 > devres_release_group+0x1f2/0x2c0 > > >> ... > > >> Call Trace: > > >> <TASK> > > >> ? devres_release_group+0x1f2/0x2c0 > > >> ? __warn+0xd1/0x1c0 > > >> ? devres_release_group+0x1f2/0x2c0 > > >> ? report_bug+0x32a/0x3c0 > > >> ? handle_bug+0x53/0xa0 > > >> ? exc_invalid_op+0x18/0x50 > > >> ? asm_exc_invalid_op+0x1a/0x20 > > >> ? devres_release_group+0x1f2/0x2c0 > > >> ? devres_release_group+0x90/0x2c0 > > >> ? rcu_is_watching+0x15/0xb0 > > >> ? __pfx_devres_release_group+0x10/0x10 > > >> hid_device_remove+0xf5/0x220 > > >> device_release_driver_internal+0x371/0x540 > > >> ? klist_put+0xf3/0x170 > > >> bus_remove_device+0x1f1/0x3f0 > > >> device_del+0x33f/0x8c0 > > >> ? __pfx_device_del+0x10/0x10 > > >> ? cleanup_srcu_struct+0x337/0x500 > > >> hid_destroy_device+0xc8/0x130 > > >> mousevsc_remove+0xd2/0x1d0 [hid_hyperv] > > >> device_release_driver_internal+0x371/0x540 > > >> driver_detach+0xc5/0x180 > > >> bus_remove_driver+0x11e/0x2a0 > > >> ? __mutex_unlock_slowpath+0x160/0x5e0 > > >> vmbus_driver_unregister+0x62/0x2b0 [hv_vmbus] > > >> ... > > >> > > >> And the issue seems to be that the corresponding devres group is not > > >> allocated. Normally, devres_open_group() is called from > > >> __hid_device_probe() but Hyper-V HID driver overrides 'hid_dev->driver' > > >> with 'mousevsc_hid_driver' stub and basically re-implements > > >> __hid_device_probe() by calling hid_parse() and hid_hw_start() but not > > >> devres_open_group(). hid_device_probe() does not call __hid_device_probe() > > >> for it. Later, when the driver is removed, hid_device_remove() calls > > >> devres_release_group() as it doesn't check whether hdev->driver was > > >> initially overridden or not. > > >> > > >> The issue seems to be related to the commit 62c68e7cee33 ("HID: ensure > > >> timely release of driver-allocated resources") but the commit itself seems > > >> to be correct. > > >> > > >> Fix the issue by dropping the 'hid_dev->driver' override and the > > >> now unneeded hid_parse()/hid_hw_start() calls. One notable difference of > > >> the change is hid_hw_start() is now called with HID_CONNECT_DEFAULT which > > >> implies HID_CONNECT_HIDRAW. This doesn't seem to cause any immediate issues > > >> but 'HID_CONNECT_HIDINPUT | HID_CONNECT_HIDDEV' combo was used in the > > >> driver for a long time and it is unclear whether hidraw was excluded on > > >> purpose or not. > > >> > > >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > > > > > A fixme tag would be helpful. > > > > I concluded that it's the unusual 'hid_dev->driver' override in > > hid-hyperv to blame and not the 62c68e7cee33 ("HID: ensure timely > > release of driver-allocated resources") but the override was there since > > the inception of the driver so I'm not sure, mentioning 62c68e7cee33 > > probably makes more sense... > > I've finished looking at the linux-next issue in detail, and I agree that > the hid_dev->driver override is the underlying cause. I was > commenting out that line Monday night, but had not gotten as far as > removing the the hid_parse() and hid_hw_start(). Then your patch > came out, Vitaly, which is great! > > > > > > > > >> --- > > >> drivers/hid/hid-hyperv.c | 17 ----------------- > > >> 1 file changed, 17 deletions(-) > > >> > > >> diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c > > >> index f33485d83d24..1609a56ffa7c 100644 > > >> --- a/drivers/hid/hid-hyperv.c > > >> +++ b/drivers/hid/hid-hyperv.c > > >> @@ -431,8 +431,6 @@ static const struct hid_ll_driver mousevsc_ll_driver = { > > >> .raw_request = mousevsc_hid_raw_request, > > >> }; > > >> > > >> -static struct hid_driver mousevsc_hid_driver; > > >> - > > >> static int mousevsc_probe(struct hv_device *device, > > >> const struct hv_vmbus_device_id *dev_id) > > >> { > > >> @@ -473,7 +471,6 @@ static int mousevsc_probe(struct hv_device *device, > > >> } > > >> > > >> hid_dev->ll_driver = &mousevsc_ll_driver; > > >> - hid_dev->driver = &mousevsc_hid_driver; > > >> hid_dev->bus = BUS_VIRTUAL; > > >> hid_dev->vendor = input_dev->hid_dev_info.vendor; > > >> hid_dev->product = input_dev->hid_dev_info.product; > > >> @@ -488,20 +485,6 @@ static int mousevsc_probe(struct hv_device *device, > > >> if (ret) > > >> goto probe_err2; > > >> > > >> - > > >> - ret = hid_parse(hid_dev); > > >> - if (ret) { > > >> - hid_err(hid_dev, "parse failed\n"); > > >> - goto probe_err2; > > >> - } > > >> - > > >> - ret = hid_hw_start(hid_dev, HID_CONNECT_HIDINPUT | > HID_CONNECT_HIDDEV); > > As you noted, using HID_CONNECT_DEFAULT in the default probe function > __hid_device_probe() ends up adding HID_CONNECT_HIDRAW and HID_CONNECT_FF. > The latter is benign as it only affects devices that support force-feedback. As best I > can tell, HID_CNNECT_HIDRAW causes /dev/hidraw0 to appear, which provides a raw > interface to the mouse device. See https://docs.kernel.org/hid/hidraw.html. It doesn't > seem like making this interface visible hurts anything, but I'm not 100% sure. > > The alternative is to keep the "struct hid_driver mousevsc_hid_driver;" line > and to populate it with a name, id_table, and an HID probe function specific > to the Hyper-V mouse. Then instead of the incorrect assignment to > hid_dev->driver, add a > > module_hid_driver(mousevsc_hid_driver); > > statement, which registers the driver. The new HID probe function does > the hid_parse() and hid_hw_start() which have been removed from > mousevsc_probe() as your patch does. With this arrangement, the > hid_hw_start() can be done with the desired HID_CONNECT_* > options so that /dev/hidraw0 won't appear. It's only a few lines > of code. > > I can try to code up this approach if it is preferred. But I'm likely tied > up with some personal things for the next few days, so might not get > to it for a little while. Feel free to try it yourself if you want. Here's what I had in mind. It appears to work and preserves the custom aspects of the current code in mousevsc_probe(). Turns out I can't use module_hid_driver() because it conflicts with the existing module_init() and module_exit() use, so I've directly coded hid_register/unregister_driver(). diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c index f33485d83d24..98a7fa09c4ee 100644 --- a/drivers/hid/hid-hyperv.c +++ b/drivers/hid/hid-hyperv.c @@ -422,6 +422,25 @@ static int mousevsc_hid_raw_request(struct hid_device *hid, return 0; } +static int mousevsc_hid_probe(struct hid_device *hid_dev, const struct hid_device_id *id) +{ + int ret; + + ret = hid_parse(hid_dev); + if (ret) { + hid_err(hid_dev, "parse failed\n"); + return ret; + } + + ret = hid_hw_start(hid_dev, HID_CONNECT_HIDINPUT | HID_CONNECT_HIDDEV); + if (ret) { + hid_err(hid_dev, "hw start failed\n"); + return ret; + } + + return 0; +} + static const struct hid_ll_driver mousevsc_ll_driver = { .parse = mousevsc_hid_parse, .open = mousevsc_hid_open, @@ -431,7 +450,16 @@ static const struct hid_ll_driver mousevsc_ll_driver = { .raw_request = mousevsc_hid_raw_request, }; -static struct hid_driver mousevsc_hid_driver; +static const struct hid_device_id mousevsc_devices[] = { + { HID_DEVICE(BUS_VIRTUAL, HID_GROUP_ANY, 0x045E, 0x0621) }, + { } +}; + +static struct hid_driver mousevsc_hid_driver = { + .name = "hid-hyperv", + .id_table = mousevsc_devices, + .probe = mousevsc_hid_probe, +}; static int mousevsc_probe(struct hv_device *device, const struct hv_vmbus_device_id *dev_id) @@ -473,7 +501,6 @@ static int mousevsc_probe(struct hv_device *device, } hid_dev->ll_driver = &mousevsc_ll_driver; - hid_dev->driver = &mousevsc_hid_driver; hid_dev->bus = BUS_VIRTUAL; hid_dev->vendor = input_dev->hid_dev_info.vendor; hid_dev->product = input_dev->hid_dev_info.product; @@ -488,20 +515,6 @@ static int mousevsc_probe(struct hv_device *device, if (ret) goto probe_err2; - - ret = hid_parse(hid_dev); - if (ret) { - hid_err(hid_dev, "parse failed\n"); - goto probe_err2; - } - - ret = hid_hw_start(hid_dev, HID_CONNECT_HIDINPUT | HID_CONNECT_HIDDEV); - - if (ret) { - hid_err(hid_dev, "hw start failed\n"); - goto probe_err2; - } - device_init_wakeup(&device->device, true); input_dev->connected = true; @@ -579,11 +592,21 @@ static struct hv_driver mousevsc_drv = { static int __init mousevsc_init(void) { - return vmbus_driver_register(&mousevsc_drv); + int ret; + + ret = vmbus_driver_register(&mousevsc_drv); + if (ret) + return ret; + + ret = hid_register_driver(&mousevsc_hid_driver); + if (ret) + vmbus_driver_unregister(&mousevsc_drv); + return ret; } static void __exit mousevsc_exit(void) { + hid_unregister_driver(&mousevsc_hid_driver); vmbus_driver_unregister(&mousevsc_drv); } Michael
diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c index f33485d83d24..1609a56ffa7c 100644 --- a/drivers/hid/hid-hyperv.c +++ b/drivers/hid/hid-hyperv.c @@ -431,8 +431,6 @@ static const struct hid_ll_driver mousevsc_ll_driver = { .raw_request = mousevsc_hid_raw_request, }; -static struct hid_driver mousevsc_hid_driver; - static int mousevsc_probe(struct hv_device *device, const struct hv_vmbus_device_id *dev_id) { @@ -473,7 +471,6 @@ static int mousevsc_probe(struct hv_device *device, } hid_dev->ll_driver = &mousevsc_ll_driver; - hid_dev->driver = &mousevsc_hid_driver; hid_dev->bus = BUS_VIRTUAL; hid_dev->vendor = input_dev->hid_dev_info.vendor; hid_dev->product = input_dev->hid_dev_info.product; @@ -488,20 +485,6 @@ static int mousevsc_probe(struct hv_device *device, if (ret) goto probe_err2; - - ret = hid_parse(hid_dev); - if (ret) { - hid_err(hid_dev, "parse failed\n"); - goto probe_err2; - } - - ret = hid_hw_start(hid_dev, HID_CONNECT_HIDINPUT | HID_CONNECT_HIDDEV); - - if (ret) { - hid_err(hid_dev, "hw start failed\n"); - goto probe_err2; - } - device_init_wakeup(&device->device, true); input_dev->connected = true;
It was found that unloading 'hid_hyperv' module results in a devres complaint: ... hv_vmbus: unregistering driver hid_hyperv ------------[ cut here ]------------ WARNING: CPU: 2 PID: 3983 at drivers/base/devres.c:691 devres_release_group+0x1f2/0x2c0 ... Call Trace: <TASK> ? devres_release_group+0x1f2/0x2c0 ? __warn+0xd1/0x1c0 ? devres_release_group+0x1f2/0x2c0 ? report_bug+0x32a/0x3c0 ? handle_bug+0x53/0xa0 ? exc_invalid_op+0x18/0x50 ? asm_exc_invalid_op+0x1a/0x20 ? devres_release_group+0x1f2/0x2c0 ? devres_release_group+0x90/0x2c0 ? rcu_is_watching+0x15/0xb0 ? __pfx_devres_release_group+0x10/0x10 hid_device_remove+0xf5/0x220 device_release_driver_internal+0x371/0x540 ? klist_put+0xf3/0x170 bus_remove_device+0x1f1/0x3f0 device_del+0x33f/0x8c0 ? __pfx_device_del+0x10/0x10 ? cleanup_srcu_struct+0x337/0x500 hid_destroy_device+0xc8/0x130 mousevsc_remove+0xd2/0x1d0 [hid_hyperv] device_release_driver_internal+0x371/0x540 driver_detach+0xc5/0x180 bus_remove_driver+0x11e/0x2a0 ? __mutex_unlock_slowpath+0x160/0x5e0 vmbus_driver_unregister+0x62/0x2b0 [hv_vmbus] ... And the issue seems to be that the corresponding devres group is not allocated. Normally, devres_open_group() is called from __hid_device_probe() but Hyper-V HID driver overrides 'hid_dev->driver' with 'mousevsc_hid_driver' stub and basically re-implements __hid_device_probe() by calling hid_parse() and hid_hw_start() but not devres_open_group(). hid_device_probe() does not call __hid_device_probe() for it. Later, when the driver is removed, hid_device_remove() calls devres_release_group() as it doesn't check whether hdev->driver was initially overridden or not. The issue seems to be related to the commit 62c68e7cee33 ("HID: ensure timely release of driver-allocated resources") but the commit itself seems to be correct. Fix the issue by dropping the 'hid_dev->driver' override and the now unneeded hid_parse()/hid_hw_start() calls. One notable difference of the change is hid_hw_start() is now called with HID_CONNECT_DEFAULT which implies HID_CONNECT_HIDRAW. This doesn't seem to cause any immediate issues but 'HID_CONNECT_HIDINPUT | HID_CONNECT_HIDDEV' combo was used in the driver for a long time and it is unclear whether hidraw was excluded on purpose or not. Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- drivers/hid/hid-hyperv.c | 17 ----------------- 1 file changed, 17 deletions(-)