| 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 > > > >
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(-)