Re: [PATCH] HID: hyperv: streamline driver probe to avoid devres issues

[Vitaly Kuznetsov <vkuznets@redhat.com>]

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.

-- 
Vitaly