Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/1] usb: gadget: core: wait gadget device .release finishing at usb_del_gadget_udc
@ 2020-07-31  9:59 Peter Chen
  2020-07-31 11:55 ` Greg Kroah-Hartman
  0 siblings, 1 reply; 11+ messages in thread
From: Peter Chen @ 2020-07-31  9:59 UTC (permalink / raw)
  To: balbi; +Cc: linux-usb, linux-imx, Peter Chen, Greg Kroah-Hartman, Alan Stern

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.

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://www.spinics.net/lists/linux-usb/msg198790.html

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));
 }
 
 /* should be called with udc_lock held */
@@ -1184,7 +1190,7 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
 	if (release)
 		gadget->dev.release = release;
 	else
-		gadget->dev.release = usb_udc_nop_release;
+		gadget->dev.release = usb_gadget_release;
 
 	device_initialize(&gadget->dev);
 
@@ -1324,6 +1330,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
 	dev_vdbg(gadget->dev.parent, "unregistering gadget\n");
 
 	mutex_lock(&udc_lock);
+	init_completion(&gadget->done);
 	list_del(&udc->list);
 
 	if (udc->driver) {
@@ -1338,7 +1345,8 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
 	flush_work(&gadget->work);
 	device_unregister(&udc->dev);
 	device_unregister(&gadget->dev);
-	memset(&gadget->dev, 0x00, sizeof(gadget->dev));
+	/* Wait gadget release() is done */
+	wait_for_completion(&gadget->done);
 }
 EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
 
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 298b334e2951..ae346b524591 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -378,6 +378,7 @@ struct usb_gadget_ops {
  * @lpm_capable: If the gadget max_speed is FULL or HIGH, this flag
  *	indicates that it supports LPM as per the LPM ECN & errata.
  * @irq: the interrupt number for device controller.
+ * @done: gadget device's release() is done
  *
  * Gadgets have a mostly-portable "gadget driver" implementing device
  * functions, handling all usb configurations and interfaces.  Gadget
@@ -433,6 +434,7 @@ struct usb_gadget {
 	unsigned			connected:1;
 	unsigned			lpm_capable:1;
 	int				irq;
+	struct completion		done;
 };
 #define work_to_gadget(w)	(container_of((w), struct usb_gadget, work))
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] usb: gadget: core: wait gadget device .release finishing at usb_del_gadget_udc
  2020-07-31  9:59 [PATCH 1/1] usb: gadget: core: wait gadget device .release finishing at usb_del_gadget_udc Peter Chen
@ 2020-07-31 11:55 ` Greg Kroah-Hartman
  2020-07-31 12:11   ` Peter Chen
  0 siblings, 1 reply; 11+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-31 11:55 UTC (permalink / raw)
  To: Peter Chen; +Cc: balbi, linux-usb, linux-imx, Alan Stern

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.

Why does the gadget core care about this at all?  It shouldn't.



> 
> 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://www.spinics.net/lists/linux-usb/msg198790.html
> 
> 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.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH 1/1] usb: gadget: core: wait gadget device .release finishing at usb_del_gadget_udc
  2020-07-31 11:55 ` Greg Kroah-Hartman
@ 2020-07-31 12:11   ` Peter Chen
  2020-07-31 12:25     ` Greg Kroah-Hartman
  0 siblings, 1 reply; 11+ messages in thread
From: Peter Chen @ 2020-07-31 12:11 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: balbi, linux-usb, dl-linux-imx, Alan Stern

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

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 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] usb: gadget: core: wait gadget device .release finishing at usb_del_gadget_udc
  2020-07-31 12:11   ` Peter Chen
@ 2020-07-31 12:25     ` Greg Kroah-Hartman
  2020-07-31 14:06       ` Peter Chen
  0 siblings, 1 reply; 11+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-31 12:25 UTC (permalink / raw)
  To: Peter Chen; +Cc: balbi, linux-usb, dl-linux-imx, Alan Stern

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] usb: gadget: core: wait gadget device .release finishing at usb_del_gadget_udc
  2020-07-31 12:25     ` Greg Kroah-Hartman
