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.
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.
> >> -
> >> - 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>
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
