> > On Fri, Jul 31, 2020 at 05:59:35PM +0800, Peter Chen wrote: > > Per discussion[1], to avoid UDC driver possible freeing gadget device > > memory before device core finishes using it, we add wait-complete > > mechanism at usb_del_gadget_udc and gadget device .release callback. > > After that, usb_del_gadget_udc will not return back until device core > > finishes using gadget device. > > Ick, no, that's a sure way for a deadlock to happen. > I tested multiple add and delete, did not trigger. What kinds of possible deadlock? > Why does the gadget core care about this at all? It shouldn't. > The UDC driver will free gadget device memory after that, the driver core may still reference it. [1] > > > > > > For UDC drivers who have own .release callback, it needs to call > > complete(&gadget->done) by themselves, if not, the UDC core will > > handle it by default .release callback usb_gadget_release. > > > > [1] > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww. > > spinics.net%2Flists%2Flinux- > usb%2Fmsg198790.html&data=02%7C01%7Cpe > > > ter.chen%40nxp.com%7Cff1ece31761e40be0c7b08d83548ac19%7C686ea1d3bc2b > 4c > > > 6fa92cd99c5c301635%7C0%7C0%7C637317933526709832&sdata=q7%2F1S > mwujv > > tg4DsW0iUaM2Mqf0cdkzL%2FKjJNBRfm6hc%3D&reserved=0 > > > > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> > > Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> > > Suggested-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> > > Signed-off-by: Peter Chen <peter.chen@xxxxxxx> > > --- > > If this RFC patch is ok, I will create the formal patches which will > > change UDC drivers who have their own .release function. > > > > drivers/usb/gadget/udc/core.c | 14 +++++++++++--- > > include/linux/usb/gadget.h | 2 ++ > > 2 files changed, 13 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/usb/gadget/udc/core.c > > b/drivers/usb/gadget/udc/core.c index ee226ad802a4..ed141e1a0dcf > > 100644 > > --- a/drivers/usb/gadget/udc/core.c > > +++ b/drivers/usb/gadget/udc/core.c > > @@ -1138,9 +1138,15 @@ static void usb_udc_release(struct device *dev) > > > > static const struct attribute_group *usb_udc_attr_groups[]; > > > > -static void usb_udc_nop_release(struct device *dev) > > +static void usb_gadget_release(struct device *dev) > > { > > + struct usb_gadget *gadget; > > + > > dev_vdbg(dev, "%s\n", __func__); > > + > > + gadget = container_of(dev, struct usb_gadget, dev); > > + complete(&gadget->done); > > + memset(dev, 0x0, sizeof(*dev)); > > No, the memory should be freed here, not memset. > This memory is allocated at UDC driver and is freed by UDC driver too. This memset may not need for other drivers, only dwc3 is needed. I added it here to avoid regression. See [2] for detail please. [1] https://www.spinics.net/lists/linux-usb/msg198767.html [2] https://www.spinics.net/lists/linux-usb/msg198725.html Peter
