Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
From: Peter Chen <peter.chen@nxp.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "balbi@kernel.org" <balbi@kernel.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	dl-linux-imx <linux-imx@nxp.com>,
	Alan Stern <stern@rowland.harvard.edu>
Subject: Re: [PATCH 1/1] usb: gadget: core: wait gadget device .release finishing at usb_del_gadget_udc
Date: Fri, 31 Jul 2020 14:06:20 +0000
Message-ID: <20200731140553.GA8013@b29397-desktop> (raw)
In-Reply-To: <20200731122520.GB1655976@kroah.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

  reply index

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-31  9:59 Peter Chen
2020-07-31 11:55 ` Greg Kroah-Hartman
2020-07-31 12:11   ` Peter Chen
2020-07-31 12:25     ` Greg Kroah-Hartman
2020-07-31 14:06       ` Peter Chen [this message]
2020-07-31 14:12         ` Alan Stern
2020-07-31 23:42           ` Peter Chen
2020-08-01  6:53             ` Peter Chen
2020-08-01  7:04               ` Jun Li
2020-08-01 15:44               ` Alan Stern
2020-07-31 14:16         ` Greg Kroah-Hartman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200731140553.GA8013@b29397-desktop \
    --to=peter.chen@nxp.com \
    --cc=balbi@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-usb@vger.kernel.org \
    --cc=stern@rowland.harvard.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git