@ 2020-07-31 14:06       ` Peter Chen
  2020-07-31 14:12         ` Alan Stern
  2020-07-31 14:16         ` Greg Kroah-Hartman
  0 siblings, 2 replies; 11+ messages in thread
From: Peter Chen @ 2020-07-31 14:06 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: balbi, linux-usb, dl-linux-imx, Alan Stern

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] usb: gadget: core: wait gadget device .release finishing at usb_del_gadget_udc
  2020-07-31 14:06       ` Peter Chen
@ 2020-07-31 14:12         ` Alan Stern
  2020-07-31 23:42           ` Peter Chen
  2020-07-31 14:16         ` Greg Kroah-Hartman
  1 sibling, 1 reply; 11+ messages in thread
From: Alan Stern @ 2020-07-31 14:12 UTC (permalink / raw)
  To: Peter Chen; +Cc: Greg Kroah-Hartman, balbi, linux-usb, dl-linux-imx

On Fri, Jul 31, 2020 at 02:06:20PM +0000, Peter Chen wrote:
> 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?

No, the real solution is to fix the UDC drivers.  They need to allocate 
the gadget structure dynamically instead of reusing it.  And they should 
have a real release routine that deallocates the gadget structure.

Alan Stern

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] usb: gadget: core: wait gadget device .release finishing at usb_del_gadget_udc
  2020-07-31 14:06       ` Peter Chen
  2020-07-31 14:12         ` Alan Stern
@ 2020-07-31 14:16         ` Greg Kroah-Hartman
  1 sibling, 0 replies; 11+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-31 14:16 UTC (permalink / raw)
  To: Peter Chen; +Cc: balbi, linux-usb, dl-linux-imx, Alan Stern

On Fri, Jul 31, 2020 at 02:06:20PM +0000, Peter Chen wrote:
> 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?

NO!!!

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

Nope.  Remove it please.

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

Depends on how this all works, but again, the release function needs to
free the memory, otherwise this is broken.

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

It should get a totally different, dynamically allocated structure.
NEVER recycle them.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] usb: gadget: core: wait gadget device .release finishing at usb_del_gadget_udc
  2020-07-31 14:12         ` Alan Stern
@ 2020-07-31 23:42           ` Peter Chen
  2020-08-01  6:53             ` Peter Chen
  0 siblings, 1 reply; 11+ messages in thread
From: Peter Chen @ 2020-07-31 23:42 UTC (permalink / raw)
  To: Alan Stern; +Cc: Greg Kroah-Hartman, balbi, linux-usb, dl-linux-imx

On 20-07-31 10:12:48, Alan Stern wrote:
> On Fri, Jul 31, 2020 at 02:06:20PM +0000, Peter Chen wrote:
> > 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?
> 
> No, the real solution is to fix the UDC drivers.  They need to allocate 
> the gadget structure dynamically instead of reusing it.  And they should 
> have a real release routine that deallocates the gadget structure.
> 
> Alan Stern

So, the basic routine should like below. I thought the usb_gadget should
be deallocated before the UDC driver remove itself (UDC device
is the parent of usb_gadget device), I may not need to wrong about it,
it is just a memory region, it could release later.

xxx_udc_release(struct device *gadget_dev)
{
	struct usb_gadget *gadget = container_of(gadget_dev, struct
			usb_gadget, dev);
	kfree(gadget);
}


xxx_udc_probe(pdev)
{
	udc_priv_data = kzalloc(sizeof(*udc_priv_data), GFP_KERNEL);
	gadget = kzalloc(sizeof(struct usb_gadget), GFP_KERNEL);
	udc_priv_data->gadget = gadget;
	...
	usb_add_gadget_udc_release(&pdev->dev, gadget, xxx_udc_release);

}

At xxx_udc_remove(pdev)
{
	usb_del_gadget_udc(udc_priv_data->gadget);
	/* need to never reference udc_priv_data->gadget any more */
	udc_priv_data other deinit;
	kfree(udc_priv_data);
}

Since all structures xxx_udc_release uses are common one, it could
replace usb_udc_nop_release at udc/core.c.

-- 

Thanks,
Peter Chen

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH 1/1] usb: gadget: core: wait gadget device .release finishing at usb_del_gadget_udc
  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
  0 siblings, 2 replies; 11+ messages in thread
From: Peter Chen @ 2020-08-01  6:53 UTC (permalink / raw)
  To: Alan Stern; +Cc: Greg Kroah-Hartman, balbi, linux-usb, dl-linux-imx

 
> 
> On 20-07-31 10:12:48, Alan Stern wrote:
> > On Fri, Jul 31, 2020 at 02:06:20PM +0000, Peter Chen wrote:
> > > 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?
> >
> > No, the real solution is to fix the UDC drivers.  They need to
> > allocate the gadget structure dynamically instead of reusing it.  And
> > they should have a real release routine that deallocates the gadget structure.
> >
> > Alan Stern
> 
> So, the basic routine should like below. I thought the usb_gadget should be
> deallocated before the UDC driver remove itself (UDC device is the parent of
> usb_gadget device), I may not need to wrong about it, it is just a memory region, it
> could release later.
> 
> xxx_udc_release(struct device *gadget_dev) {
> 	struct usb_gadget *gadget = container_of(gadget_dev, struct
> 			usb_gadget, dev);
> 	kfree(gadget);
> }
> 
> 
> xxx_udc_probe(pdev)
> {
> 	udc_priv_data = kzalloc(sizeof(*udc_priv_data), GFP_KERNEL);
> 	gadget = kzalloc(sizeof(struct usb_gadget), GFP_KERNEL);
> 	udc_priv_data->gadget = gadget;
> 	...
> 	usb_add_gadget_udc_release(&pdev->dev, gadget, xxx_udc_release);
> 
> }
> 
> At xxx_udc_remove(pdev)
> {
> 	usb_del_gadget_udc(udc_priv_data->gadget);
> 	/* need to never reference udc_priv_data->gadget any more */
> 	udc_priv_data other deinit;
> 	kfree(udc_priv_data);
> }
> 
> Since all structures xxx_udc_release uses are common one, it could replace
> usb_udc_nop_release at udc/core.c.
> 
 
Since gadget structure is allocated at UDC drivers, I think it should be freed at
the same place. Current common release function at udc/core.c may not a
good idea per our discussion.

Peter

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH 1/1] usb: gadget: core: wait gadget device .release finishing at usb_del_gadget_udc
  2020-08-01  6:53             ` Peter Chen
