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.
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).
>
> > 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.
Best Regards,
Felix
--
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
