On 12/03/2016 07:47 PM, Felix Hädicke wrote:
> Hi,
>
>> 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.
>
> As I understand 855ed04, it is the new expected behaviour that
> usb_gadget_probe_driver() returns 0, even if there is no UDC (with the
> given name) available yet, or another gadget driver is already active.
At least for configfs gadgets this should fail with -EBUSY.
> So there is no bug here (at least none concerning the
> usb_gadget_probe_driver() return code).
>
> But with commit f1bddbb, there is a new code path in which a strcmp()
> return code is returned from usb_gadget_probe_driver(), which does not
> make sense.
>
> And the issue you are describing is still there, my patch does not fix it:
> 1. rmmod <all UDC driver modules>
> 2. modprobe <a first legacy gadget module, e. g. g-serial>
> 2. modprobe <a second legacy gadget module, e. g. g-zero>
> 3. modprobe <a UDC driver module>
> The first gadget driver is now active.
> 4. rmmod <the first legacy gadget module>
> -> The second driver is not activated (although it is in the pending list).
>
True, but only for gadget's which didn't specify udc_name and all
gadgets composed using ConfigFS specify this (that's why I gave you
configfs example above). Nevertheless for legacy gadgets which don't
specify udc_name this should be also fixed. I'll submit a patch in a moment.
>>
>>> 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).
>
> But it didn't introduce the bug which is fixed with this patch.
>
Ok, leave it as is.
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