@ 2020-08-01  7:04               ` Jun Li
  2020-08-01 15:44               ` Alan Stern
  1 sibling, 0 replies; 11+ messages in thread
From: Jun Li @ 2020-08-01  7:04 UTC (permalink / raw)
  To: Peter Chen, Alan Stern; +Cc: Greg Kroah-Hartman, balbi, linux-usb, dl-linux-imx



> -----Original Message-----
> From: Peter Chen <peter.chen@nxp.com>
> Sent: Saturday, August 1, 2020 2:54 PM
> To: Alan Stern <stern@rowland.harvard.edu>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; balbi@kernel.org;
> linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: RE: [PATCH 1/1] usb: gadget: core: wait gadget device .release finishing
> at usb_del_gadget_udc
> 
> 
> >
> > On 20-07-31 10:12:48, Alan Stern wrote:
> > > On Fri, Jul 31, 2020 at 02:06:20PM +0000, Peter Chen wrote:
> > > > 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?
> > >
> > > No, the real solution is to fix the UDC drivers.  They need to
> > > allocate the gadget structure dynamically instead of reusing it.
> > > And they should have a real release routine that deallocates the gadget structure.
> > >
> > > Alan Stern
> >
> > So, the basic routine should like below. I thought the usb_gadget
> > should be deallocated before the UDC driver remove itself (UDC device
> > is the parent of usb_gadget device), I may not need to wrong about it,
> > it is just a memory region, it could release later.
> >
> > xxx_udc_release(struct device *gadget_dev) {
> > 	struct usb_gadget *gadget = container_of(gadget_dev, struct
> > 			usb_gadget, dev);
> > 	kfree(gadget);
> > }
> >
> >
> > xxx_udc_probe(pdev)
> > {
> > 	udc_priv_data = kzalloc(sizeof(*udc_priv_data), GFP_KERNEL);
> > 	gadget = kzalloc(sizeof(struct usb_gadget), GFP_KERNEL);
> > 	udc_priv_data->gadget = gadget;
> > 	...
> > 	usb_add_gadget_udc_release(&pdev->dev, gadget, xxx_udc_release);
> >
> > }
> >
> > At xxx_udc_remove(pdev)
> > {
> > 	usb_del_gadget_udc(udc_priv_data->gadget);
> > 	/* need to never reference udc_priv_data->gadget any more */
> > 	udc_priv_data other deinit;
> > 	kfree(udc_priv_data);
> > }
> >
> > Since all structures xxx_udc_release uses are common one, it could
> > replace usb_udc_nop_release at udc/core.c.
> >
> 
> Since gadget structure is allocated at UDC drivers, I think it should be freed at
> the same place. Current common release function at udc/core.c may not a good idea
> per our discussion.

