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

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

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