Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* Adding and removing the same gadget multiple times
@ 2020-07-28 19:32 Alan Stern
  2020-07-29  1:47 ` Peter Chen
  0 siblings, 1 reply; 5+ messages in thread
From: Alan Stern @ 2020-07-28 19:32 UTC (permalink / raw)
  To: Roger Quadros; +Cc: USB mailing list

Roger:

Your commit fac323471df6 ("usb: udc: allow adding and removing the same 
gadget device") from a few years ago just caught my eye.  (I don't 
recall whether I noticed it at the time.)

In any case, we need to talk about it.  What you're doing -- 
unregistering and re-registering the struct device embedded in the 
gadget structure -- is strictly forbidden by the kernel's device model. 
It's even mentioned specifically in the kerneldoc for device_add().

Now, I guess doing this would be okay _if_ you took care not to 
re-register the device until all references to the previous incarnation 
have been dropped.  In particular, setting the structure's memory to 0 
should not be done immediately after calling device_unregister() -- 
which is what the commit does -- but rather in the release routine.

Do you know which UDC drivers actually do re-register their gadgets?  In 
particular, do they have their own release routines or do they rely on 
the default usb_udc_nop_release() provided by the UDC core?  Moving the 
memset into that routine ought to be okay; leaving it where it is would 
be a time bomb waiting to go off.  I'm suprised it hasn't caused 
problems already.

Furthermore, drivers that do re-register their gadgets should wait until 
gadget->dev.release is NULL, indicating that the release routine has 
been called and the memory has been wiped.  If they re-register before 
that, the re-registration will fail for the same reasons you observed 
when you wrote the commit.

Of course, a cleaner alternative would be to allocate the gadget 
structure dynamically.  Then instead of re-registering the old one, the 
driver could allocate a new one and register it instead, with no 
concerns about reuse.

Alan Stern

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

* Re: Adding and removing the same gadget multiple times
  2020-07-28 19:32 Adding and removing the same gadget multiple times Alan Stern
@ 2020-07-29  1:47 ` Peter Chen
  2020-07-29 14:14   ` Alan Stern
  0 siblings, 1 reply; 5+ messages in thread
From: Peter Chen @ 2020-07-29  1:47 UTC (permalink / raw)
  To: Alan Stern; +Cc: Roger Quadros, USB mailing list

On 20-07-28 15:32:46, Alan Stern wrote:
> Roger:
> 
> Your commit fac323471df6 ("usb: udc: allow adding and removing the same 
> gadget device") from a few years ago just caught my eye.  (I don't 
> recall whether I noticed it at the time.)
> 
> In any case, we need to talk about it.  What you're doing -- 
> unregistering and re-registering the struct device embedded in the 
> gadget structure -- is strictly forbidden by the kernel's device model. 
> It's even mentioned specifically in the kerneldoc for device_add().
> 
> Now, I guess doing this would be okay _if_ you took care not to 
> re-register the device until all references to the previous incarnation 
> have been dropped.  In particular, setting the structure's memory to 0 
> should not be done immediately after calling device_unregister() -- 
> which is what the commit does -- but rather in the release routine.
> 
> Do you know which UDC drivers actually do re-register their gadgets?  In 
> particular, do they have their own release routines or do they rely on 
> the default usb_udc_nop_release() provided by the UDC core?

dwc3 and cdns3 gadget driver do that, they use default usb_udc_nop_release()
provided by the UDC core. The usb_add_gadget_udc is called when the
controller role switch to device mode (the host VBUS is seen at device
side), and usb_del_gadget_udc is called when the cable is disconnected
from host.

> Moving the 
> memset into that routine ought to be okay; leaving it where it is would 
> be a time bomb waiting to go off.  I'm suprised it hasn't caused 
> problems already.

I have not seen problem when do hot plug, maybe one second is enough to
free all resources for gadget device? Below is the log for one iteration
for plug in and out.

[  156.308659] cdns-usb3 5b130000.usb: Waiting till Device mode is turned on
[  156.308688] cdns-usb3 5b130000.usb: Initializing non-zero endpoints
[  156.308713] cdns-usb3 5b130000.usb: Initialized  ep0 support:  
[  156.308725] cdns-usb3 5b130000.usb: Initialized  ep1out support: BULK, INT ISO
[  156.308736] cdns-usb3 5b130000.usb: Initialized  ep2out support: BULK, INT ISO
[  156.308747] cdns-usb3 5b130000.usb: Initialized  ep3out support: BULK, INT ISO
[  156.308758] cdns-usb3 5b130000.usb: Initialized  ep4out support: BULK, INT ISO
[  156.308768] cdns-usb3 5b130000.usb: Initialized  ep5out support: BULK, INT ISO
[  156.308779] cdns-usb3 5b130000.usb: Initialized  ep6out support: BULK, INT ISO
[  156.308791] cdns-usb3 5b130000.usb: Initialized  ep7out support: BULK, INT ISO
[  156.308804] cdns-usb3 5b130000.usb: Initialized  ep1in support: BULK, INT ISO
[  156.308815] cdns-usb3 5b130000.usb: Initialized  ep2in support: BULK, INT ISO
[  156.308826] cdns-usb3 5b130000.usb: Initialized  ep3in support: BULK, INT ISO
[  156.308837] cdns-usb3 5b130000.usb: Initialized  ep4in support: BULK, INT ISO
[  156.308847] cdns-usb3 5b130000.usb: Initialized  ep5in support: BULK, INT ISO
[  156.308858] cdns-usb3 5b130000.usb: Initialized  ep6in support: BULK, INT ISO
[  156.308869] cdns-usb3 5b130000.usb: Initialized  ep7in support: BULK, INT ISO
[  156.308880] cdns-usb3 5b130000.usb: Device Controller version: 0302450c
[  156.308889] cdns-usb3 5b130000.usb: USB Capabilities:: 19203324
[  156.308897] cdns-usb3 5b130000.usb: On-Chip memory configuration: 00000b12
[  156.309120] udc 5b130000.usb: registering UDC driver [g1]
[  156.309150] configfs-gadget gadget: adding 'Mass Storage Function'/00000000d982ebdb to config 'c'/00000000160499b7
[  156.309170] cdns-usb3 5b130000.usb: match endpoint: ep1in
[  156.309180] cdns-usb3 5b130000.usb: match endpoint: ep1out
[  156.612127] cdns-usb3 5b130000.usb: Configure ep1out: with val 0c000114
[  156.612143] cdns-usb3 5b130000.usb: Configure ep1in: with val 0c000104
[  156.612155] configfs-gadget gadget: super-speed config #1: c
[  156.612186] configfs-gadget gadget: set_config: interface 0 (Mass Storage Function) requested delayed status
[  156.612191] configfs-gadget gadget: delayed_status count 1
[  156.612232] configfs-gadget gadget: usb_composite_setup_continue
[  156.612241] configfs-gadget gadget: usb_composite_setup_continue: Completing delayed status
[  156.645179] configfs-gadget gadget: reset config
[  156.645235] configfs-gadget gadget: bulk_out_complete --> -104, 0/31
[  156.645246] configfs-gadget gadget: reset interface
[  156.714984] configfs-gadget 5b130000.usb: unregistering UDC driver [g1]
[  156.715042] configfs-gadget gadget: unbind function 'Mass Storage Function'/00000000d982ebdb
[  156.715048] configfs-gadget gadget: unbind
[  156.715246] udc 5b130000.usb: releasing '5b130000.usb'
[  158.312637] cdns-usb3 5b130000.usb: Waiting till Device mode is turned on

> 
> Furthermore, drivers that do re-register their gadgets should wait until 
> gadget->dev.release is NULL, indicating that the release routine has 
> been called and the memory has been wiped.  If they re-register before 
> that, the re-registration will fail for the same reasons you observed 
> when you wrote the commit.
> 
> Of course, a cleaner alternative would be to allocate the gadget 
> structure dynamically.  Then instead of re-registering the old one, the 
> driver could allocate a new one and register it instead, with no 
> concerns about reuse.
> 
> Alan Stern

-- 

Thanks,
Peter Chen

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

* Re: Adding and removing the same gadget multiple times
  2020-07-29  1:47 ` Peter Chen
