* [PATCH] usb: udc: core: Introduce started state
@ 2021-01-11 20:38 Thinh Nguyen
2021-01-11 21:29 ` Alan Stern
2021-01-12 8:20 ` Felipe Balbi
0 siblings, 2 replies; 7+ messages in thread
From: Thinh Nguyen @ 2021-01-11 20:38 UTC (permalink / raw)
To: Felipe Balbi, Greg Kroah-Hartman, linux-usb, Peter Chen,
Lee Jones, Alan Stern, Dejin Zheng, Ahmed S. Darwish,
Marek Szyprowski
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(-)
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);
base-commit: 7c53f6b671f4aba70ff15e1b05148b10d58c2837
--
2.28.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] usb: udc: core: Introduce started state
2021-01-11 20:38 [PATCH] usb: udc: core: Introduce started state Thinh Nguyen
@ 2021-01-11 21:29 ` Alan Stern
2021-01-11 21:45 ` Thinh Nguyen
2021-01-12 8:20 ` Felipe Balbi
1 sibling, 1 reply; 7+ messages in thread
From: Alan Stern @ 2021-01-11 21:29 UTC (permalink / raw)
To: Thinh Nguyen
Cc: Felipe Balbi, Greg Kroah-Hartman, linux-usb, Peter Chen,
Lee Jones, Dejin Zheng, Ahmed S. Darwish, Marek Szyprowski
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] usb: udc: core: Introduce started state
2021-01-11 21:29 ` Alan Stern
@ 2021-01-11 21:45 ` Thinh Nguyen
2021-01-11 22:35 ` Thinh Nguyen
0 siblings, 1 reply; 7+ messages in thread
From: Thinh Nguyen @ 2021-01-11 21:45 UTC (permalink / raw)
To: Alan Stern, Thinh Nguyen
Cc: Felipe Balbi, Greg Kroah-Hartman, linux-usb, Peter Chen,
Lee Jones, Dejin Zheng, Ahmed S. Darwish, Marek Szyprowski
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] usb: udc: core: Introduce started state
2021-01-11 21:45 ` Thinh Nguyen
@ 2021-01-11 22:35 ` Thinh Nguyen
2021-01-12 1:27 ` Peter Chen
0 siblings, 1 reply; 7+ messages in thread
From: Thinh Nguyen @ 2021-01-11 22:35 UTC (permalink / raw)
To: Alan Stern
Cc: Felipe Balbi, Greg Kroah-Hartman, linux-usb, Peter Chen,
Lee Jones, Dejin Zheng, Ahmed S. Darwish, Marek Szyprowski
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] usb: udc: core: Introduce started state
2021-01-11 22:35 ` Thinh Nguyen
@ 2021-01-12 1:27 ` Peter Chen
2021-01-12 1:33 ` Thinh Nguyen
0 siblings, 1 reply; 7+ messages in thread
From: Peter Chen @ 2021-01-12 1:27 UTC (permalink / raw)
To: Thinh Nguyen
Cc: Alan Stern, Felipe Balbi, Greg Kroah-Hartman, linux-usb,
Peter Chen, Lee Jones, Dejin Zheng, Ahmed S. Darwish,
Marek Szyprowski
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.
--
Thanks,
Peter Chen
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] usb: udc: core: Introduce started state
2021-01-12 1:27 ` Peter Chen
@ 2021-01-12 1:33 ` Thinh Nguyen
0 siblings, 0 replies; 7+ messages in thread
From: Thinh Nguyen @ 2021-01-12 1:33 UTC (permalink / raw)
To: Peter Chen, Thinh Nguyen
Cc: Alan Stern, Felipe Balbi, Greg Kroah-Hartman, linux-usb,
Peter Chen, Lee Jones, Dejin Zheng, Ahmed S. Darwish,
Marek Szyprowski
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] usb: udc: core: Introduce started state
2021-01-11 20:38 [PATCH] usb: udc: core: Introduce started state Thinh Nguyen
2021-01-11 21:29 ` Alan Stern
@ 2021-01-12 8:20 ` Felipe Balbi
1 sibling, 0 replies; 7+ messages in thread
From: Felipe Balbi @ 2021-01-12 8:20 UTC (permalink / raw)
To: Thinh Nguyen, Greg Kroah-Hartman, linux-usb, Peter Chen,
Lee Jones, Alan Stern, Dejin Zheng, Ahmed S. Darwish,
Marek Szyprowski
[-- Attachment #1: Type: text/plain, Size: 745 bytes --]
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>
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-01-12 8:21 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-11 20:38 [PATCH] usb: udc: core: Introduce started state Thinh Nguyen
2021-01-11 21:29 ` Alan Stern
2021-01-11 21:45 ` Thinh Nguyen
2021-01-11 22:35 ` Thinh Nguyen
2021-01-12 1:27 ` Peter Chen
2021-01-12 1:33 ` Thinh Nguyen
2021-01-12 8:20 ` Felipe Balbi
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.