On 12/01/2016 10:44 PM, Felix Hädicke wrote:
> Hi,
>
>> Hi,
>>
>> Good catch but I have a few comments to commit message:
>>
>> On 12/01/2016 12:24 AM, Felix Hädicke wrote:
>>> This fixes a regression which was introduced by commit f1bddbb, by
>>> reverting a small fragment of commit 855ed04.
>>>
>> Not exactly. The regression has been introduced by commit 855ed04
>> itself.This commit replaced previous construction similar what you sent
>> now with simple if():
>>
>> @@ -535,11 +556,7 @@ int usb_gadget_probe_driver(struct
>> usb_gadget_driver *driver)
>> if (!ret)
>> break;
>> }
>> - if (ret)
>> - ret = -ENODEV;
>> - else if (udc->driver)
>> - ret = -EBUSY;
>> - else
>> + if (!ret && !udc->driver)
>> goto found;
>> } else {
>> list_for_each_entry(udc, &udc_list, list) {
>
> The regression bug I want to fix with this patch was introduced with
> commit f1bddbb, not 855ed04. With 855ed04, the this if/else construct
> was changed. But concerning the usb_gadget_probe_driver() return code,
> this was ok.
Nope. Return code was 0 and gadget has not been bound to any udc but
added to pending list. Consider the following sequence:
echo "udc" > g1/UDC
echo "udc" > g2/UDC
echo "" > g1/UDC
The expected result is that second line should fail with EBUSY. And
without f1bddbb the returned value will be 0 and g2 will be pending on a
list. After line 3 udc will be free but g2 will still be hanging on a
pending list.
> There was no code path were a value which was not meant to
> be the function return code was accidentally returned. With commit
> f1bddbb, and the introduction of a new code path for the flag
> match_existing_only, this changed.
>
This flag changed the error from "leave it on the list forever when udc
is busy" to "NULL ptr dereference" which is obviously much worse.
>> After that commit, if UDC is busy, gadget is added to the list of
>> pending gadgets. Unfortunately this list is not rescanned in
>> usb_gadget_unregister_driver(). This means that such gadget is going to
>> stay on this list forever so it's a bug.
>
> Ok, I can confirm this bug. But it is not the issue which I am
> addressing with this patch. This is just about the
> usb_gadget_probe_driver() return code (on which other functions,
> paritcularly gadget configfs's|gadget_dev_desc_UDC_store|() rely).
>
This commit fixes both f1bddbb and 855ed04 just the bug is a little bit
different but it's still a bug.
>> Commit f1bddbb make this bug more visible (as it causes NULL pointer
>> dereference) as gadget has not been added to the list of pending gadgets
>> and we try to remove it from there.
>>
>> Summing up, in my personal opinion I think that you should add:
>>
>> Fixes: 855ed04 ("usb: gadget: udc-core: independent registration of
>> gadgets and gadget drivers")
>
> As explained above, I think this would be wrong.
>
I understand that your target was to fix NULL ptr dereference but you
can kill two birds with the same stone as you fix also the previous bug;).
I suggested to place commit 855ed04 instead of f1bddbb because 855ed04
appears earlier in a tree and even if someone doesn't have f1bddbb your
patch is useful for him because it fix a bug introduced in that commit
(gadget pending forever).
Best regards,
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
