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...
>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
>
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@xxxxxxxxxxxxxxxxxxx>
Thanks! I was reproducing the issue on 6.12-rc6 by just doing 'rmmod
hid_hyperv' and tested my patch there.
--
Vitaly
