Changes in v2:
- Revised comments for usb_ep_disable() as it should be safe to be
executed in atomic contexts as well. Other FDs are currently
calling ep disable during the disconnect event as well.
This series calls the usb_ep_disable() API directly from fsg_disable()
as there is a possibility that UDCs that support runtime PM may
already be in a suspended state, leading to HW access while resources
are disabled.
Wesley Cheng (2):
usb: gadget: udc: core: Revise comments for usb_ep_disable()
usb: gadget: f_mass_storage: Disable eps during disconnect
drivers/usb/gadget/function/f_mass_storage.c | 10 ++++++++++
drivers/usb/gadget/udc/core.c | 2 --
2 files changed, 10 insertions(+), 2 deletions(-)
When receiving a disconnect event from the UDC, the mass storage
function driver currently runs the handle_exception() routine
asynchronously. For UDCs that support runtime PM, there is a
possibility the UDC is already suspended by the time the
do_set_interface() is executed. This can lead to HW register access
while the UDC is already suspended.
Signed-off-by: Wesley Cheng <[email protected]>
---
drivers/usb/gadget/function/f_mass_storage.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 3cabf76..7524396 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -2342,6 +2342,16 @@ static void fsg_disable(struct usb_function *f)
{
struct fsg_dev *fsg = fsg_from_func(f);
+ /* Disable the endpoints */
+ if (fsg->bulk_in_enabled) {
+ usb_ep_disable(fsg->bulk_in);
+ fsg->bulk_in_enabled = 0;
+ }
+ if (fsg->bulk_out_enabled) {
+ usb_ep_disable(fsg->bulk_out);
+ fsg->bulk_out_enabled = 0;
+ }
+
__raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE, NULL);
}
The usb_ep_disable() routine is being widely used directly in the
disconnect callback path by function drivers. Hence, the statement
about it being able to only run in process context may not be true.
Signed-off-by: Wesley Cheng <[email protected]>
---
drivers/usb/gadget/udc/core.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index d626511..e1f90d8 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -136,8 +136,6 @@ EXPORT_SYMBOL_GPL(usb_ep_enable);
* gadget drivers must call usb_ep_enable() again before queueing
* requests to the endpoint.
*
- * This routine must be called in process context.
- *
* returns zero, or a negative error code.
*/
int usb_ep_disable(struct usb_ep *ep)
On Tue, Oct 26, 2021 at 07:50:24PM -0700, Wesley Cheng wrote:
> The usb_ep_disable() routine is being widely used directly in the
> disconnect callback path by function drivers. Hence, the statement
> about it being able to only run in process context may not be true.
>
> Signed-off-by: Wesley Cheng <[email protected]>
> ---
> drivers/usb/gadget/udc/core.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index d626511..e1f90d8 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -136,8 +136,6 @@ EXPORT_SYMBOL_GPL(usb_ep_enable);
> * gadget drivers must call usb_ep_enable() again before queueing
> * requests to the endpoint.
> *
> - * This routine must be called in process context.
> - *
> * returns zero, or a negative error code.
> */
> int usb_ep_disable(struct usb_ep *ep)
You should also change the kerneldoc for usb_ep_enable. Neither routine
needs to be called in process context.
In fact, it might be good to change both comments to:
* This routine may be called in an atomic (interrupt) context.
just to be totally explicit.
Alan Stern
Hi Alan,
On 10/27/2021 7:24 AM, Alan Stern wrote:
> On Tue, Oct 26, 2021 at 07:50:24PM -0700, Wesley Cheng wrote:
>> The usb_ep_disable() routine is being widely used directly in the
>> disconnect callback path by function drivers. Hence, the statement
>> about it being able to only run in process context may not be true.
>>
>> Signed-off-by: Wesley Cheng <[email protected]>
>> ---
>> drivers/usb/gadget/udc/core.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
>> index d626511..e1f90d8 100644
>> --- a/drivers/usb/gadget/udc/core.c
>> +++ b/drivers/usb/gadget/udc/core.c
>> @@ -136,8 +136,6 @@ EXPORT_SYMBOL_GPL(usb_ep_enable);
>> * gadget drivers must call usb_ep_enable() again before queueing
>> * requests to the endpoint.
>> *
>> - * This routine must be called in process context.
>> - *
>> * returns zero, or a negative error code.
>> */
>> int usb_ep_disable(struct usb_ep *ep)
>
> You should also change the kerneldoc for usb_ep_enable. Neither routine
> needs to be called in process context.
>
> In fact, it might be good to change both comments to:
>
> * This routine may be called in an atomic (interrupt) context.
>
> just to be totally explicit.
>
Ah, missed the ep enable case as well, thanks for the catch. Sounds
good, I'll add that statement.
Thanks
Wesley Cheng