| Message ID | 20241104164824.1213529-1-vkuznets@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | HID: hyperv: streamline driver probe to avoid devres issues | expand |
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: 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
Michael Kelley <mhklinux@outlook.com> writes: > 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. I have tested my patch with a standard Wayland/Gnome UI and I didn't see any issues because of hidraw. I tried rmmod/modprobe cycle, no trace appeared and mouse worked well after. It would be nice to check whether hidraw actually works or not. In particular, I see that we don't implement .raw_request() hook for mousevsc (mousevsc_hid_raw_request() exists but always returns 0). I tried 'hid-example' from samples/hidraw and it gives me: # ./hid-example Report Descriptor Size: 67 Report Descriptor: 5 1 9 2 a1 1 9 1 a1 0 5 9 19 1 29 5 15 0 25 1 95 5 75 1 81 2 95 1 75 3 81 1 5 1 9 30 9 31 15 0 26 ff 7f 75 10 95 2 81 2 5 1 9 38 16 1 80 26 ff 7f 75 10 95 1 81 6 c0 c0 Raw Name: Microsoft Vmbus HID-compliant Mouse Raw Phys: Raw Info: bustype: 6 (Virtual) vendor: 0x045e product: 0x0621 ioctl HIDIOCSFEATURE returned: 0 ioctl HIDIOCGFEATURE returned: 0 Report data: write() wrote 0 bytes read: Resource temporarily unavailable but I can't really tell if this is totally broken or not. In case it is, your approach to get rid of hidraw sounds reasonable, I can probably try it early next week or so. > >> >> - >> >> - 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 >
Michael Kelley <mhklinux@outlook.com> writes: > 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(). Thanks! I'll give it a try. As an alternative, I was thinking of introducing something like HID_QUIRK_NO_HIDRAW to make hid_connect() do what we want. > > 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 > > > >
Michael Kelley <mhklinux@outlook.com> writes: > 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); This works for me with one minor issue. If we do hid_unregister_driver() first, the device gets pickup up by hid-generic during unload: [ 174.988727] input: Microsoft Vmbus HID-compliant Mouse as /devices/0006:045E:0621.0001/input/input4 [ 174.995261] hid-generic 0006:045E:0621.0001: input,hidraw0: VIRTUAL HID v0.01 Mouse [Microsoft Vmbus HID-compliant Mouse] on [ 174.999222] hv_vmbus: unregistering driver hid_hyperv so I think hid_unregister_driver() and vmbus_driver_unregister() calls must be swapped. It also seems logical to invert the order in mousevsc_init(): do hid_register_driver() first and then call vmbus_driver_register() to avoid the race where a mousevsc device shows up but the HID driver for it is not yet registered. I've tested the suggested patch with the change and it seems to work as expected so I'll send it as v2 shortly. Thanks! > } > > Michael > > > >
From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Monday, November 11, 2024 5:01 AM > > Michael Kelley <mhklinux@outlook.com> writes: > > > 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 [snip] > >> > >> 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); > > This works for me with one minor issue. If we do hid_unregister_driver() > first, the device gets pickup up by hid-generic during unload: > > [ 174.988727] input: Microsoft Vmbus HID-compliant Mouse as > /devices/0006:045E:0621.0001/input/input4 > [ 174.995261] hid-generic 0006:045E:0621.0001: input,hidraw0: VIRTUAL HID v0.01 > Mouse [Microsoft Vmbus HID-compliant Mouse] on > [ 174.999222] hv_vmbus: unregistering driver hid_hyperv > > so I think hid_unregister_driver() and vmbus_driver_unregister() calls > must be swapped. It also seems logical to invert the order in > mousevsc_init(): do hid_register_driver() first and then call > vmbus_driver_register() to avoid the race where a mousevsc device shows > up but the HID driver for it is not yet registered. > Yes, that makes perfect sense. 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(-)