diff mbox series

[v1,1/2] HID: generic: Add ->match() check to __check_hid_generic()

Message ID 4809717.31r3eYUQgx@kreacher
State New
Headers
Series HID: Fix regression resulting from commit 532223c8ac57 |

Commit Message

Rafael J. Wysocki Dec. 7, 2022, 9:11 a.m. UTC 
  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.

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.

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(-)

Comments

Benjamin Tissoires Dec. 7, 2022, 9:27 a.m. UTC | #1 
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,
>
>
>
Rafael J. Wysocki Dec. 7, 2022, 9:54 a.m. UTC | #2 
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,
> >
> >
> >
>
diff mbox series

Patch

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,