On Thu, Aug 06, 2020 at 10:54:14AM -0400, Alan Stern wrote:
> On Thu, Aug 06, 2020 at 05:07:16PM +0800, Peter Chen wrote:
> > The current code uses commit fac323471df6 ("usb: udc: allow adding
> > and removing the same gadget device") as the workaround to let
> > the gadget device is re-used, but it is not allowed from driver
> > core point. In this commit, we allocate gadget structure dynamically,
> > and free it at its release function. Since the gadget device's
> > driver_data has already occupied by usb_composite_dev structure, we have
> > to use gadget device's platform data to store dwc3 structure.
> >
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Peter Chen <peter.chen@xxxxxxx>
>
> There's one problem with this patch...
>
> > @@ -3670,21 +3685,22 @@ int dwc3_gadget_init(struct dwc3 *dwc)
> >
> > ret = dwc3_gadget_init_endpoints(dwc, dwc->num_eps);
> > if (ret)
> > - goto err3;
> > + goto err4;
> >
> > - ret = usb_add_gadget_udc(dwc->dev, &dwc->gadget);
> > + ret = usb_add_gadget_udc_release(dwc->dev, dwc->gadget, dwc_gadget_release);
> > if (ret) {
> > dev_err(dwc->dev, "failed to register udc\n");
> > - goto err4;
> > + goto err5;
> > }
> >
> > - dwc3_gadget_set_speed(&dwc->gadget, dwc->maximum_speed);
> > + dwc3_gadget_set_speed(dwc->gadget, dwc->maximum_speed);
> >
> > return 0;
> >
> > -err4:
> > +err5:
> > dwc3_gadget_free_endpoints(dwc);
> > -
> > +err4:
> > + kfree(dwc->gadget);
>
> Once you call usb_add_gadget_udc_release() -- even if the call fails --
> you must not call kfree() directly. Instead you have to do
> put_device(&dwc->gadget.dev). That's a little awkward given the code
> arrangement here.
Sorry, I got that wrong. If usb_add_gadget_udc_release() succeeds then
you must call put_device() in order to destroy the gadget. But if it
fails then it calls put_device() for you, so that dwc->gadget is a stale
pointer. In this case you must not call either kfree() or put_device().
Alan Stern
