Re: [PATCH 1/2] usbcore/driver: Fix specific driver selection

["M. Vefa Bicakci" <m.v.b@runbox.com>]

On 17/09/2020 13.23, Bastien Nocera wrote:
> On Thu, 2020-09-17 at 12:59 +0300, M. Vefa Bicakci wrote:
>> This commit resolves two minor bugs in the selection/discovery of
>> more
>> specific USB device drivers for devices that are currently bound to
>> generic USB device drivers.
>>
>> The first bug is related to the way a candidate USB device driver is
>> compared against the generic USB device driver. The code in
>> is_dev_usb_generic_driver() used to unconditionally use
>> to_usb_device_driver() on each device driver, without verifying that
>> the device driver in question is a USB device driver (as opposed to a
>> USB interface driver).
> 
> You could also return early if is_usb_device() fails in
> __usb_bus_reprobe_drivers(). Each of the interface and the device
> itself is a separate "struct device", and "non-interface" devices won't
> have interface devices assigned to them.

Will do! If I understand you correctly, you mean something like the
following:

static int __usb_bus_reprobe_drivers(struct device *dev, void *data)
{
         struct usb_device_driver *new_udriver = data;
         struct usb_device *udev;
         int ret;

	/* Proposed addition begins */

	if (!is_usb_device(dev))
		return 0;

	/* Proposed addition ends */

         if (!is_dev_usb_generic_driver(dev))
                 return 0;


>> The second bug is related to the logic that determines whether a
>> device
>> currently bound to a generic USB device driver should be re-probed by
>> a
>> more specific USB device driver or not. The code in
>> __usb_bus_reprobe_drivers() used to have the following lines:
>>
>>    if (usb_device_match_id(udev, new_udriver->id_table) == NULL &&
>>        (!new_udriver->match || new_udriver->match(udev) != 0))
>>   		return 0;
>>
>>    ret = device_reprobe(dev);
>>
>> As the reader will notice, the code checks whether the USB device in
>> consideration matches the identifier table (id_table) of a specific
>> USB device_driver (new_udriver), followed by a similar check, but
>> this
>> time with the USB device driver's match function. However, the match
>> function's return value is not checked correctly. When match()
>> returns
>> zero, it means that the specific USB device driver is *not*
>> applicable
>> to the USB device in question, but the code then goes on to reprobe
>> the
>> device with the new USB device driver under consideration. All this
>> to
>> say, the logic is inverted.
> 
> Could you split that change as the first commit in your patchset?

Of course!

> I'll test your patches locally once you've respun them. Thanks for
> working on this.
> 
> Cheers

Thank you for reviewing the patches! I intend to address your comments,
test the patches and publish them soon (i.e., likely within a few hours).

Thank you,

Vefa