Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Peter Chen <peter.chen@nxp.com>
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:25:20 +0200
Message-ID: <20200731122520.GB1655976@kroah.com> (raw)
In-Reply-To: <DB8PR04MB7162B2FAC7FACD0A11F6D8218B4E0@DB8PR04MB7162.eurprd04.prod.outlook.com>

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&amp;data=02%7C01%7Cpe
> > >
> > ter.chen%40nxp.com%7Cff1ece31761e40be0c7b08d83548ac19%7C686ea1d3bc2b
> > 4c
> > >
> > 6fa92cd99c5c301635%7C0%7C0%7C637317933526709832&amp;sdata=q7%2F1S
> > mwujv
> > > tg4DsW0iUaM2Mqf0cdkzL%2FKjJNBRfm6hc%3D&amp;reserved=0
> > >
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Cc: Alan Stern <stern@rowland.harvard.edu>
> > > Suggested-by: Alan Stern <stern@rowland.harvard.edu>
> > > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > > ---
> > > 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

  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 [this message]
2020-07-31 14:06       ` Peter Chen
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=20200731122520.GB1655976@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=balbi@kernel.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-usb@vger.kernel.org \
    --cc=peter.chen@nxp.com \
    --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