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

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

[Peter Chen]

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

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

[Greg Kroah-Hartman]

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

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

[Peter Chen]

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

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

[Greg Kroah-Hartman]

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

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

[Peter Chen]

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

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

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

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

[Greg Kroah-Hartman]

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

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

[Peter Chen]

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

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

[Peter Chen]

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

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

[Jun Li]

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

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

[Alan Stern]

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

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 954 bytes --]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

Hi,

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

then we break usage of container_of(). That's okay, but we have to add
some sort of private_data to the gadget structure so UDC drivers can get
their own pointers back.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

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

[Alan Stern]

On Mon, Sep 07, 2020 at 10:48:29AM +0300, Felipe Balbi wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> Hi,
> 
> > On Fri, Jul 31, 2020 at 02:06:20PM +0000, Peter Chen wrote:
> >> > 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.
> 
> then we break usage of container_of(). That's okay, but we have to add
> some sort of private_data to the gadget structure so UDC drivers can get
> their own pointers back.

As you've probably seen by now, Peter solved this problem by storing the 
private back-pointer in gadget->dev.platform_data.

Alan Stern

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1319 bytes --]


Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
> On Mon, Sep 07, 2020 at 10:48:29AM +0300, Felipe Balbi wrote:
>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>> 
>> Hi,
>> 
>> > On Fri, Jul 31, 2020 at 02:06:20PM +0000, Peter Chen wrote:
>> >> > 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.
>> 
>> then we break usage of container_of(). That's okay, but we have to add
>> some sort of private_data to the gadget structure so UDC drivers can get
>> their own pointers back.
>
> As you've probably seen by now, Peter solved this problem by storing the 
> private back-pointer in gadget->dev.platform_data.

Cool, as long as we have a solution :-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]