From: Vitaly Kuznetsov <vkuzn...@redhat.com> Sent: Monday, November 11, 2024
5:01 AM
>
> Michael Kelley <mhkli...@outlook.com> writes:
>
> > From: Michael Kelley Sent: Wednesday, November 6, 2024 10:36 AM
> >> From: Vitaly Kuznetsov <vkuzn...@redhat.com> Sent: Tuesday, November 5,
> >> 2024 9:45 AM
[snip]
> >>
> >> 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.
>
Yes, that makes perfect sense.
Michael
