| Message ID | 4809717.31r3eYUQgx@kreacher (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: Fix regression resulting from commit 532223c8ac57 | expand |
On Wed, Dec 7, 2022 at 10:13 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Some special HID drivers (for example, hid-logitech-hidpp) use ->match() > callbacks to reject specific devices that otherwise would match the > driver's device ID list, with the expectation that those devices will > be handled by some other drivers. However, this doesn't work if > hid-generic is expected to bind to the given device, because its > ->match() callback, hid_generic_match(), rejects all devices that match > device ID lists of the other HID drivers regardless of what is returned > by the other drivers' ->match() callbacks. Thanks Rafael for spotting that corner case in the ->match() processing. > > To make it work, amend the function used by hid_generic_match() for > checking an individual driver, __check_hid_generic(), with a check > involving the given driver's ->match() callback, so 0 is returned > when that callback rejects the device in question. Shouldn't we add that logic to hid_match_device() directly in hid-core.c instead? It feels wrong to have a function named "hid_match_device()" and have to manually call later "->match()" on the driver itself. Ack on the general idea anyway. > > Fixes: 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices") > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/hid/hid-generic.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > Index: linux-pm/drivers/hid/hid-generic.c > =================================================================== > --- linux-pm.orig/drivers/hid/hid-generic.c > +++ linux-pm/drivers/hid/hid-generic.c > @@ -31,7 +31,13 @@ static int __check_hid_generic(struct de > if (hdrv == &hid_generic) > return 0; > > - return hid_match_device(hdev, hdrv) != NULL; > + if (!hid_match_device(hdev, hdrv)) > + return 0; > + > + if (hdrv->match) > + return hdrv->match(hdev, false); > + > + return 1; > } > > static bool hid_generic_match(struct hid_device *hdev, > > >
On Wed, Dec 7, 2022 at 10:27 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Wed, Dec 7, 2022 at 10:13 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Some special HID drivers (for example, hid-logitech-hidpp) use ->match() > > callbacks to reject specific devices that otherwise would match the > > driver's device ID list, with the expectation that those devices will > > be handled by some other drivers. However, this doesn't work if > > hid-generic is expected to bind to the given device, because its > > ->match() callback, hid_generic_match(), rejects all devices that match > > device ID lists of the other HID drivers regardless of what is returned > > by the other drivers' ->match() callbacks. > > Thanks Rafael for spotting that corner case in the ->match() processing. > > > > > To make it work, amend the function used by hid_generic_match() for > > checking an individual driver, __check_hid_generic(), with a check > > involving the given driver's ->match() callback, so 0 is returned > > when that callback rejects the device in question. > > Shouldn't we add that logic to hid_match_device() directly in > hid-core.c instead? > It feels wrong to have a function named "hid_match_device()" and have > to manually call later "->match()" on the driver itself. Well, I've followed the pattern present in hid_device_probe(), where hid_match_device() is first called to check against the device ID list and then ->match() is invoked later only if that doesn't fail. Also changing hid_match_device() would change the way in which hid_bus_match() works and that may lead to subsequent regressions, potentially, so I'd rather avoid doing that ATM. > Ack on the general idea anyway. Thanks! > > > > Fixes: 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices") > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/hid/hid-generic.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > Index: linux-pm/drivers/hid/hid-generic.c > > =================================================================== > > --- linux-pm.orig/drivers/hid/hid-generic.c > > +++ linux-pm/drivers/hid/hid-generic.c > > @@ -31,7 +31,13 @@ static int __check_hid_generic(struct de > > if (hdrv == &hid_generic) > > return 0; > > > > - return hid_match_device(hdev, hdrv) != NULL; > > + if (!hid_match_device(hdev, hdrv)) > > + return 0; > > + > > + if (hdrv->match) > > + return hdrv->match(hdev, false); > > + > > + return 1; > > } > > > > static bool hid_generic_match(struct hid_device *hdev, > > > > > > >
Index: linux-pm/drivers/hid/hid-generic.c =================================================================== --- linux-pm.orig/drivers/hid/hid-generic.c +++ linux-pm/drivers/hid/hid-generic.c @@ -31,7 +31,13 @@ static int __check_hid_generic(struct de if (hdrv == &hid_generic) return 0; - return hid_match_device(hdev, hdrv) != NULL; + if (!hid_match_device(hdev, hdrv)) + return 0; + + if (hdrv->match) + return hdrv->match(hdev, false); + + return 1; } static bool hid_generic_match(struct hid_device *hdev,