[PATCH v2 0/2] Disable mass storage endpoints during disconnect

[PATCH v2 0/2] Disable mass storage endpoints during disconnect

[Wesley Cheng]

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(-)

[PATCH v2 1/2] usb: gadget: udc: core: Revise comments for usb_ep_disable()

[Wesley Cheng]

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 <quic_wcheng@quicinc.com>
---
 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)

Re: [PATCH v2 1/2] usb: gadget: udc: core: Revise comments for usb_ep_disable()

[Alan Stern]

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 <quic_wcheng@quicinc.com>
> ---
>  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

Re: [PATCH v2 1/2] usb: gadget: udc: core: Revise comments for usb_ep_disable()

[Wesley Cheng]

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 <quic_wcheng@quicinc.com>
>> ---
>>  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

[PATCH v2 2/2] usb: gadget: f_mass_storage: Disable eps during disconnect

[Wesley Cheng]

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 <quic_wcheng@quicinc.com>
---
 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);
 }