Re: [PATCH 1/1] usb: gadget: core: wait gadget device .release finishing at usb_del_gadget_udc

[Peter Chen <peter.chen@nxp.com>]

On 20-07-31 14:25:20, Greg Kroah-Hartman wrote:
> On Fri, Jul 31, 2020 at 12:11:32PM +0000, Peter Chen wrote:
> 
> Grab a reference from somewhere else and do not give it up for a long
> time.
> 

So wait_for_completion_timeout is suitable? The similar use case is when
we open the file at the USB Drive at Windows, and we click "Eject", it
will say "The device is currently in use", and refuse our "Eject"
operation.

When we try to remove the gadget, if the gadget is in use, we could
refuse the remove operation, reasonable?

> > > > -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.

So, the release function should be at individual UDC driver, a common
release function is improper, right?

> 
> 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???"
> 

So, like kernel doc for device_initialize said, a proper fix for dwc3
should be zeroed gadget device memory at its own driver before the 
gadget device register to driver core, right?


 * All fields in @dev must be initialized by the caller to 0, except
 * for those explicitly set to some other value.  The simplest
 * approach is to use kzalloc() to allocate the structure containing
 * @dev.


-- 

Thanks,
Peter Chen