Re: [PATCH] HID: generic: Check other drivers match callback from __check_hid_generic

[Hans de Goede <hdegoede@redhat.com>]

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