On Fri, Jul 31, 2020 at 12:11:32PM +0000, Peter Chen wrote: > > > > > 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? Grab a reference from somewhere else and do not give it up for a long time. > > 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. That's wrong, the release function should be where this is released. And this no-op function is horrid. There used to be documentation in the kernel where I could rant about this, but instead, I'll just say, "why are people trying to work around warnings we put in the core kernel to fix common problems? Do they think we did that just because we wanted to be mean???" Please fix this properly. greg k-h
