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