On Fri, Jan 31, 2020 at 2:49 PM Hans de Goede [off-list ref] wrote:

Hi,

On 1/31/20 2:39 PM, Benjamin Tissoires wrote:

quoted

Hi Hans,

On Fri, Jan 31, 2020 at 1:46 PM Hans de Goede [off-list ref] wrote:

quoted

__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