On Fri, Jan 31, 2020 at 2:49 PM Hans de Goede <hdegoede@xxxxxxxxxx> 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@xxxxxxxxxx> 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
>
