| Message ID | 20200731095935.23034-1-peter.chen@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] usb: gadget: core: wait gadget device .release finishing at usb_del_gadget_udc | expand |
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
> > 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
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
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.
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
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
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.
> > 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
> -----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
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
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.
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
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 :-)
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))
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(-)