@ 2020-07-29 14:14   ` Alan Stern
  2020-07-29 14:19     ` Greg KH
  2020-07-30  2:37     ` Peter Chen
  0 siblings, 2 replies; 5+ messages in thread
From: Alan Stern @ 2020-07-29 14:14 UTC (permalink / raw)
  To: Peter Chen; +Cc: Roger Quadros, USB mailing list

On Wed, Jul 29, 2020 at 01:47:20AM +0000, Peter Chen wrote:
> On 20-07-28 15:32:46, Alan Stern wrote:
> > Roger:
> > 
> > Your commit fac323471df6 ("usb: udc: allow adding and removing the same 
> > gadget device") from a few years ago just caught my eye.  (I don't 
> > recall whether I noticed it at the time.)
> > 
> > In any case, we need to talk about it.  What you're doing -- 
> > unregistering and re-registering the struct device embedded in the 
> > gadget structure -- is strictly forbidden by the kernel's device model. 
> > It's even mentioned specifically in the kerneldoc for device_add().
> > 
> > Now, I guess doing this would be okay _if_ you took care not to 
> > re-register the device until all references to the previous incarnation 
> > have been dropped.  In particular, setting the structure's memory to 0 
> > should not be done immediately after calling device_unregister() -- 
> > which is what the commit does -- but rather in the release routine.
> > 
> > Do you know which UDC drivers actually do re-register their gadgets?  In 
> > particular, do they have their own release routines or do they rely on 
> > the default usb_udc_nop_release() provided by the UDC core?
> 
> dwc3 and cdns3 gadget driver do that, they use default usb_udc_nop_release()
> provided by the UDC core. The usb_add_gadget_udc is called when the
> controller role switch to device mode (the host VBUS is seen at device
> side), and usb_del_gadget_udc is called when the cable is disconnected
> from host.

What if the role switches back to host without the cable being
disconnected?

> > Moving the 
> > memset into that routine ought to be okay; leaving it where it is would 
> > be a time bomb waiting to go off.  I'm suprised it hasn't caused 
> > problems already.
> 
> I have not seen problem when do hot plug, maybe one second is enough to
> free all resources for gadget device?

Maybe.  I don't know what other parts of the kernel might take a
reference to the gadget's embedded struct device, but it certainly is
not safe to wipe the struct device memory until the last reference
has been dropped.  And it is not safe to re-register the gadget until
the memory has been wiped.

(It used to be that userspace could keep a reference to a device
indefinitely, just by holding open one of its sysfs attribute files.
That may not be true any more, but there may be other ways for
userspace to accomplish the same thing.)

Alan Stern

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

* Re: Adding and removing the same gadget multiple times
  2020-07-29 14:14   ` Alan Stern
