* [PATCH v3 0/3] usb: dwc3: dual-role support
@ 2017-04-03 12:20 Roger Quadros
2017-04-03 12:20 ` [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device Roger Quadros
` (2 more replies)
0 siblings, 3 replies; 21+ messages in thread
From: Roger Quadros @ 2017-04-03 12:20 UTC (permalink / raw)
To: balbi; +Cc: vivek.gautam, linux-usb, linux-kernel, Roger Quadros
Hi,
Taking a simplistic approach this time. We don't use the OTG controller
block at all. Instead we just rely on ID events via extcon framework.
I tried to get rid of workqueue but unfortunately it causes too much pain
when the UDC is unregistered.
https://hastebin.com/upugaqogol.xml
https://hastebin.com/yayaduqodo.xml
So I've still kept the workqueue around.
We also have debugfs role switching, but I don't yet see how we
can use this for real testing as I still need to manually plug/unplug
the USB cables (host vs device) to test switching :).
v3:
- restructure and simplify. Remove OTG controller code, only rely on extcon.
cheers,
-roger
Roger Quadros (3):
usb: udc: allow adding and removing the same gadget device
usb: dwc3: make role-switching work with debugfs/mode
usb: dwc3: Add dual-role support
drivers/usb/dwc3/Makefile | 4 +
drivers/usb/dwc3/core.c | 18 ++---
drivers/usb/dwc3/core.h | 22 ++++++
drivers/usb/dwc3/debugfs.c | 49 +++++++++++--
drivers/usb/dwc3/drd.c | 167 ++++++++++++++++++++++++++++++++++++++++++
drivers/usb/gadget/udc/core.c | 1 +
6 files changed, 242 insertions(+), 19 deletions(-)
create mode 100644 drivers/usb/dwc3/drd.c
--
2.7.4
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device 2017-04-03 12:20 [PATCH v3 0/3] usb: dwc3: dual-role support Roger Quadros @ 2017-04-03 12:20 ` Roger Quadros 2017-04-03 14:19 ` Alan Stern 2017-04-03 12:20 ` [PATCH v3 2/3] usb: dwc3: make role-switching work with debugfs/mode Roger Quadros 2017-04-03 12:20 ` [PATCH v3 3/3] usb: dwc3: Add dual-role support Roger Quadros 2 siblings, 1 reply; 21+ messages in thread From: Roger Quadros @ 2017-04-03 12:20 UTC (permalink / raw) To: balbi; +Cc: vivek.gautam, linux-usb, linux-kernel, Roger Quadros allow usb_del_gadget_udc() and usb add_gadget_udc() to be called repeatedly on the same gadget->dev structure. We need to clear the gadget->dev structure so that kobject_init() doesn't complain about already initialized object. Signed-off-by: Roger Quadros <rogerq@ti.com> --- drivers/usb/gadget/udc/core.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c index d685d82..efce68e 100644 --- a/drivers/usb/gadget/udc/core.c +++ b/drivers/usb/gadget/udc/core.c @@ -1273,6 +1273,7 @@ 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)); } EXPORT_SYMBOL_GPL(usb_del_gadget_udc); -- 2.7.4 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device 2017-04-03 12:20 ` [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device Roger Quadros @ 2017-04-03 14:19 ` Alan Stern 2017-04-04 7:47 ` Felipe Balbi 0 siblings, 1 reply; 21+ messages in thread From: Alan Stern @ 2017-04-03 14:19 UTC (permalink / raw) To: Roger Quadros; +Cc: balbi, vivek.gautam, linux-usb, linux-kernel On Mon, 3 Apr 2017, Roger Quadros wrote: > allow usb_del_gadget_udc() and usb add_gadget_udc() to be called > repeatedly on the same gadget->dev structure. > > We need to clear the gadget->dev structure so that kobject_init() > doesn't complain about already initialized object. > > Signed-off-by: Roger Quadros <rogerq@ti.com> > --- > drivers/usb/gadget/udc/core.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c > index d685d82..efce68e 100644 > --- a/drivers/usb/gadget/udc/core.c > +++ b/drivers/usb/gadget/udc/core.c > @@ -1273,6 +1273,7 @@ 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)); > } > EXPORT_SYMBOL_GPL(usb_del_gadget_udc); Isn't this dangerous? It's quite possible that the device_unregister() call on the previous line invokes the gadget->dev.release callback, which might deallocate gadget. If that happens, your new memset will oops. In general, if an object relies on reference counting for its lifetime, you cannot register and unregister it more than once. A typical issue is that some code retains a reference to the old instance and tries to use it after the new instance has been registered, thereby messing up the new instance. I don't know if that is possible in this case, but it is something to watch out for. Alan Stern ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device 2017-04-03 14:19 ` Alan Stern @ 2017-04-04 7:47 ` Felipe Balbi 2017-04-04 14:17 ` Alan Stern 0 siblings, 1 reply; 21+ messages in thread From: Felipe Balbi @ 2017-04-04 7:47 UTC (permalink / raw) To: Alan Stern, Roger Quadros; +Cc: vivek.gautam, linux-usb, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1617 bytes --] Hi, Alan Stern <stern@rowland.harvard.edu> writes: > On Mon, 3 Apr 2017, Roger Quadros wrote: > >> allow usb_del_gadget_udc() and usb add_gadget_udc() to be called >> repeatedly on the same gadget->dev structure. >> >> We need to clear the gadget->dev structure so that kobject_init() >> doesn't complain about already initialized object. >> >> Signed-off-by: Roger Quadros <rogerq@ti.com> >> --- >> drivers/usb/gadget/udc/core.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c >> index d685d82..efce68e 100644 >> --- a/drivers/usb/gadget/udc/core.c >> +++ b/drivers/usb/gadget/udc/core.c >> @@ -1273,6 +1273,7 @@ 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)); >> } >> EXPORT_SYMBOL_GPL(usb_del_gadget_udc); > > Isn't this dangerous? It's quite possible that the device_unregister() not on the gadget API, no. > call on the previous line invokes the gadget->dev.release callback, > which might deallocate gadget. If that happens, your new memset will > oops. that won't happen. struct usb_gadget is a member of the UDC's private structure, like this: struct dwc3 { [...] struct usb_gadget gadget; struct usb_gadget_driver *gadget_driver; [...] }; I'm actually thinking that struct usb_gadget shouldn't have a struct device at all. Just a pointer to a device, that would solve all these issues. -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device 2017-04-04 7:47 ` Felipe Balbi @ 2017-04-04 14:17 ` Alan Stern 2017-04-05 8:34 ` Felipe Balbi 0 siblings, 1 reply; 21+ messages in thread From: Alan Stern @ 2017-04-04 14:17 UTC (permalink / raw) To: Felipe Balbi; +Cc: Roger Quadros, vivek.gautam, linux-usb, linux-kernel On Tue, 4 Apr 2017, Felipe Balbi wrote: > Hi, > > Alan Stern <stern@rowland.harvard.edu> writes: > > On Mon, 3 Apr 2017, Roger Quadros wrote: > > > >> allow usb_del_gadget_udc() and usb add_gadget_udc() to be called > >> repeatedly on the same gadget->dev structure. > >> > >> We need to clear the gadget->dev structure so that kobject_init() > >> doesn't complain about already initialized object. > >> > >> Signed-off-by: Roger Quadros <rogerq@ti.com> > >> --- > >> drivers/usb/gadget/udc/core.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c > >> index d685d82..efce68e 100644 > >> --- a/drivers/usb/gadget/udc/core.c > >> +++ b/drivers/usb/gadget/udc/core.c > >> @@ -1273,6 +1273,7 @@ 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)); > >> } > >> EXPORT_SYMBOL_GPL(usb_del_gadget_udc); > > > > Isn't this dangerous? It's quite possible that the device_unregister() > > not on the gadget API, no. > > > call on the previous line invokes the gadget->dev.release callback, > > which might deallocate gadget. If that happens, your new memset will > > oops. > > that won't happen. struct usb_gadget is a member of the UDC's private > structure, like this: > > struct dwc3 { > [...] > struct usb_gadget gadget; > struct usb_gadget_driver *gadget_driver; > [...] > }; Yes. So what? Can't the UDC driver use the refcount inside struct usb_gadget to control the lifetime of its private structure? (By the way, can you tell what's going on in net2280.c? I must be missing something; it looks like gadget_release() would quickly run into problems because it calls dev_get_drvdata() for &gadget->dev, but net2280_probe() never calls dev_set_drvdata() for that device. Furthermore, net2280_remove() continues to reference the net2280 struct after calling usb_del_gadget_udc(), and it never does seem to do a final put.) > I'm actually thinking that struct usb_gadget shouldn't have a struct > device at all. Just a pointer to a device, that would solve all these > issues. A pointer to which device? The UDC? That would change the directory layout in sysfs. Or a pointer to a separate dynamically allocated device (the way struct usb_hcd contains a pointer to the root hub device)? That would work. If the UDC driver wanted to re-register the gadget, it would have to allocate a new device. Alan Stern ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device 2017-04-04 14:17 ` Alan Stern @ 2017-04-05 8:34 ` Felipe Balbi 2017-04-05 14:12 ` Alan Stern 0 siblings, 1 reply; 21+ messages in thread From: Felipe Balbi @ 2017-04-05 8:34 UTC (permalink / raw) To: Alan Stern; +Cc: Roger Quadros, vivek.gautam, linux-usb, linux-kernel [-- Attachment #1: Type: text/plain, Size: 3181 bytes --] Hi, Alan Stern <stern@rowland.harvard.edu> writes: >> >> allow usb_del_gadget_udc() and usb add_gadget_udc() to be called >> >> repeatedly on the same gadget->dev structure. >> >> >> >> We need to clear the gadget->dev structure so that kobject_init() >> >> doesn't complain about already initialized object. >> >> >> >> Signed-off-by: Roger Quadros <rogerq@ti.com> >> >> --- >> >> drivers/usb/gadget/udc/core.c | 1 + >> >> 1 file changed, 1 insertion(+) >> >> >> >> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c >> >> index d685d82..efce68e 100644 >> >> --- a/drivers/usb/gadget/udc/core.c >> >> +++ b/drivers/usb/gadget/udc/core.c >> >> @@ -1273,6 +1273,7 @@ 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)); >> >> } >> >> EXPORT_SYMBOL_GPL(usb_del_gadget_udc); >> > >> > Isn't this dangerous? It's quite possible that the device_unregister() >> >> not on the gadget API, no. >> >> > call on the previous line invokes the gadget->dev.release callback, >> > which might deallocate gadget. If that happens, your new memset will >> > oops. >> >> that won't happen. struct usb_gadget is a member of the UDC's private >> structure, like this: >> >> struct dwc3 { >> [...] >> struct usb_gadget gadget; >> struct usb_gadget_driver *gadget_driver; >> [...] >> }; > > Yes. So what? Can't the UDC driver use the refcount inside struct > usb_gadget to control the lifetime of its private structure? nope, not being used. At least not yet. > (By the way, can you tell what's going on in net2280.c? I must be > missing something; it looks like gadget_release() would quickly run > into problems because it calls dev_get_drvdata() for &gadget->dev, but > net2280_probe() never calls dev_set_drvdata() for that device. > Furthermore, net2280_remove() continues to reference the net2280 struct > after calling usb_del_gadget_udc(), and it never does seem to do a > final put.) static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id) { struct net2280 *dev; unsigned long resource, len; void __iomem *base = NULL; int retval, i; /* alloc, and start init */ dev = kzalloc(sizeof(*dev), GFP_KERNEL); if (dev == NULL) { retval = -ENOMEM; goto done; } pci_set_drvdata(pdev, dev); ^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> I'm actually thinking that struct usb_gadget shouldn't have a struct >> device at all. Just a pointer to a device, that would solve all these >> issues. > > A pointer to which device? The UDC? That would change the directory > layout in sysfs. indeed. Would that be a problem? > Or a pointer to a separate dynamically allocated device (the way struct > usb_hcd contains a pointer to the root hub device)? That would work. > If the UDC driver wanted to re-register the gadget, it would have to > allocate a new device. That could be done as well, if maintaining the directory structure is a must. -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device 2017-04-05 8:34 ` Felipe Balbi @ 2017-04-05 14:12 ` Alan Stern 2017-04-10 10:05 ` Felipe Balbi 0 siblings, 1 reply; 21+ messages in thread From: Alan Stern @ 2017-04-05 14:12 UTC (permalink / raw) To: Felipe Balbi; +Cc: Roger Quadros, vivek.gautam, linux-usb, linux-kernel On Wed, 5 Apr 2017, Felipe Balbi wrote: > >> >> --- a/drivers/usb/gadget/udc/core.c > >> >> +++ b/drivers/usb/gadget/udc/core.c > >> >> @@ -1273,6 +1273,7 @@ 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)); > >> >> } > >> >> EXPORT_SYMBOL_GPL(usb_del_gadget_udc); > >> > > >> > Isn't this dangerous? It's quite possible that the device_unregister() > >> > >> not on the gadget API, no. > >> > >> > call on the previous line invokes the gadget->dev.release callback, > >> > which might deallocate gadget. If that happens, your new memset will > >> > oops. > >> > >> that won't happen. struct usb_gadget is a member of the UDC's private > >> structure, like this: > >> > >> struct dwc3 { > >> [...] > >> struct usb_gadget gadget; > >> struct usb_gadget_driver *gadget_driver; > >> [...] > >> }; > > > > Yes. So what? Can't the UDC driver use the refcount inside struct > > usb_gadget to control the lifetime of its private structure? > > nope, not being used. At least not yet. I'm not convinced (yet)... > > (By the way, can you tell what's going on in net2280.c? I must be > > missing something; it looks like gadget_release() would quickly run > > into problems because it calls dev_get_drvdata() for &gadget->dev, but > > net2280_probe() never calls dev_set_drvdata() for that device. > > Furthermore, net2280_remove() continues to reference the net2280 struct > > after calling usb_del_gadget_udc(), and it never does seem to do a > > final put.) > > static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id) > { > struct net2280 *dev; > unsigned long resource, len; > void __iomem *base = NULL; > int retval, i; > > /* alloc, and start init */ > dev = kzalloc(sizeof(*dev), GFP_KERNEL); > if (dev == NULL) { > retval = -ENOMEM; > goto done; > } > > pci_set_drvdata(pdev, dev); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^ That sets the driver data in the struct pci_dev, not in dev->gadget.dev. As far as I can see, _nothing_ in the driver sets the driver data in dev->gadget.dev. (Even after all these years, I still get bothered by the way Dave Brownell used to call everything "dev"... IIRC, at one time he had a line of code that went something like: dev->dev.dev = &pdev->dev !) > >> I'm actually thinking that struct usb_gadget shouldn't have a struct > >> device at all. Just a pointer to a device, that would solve all these > >> issues. > > > > A pointer to which device? The UDC? That would change the directory > > layout in sysfs. > > indeed. Would that be a problem? Possibly for some userspace tool. > > Or a pointer to a separate dynamically allocated device (the way struct > > usb_hcd contains a pointer to the root hub device)? That would work. > > If the UDC driver wanted to re-register the gadget, it would have to > > allocate a new device. > > That could be done as well, if maintaining the directory structure is a > must. Alan Stern ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device 2017-04-05 14:12 ` Alan Stern @ 2017-04-10 10:05 ` Felipe Balbi 2017-04-10 15:17 ` Alan Stern 0 siblings, 1 reply; 21+ messages in thread From: Felipe Balbi @ 2017-04-10 10:05 UTC (permalink / raw) To: Alan Stern; +Cc: Roger Quadros, vivek.gautam, linux-usb, linux-kernel [-- Attachment #1: Type: text/plain, Size: 3471 bytes --] Hi, Alan Stern <stern@rowland.harvard.edu> writes: > On Wed, 5 Apr 2017, Felipe Balbi wrote: > >> >> >> --- a/drivers/usb/gadget/udc/core.c >> >> >> +++ b/drivers/usb/gadget/udc/core.c >> >> >> @@ -1273,6 +1273,7 @@ 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)); >> >> >> } >> >> >> EXPORT_SYMBOL_GPL(usb_del_gadget_udc); >> >> > >> >> > Isn't this dangerous? It's quite possible that the device_unregister() >> >> >> >> not on the gadget API, no. >> >> >> >> > call on the previous line invokes the gadget->dev.release callback, >> >> > which might deallocate gadget. If that happens, your new memset will >> >> > oops. >> >> >> >> that won't happen. struct usb_gadget is a member of the UDC's private >> >> structure, like this: >> >> >> >> struct dwc3 { >> >> [...] >> >> struct usb_gadget gadget; >> >> struct usb_gadget_driver *gadget_driver; >> >> [...] >> >> }; >> > >> > Yes. So what? Can't the UDC driver use the refcount inside struct >> > usb_gadget to control the lifetime of its private structure? >> >> nope, not being used. At least not yet. > > I'm not convinced (yet)... > >> > (By the way, can you tell what's going on in net2280.c? I must be >> > missing something; it looks like gadget_release() would quickly run >> > into problems because it calls dev_get_drvdata() for &gadget->dev, but >> > net2280_probe() never calls dev_set_drvdata() for that device. >> > Furthermore, net2280_remove() continues to reference the net2280 struct >> > after calling usb_del_gadget_udc(), and it never does seem to do a >> > final put.) >> >> static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id) >> { >> struct net2280 *dev; >> unsigned long resource, len; >> void __iomem *base = NULL; >> int retval, i; >> >> /* alloc, and start init */ >> dev = kzalloc(sizeof(*dev), GFP_KERNEL); >> if (dev == NULL) { >> retval = -ENOMEM; >> goto done; >> } >> >> pci_set_drvdata(pdev, dev); >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > That sets the driver data in the struct pci_dev, not in > dev->gadget.dev. As far as I can see, _nothing_ in the driver sets the > driver data in dev->gadget.dev. hmmm, indeed. The same is happening with other callers of usb_add_gadget_udc_release(). I guess this should be enough? @@ -3557,7 +3557,7 @@ static irqreturn_t net2280_irq(int irq, void *_dev) static void gadget_release(struct device *_dev) { - struct net2280 *dev = dev_get_drvdata(_dev); + struct net2280 *dev = dev_get_drvdata(_dev->parent); kfree(dev); } > (Even after all these years, I still get bothered by the way Dave > Brownell used to call everything "dev"... IIRC, at one time he had a > line of code that went something like: dev->dev.dev = &pdev->dev !) :-) >> >> I'm actually thinking that struct usb_gadget shouldn't have a struct >> >> device at all. Just a pointer to a device, that would solve all these >> >> issues. >> > >> > A pointer to which device? The UDC? That would change the directory >> > layout in sysfs. >> >> indeed. Would that be a problem? > > Possibly for some userspace tool. yeah, we can do dynamic allocation of the device pointer, no issue. -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device 2017-04-10 10:05 ` Felipe Balbi @ 2017-04-10 15:17 ` Alan Stern 2017-04-11 7:34 ` Felipe Balbi 0 siblings, 1 reply; 21+ messages in thread From: Alan Stern @ 2017-04-10 15:17 UTC (permalink / raw) To: Felipe Balbi; +Cc: Roger Quadros, vivek.gautam, linux-usb, linux-kernel On Mon, 10 Apr 2017, Felipe Balbi wrote: > Hi, > > Alan Stern <stern@rowland.harvard.edu> writes: > > On Wed, 5 Apr 2017, Felipe Balbi wrote: > > > >> >> >> --- a/drivers/usb/gadget/udc/core.c > >> >> >> +++ b/drivers/usb/gadget/udc/core.c > >> >> >> @@ -1273,6 +1273,7 @@ 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)); > >> >> >> } > >> >> >> EXPORT_SYMBOL_GPL(usb_del_gadget_udc); > >> >> > > >> >> > Isn't this dangerous? It's quite possible that the device_unregister() > >> >> > >> >> not on the gadget API, no. > >> >> > >> >> > call on the previous line invokes the gadget->dev.release callback, > >> >> > which might deallocate gadget. If that happens, your new memset will > >> >> > oops. > >> >> > >> >> that won't happen. struct usb_gadget is a member of the UDC's private > >> >> structure, like this: > >> >> > >> >> struct dwc3 { > >> >> [...] > >> >> struct usb_gadget gadget; > >> >> struct usb_gadget_driver *gadget_driver; > >> >> [...] > >> >> }; > >> > > >> > Yes. So what? Can't the UDC driver use the refcount inside struct > >> > usb_gadget to control the lifetime of its private structure? > >> > >> nope, not being used. At least not yet. > > > > I'm not convinced (yet)... > > > >> > (By the way, can you tell what's going on in net2280.c? I must be > >> > missing something; it looks like gadget_release() would quickly run > >> > into problems because it calls dev_get_drvdata() for &gadget->dev, but > >> > net2280_probe() never calls dev_set_drvdata() for that device. > >> > Furthermore, net2280_remove() continues to reference the net2280 struct > >> > after calling usb_del_gadget_udc(), and it never does seem to do a > >> > final put.) > >> > >> static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id) > >> { > >> struct net2280 *dev; > >> unsigned long resource, len; > >> void __iomem *base = NULL; > >> int retval, i; > >> > >> /* alloc, and start init */ > >> dev = kzalloc(sizeof(*dev), GFP_KERNEL); > >> if (dev == NULL) { > >> retval = -ENOMEM; > >> goto done; > >> } > >> > >> pci_set_drvdata(pdev, dev); > >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > That sets the driver data in the struct pci_dev, not in > > dev->gadget.dev. As far as I can see, _nothing_ in the driver sets the > > driver data in dev->gadget.dev. > > hmmm, indeed. The same is happening with other callers of > usb_add_gadget_udc_release(). > > I guess this should be enough? > > @@ -3557,7 +3557,7 @@ static irqreturn_t net2280_irq(int irq, void *_dev) > > static void gadget_release(struct device *_dev) > { > - struct net2280 *dev = dev_get_drvdata(_dev); > + struct net2280 *dev = dev_get_drvdata(_dev->parent); > > kfree(dev); > } Oddly enough, yes. But it doesn't explain why this code doesn't blow up every time it gets called, in its current form. And it doesn't help with the fact that net2280_remove() continues to access the private data structure after calling usb_del_gadget_udc(). Strictly speaking, that routine should do get_device(&dev->gadget.dev); at the start, with a corresponding put_device() at the end. There's another problem. Suppose a call to usb_add_gadget_udc_release() fails. At the end of that routine, the error pathway does put_device(&gadget->dev). This will invoke the release callback, deallocating the private data structure without giving the caller (i.e., the UDC driver) a chance to clean up. Alan Stern ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device 2017-04-10 15:17 ` Alan Stern @ 2017-04-11 7:34 ` Felipe Balbi 2017-04-11 14:12 ` Alan Stern 0 siblings, 1 reply; 21+ messages in thread From: Felipe Balbi @ 2017-04-11 7:34 UTC (permalink / raw) To: Alan Stern; +Cc: Roger Quadros, vivek.gautam, linux-usb, linux-kernel [-- Attachment #1: Type: text/plain, Size: 5229 bytes --] Hi, Alan Stern <stern@rowland.harvard.edu> writes: >> >> >> >> --- a/drivers/usb/gadget/udc/core.c >> >> >> >> +++ b/drivers/usb/gadget/udc/core.c >> >> >> >> @@ -1273,6 +1273,7 @@ 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)); >> >> >> >> } >> >> >> >> EXPORT_SYMBOL_GPL(usb_del_gadget_udc); >> >> >> > >> >> >> > Isn't this dangerous? It's quite possible that the device_unregister() >> >> >> >> >> >> not on the gadget API, no. >> >> >> >> >> >> > call on the previous line invokes the gadget->dev.release callback, >> >> >> > which might deallocate gadget. If that happens, your new memset will >> >> >> > oops. >> >> >> >> >> >> that won't happen. struct usb_gadget is a member of the UDC's private >> >> >> structure, like this: >> >> >> >> >> >> struct dwc3 { >> >> >> [...] >> >> >> struct usb_gadget gadget; >> >> >> struct usb_gadget_driver *gadget_driver; >> >> >> [...] >> >> >> }; >> >> > >> >> > Yes. So what? Can't the UDC driver use the refcount inside struct >> >> > usb_gadget to control the lifetime of its private structure? >> >> >> >> nope, not being used. At least not yet. >> > >> > I'm not convinced (yet)... >> > >> >> > (By the way, can you tell what's going on in net2280.c? I must be >> >> > missing something; it looks like gadget_release() would quickly run >> >> > into problems because it calls dev_get_drvdata() for &gadget->dev, but >> >> > net2280_probe() never calls dev_set_drvdata() for that device. >> >> > Furthermore, net2280_remove() continues to reference the net2280 struct >> >> > after calling usb_del_gadget_udc(), and it never does seem to do a >> >> > final put.) >> >> >> >> static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id) >> >> { >> >> struct net2280 *dev; >> >> unsigned long resource, len; >> >> void __iomem *base = NULL; >> >> int retval, i; >> >> >> >> /* alloc, and start init */ >> >> dev = kzalloc(sizeof(*dev), GFP_KERNEL); >> >> if (dev == NULL) { >> >> retval = -ENOMEM; >> >> goto done; >> >> } >> >> >> >> pci_set_drvdata(pdev, dev); >> >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> > >> > That sets the driver data in the struct pci_dev, not in >> > dev->gadget.dev. As far as I can see, _nothing_ in the driver sets the >> > driver data in dev->gadget.dev. >> >> hmmm, indeed. The same is happening with other callers of >> usb_add_gadget_udc_release(). >> >> I guess this should be enough? >> >> @@ -3557,7 +3557,7 @@ static irqreturn_t net2280_irq(int irq, void *_dev) >> >> static void gadget_release(struct device *_dev) >> { >> - struct net2280 *dev = dev_get_drvdata(_dev); >> + struct net2280 *dev = dev_get_drvdata(_dev->parent); >> >> kfree(dev); >> } > > Oddly enough, yes. But it doesn't explain why this code doesn't blow > up every time it gets called, in its current form. Well, it does :-) dev_get_drvdata(_dev) -> NULL -> kfree(NULL) We're just leaking memory. I guess a patch like below would be best: diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c index 3828c2ec8623..4dc04253da61 100644 --- a/drivers/usb/gadget/udc/net2280.c +++ b/drivers/usb/gadget/udc/net2280.c @@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void *_dev) /*-------------------------------------------------------------------------*/ -static void gadget_release(struct device *_dev) -{ - struct net2280 *dev = dev_get_drvdata(_dev); - - kfree(dev); -} - /* tear down the binding between this driver and the pci device */ static void net2280_remove(struct pci_dev *pdev) @@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev) device_remove_file(&pdev->dev, &dev_attr_registers); ep_info(dev, "unbind\n"); + + kfree(dev); } /* wrap this driver around the specified device, but @@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id) if (retval) goto done; - retval = usb_add_gadget_udc_release(&pdev->dev, &dev->gadget, - gadget_release); + retval = usb_add_gadget_udc(&pdev->dev, &dev->gadget); if (retval) goto done; return 0; > And it doesn't help with the fact that net2280_remove() continues to > access the private data structure after calling usb_del_gadget_udc(). > Strictly speaking, that routine should do > > get_device(&dev->gadget.dev); > > at the start, with a corresponding put_device() at the end. > > There's another problem. Suppose a call to > usb_add_gadget_udc_release() fails. At the end of that routine, the > error pathway does put_device(&gadget->dev). This will invoke the > release callback, deallocating the private data structure without > giving the caller (i.e., the UDC driver) a chance to clean up. it won't deallocate anything :-) dev_set_drvdata() was never called, we will endup with kfree(NULL) which is safe and just silently returns. -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device 2017-04-11 7:34 ` Felipe Balbi @ 2017-04-11 14:12 ` Alan Stern 2017-04-11 14:19 ` Greg KH 0 siblings, 1 reply; 21+ messages in thread From: Alan Stern @ 2017-04-11 14:12 UTC (permalink / raw) To: Felipe Balbi, Greg KH Cc: Roger Quadros, vivek.gautam, USB list, Kernel development list On Tue, 11 Apr 2017, Felipe Balbi wrote: > > Oddly enough, yes. But it doesn't explain why this code doesn't blow > > up every time it gets called, in its current form. > > Well, it does :-) > > dev_get_drvdata(_dev) -> NULL -> kfree(NULL) > > We're just leaking memory. I guess a patch like below would be best: > > diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c > index 3828c2ec8623..4dc04253da61 100644 > --- a/drivers/usb/gadget/udc/net2280.c > +++ b/drivers/usb/gadget/udc/net2280.c > @@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void *_dev) > > /*-------------------------------------------------------------------------*/ > > -static void gadget_release(struct device *_dev) > -{ > - struct net2280 *dev = dev_get_drvdata(_dev); > - > - kfree(dev); > -} > - > /* tear down the binding between this driver and the pci device */ > > static void net2280_remove(struct pci_dev *pdev) > @@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev) > device_remove_file(&pdev->dev, &dev_attr_registers); > > ep_info(dev, "unbind\n"); > + > + kfree(dev); > } > > /* wrap this driver around the specified device, but > @@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id) > if (retval) > goto done; > > - retval = usb_add_gadget_udc_release(&pdev->dev, &dev->gadget, > - gadget_release); > + retval = usb_add_gadget_udc(&pdev->dev, &dev->gadget); > if (retval) > goto done; > return 0; Maybe... But I can't shake the feeling that Greg KH would strongly disagree. Hasn't he said, many times in the past, that any dynamically allocated device structure _must_ have a real release routine? usb_udc_nop_release() doesn't qualify. The issue is outstanding references to gadget.dev. The driver core does not guarantee that all references will have been dropped by the time device_unregister() returns. So what if some other part of the kernel still has a reference to gadget.dev when we reach the end of net2280_remove() and deallocate the private structure? Unless you can be certain that there are no outstanding references when the gadget is unregistered, this approach isn't safe. (And even if you can be certain, how can you know that future changes to the kernel won't affect the situation?) > > And it doesn't help with the fact that net2280_remove() continues to > > access the private data structure after calling usb_del_gadget_udc(). > > Strictly speaking, that routine should do > > > > get_device(&dev->gadget.dev); > > > > at the start, with a corresponding put_device() at the end. > > > > There's another problem. Suppose a call to > > usb_add_gadget_udc_release() fails. At the end of that routine, the > > error pathway does put_device(&gadget->dev). This will invoke the > > release callback, deallocating the private data structure without > > giving the caller (i.e., the UDC driver) a chance to clean up. > > it won't deallocate anything :-) dev_set_drvdata() was never called, > we will endup with kfree(NULL) which is safe and just silently returns. But if you change gadget_release() to use the parent's drvdata field, like you proposed earlier, and fix up the refcounting in net2280_remove() so that the structure doesn't get deallocated too quickly, then this does become a real problem. And it can affect other UDC drivers, not just net2280. Alan Stern ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device 2017-04-11 14:12 ` Alan Stern @ 2017-04-11 14:19 ` Greg KH 2017-04-12 6:01 ` Felipe Balbi 0 siblings, 1 reply; 21+ messages in thread From: Greg KH @ 2017-04-11 14:19 UTC (permalink / raw) To: Alan Stern Cc: Felipe Balbi, Roger Quadros, vivek.gautam, USB list, Kernel development list On Tue, Apr 11, 2017 at 10:12:01AM -0400, Alan Stern wrote: > On Tue, 11 Apr 2017, Felipe Balbi wrote: > > > > Oddly enough, yes. But it doesn't explain why this code doesn't blow > > > up every time it gets called, in its current form. > > > > Well, it does :-) > > > > dev_get_drvdata(_dev) -> NULL -> kfree(NULL) > > > > We're just leaking memory. I guess a patch like below would be best: > > > > diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c > > index 3828c2ec8623..4dc04253da61 100644 > > --- a/drivers/usb/gadget/udc/net2280.c > > +++ b/drivers/usb/gadget/udc/net2280.c > > @@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void *_dev) > > > > /*-------------------------------------------------------------------------*/ > > > > -static void gadget_release(struct device *_dev) > > -{ > > - struct net2280 *dev = dev_get_drvdata(_dev); > > - > > - kfree(dev); > > -} > > - > > /* tear down the binding between this driver and the pci device */ > > > > static void net2280_remove(struct pci_dev *pdev) > > @@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev) > > device_remove_file(&pdev->dev, &dev_attr_registers); > > > > ep_info(dev, "unbind\n"); > > + > > + kfree(dev); > > } > > > > /* wrap this driver around the specified device, but > > @@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > if (retval) > > goto done; > > > > - retval = usb_add_gadget_udc_release(&pdev->dev, &dev->gadget, > > - gadget_release); > > + retval = usb_add_gadget_udc(&pdev->dev, &dev->gadget); > > if (retval) > > goto done; > > return 0; > > Maybe... But I can't shake the feeling that Greg KH would strongly > disagree. Hasn't he said, many times in the past, that any dynamically > allocated device structure _must_ have a real release routine? > usb_udc_nop_release() doesn't qualify. Aw, I wanted to publically yell at someone like the kernel documentation says I am allowed to do so if anyone does such a foolish thing :) ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device 2017-04-11 14:19 ` Greg KH @ 2017-04-12 6:01 ` Felipe Balbi 2017-04-12 6:45 ` Greg KH 2017-04-12 14:30 ` Alan Stern 0 siblings, 2 replies; 21+ messages in thread From: Felipe Balbi @ 2017-04-12 6:01 UTC (permalink / raw) To: Greg KH, Alan Stern Cc: Roger Quadros, vivek.gautam, USB list, Kernel development list [-- Attachment #1: Type: text/plain, Size: 2989 bytes --] Hi, Greg KH <greg@kroah.com> writes: > On Tue, Apr 11, 2017 at 10:12:01AM -0400, Alan Stern wrote: >> On Tue, 11 Apr 2017, Felipe Balbi wrote: >> >> > > Oddly enough, yes. But it doesn't explain why this code doesn't blow >> > > up every time it gets called, in its current form. >> > >> > Well, it does :-) >> > >> > dev_get_drvdata(_dev) -> NULL -> kfree(NULL) >> > >> > We're just leaking memory. I guess a patch like below would be best: >> > >> > diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c >> > index 3828c2ec8623..4dc04253da61 100644 >> > --- a/drivers/usb/gadget/udc/net2280.c >> > +++ b/drivers/usb/gadget/udc/net2280.c >> > @@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void *_dev) >> > >> > /*-------------------------------------------------------------------------*/ >> > >> > -static void gadget_release(struct device *_dev) >> > -{ >> > - struct net2280 *dev = dev_get_drvdata(_dev); >> > - >> > - kfree(dev); >> > -} >> > - >> > /* tear down the binding between this driver and the pci device */ >> > >> > static void net2280_remove(struct pci_dev *pdev) >> > @@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev) >> > device_remove_file(&pdev->dev, &dev_attr_registers); >> > >> > ep_info(dev, "unbind\n"); >> > + >> > + kfree(dev); >> > } >> > >> > /* wrap this driver around the specified device, but >> > @@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id) >> > if (retval) >> > goto done; >> > >> > - retval = usb_add_gadget_udc_release(&pdev->dev, &dev->gadget, >> > - gadget_release); >> > + retval = usb_add_gadget_udc(&pdev->dev, &dev->gadget); >> > if (retval) >> > goto done; >> > return 0; >> >> Maybe... But I can't shake the feeling that Greg KH would strongly >> disagree. Hasn't he said, many times in the past, that any dynamically >> allocated device structure _must_ have a real release routine? >> usb_udc_nop_release() doesn't qualify. > > Aw, I wanted to publically yell at someone like the kernel documentation > says I am allowed to do so if anyone does such a foolish thing :) heh, except that we're not dynamically allocating struct device at all :-) Here's what we have for most UDCs (net2280.c included): struct my_udc { struct gadget gadget; [...] }; probe() { struct my_udc *u; u = kzalloc(sizeof(*u), GFP_KERNEL); [...] return 0; } Now, if this kzalloc() would be replaced with devm_kzalloc() wouldn't this result on a functionally equivalent execution to the patch I proposed above? Iff we change struct gadget to contain a struct device *dev instead of a struct device dev, then sure, we will need to cope with proper ->release() implementations. As it is, it brings nothing to the table, IMO. -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device 2017-04-12 6:01 ` Felipe Balbi @ 2017-04-12 6:45 ` Greg KH 2017-04-12 7:33 ` Felipe Balbi 2017-04-12 14:30 ` Alan Stern 1 sibling, 1 reply; 21+ messages in thread From: Greg KH @ 2017-04-12 6:45 UTC (permalink / raw) To: Felipe Balbi Cc: Alan Stern, Roger Quadros, vivek.gautam, USB list, Kernel development list On Wed, Apr 12, 2017 at 09:01:44AM +0300, Felipe Balbi wrote: > > Hi, > > Greg KH <greg@kroah.com> writes: > > On Tue, Apr 11, 2017 at 10:12:01AM -0400, Alan Stern wrote: > >> On Tue, 11 Apr 2017, Felipe Balbi wrote: > >> > >> > > Oddly enough, yes. But it doesn't explain why this code doesn't blow > >> > > up every time it gets called, in its current form. > >> > > >> > Well, it does :-) > >> > > >> > dev_get_drvdata(_dev) -> NULL -> kfree(NULL) > >> > > >> > We're just leaking memory. I guess a patch like below would be best: > >> > > >> > diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c > >> > index 3828c2ec8623..4dc04253da61 100644 > >> > --- a/drivers/usb/gadget/udc/net2280.c > >> > +++ b/drivers/usb/gadget/udc/net2280.c > >> > @@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void *_dev) > >> > > >> > /*-------------------------------------------------------------------------*/ > >> > > >> > -static void gadget_release(struct device *_dev) > >> > -{ > >> > - struct net2280 *dev = dev_get_drvdata(_dev); > >> > - > >> > - kfree(dev); > >> > -} > >> > - > >> > /* tear down the binding between this driver and the pci device */ > >> > > >> > static void net2280_remove(struct pci_dev *pdev) > >> > @@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev) > >> > device_remove_file(&pdev->dev, &dev_attr_registers); > >> > > >> > ep_info(dev, "unbind\n"); > >> > + > >> > + kfree(dev); > >> > } > >> > > >> > /* wrap this driver around the specified device, but > >> > @@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id) > >> > if (retval) > >> > goto done; > >> > > >> > - retval = usb_add_gadget_udc_release(&pdev->dev, &dev->gadget, > >> > - gadget_release); > >> > + retval = usb_add_gadget_udc(&pdev->dev, &dev->gadget); > >> > if (retval) > >> > goto done; > >> > return 0; > >> > >> Maybe... But I can't shake the feeling that Greg KH would strongly > >> disagree. Hasn't he said, many times in the past, that any dynamically > >> allocated device structure _must_ have a real release routine? > >> usb_udc_nop_release() doesn't qualify. > > > > Aw, I wanted to publically yell at someone like the kernel documentation > > says I am allowed to do so if anyone does such a foolish thing :) > > heh, except that we're not dynamically allocating struct device at all > :-) Please don't say that, that's even worse :( > Here's what we have for most UDCs (net2280.c included): > > struct my_udc { > struct gadget gadget; > [...] > }; > > probe() > { > struct my_udc *u; > > u = kzalloc(sizeof(*u), GFP_KERNEL); > [...] > return 0; > } > > Now, if this kzalloc() would be replaced with devm_kzalloc() wouldn't > this result on a functionally equivalent execution to the patch I > proposed above? > > Iff we change struct gadget to contain a struct device *dev instead of a > struct device dev, then sure, we will need to cope with proper > ->release() implementations. > > As it is, it brings nothing to the table, IMO. You always have to have a release function for a kobject, no matter where it is, as it is being reference counted. To not do so, is a huge indication of a problem in the design. thanks, greg k-h ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device 2017-04-12 6:45 ` Greg KH @ 2017-04-12 7:33 ` Felipe Balbi 0 siblings, 0 replies; 21+ messages in thread From: Felipe Balbi @ 2017-04-12 7:33 UTC (permalink / raw) To: Greg KH Cc: Alan Stern, Roger Quadros, vivek.gautam, USB list, Kernel development list [-- Attachment #1: Type: text/plain, Size: 1156 bytes --] Hi, Greg KH <greg@kroah.com> writes: >> Here's what we have for most UDCs (net2280.c included): >> >> struct my_udc { >> struct gadget gadget; >> [...] >> }; >> >> probe() >> { >> struct my_udc *u; >> >> u = kzalloc(sizeof(*u), GFP_KERNEL); >> [...] >> return 0; >> } >> >> Now, if this kzalloc() would be replaced with devm_kzalloc() wouldn't >> this result on a functionally equivalent execution to the patch I >> proposed above? >> >> Iff we change struct gadget to contain a struct device *dev instead of a >> struct device dev, then sure, we will need to cope with proper >> ->release() implementations. >> >> As it is, it brings nothing to the table, IMO. > > You always have to have a release function for a kobject, no matter > where it is, as it is being reference counted. To not do so, is a huge > indication of a problem in the design. okay, this goes all the way back to when Dave B wrote the API, it has always had empty ->release() functions (not all UDCs, though). I'll make sure to review all of this for v4.13. -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device 2017-04-12 6:01 ` Felipe Balbi 2017-04-12 6:45 ` Greg KH @ 2017-04-12 14:30 ` Alan Stern 1 sibling, 0 replies; 21+ messages in thread From: Alan Stern @ 2017-04-12 14:30 UTC (permalink / raw) To: Felipe Balbi Cc: Greg KH, Roger Quadros, vivek.gautam, USB list, Kernel development list On Wed, 12 Apr 2017, Felipe Balbi wrote: > >> Maybe... But I can't shake the feeling that Greg KH would strongly > >> disagree. Hasn't he said, many times in the past, that any dynamically > >> allocated device structure _must_ have a real release routine? > >> usb_udc_nop_release() doesn't qualify. > > > > Aw, I wanted to publically yell at someone like the kernel documentation > > says I am allowed to do so if anyone does such a foolish thing :) > > heh, except that we're not dynamically allocating struct device at all > :-) Here's what we have for most UDCs (net2280.c included): > > struct my_udc { > struct gadget gadget; > [...] > }; > > probe() > { > struct my_udc *u; > > u = kzalloc(sizeof(*u), GFP_KERNEL); > [...] > return 0; > } Allow me to point out that the struct device is embedded inside the struct gadget (actually struct usb_gadget) embedded inside the struct my_udc, which _is_ dynamically allocated. Therefore the struct device is located in dynamically allocated memory. > Now, if this kzalloc() would be replaced with devm_kzalloc() wouldn't > this result on a functionally equivalent execution to the patch I > proposed above? It would, and it would be equally wrong. Alan Stern ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 2/3] usb: dwc3: make role-switching work with debugfs/mode 2017-04-03 12:20 [PATCH v3 0/3] usb: dwc3: dual-role support Roger Quadros 2017-04-03 12:20 ` [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device Roger Quadros @ 2017-04-03 12:20 ` Roger Quadros 2017-04-03 12:20 ` [PATCH v3 3/3] usb: dwc3: Add dual-role support Roger Quadros 2 siblings, 0 replies; 21+ messages in thread From: Roger Quadros @ 2017-04-03 12:20 UTC (permalink / raw) To: balbi; +Cc: vivek.gautam, linux-usb, linux-kernel, Roger Quadros If dr_mode == "otg", we start by default in PERIPHERAL mode. Keep track of current role in "current_dr_role" whenever dwc3_set_mode() is called. When debugfs/mode is changed AND we're in dual-role mode, handle the switch by stopping and starting the respective host/gadget controllers. Signed-off-by: Roger Quadros <rogerq@ti.com> --- drivers/usb/dwc3/core.c | 21 +++++++++++--------- drivers/usb/dwc3/core.h | 3 +++ drivers/usb/dwc3/debugfs.c | 49 +++++++++++++++++++++++++++++++++++++++------- 3 files changed, 57 insertions(+), 16 deletions(-) diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 369bab1..bf917c2 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -108,6 +108,8 @@ void dwc3_set_mode(struct dwc3 *dwc, u32 mode) reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG)); reg |= DWC3_GCTL_PRTCAPDIR(mode); dwc3_writel(dwc->regs, DWC3_GCTL, reg); + + dwc->current_dr_role = mode; } u32 dwc3_core_fifo_space(struct dwc3_ep *dep, u8 type) @@ -277,7 +279,7 @@ static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned length) * * Returns 0 on success otherwise negative errno. */ -static int dwc3_event_buffers_setup(struct dwc3 *dwc) +int dwc3_event_buffers_setup(struct dwc3 *dwc) { struct dwc3_event_buffer *evt; @@ -862,13 +864,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc) } break; case USB_DR_MODE_OTG: - ret = dwc3_host_init(dwc); - if (ret) { - if (ret != -EPROBE_DEFER) - dev_err(dev, "failed to initialize host\n"); - return ret; - } - + /* start in peripheral role by default */ + dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE); ret = dwc3_gadget_init(dwc); if (ret) { if (ret != -EPROBE_DEFER) @@ -894,7 +891,13 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc) dwc3_host_exit(dwc); break; case USB_DR_MODE_OTG: - dwc3_host_exit(dwc); + /* role might have changed since start */ + if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) { + dwc3_host_exit(dwc); + /* Add back UDC to match dwc3_gadget_exit() */ + usb_add_gadget_udc(dwc->dev, &dwc->gadget); + } + dwc3_gadget_exit(dwc); break; default: diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 7ffdee5..adb04af 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -785,6 +785,7 @@ struct dwc3_scratchpad_array { * @maximum_speed: maximum speed requested (mainly for testing purposes) * @revision: revision register contents * @dr_mode: requested mode of operation + * @current_dr_role: current role of operation when in dual-role mode * @hsphy_mode: UTMI phy mode, one of following: * - USBPHY_INTERFACE_MODE_UTMI * - USBPHY_INTERFACE_MODE_UTMIW @@ -901,6 +902,7 @@ struct dwc3 { size_t regs_size; enum usb_dr_mode dr_mode; + u32 current_dr_role; enum usb_phy_interface hsphy_mode; u32 fladj; @@ -1167,6 +1169,7 @@ struct dwc3_gadget_ep_cmd_params { /* prototypes */ void dwc3_set_mode(struct dwc3 *dwc, u32 mode); u32 dwc3_core_fifo_space(struct dwc3_ep *dep, u8 type); +int dwc3_event_buffers_setup(struct dwc3 *dwc); /* check whether we are on the DWC_usb3 core */ static inline bool dwc3_is_usb3(struct dwc3 *dwc) diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c index 31926dd..a74a83b 100644 --- a/drivers/usb/dwc3/debugfs.c +++ b/drivers/usb/dwc3/debugfs.c @@ -327,19 +327,54 @@ static ssize_t dwc3_mode_write(struct file *file, return -EFAULT; if (!strncmp(buf, "host", 4)) - mode |= DWC3_GCTL_PRTCAP_HOST; + mode = DWC3_GCTL_PRTCAP_HOST; if (!strncmp(buf, "device", 6)) - mode |= DWC3_GCTL_PRTCAP_DEVICE; + mode = DWC3_GCTL_PRTCAP_DEVICE; if (!strncmp(buf, "otg", 3)) - mode |= DWC3_GCTL_PRTCAP_OTG; + mode = DWC3_GCTL_PRTCAP_OTG; - if (mode) { - spin_lock_irqsave(&dwc->lock, flags); - dwc3_set_mode(dwc, mode); - spin_unlock_irqrestore(&dwc->lock, flags); + if (!mode) + return -EINVAL; + + if (mode == dwc->current_dr_role) + goto exit; + + /* prevent role switching if we're not dual-role */ + if (dwc->dr_mode != USB_DR_MODE_OTG) + return -EINVAL; + + /* stop old role */ + switch (dwc->current_dr_role) { + case DWC3_GCTL_PRTCAP_HOST: + dwc3_host_exit(dwc); + break; + case DWC3_GCTL_PRTCAP_DEVICE: + usb_del_gadget_udc(&dwc->gadget); + break; + default: + break; + } + + /* switch PRTCAP mode. updates current_dr_role */ + spin_lock_irqsave(&dwc->lock, flags); + dwc3_set_mode(dwc, mode); + spin_unlock_irqrestore(&dwc->lock, flags); + + /* start new role */ + switch (dwc->current_dr_role) { + case DWC3_GCTL_PRTCAP_HOST: + dwc3_host_init(dwc); + break; + case DWC3_GCTL_PRTCAP_DEVICE: + dwc3_event_buffers_setup(dwc); + usb_add_gadget_udc(dwc->dev, &dwc->gadget); + break; + default: + break; } +exit: return count; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 3/3] usb: dwc3: Add dual-role support 2017-04-03 12:20 [PATCH v3 0/3] usb: dwc3: dual-role support Roger Quadros 2017-04-03 12:20 ` [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device Roger Quadros 2017-04-03 12:20 ` [PATCH v3 2/3] usb: dwc3: make role-switching work with debugfs/mode Roger Quadros @ 2017-04-03 12:20 ` Roger Quadros 2017-04-03 19:21 ` kbuild test robot 2017-04-04 7:46 ` [PATCH v4 " Roger Quadros 2 siblings, 2 replies; 21+ messages in thread From: Roger Quadros @ 2017-04-03 12:20 UTC (permalink / raw) To: balbi; +Cc: vivek.gautam, linux-usb, linux-kernel, Roger Quadros If dr_mode is "otg" then support dual role mode of operation. Currently this mode is only supported when an extcon handle is present in the dwc3 device tree node. This is needed to get the ID status events of the port. We're using a workqueue to manage the dual-role state transitions as the extcon notifier (dwc3_drd_notifier) is called in an atomic context by extcon_sync() and this doesn't go well with usb_del_gadget_udc() causing a lockdep and softirq warning. Signed-off-by: Roger Quadros <rogerq@ti.com> --- drivers/usb/dwc3/Makefile | 4 ++ drivers/usb/dwc3/core.c | 15 +---- drivers/usb/dwc3/core.h | 19 ++++++ drivers/usb/dwc3/drd.c | 167 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 193 insertions(+), 12 deletions(-) create mode 100644 drivers/usb/dwc3/drd.c diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile index ffca340..f15fabb 100644 --- a/drivers/usb/dwc3/Makefile +++ b/drivers/usb/dwc3/Makefile @@ -17,6 +17,10 @@ ifneq ($(filter y,$(CONFIG_USB_DWC3_GADGET) $(CONFIG_USB_DWC3_DUAL_ROLE)),) dwc3-y += gadget.o ep0.o endif +ifneq ($(CONFIG_USB_DWC3_DUAL_ROLE),) + dwc3-y += drd.o +endif + ifneq ($(CONFIG_USB_DWC3_ULPI),) dwc3-y += ulpi.o endif diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index bf917c2..15c05a1 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -864,12 +864,10 @@ static int dwc3_core_init_mode(struct dwc3 *dwc) } break; case USB_DR_MODE_OTG: - /* start in peripheral role by default */ - dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE); - ret = dwc3_gadget_init(dwc); + ret = dwc3_drd_init(dwc); if (ret) { if (ret != -EPROBE_DEFER) - dev_err(dev, "failed to initialize gadget\n"); + dev_err(dev, "failed to initialize dual-role\n"); return ret; } break; @@ -891,14 +889,7 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc) dwc3_host_exit(dwc); break; case USB_DR_MODE_OTG: - /* role might have changed since start */ - if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) { - dwc3_host_exit(dwc); - /* Add back UDC to match dwc3_gadget_exit() */ - usb_add_gadget_udc(dwc->dev, &dwc->gadget); - } - - dwc3_gadget_exit(dwc); + dwc3_drd_exit(dwc); break; default: /* do nothing */ diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index adb04af..8402d8f 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -786,6 +786,10 @@ struct dwc3_scratchpad_array { * @revision: revision register contents * @dr_mode: requested mode of operation * @current_dr_role: current role of operation when in dual-role mode + * @edev: extcon handle + * @edev_nb: extcon notifier + * @drd_work: dual-role work + * @drd_prevent_change: flag to prevent dual-role state change * @hsphy_mode: UTMI phy mode, one of following: * - USBPHY_INTERFACE_MODE_UTMI * - USBPHY_INTERFACE_MODE_UTMIW @@ -903,6 +907,11 @@ struct dwc3 { enum usb_dr_mode dr_mode; u32 current_dr_role; + struct extcon_dev *edev; + struct notifier_block edev_nb; + bool drd_prevent_change; + struct work_struct drd_work; + enum usb_phy_interface hsphy_mode; u32 fladj; @@ -1225,6 +1234,16 @@ static inline int dwc3_send_gadget_generic_command(struct dwc3 *dwc, { return 0; } #endif +#if IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE) +int dwc3_drd_init(struct dwc3 *dwc); +void dwc3_drd_exit(struct dwc3 *dwc); +#else +static inline int dwc3_drd_init(struct dwc3 *dwc) +{ return 0; } +static void dwc3_drd_exit(struct dwc3 *dwc); +{ } +#endif + /* power management interface */ #if !IS_ENABLED(CONFIG_USB_DWC3_HOST) int dwc3_gadget_suspend(struct dwc3 *dwc); diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c new file mode 100644 index 0000000..c9b02a3 --- /dev/null +++ b/drivers/usb/dwc3/drd.c @@ -0,0 +1,167 @@ +/** + * drd.c - DesignWare USB3 DRD Controller Dual-role support + * + * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com + * + * Authors: Roger Quadros <rogerq@ti.com> + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 of + * the License as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#include <linux/extcon.h> + +#include "debug.h" +#include "core.h" +#include "gadget.h" + +static void dwc3_drd_update(struct dwc3 *dwc) +{ + int id; + int new_role; + unsigned long flags; + + if (dwc->drd_prevent_change) + return; + + id = extcon_get_state(dwc->edev, EXTCON_USB_HOST); + /* Host means ID is 0 */ + id = !id; + + if (!id) + new_role = DWC3_GCTL_PRTCAP_HOST; + else + new_role = DWC3_GCTL_PRTCAP_DEVICE; + + if (dwc->current_dr_role == new_role) + return; + + /* stop old role */ + switch (dwc->current_dr_role) { + case DWC3_GCTL_PRTCAP_HOST: + dwc3_host_exit(dwc); + break; + case DWC3_GCTL_PRTCAP_DEVICE: + usb_del_gadget_udc(&dwc->gadget); + break; + default: + break; + } + + /* switch PRTCAP mode. updates current_dr_role */ + spin_lock_irqsave(&dwc->lock, flags); + dwc3_set_mode(dwc, new_role); + spin_unlock_irqrestore(&dwc->lock, flags); + + /* start new role */ + switch (dwc->current_dr_role) { + case DWC3_GCTL_PRTCAP_HOST: + dwc3_host_init(dwc); + break; + case DWC3_GCTL_PRTCAP_DEVICE: + dwc3_event_buffers_setup(dwc); + usb_add_gadget_udc(dwc->dev, &dwc->gadget); + break; + default: + break; + } +} + +static void dwc3_drd_work(struct work_struct *work) +{ + struct dwc3 *dwc = container_of(work, struct dwc3, + drd_work); + dwc3_drd_update(dwc); +} + +static int dwc3_drd_notifier(struct notifier_block *nb, + unsigned long event, void *ptr) +{ + struct dwc3 *dwc = container_of(nb, struct dwc3, edev_nb); + + queue_work(system_power_efficient_wq, &dwc->drd_work); + + return NOTIFY_DONE; +} + +int dwc3_drd_init(struct dwc3 *dwc) +{ + int ret; + int id; + struct device *dev = dwc->dev; + + INIT_WORK(&dwc->drd_work, dwc3_drd_work); + + if (dev->of_node) { + if (of_property_read_bool(dev->of_node, "extcon")) + dwc->edev = extcon_get_edev_by_phandle(dev, 0); + + if (IS_ERR(dwc->edev)) + return PTR_ERR(dwc->edev); + } else { + return -ENODEV; + } + + dwc->edev_nb.notifier_call = dwc3_drd_notifier; + ret = extcon_register_notifier(dwc->edev, EXTCON_USB_HOST, + &dwc->edev_nb); + if (ret < 0) { + dev_err(dwc->dev, "Couldn't register USB-HOST cable notifier\n"); + return -ENODEV; + } + + /* sanity check id & vbus states */ + id = extcon_get_state(dwc->edev, EXTCON_USB_HOST); + if (id < 0) { + dev_err(dwc->dev, "Invalid USB cable state. ID %d", id); + ret = -ENODEV; + goto fail; + } + + /* start in peripheral role by default */ + dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE); + ret = dwc3_gadget_init(dwc); + if (ret) + goto fail; + + /* check & update drd state */ + dwc3_drd_update(dwc); + + return 0; + +fail: + extcon_unregister_notifier(dwc->edev, EXTCON_USB_HOST, + &dwc->edev_nb); + + return ret; +} + +void dwc3_drd_exit(struct dwc3 *dwc) +{ + unsigned long flags; + + spin_lock_irqsave(&dwc->lock, flags); + dwc->drd_prevent_change = true; + spin_unlock_irqrestore(&dwc->lock, flags); + + extcon_unregister_notifier(dwc->edev, EXTCON_USB_HOST, + &dwc->edev_nb); + + /* role might have changed since start, stop active controller */ + if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) { + dwc3_host_exit(dwc); + /* Add back UDC to match dwc3_gadget_exit() */ + usb_add_gadget_udc(dwc->dev, &dwc->gadget); + } + + dwc3_gadget_exit(dwc); +} -- 2.7.4 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v3 3/3] usb: dwc3: Add dual-role support 2017-04-03 12:20 ` [PATCH v3 3/3] usb: dwc3: Add dual-role support Roger Quadros @ 2017-04-03 19:21 ` kbuild test robot 2017-04-04 7:42 ` Roger Quadros 2017-04-04 7:46 ` [PATCH v4 " Roger Quadros 1 sibling, 1 reply; 21+ messages in thread From: kbuild test robot @ 2017-04-03 19:21 UTC (permalink / raw) To: Roger Quadros Cc: kbuild-all, balbi, vivek.gautam, linux-usb, linux-kernel, Roger Quadros [-- Attachment #1: Type: text/plain, Size: 2018 bytes --] Hi Roger, [auto build test ERROR on balbi-usb/next] [also build test ERROR on v4.11-rc5 next-20170403] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Roger-Quadros/usb-dwc3-dual-role-support/20170404-022401 base: https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next config: i386-randconfig-x010-201714 (attached as .config) compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce: # save the attached .config to linux build tree make ARCH=i386 All error/warnings (new ones prefixed by >>): In file included from drivers/usb/dwc3/core.c:44:0: >> drivers/usb/dwc3/core.h:1243:1: error: expected identifier or '(' before '{' token { } ^ >> drivers/usb/dwc3/core.h:1242:13: warning: 'dwc3_drd_exit' used but never defined static void dwc3_drd_exit(struct dwc3 *dwc); ^~~~~~~~~~~~~ -- In file included from drivers/usb/dwc3/trace.h:27:0, from drivers/usb/dwc3/trace.c:19: >> drivers/usb/dwc3/core.h:1243:1: error: expected identifier or '(' before '{' token { } ^ In file included from drivers/usb/dwc3/trace.h:27:0, from drivers/usb/dwc3/trace.c:19: drivers/usb/dwc3/core.h:1242:13: warning: 'dwc3_drd_exit' declared 'static' but never defined [-Wunused-function] static void dwc3_drd_exit(struct dwc3 *dwc); ^~~~~~~~~~~~~ vim +1243 drivers/usb/dwc3/core.h 1236 #if IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE) 1237 int dwc3_drd_init(struct dwc3 *dwc); 1238 void dwc3_drd_exit(struct dwc3 *dwc); 1239 #else 1240 static inline int dwc3_drd_init(struct dwc3 *dwc) 1241 { return 0; } > 1242 static void dwc3_drd_exit(struct dwc3 *dwc); > 1243 { } 1244 #endif 1245 1246 /* power management interface */ --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 27087 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 3/3] usb: dwc3: Add dual-role support 2017-04-03 19:21 ` kbuild test robot @ 2017-04-04 7:42 ` Roger Quadros 0 siblings, 0 replies; 21+ messages in thread From: Roger Quadros @ 2017-04-04 7:42 UTC (permalink / raw) To: kbuild test robot, balbi Cc: kbuild-all, vivek.gautam, linux-usb, linux-kernel Felipe, I'll send a revised patch to fix this. cheers, -roger On 03/04/17 22:21, kbuild test robot wrote: > Hi Roger, > > [auto build test ERROR on balbi-usb/next] > [also build test ERROR on v4.11-rc5 next-20170403] > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] > > url: https://github.com/0day-ci/linux/commits/Roger-Quadros/usb-dwc3-dual-role-support/20170404-022401 > base: https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next > config: i386-randconfig-x010-201714 (attached as .config) > compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 > reproduce: > # save the attached .config to linux build tree > make ARCH=i386 > > All error/warnings (new ones prefixed by >>): > > In file included from drivers/usb/dwc3/core.c:44:0: >>> drivers/usb/dwc3/core.h:1243:1: error: expected identifier or '(' before '{' token > { } > ^ >>> drivers/usb/dwc3/core.h:1242:13: warning: 'dwc3_drd_exit' used but never defined > static void dwc3_drd_exit(struct dwc3 *dwc); > ^~~~~~~~~~~~~ > -- > In file included from drivers/usb/dwc3/trace.h:27:0, > from drivers/usb/dwc3/trace.c:19: >>> drivers/usb/dwc3/core.h:1243:1: error: expected identifier or '(' before '{' token > { } > ^ > In file included from drivers/usb/dwc3/trace.h:27:0, > from drivers/usb/dwc3/trace.c:19: > drivers/usb/dwc3/core.h:1242:13: warning: 'dwc3_drd_exit' declared 'static' but never defined [-Wunused-function] > static void dwc3_drd_exit(struct dwc3 *dwc); > ^~~~~~~~~~~~~ > > vim +1243 drivers/usb/dwc3/core.h > > 1236 #if IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE) > 1237 int dwc3_drd_init(struct dwc3 *dwc); > 1238 void dwc3_drd_exit(struct dwc3 *dwc); > 1239 #else > 1240 static inline int dwc3_drd_init(struct dwc3 *dwc) > 1241 { return 0; } >> 1242 static void dwc3_drd_exit(struct dwc3 *dwc); >> 1243 { } > 1244 #endif > 1245 > 1246 /* power management interface */ > > --- > 0-DAY kernel test infrastructure Open Source Technology Center > https://lists.01.org/pipermail/kbuild-all Intel Corporation > ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v4 3/3] usb: dwc3: Add dual-role support 2017-04-03 12:20 ` [PATCH v3 3/3] usb: dwc3: Add dual-role support Roger Quadros 2017-04-03 19:21 ` kbuild test robot @ 2017-04-04 7:46 ` Roger Quadros 1 sibling, 0 replies; 21+ messages in thread From: Roger Quadros @ 2017-04-04 7:46 UTC (permalink / raw) To: balbi; +Cc: vivek.gautam, linux-usb, linux-kernel, rogerq If dr_mode is "otg" then support dual role mode of operation. Currently this mode is only supported when an extcon handle is present in the dwc3 device tree node. This is needed to get the ID status events of the port. We're using a workqueue to manage the dual-role state transitions as the extcon notifier (dwc3_drd_notifier) is called in an atomic context by extcon_sync() and this doesn't go well with usb_del_gadget_udc() causing a lockdep and softirq warning. Signed-off-by: Roger Quadros <rogerq@ti.com> --- v4: -fix build error if CONFIG_USB_DWC3_DUAL_ROLE is not set drivers/usb/dwc3/Makefile | 4 ++ drivers/usb/dwc3/core.c | 15 +---- drivers/usb/dwc3/core.h | 19 ++++++ drivers/usb/dwc3/drd.c | 167 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 193 insertions(+), 12 deletions(-) create mode 100644 drivers/usb/dwc3/drd.c diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile index ffca340..f15fabb 100644 --- a/drivers/usb/dwc3/Makefile +++ b/drivers/usb/dwc3/Makefile @@ -17,6 +17,10 @@ ifneq ($(filter y,$(CONFIG_USB_DWC3_GADGET) $(CONFIG_USB_DWC3_DUAL_ROLE)),) dwc3-y += gadget.o ep0.o endif +ifneq ($(CONFIG_USB_DWC3_DUAL_ROLE),) + dwc3-y += drd.o +endif + ifneq ($(CONFIG_USB_DWC3_ULPI),) dwc3-y += ulpi.o endif diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index bf917c2..15c05a1 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -864,12 +864,10 @@ static int dwc3_core_init_mode(struct dwc3 *dwc) } break; case USB_DR_MODE_OTG: - /* start in peripheral role by default */ - dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE); - ret = dwc3_gadget_init(dwc); + ret = dwc3_drd_init(dwc); if (ret) { if (ret != -EPROBE_DEFER) - dev_err(dev, "failed to initialize gadget\n"); + dev_err(dev, "failed to initialize dual-role\n"); return ret; } break; @@ -891,14 +889,7 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc) dwc3_host_exit(dwc); break; case USB_DR_MODE_OTG: - /* role might have changed since start */ - if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) { - dwc3_host_exit(dwc); - /* Add back UDC to match dwc3_gadget_exit() */ - usb_add_gadget_udc(dwc->dev, &dwc->gadget); - } - - dwc3_gadget_exit(dwc); + dwc3_drd_exit(dwc); break; default: /* do nothing */ diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index adb04af..b1be4a3 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -786,6 +786,10 @@ struct dwc3_scratchpad_array { * @revision: revision register contents * @dr_mode: requested mode of operation * @current_dr_role: current role of operation when in dual-role mode + * @edev: extcon handle + * @edev_nb: extcon notifier + * @drd_work: dual-role work + * @drd_prevent_change: flag to prevent dual-role state change * @hsphy_mode: UTMI phy mode, one of following: * - USBPHY_INTERFACE_MODE_UTMI * - USBPHY_INTERFACE_MODE_UTMIW @@ -903,6 +907,11 @@ struct dwc3 { enum usb_dr_mode dr_mode; u32 current_dr_role; + struct extcon_dev *edev; + struct notifier_block edev_nb; + bool drd_prevent_change; + struct work_struct drd_work; + enum usb_phy_interface hsphy_mode; u32 fladj; @@ -1225,6 +1234,16 @@ static inline int dwc3_send_gadget_generic_command(struct dwc3 *dwc, { return 0; } #endif +#if IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE) +int dwc3_drd_init(struct dwc3 *dwc); +void dwc3_drd_exit(struct dwc3 *dwc); +#else +static inline int dwc3_drd_init(struct dwc3 *dwc) +{ return 0; } +static inline void dwc3_drd_exit(struct dwc3 *dwc) +{ } +#endif + /* power management interface */ #if !IS_ENABLED(CONFIG_USB_DWC3_HOST) int dwc3_gadget_suspend(struct dwc3 *dwc); diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c new file mode 100644 index 0000000..c9b02a3 --- /dev/null +++ b/drivers/usb/dwc3/drd.c @@ -0,0 +1,167 @@ +/** + * drd.c - DesignWare USB3 DRD Controller Dual-role support + * + * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com + * + * Authors: Roger Quadros <rogerq@ti.com> + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 of + * the License as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#include <linux/extcon.h> + +#include "debug.h" +#include "core.h" +#include "gadget.h" + +static void dwc3_drd_update(struct dwc3 *dwc) +{ + int id; + int new_role; + unsigned long flags; + + if (dwc->drd_prevent_change) + return; + + id = extcon_get_state(dwc->edev, EXTCON_USB_HOST); + /* Host means ID is 0 */ + id = !id; + + if (!id) + new_role = DWC3_GCTL_PRTCAP_HOST; + else + new_role = DWC3_GCTL_PRTCAP_DEVICE; + + if (dwc->current_dr_role == new_role) + return; + + /* stop old role */ + switch (dwc->current_dr_role) { + case DWC3_GCTL_PRTCAP_HOST: + dwc3_host_exit(dwc); + break; + case DWC3_GCTL_PRTCAP_DEVICE: + usb_del_gadget_udc(&dwc->gadget); + break; + default: + break; + } + + /* switch PRTCAP mode. updates current_dr_role */ + spin_lock_irqsave(&dwc->lock, flags); + dwc3_set_mode(dwc, new_role); + spin_unlock_irqrestore(&dwc->lock, flags); + + /* start new role */ + switch (dwc->current_dr_role) { + case DWC3_GCTL_PRTCAP_HOST: + dwc3_host_init(dwc); + break; + case DWC3_GCTL_PRTCAP_DEVICE: + dwc3_event_buffers_setup(dwc); + usb_add_gadget_udc(dwc->dev, &dwc->gadget); + break; + default: + break; + } +} + +static void dwc3_drd_work(struct work_struct *work) +{ + struct dwc3 *dwc = container_of(work, struct dwc3, + drd_work); + dwc3_drd_update(dwc); +} + +static int dwc3_drd_notifier(struct notifier_block *nb, + unsigned long event, void *ptr) +{ + struct dwc3 *dwc = container_of(nb, struct dwc3, edev_nb); + + queue_work(system_power_efficient_wq, &dwc->drd_work); + + return NOTIFY_DONE; +} + +int dwc3_drd_init(struct dwc3 *dwc) +{ + int ret; + int id; + struct device *dev = dwc->dev; + + INIT_WORK(&dwc->drd_work, dwc3_drd_work); + + if (dev->of_node) { + if (of_property_read_bool(dev->of_node, "extcon")) + dwc->edev = extcon_get_edev_by_phandle(dev, 0); + + if (IS_ERR(dwc->edev)) + return PTR_ERR(dwc->edev); + } else { + return -ENODEV; + } + + dwc->edev_nb.notifier_call = dwc3_drd_notifier; + ret = extcon_register_notifier(dwc->edev, EXTCON_USB_HOST, + &dwc->edev_nb); + if (ret < 0) { + dev_err(dwc->dev, "Couldn't register USB-HOST cable notifier\n"); + return -ENODEV; + } + + /* sanity check id & vbus states */ + id = extcon_get_state(dwc->edev, EXTCON_USB_HOST); + if (id < 0) { + dev_err(dwc->dev, "Invalid USB cable state. ID %d", id); + ret = -ENODEV; + goto fail; + } + + /* start in peripheral role by default */ + dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE); + ret = dwc3_gadget_init(dwc); + if (ret) + goto fail; + + /* check & update drd state */ + dwc3_drd_update(dwc); + + return 0; + +fail: + extcon_unregister_notifier(dwc->edev, EXTCON_USB_HOST, + &dwc->edev_nb); + + return ret; +} + +void dwc3_drd_exit(struct dwc3 *dwc) +{ + unsigned long flags; + + spin_lock_irqsave(&dwc->lock, flags); + dwc->drd_prevent_change = true; + spin_unlock_irqrestore(&dwc->lock, flags); + + extcon_unregister_notifier(dwc->edev, EXTCON_USB_HOST, + &dwc->edev_nb); + + /* role might have changed since start, stop active controller */ + if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) { + dwc3_host_exit(dwc); + /* Add back UDC to match dwc3_gadget_exit() */ + usb_add_gadget_udc(dwc->dev, &dwc->gadget); + } + + dwc3_gadget_exit(dwc); +} -- 2.7.4 ^ permalink raw reply related [flat|nested] 21+ messages in thread
end of thread, other threads:[~2017-04-12 14:30 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-03 12:20 [PATCH v3 0/3] usb: dwc3: dual-role support Roger Quadros 2017-04-03 12:20 ` [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device Roger Quadros 2017-04-03 14:19 ` Alan Stern 2017-04-04 7:47 ` Felipe Balbi 2017-04-04 14:17 ` Alan Stern 2017-04-05 8:34 ` Felipe Balbi 2017-04-05 14:12 ` Alan Stern 2017-04-10 10:05 ` Felipe Balbi 2017-04-10 15:17 ` Alan Stern 2017-04-11 7:34 ` Felipe Balbi 2017-04-11 14:12 ` Alan Stern 2017-04-11 14:19 ` Greg KH 2017-04-12 6:01 ` Felipe Balbi 2017-04-12 6:45 ` Greg KH 2017-04-12 7:33 ` Felipe Balbi 2017-04-12 14:30 ` Alan Stern 2017-04-03 12:20 ` [PATCH v3 2/3] usb: dwc3: make role-switching work with debugfs/mode Roger Quadros 2017-04-03 12:20 ` [PATCH v3 3/3] usb: dwc3: Add dual-role support Roger Quadros 2017-04-03 19:21 ` kbuild test robot 2017-04-04 7:42 ` Roger Quadros 2017-04-04 7:46 ` [PATCH v4 " Roger Quadros
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.