Could all those allocation and free in UDC core? Have them in one central place
will ease/force UDC drivers to correctly handle this.

Li Jun
> 
> Peter

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] usb: gadget: core: wait gadget device .release finishing at usb_del_gadget_udc
  2020-08-01  6:53             ` Peter Chen
  2020-08-01  7:04               ` Jun Li
@ 2020-08-01 15:44               ` Alan Stern
  1 sibling, 0 replies; 11+ messages in thread
From: Alan Stern @ 2020-08-01 15:44 UTC (permalink / raw)
  To: Peter Chen; +Cc: Greg Kroah-Hartman, balbi, linux-usb, dl-linux-imx

On Sat, Aug 01, 2020 at 06:53:40AM +0000, Peter Chen wrote:
> > So, the basic routine should like below. I thought the usb_gadget should be
> > deallocated before the UDC driver remove itself (UDC device is the parent of
> > usb_gadget device), I may not need to wrong about it, it is just a memory region, it
> > could release later.
> > 
> > xxx_udc_release(struct device *gadget_dev) {
> > 	struct usb_gadget *gadget = container_of(gadget_dev, struct
> > 			usb_gadget, dev);
> > 	kfree(gadget);
> > }
> > 
> > 
> > xxx_udc_probe(pdev)
> > {
> > 	udc_priv_data = kzalloc(sizeof(*udc_priv_data), GFP_KERNEL);
> > 	gadget = kzalloc(sizeof(struct usb_gadget), GFP_KERNEL);
> > 	udc_priv_data->gadget = gadget;
> > 	...
> > 	usb_add_gadget_udc_release(&pdev->dev, gadget, xxx_udc_release);
> > 
> > }
> > 
> > At xxx_udc_remove(pdev)
> > {
> > 	usb_del_gadget_udc(udc_priv_data->gadget);
> > 	/* need to never reference udc_priv_data->gadget any more */
> > 	udc_priv_data other deinit;
> > 	kfree(udc_priv_data);
> > }

That would work.  It doesn't have to be done exactly this way.  
Depending on the driver's needs, you could do:

xxx_udc_release(struct device *dev)
{
	udc_priv_data = dev_get_drvdata(dev);
	kfree(udc_priv_data);
}

xxx_udc_probe(pdev)
{
	udc_priv_data = kzalloc(sizeof(*udc_priv_data), GFP_KERNEL);
	dev_set_drvdata(&udc_priv_data->gadget.dev, udc_priv_data);
	platform_set_drvdata(pdev, udc_priv_data);
	...
	usb_add_gadget_udc_release(&pdev->dev, &udc_priv_data->gadget,
			xxx_udc_release);
}

xxx_udc_remove(pdev)
{
	udc_priv_data = platform_get_drvdata(pdev);
	usb_del_gadget_udc(&udc_priv_data->gadget);
}

In other words, embed the struct gadget inside the udc_priv_data 
structure.  The difference is whether you want to keep the udc_priv_data 
structure hanging around even while the controller is in host mode; if 
you do then your approach (a separate struct gadget) is better.  For a 
peripheral-only controller, my approach would be better.

> > Since all structures xxx_udc_release uses are common one, it could replace
> > usb_udc_nop_release at udc/core.c.

Yes, it could.  But first all the UDC drivers would have to be modified.

> Since gadget structure is allocated at UDC drivers, I think it should be freed at
> the same place. Current common release function at udc/core.c may not a
> good idea per our discussion.

Agreed.

Alan Stern

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-31  9:59 [PATCH 1/1] usb: gadget: core: wait gadget device .release finishing at usb_del_gadget_udc 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
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

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