@ 2020-07-29 14:19     ` Greg KH
  2020-07-30  2:37     ` Peter Chen
  1 sibling, 0 replies; 5+ messages in thread
From: Greg KH @ 2020-07-29 14:19 UTC (permalink / raw)
  To: Alan Stern; +Cc: Peter Chen, Roger Quadros, USB mailing list

On Wed, Jul 29, 2020 at 10:14:48AM -0400, Alan Stern wrote:
> On Wed, Jul 29, 2020 at 01:47:20AM +0000, Peter Chen wrote:
> > On 20-07-28 15:32:46, Alan Stern wrote:
> > > Roger:
> > > 
> > > Your commit fac323471df6 ("usb: udc: allow adding and removing the same 
> > > gadget device") from a few years ago just caught my eye.  (I don't 
> > > recall whether I noticed it at the time.)
> > > 
> > > In any case, we need to talk about it.  What you're doing -- 
> > > unregistering and re-registering the struct device embedded in the 
> > > gadget structure -- is strictly forbidden by the kernel's device model. 
> > > It's even mentioned specifically in the kerneldoc for device_add().
> > > 
> > > Now, I guess doing this would be okay _if_ you took care not to 
> > > re-register the device until all references to the previous incarnation 
> > > have been dropped.  In particular, setting the structure's memory to 0 
> > > should not be done immediately after calling device_unregister() -- 
> > > which is what the commit does -- but rather in the release routine.
> > > 
> > > Do you know which UDC drivers actually do re-register their gadgets?  In 
> > > particular, do they have their own release routines or do they rely on 
> > > the default usb_udc_nop_release() provided by the UDC core?
> > 
> > dwc3 and cdns3 gadget driver do that, they use default usb_udc_nop_release()
> > provided by the UDC core. The usb_add_gadget_udc is called when the
> > controller role switch to device mode (the host VBUS is seen at device
> > side), and usb_del_gadget_udc is called when the cable is disconnected
> > from host.
> 
> What if the role switches back to host without the cable being
> disconnected?
> 
> > > Moving the 
> > > memset into that routine ought to be okay; leaving it where it is would 
> > > be a time bomb waiting to go off.  I'm suprised it hasn't caused 
> > > problems already.
> > 
> > I have not seen problem when do hot plug, maybe one second is enough to
> > free all resources for gadget device?
> 
> Maybe.  I don't know what other parts of the kernel might take a
> reference to the gadget's embedded struct device, but it certainly is
> not safe to wipe the struct device memory until the last reference
> has been dropped.  And it is not safe to re-register the gadget until
> the memory has been wiped.
> 
> (It used to be that userspace could keep a reference to a device
> indefinitely, just by holding open one of its sysfs attribute files.
> That may not be true any more, but there may be other ways for
> userspace to accomplish the same thing.)

