| Message ID | a7c4112fcd4dc2f0169af94a24f5685ca77f09fd.1610395599.git.Thinh.Nguyen@synopsys.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 49d08cfc7830440517216b6d51bd0c38b5314c7f |
| Headers | show |
| Series | usb: udc: core: Introduce started state | expand |
On Mon, Jan 11, 2021 at 12:38:05PM -0800, Thinh Nguyen wrote: > For some UDCs, the initialization sequence by udc_start() should not be > repeated until it is properly cleaned up with udc_stop() and vise versa. > We may run into some cleanup failure as seen with the DWC3 driver during > the irq cleanup. This issue can occur when the user triggers > soft-connect/soft-disconnect from the soft_connect sysfs. To avoid > adding checks to every UDC driver, at the UDC framework, introduce a > "started" state to track and prevent the UDC from repeating the > udc_start() and udc_stop() if it had already started/stopped. > > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> Is the new "started" field subject to races? What happens if there are concurrent requests to start and stop the UDC? Alan Stern
Hi, Alan Stern wrote: > On Mon, Jan 11, 2021 at 12:38:05PM -0800, Thinh Nguyen wrote: >> For some UDCs, the initialization sequence by udc_start() should not be >> repeated until it is properly cleaned up with udc_stop() and vise versa. >> We may run into some cleanup failure as seen with the DWC3 driver during >> the irq cleanup. This issue can occur when the user triggers >> soft-connect/soft-disconnect from the soft_connect sysfs. To avoid >> adding checks to every UDC driver, at the UDC framework, introduce a >> "started" state to track and prevent the UDC from repeating the >> udc_start() and udc_stop() if it had already started/stopped. >> >> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > Is the new "started" field subject to races? What happens if there are > concurrent requests to start and stop the UDC? > > Alan Stern The caller of this usb_gadget_udc_start/stop() should take care of the locking. It's already done in the case of driver probe/remove, but not for the sysfs soft_connect. Maybe I should add that to this patch. Thanks, Thinh
Thinh Nguyen wrote: > Hi, > > Alan Stern wrote: >> On Mon, Jan 11, 2021 at 12:38:05PM -0800, Thinh Nguyen wrote: >>> For some UDCs, the initialization sequence by udc_start() should not be >>> repeated until it is properly cleaned up with udc_stop() and vise versa. >>> We may run into some cleanup failure as seen with the DWC3 driver during >>> the irq cleanup. This issue can occur when the user triggers >>> soft-connect/soft-disconnect from the soft_connect sysfs. To avoid >>> adding checks to every UDC driver, at the UDC framework, introduce a >>> "started" state to track and prevent the UDC from repeating the >>> udc_start() and udc_stop() if it had already started/stopped. >>> >>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >> Is the new "started" field subject to races? What happens if there are >> concurrent requests to start and stop the UDC? >> >> Alan Stern > The caller of this usb_gadget_udc_start/stop() should take care of the > locking. It's already done in the case of driver probe/remove, but not > for the sysfs soft_connect. Maybe I should add that to this patch. > Actually, I still think that the locking for soft_connect_store() should be done in a separate patch since that's needed there regardless whether we introduced this new field. Let me know if you have any concern. Thanks, Thinh
On 21-01-11 22:35:07, Thinh Nguyen wrote: > Thinh Nguyen wrote: > > Hi, > > > > Alan Stern wrote: > >> On Mon, Jan 11, 2021 at 12:38:05PM -0800, Thinh Nguyen wrote: > >>> For some UDCs, the initialization sequence by udc_start() should not be > >>> repeated until it is properly cleaned up with udc_stop() and vise versa. > >>> We may run into some cleanup failure as seen with the DWC3 driver during > >>> the irq cleanup. This issue can occur when the user triggers > >>> soft-connect/soft-disconnect from the soft_connect sysfs. To avoid > >>> adding checks to every UDC driver, at the UDC framework, introduce a > >>> "started" state to track and prevent the UDC from repeating the > >>> udc_start() and udc_stop() if it had already started/stopped. > >>> > >>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > >> Is the new "started" field subject to races? What happens if there are > >> concurrent requests to start and stop the UDC? > >> > >> Alan Stern > > The caller of this usb_gadget_udc_start/stop() should take care of the > > locking. It's already done in the case of driver probe/remove, but not > > for the sysfs soft_connect. Maybe I should add that to this patch. > > The udc_bind_to_driver (calls ->start) and usb_gadget_remove_driver(call ->stop) are protected by mutex udc_lock, so don't have concurrent issue for it. For soft_connect, I think you could add the same mutex.
Peter Chen wrote: > On 21-01-11 22:35:07, Thinh Nguyen wrote: >> Thinh Nguyen wrote: >>> Hi, >>> >>> Alan Stern wrote: >>>> On Mon, Jan 11, 2021 at 12:38:05PM -0800, Thinh Nguyen wrote: >>>>> For some UDCs, the initialization sequence by udc_start() should not be >>>>> repeated until it is properly cleaned up with udc_stop() and vise versa. >>>>> We may run into some cleanup failure as seen with the DWC3 driver during >>>>> the irq cleanup. This issue can occur when the user triggers >>>>> soft-connect/soft-disconnect from the soft_connect sysfs. To avoid >>>>> adding checks to every UDC driver, at the UDC framework, introduce a >>>>> "started" state to track and prevent the UDC from repeating the >>>>> udc_start() and udc_stop() if it had already started/stopped. >>>>> >>>>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >>>> Is the new "started" field subject to races? What happens if there are >>>> concurrent requests to start and stop the UDC? >>>> >>>> Alan Stern >>> The caller of this usb_gadget_udc_start/stop() should take care of the >>> locking. It's already done in the case of driver probe/remove, but not >>> for the sysfs soft_connect. Maybe I should add that to this patch. >>> > The udc_bind_to_driver (calls ->start) and usb_gadget_remove_driver(call > ->stop) are protected by mutex udc_lock, so don't have concurrent issue for > it. For soft_connect, I think you could add the same mutex. > Right. I was suggesting to do that in a separate patch :) Thanks, Thinh
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > For some UDCs, the initialization sequence by udc_start() should not be > repeated until it is properly cleaned up with udc_stop() and vise versa. > We may run into some cleanup failure as seen with the DWC3 driver during > the irq cleanup. This issue can occur when the user triggers > soft-connect/soft-disconnect from the soft_connect sysfs. To avoid > adding checks to every UDC driver, at the UDC framework, introduce a > "started" state to track and prevent the UDC from repeating the > udc_start() and udc_stop() if it had already started/stopped. > > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> Acked-by: Felipe Balbi <balbi@kernel.org>
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c index 6a62bbd01324..98cf9216f3cb 100644 --- a/drivers/usb/gadget/udc/core.c +++ b/drivers/usb/gadget/udc/core.c @@ -29,6 +29,7 @@ * @list: for use by the udc class driver * @vbus: for udcs who care about vbus status, this value is real vbus status; * for udcs who do not care about vbus status, this value is always true + * @started: the UDC's started state. True if the UDC had started. * * This represents the internal data structure which is used by the UDC-class * to hold information about udc driver and gadget together. @@ -39,6 +40,7 @@ struct usb_udc { struct device dev; struct list_head list; bool vbus; + bool started; }; static struct class *udc_class; @@ -1082,7 +1084,18 @@ EXPORT_SYMBOL_GPL(usb_gadget_udc_reset); */ static inline int usb_gadget_udc_start(struct usb_udc *udc) { - return udc->gadget->ops->udc_start(udc->gadget, udc->driver); + int ret; + + if (udc->started) { + dev_err(&udc->dev, "UDC had already started\n"); + return -EBUSY; + } + + ret = udc->gadget->ops->udc_start(udc->gadget, udc->driver); + if (!ret) + udc->started = true; + + return ret; } /** @@ -1098,7 +1111,13 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc) */ static inline void usb_gadget_udc_stop(struct usb_udc *udc) { + if (!udc->started) { + dev_err(&udc->dev, "UDC had already stopped\n"); + return; + } + udc->gadget->ops->udc_stop(udc->gadget); + udc->started = false; } /** @@ -1222,6 +1241,8 @@ int usb_add_gadget(struct usb_gadget *gadget) udc->gadget = gadget; gadget->udc = udc; + udc->started = false; + mutex_lock(&udc_lock); list_add_tail(&udc->list, &udc_list);
For some UDCs, the initialization sequence by udc_start() should not be repeated until it is properly cleaned up with udc_stop() and vise versa. We may run into some cleanup failure as seen with the DWC3 driver during the irq cleanup. This issue can occur when the user triggers soft-connect/soft-disconnect from the soft_connect sysfs. To avoid adding checks to every UDC driver, at the UDC framework, introduce a "started" state to track and prevent the UDC from repeating the udc_start() and udc_stop() if it had already started/stopped. Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> --- Suggested by Felipe: https://lore.kernel.org/linux-usb/87a6tmcxhi.fsf@kernel.org/ drivers/usb/gadget/udc/core.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) base-commit: 7c53f6b671f4aba70ff15e1b05148b10d58c2837