Michael Kelley <mhklinux@xxxxxxxxxxx> writes:
> From: Michael Kelley Sent: Wednesday, November 6, 2024 10:36 AM
>> From: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> Sent: Tuesday, November 5, 2024
>> 9:45 AM
>> >
>> > Saurabh Singh Sengar <ssengar@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
>> > >
>> > > 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
>
>
>
>
--
Vitaly