Yes, this is tricky, and as you point out, 'struct device' should never
be recycled.  This should be fixed up properly or there could be real
problems as you show.

thanks,

greg k-h

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

* Re: Adding and removing the same gadget multiple times
  2020-07-29 14:14   ` Alan Stern
  2020-07-29 14:19     ` Greg KH
@ 2020-07-30  2:37     ` Peter Chen
  1 sibling, 0 replies; 5+ messages in thread
From: Peter Chen @ 2020-07-30  2:37 UTC (permalink / raw)
  To: Alan Stern; +Cc: Roger Quadros, USB mailing list

On 20-07-29 10:14:48, Alan Stern wrote:
> On Wed, Jul 29, 2020 at 01:47:20AM +0000, Peter Chen wrote:
> > On 20-07-28 15:32:46, Alan Stern wrote:
> > > Roger:
> > > 
> > > Your commit fac323471df6 ("usb: udc: allow adding and removing the same 
> > > gadget device") from a few years ago just caught my eye.  (I don't 
> > > recall whether I noticed it at the time.)
> > > 
> > > In any case, we need to talk about it.  What you're doing -- 
> > > unregistering and re-registering the struct device embedded in the 
> > > gadget structure -- is strictly forbidden by the kernel's device model. 
> > > It's even mentioned specifically in the kerneldoc for device_add().
> > > 
> > > Now, I guess doing this would be okay _if_ you took care not to 
> > > re-register the device until all references to the previous incarnation 
> > > have been dropped.  In particular, setting the structure's memory to 0 
> > > should not be done immediately after calling device_unregister() -- 
> > > which is what the commit does -- but rather in the release routine.
> > > 
> > > Do you know which UDC drivers actually do re-register their gadgets?  In 
> > > particular, do they have their own release routines or do they rely on 
> > > the default usb_udc_nop_release() provided by the UDC core?
> > 
> > dwc3 and cdns3 gadget driver do that, they use default usb_udc_nop_release()
> > provided by the UDC core. The usb_add_gadget_udc is called when the
> > controller role switch to device mode (the host VBUS is seen at device
> > side), and usb_del_gadget_udc is called when the cable is disconnected
> > from host.
> 
> What if the role switches back to host without the cable being
> disconnected?

This kinds of role switch is through the sys entry directly, without
considering hardware signal. Taking cdns3 as an example for this use
case, usb_del_gadget_udc will be called for stopping device mode, and
create xhci platform device for starting host.

-- 

Thanks,
Peter Chen

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

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-28 19:32 Adding and removing the same gadget multiple times Alan Stern
2020-07-29  1:47 ` Peter Chen
2020-07-29 14:14   ` Alan Stern
2020-07-29 14:19     ` Greg KH
2020-07-30  2:37     ` Peter Chen

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git