On Tue, Oct 26, 2021 at 12:40:04PM -0700, Wesley Cheng wrote:

Hi Alan,

On 10/26/2021 7:41 AM, Alan Stern wrote:

quoted

On Mon, Oct 25, 2021 at 05:44:56PM -0700, Wesley Cheng wrote:

quoted

From: Wesley Cheng <redacted>

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 <redacted>
---
 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 3cabf7692ee1..752439690fda 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);

According to the kerneldoc, this routine must be called in process 
context.

quoted

+		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);
 }

Looks like you'll have to find a different way to avoid the problem.  
For example, if an exception is pending then you might prevent the 
gadget from going into runtime suspend until the exception has been 
handled.

Thanks for the suggestion.  I noticed that a lot of other FDs currently
call usb_ep_disable() in the disable/disconnect path as well.  Actually,
f_mass_storage seems to be the only one that doesn't do so.  Maybe we
should change the kerneldoc :)

You're right; I don't see any real reason why the usb_ep_{en,dis}able 
routines can't run in an atomic context.  So if you to make this change 
to f_mass_storage, you should first submit a patch changing the 
kerneldoc.

Alan Stern