| Message ID | 20200131124615.27849-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: generic: Check other drivers match callback from __check_hid_generic | expand |
Hi Hans, On Fri, Jan 31, 2020 at 1:46 PM Hans de Goede <hdegoede@redhat.com> wrote: > > __check_hid_generic is used to check if there is a driver, other then > the hid-generic driver, which wants to handle the hid-device, in which > case hid_generic_match() will return false so that the other driver can > bind. > > But what if the other driver also has a match callback and that indicates > it does not want to handle the device? Then hid-generic should bind to it > and thus hid_generic_match() should NOT return false in that case. > > This commit makes __check_hid_generic check if a matching driver has > a match callback and if it does makes its check this, so that > hid-generic will bind to devices which have a matching other driver, > but that other driver's match callback rejects the device. I get that part, but I am not sure I'll remember this in 2 months time when/if we need to extend the .match() in another driver. I am especially worried about the potential circular calls where an other driver decides to check on all the other drivers having a match callback... Could you add a little blurb either in hid-generic.c explaining the logic, or (and) in hid.h where .match is defined? Because now, we have 2 callers for .match(): hid-core and hid-generic (and 2 is usually one too many :-/). Cheers, Benjamin > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > This patch was written while fixing the issues with hid-ite on the > Acer SW5-012, where we only want to bind to one interface. In that > specific case this change is not necessary because hid-multitouch will > pick the hid-device which hid-ite's match callback is rejecting. > --- > drivers/hid/hid-generic.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/hid/hid-generic.c b/drivers/hid/hid-generic.c > index f9db991d3c5a..fe3ca7612750 100644 > --- a/drivers/hid/hid-generic.c > +++ b/drivers/hid/hid-generic.c > @@ -31,7 +31,13 @@ static int __check_hid_generic(struct device_driver *drv, void *data) > 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 1; > + > + return hdrv->match(hdev, false); > } > > static bool hid_generic_match(struct hid_device *hdev, > -- > 2.23.0 >
Hi, On 1/31/20 2:39 PM, Benjamin Tissoires wrote: > Hi Hans, > > On Fri, Jan 31, 2020 at 1:46 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> __check_hid_generic is used to check if there is a driver, other then >> the hid-generic driver, which wants to handle the hid-device, in which >> case hid_generic_match() will return false so that the other driver can >> bind. >> >> But what if the other driver also has a match callback and that indicates >> it does not want to handle the device? Then hid-generic should bind to it >> and thus hid_generic_match() should NOT return false in that case. >> >> This commit makes __check_hid_generic check if a matching driver has >> a match callback and if it does makes its check this, so that >> hid-generic will bind to devices which have a matching other driver, >> but that other driver's match callback rejects the device. > > I get that part, but I am not sure I'll remember this in 2 months time > when/if we need to extend the .match() in another driver. > I am especially worried about the potential circular calls where an > other driver decides to check on all the other drivers having a match > callback... > > Could you add a little blurb either in hid-generic.c explaining the > logic, or (and) in hid.h where .match is defined? > > Because now, we have 2 callers for .match(): hid-core and hid-generic > (and 2 is usually one too many :-/). Ok, how about the following: static int __check_hid_generic(struct device_driver *drv, void *data) { struct hid_driver *hdrv = to_hid_driver(drv); struct hid_device *hdev = data; if (hdrv == &hid_generic) return 0; if (!hid_match_device(hdev, hdrv)) return 0; /* * The purpose of looping over all drivers to see if one is a match * for the hdev, is for hid-generic to NOT bind to any devices which * have another, specialized, driver registerd. But in some cases that * specialized driver might have a match callback itself, e.g. because * it only wants to bind to a single USB interface of a device with * multiple HID interfaces. * So if another driver defines a match callback and that match * callback returns false then hid-generic should still bind to the * device and we should thus keep looping over the registered drivers. */ if (!hdrv->match) return 1; return hdrv->match(hdev, false); } ? Let me know if you like this then I'll send a v2 with this. Regards, Hans
On Fri, Jan 31, 2020 at 2:49 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 1/31/20 2:39 PM, Benjamin Tissoires wrote: > > Hi Hans, > > > > On Fri, Jan 31, 2020 at 1:46 PM Hans de Goede <hdegoede@redhat.com> wrote: > >> > >> __check_hid_generic is used to check if there is a driver, other then > >> the hid-generic driver, which wants to handle the hid-device, in which > >> case hid_generic_match() will return false so that the other driver can > >> bind. > >> > >> But what if the other driver also has a match callback and that indicates > >> it does not want to handle the device? Then hid-generic should bind to it > >> and thus hid_generic_match() should NOT return false in that case. > >> > >> This commit makes __check_hid_generic check if a matching driver has > >> a match callback and if it does makes its check this, so that > >> hid-generic will bind to devices which have a matching other driver, > >> but that other driver's match callback rejects the device. > > > > I get that part, but I am not sure I'll remember this in 2 months time > > when/if we need to extend the .match() in another driver. > > I am especially worried about the potential circular calls where an > > other driver decides to check on all the other drivers having a match > > callback... > > > > Could you add a little blurb either in hid-generic.c explaining the > > logic, or (and) in hid.h where .match is defined? > > > > Because now, we have 2 callers for .match(): hid-core and hid-generic > > (and 2 is usually one too many :-/). > > Ok, how about the following: > > static int __check_hid_generic(struct device_driver *drv, void *data) > { > struct hid_driver *hdrv = to_hid_driver(drv); > struct hid_device *hdev = data; > > if (hdrv == &hid_generic) > return 0; > > if (!hid_match_device(hdev, hdrv)) > return 0; > > /* > * The purpose of looping over all drivers to see if one is a match > * for the hdev, is for hid-generic to NOT bind to any devices which > * have another, specialized, driver registerd. But in some cases that > * specialized driver might have a match callback itself, e.g. because > * it only wants to bind to a single USB interface of a device with > * multiple HID interfaces. > * So if another driver defines a match callback and that match > * callback returns false then hid-generic should still bind to the > * device and we should thus keep looping over the registered drivers. > */ > if (!hdrv->match) > return 1; > > return hdrv->match(hdev, false); > } > > ? > > Let me know if you like this then I'll send a v2 with this. Yep, sounds good. Could you also add a blurb in the docs of struct hid_driver in include/linux/hid.h? Something along the lines of: match should return true if the driver wants the device, false otherwise. Note that hid-generic has a special handling of this in which it will also iterate over other .match() callbacks in other drivers. Please refrain from duplicating this behaviour in other drivers or dragons will come due to circular calls. Cheers, Benjamin > > Regards, > > Hans >
HI, On 1/31/20 3:00 PM, Benjamin Tissoires wrote: > On Fri, Jan 31, 2020 at 2:49 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 1/31/20 2:39 PM, Benjamin Tissoires wrote: >>> Hi Hans, >>> >>> On Fri, Jan 31, 2020 at 1:46 PM Hans de Goede <hdegoede@redhat.com> wrote: >>>> >>>> __check_hid_generic is used to check if there is a driver, other then >>>> the hid-generic driver, which wants to handle the hid-device, in which >>>> case hid_generic_match() will return false so that the other driver can >>>> bind. >>>> >>>> But what if the other driver also has a match callback and that indicates >>>> it does not want to handle the device? Then hid-generic should bind to it >>>> and thus hid_generic_match() should NOT return false in that case. >>>> >>>> This commit makes __check_hid_generic check if a matching driver has >>>> a match callback and if it does makes its check this, so that >>>> hid-generic will bind to devices which have a matching other driver, >>>> but that other driver's match callback rejects the device. >>> >>> I get that part, but I am not sure I'll remember this in 2 months time >>> when/if we need to extend the .match() in another driver. >>> I am especially worried about the potential circular calls where an >>> other driver decides to check on all the other drivers having a match >>> callback... >>> >>> Could you add a little blurb either in hid-generic.c explaining the >>> logic, or (and) in hid.h where .match is defined? >>> >>> Because now, we have 2 callers for .match(): hid-core and hid-generic >>> (and 2 is usually one too many :-/). >> >> Ok, how about the following: >> >> static int __check_hid_generic(struct device_driver *drv, void *data) >> { >> struct hid_driver *hdrv = to_hid_driver(drv); >> struct hid_device *hdev = data; >> >> if (hdrv == &hid_generic) >> return 0; >> >> if (!hid_match_device(hdev, hdrv)) >> return 0; >> >> /* >> * The purpose of looping over all drivers to see if one is a match >> * for the hdev, is for hid-generic to NOT bind to any devices which >> * have another, specialized, driver registerd. But in some cases that >> * specialized driver might have a match callback itself, e.g. because >> * it only wants to bind to a single USB interface of a device with >> * multiple HID interfaces. >> * So if another driver defines a match callback and that match >> * callback returns false then hid-generic should still bind to the >> * device and we should thus keep looping over the registered drivers. >> */ >> if (!hdrv->match) >> return 1; >> >> return hdrv->match(hdev, false); >> } >> >> ? >> >> Let me know if you like this then I'll send a v2 with this. > > Yep, sounds good. > > Could you also add a blurb in the docs of struct hid_driver in > include/linux/hid.h? > > Something along the lines of: > > match should return true if the driver wants the device, false > otherwise. Note that hid-generic has a special handling of this in > which it will also iterate over other .match() callbacks in other > drivers. Please refrain from duplicating this behaviour in other > drivers or dragons will come due to circular calls. Ack, now that we are likely not going to add a match callback to the hid-ite driver (see its thread) do we still want to move ahead with this patch? On one hand it still makes sense, OTOH if we never add a match callback to another driver ... Regards, Hans
On Fri, Jan 31, 2020 at 3:07 PM Hans de Goede <hdegoede@redhat.com> wrote: > > HI, > > On 1/31/20 3:00 PM, Benjamin Tissoires wrote: > > On Fri, Jan 31, 2020 at 2:49 PM Hans de Goede <hdegoede@redhat.com> wrote: > >> > >> Hi, > >> > >> On 1/31/20 2:39 PM, Benjamin Tissoires wrote: > >>> Hi Hans, > >>> > >>> On Fri, Jan 31, 2020 at 1:46 PM Hans de Goede <hdegoede@redhat.com> wrote: > >>>> > >>>> __check_hid_generic is used to check if there is a driver, other then > >>>> the hid-generic driver, which wants to handle the hid-device, in which > >>>> case hid_generic_match() will return false so that the other driver can > >>>> bind. > >>>> > >>>> But what if the other driver also has a match callback and that indicates > >>>> it does not want to handle the device? Then hid-generic should bind to it > >>>> and thus hid_generic_match() should NOT return false in that case. > >>>> > >>>> This commit makes __check_hid_generic check if a matching driver has > >>>> a match callback and if it does makes its check this, so that > >>>> hid-generic will bind to devices which have a matching other driver, > >>>> but that other driver's match callback rejects the device. > >>> > >>> I get that part, but I am not sure I'll remember this in 2 months time > >>> when/if we need to extend the .match() in another driver. > >>> I am especially worried about the potential circular calls where an > >>> other driver decides to check on all the other drivers having a match > >>> callback... > >>> > >>> Could you add a little blurb either in hid-generic.c explaining the > >>> logic, or (and) in hid.h where .match is defined? > >>> > >>> Because now, we have 2 callers for .match(): hid-core and hid-generic > >>> (and 2 is usually one too many :-/). > >> > >> Ok, how about the following: > >> > >> static int __check_hid_generic(struct device_driver *drv, void *data) > >> { > >> struct hid_driver *hdrv = to_hid_driver(drv); > >> struct hid_device *hdev = data; > >> > >> if (hdrv == &hid_generic) > >> return 0; > >> > >> if (!hid_match_device(hdev, hdrv)) > >> return 0; > >> > >> /* > >> * The purpose of looping over all drivers to see if one is a match > >> * for the hdev, is for hid-generic to NOT bind to any devices which > >> * have another, specialized, driver registerd. But in some cases that > >> * specialized driver might have a match callback itself, e.g. because > >> * it only wants to bind to a single USB interface of a device with > >> * multiple HID interfaces. > >> * So if another driver defines a match callback and that match > >> * callback returns false then hid-generic should still bind to the > >> * device and we should thus keep looping over the registered drivers. > >> */ > >> if (!hdrv->match) > >> return 1; > >> > >> return hdrv->match(hdev, false); > >> } > >> > >> ? > >> > >> Let me know if you like this then I'll send a v2 with this. > > > > Yep, sounds good. > > > > Could you also add a blurb in the docs of struct hid_driver in > > include/linux/hid.h? > > > > Something along the lines of: > > > > match should return true if the driver wants the device, false > > otherwise. Note that hid-generic has a special handling of this in > > which it will also iterate over other .match() callbacks in other > > drivers. Please refrain from duplicating this behaviour in other > > drivers or dragons will come due to circular calls. > > Ack, now that we are likely not going to add a match callback to > the hid-ite driver (see its thread) do we still want to move ahead > with this patch? On one hand it still makes sense, OTOH if we never > add a match callback to another driver ... > Yep, we better keep the status quo now: only hid-generic is allowed to have a .match, and we are safer now. If we need it in the future, we can always rely on this thread for a v2. Cheers, Benjamin
diff --git a/drivers/hid/hid-generic.c b/drivers/hid/hid-generic.c index f9db991d3c5a..fe3ca7612750 100644 --- a/drivers/hid/hid-generic.c +++ b/drivers/hid/hid-generic.c @@ -31,7 +31,13 @@ static int __check_hid_generic(struct device_driver *drv, void *data) 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 1; + + return hdrv->match(hdev, false); } static bool hid_generic_match(struct hid_device *hdev,
__check_hid_generic is used to check if there is a driver, other then the hid-generic driver, which wants to handle the hid-device, in which case hid_generic_match() will return false so that the other driver can bind. But what if the other driver also has a match callback and that indicates it does not want to handle the device? Then hid-generic should bind to it and thus hid_generic_match() should NOT return false in that case. This commit makes __check_hid_generic check if a matching driver has a match callback and if it does makes its check this, so that hid-generic will bind to devices which have a matching other driver, but that other driver's match callback rejects the device. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- This patch was written while fixing the issues with hid-ite on the Acer SW5-012, where we only want to bind to one interface. In that specific case this change is not necessary because hid-multitouch will pick the hid-device which hid-ite's match callback is rejecting. --- drivers/hid/hid-generic.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)