On Thu, Mar 14, 2024 at 02:59:49PM +0800, yuan linyu wrote:
> It is possible trigger below warning message from mass storage function,
>
> ------------[ cut here ]------------
> WARNING: CPU: 6 PID: 3839 at drivers/usb/gadget/udc/core.c:294 usb_ep_queue+0x7c/0x104
> CPU: 6 PID: 3839 Comm: file-storage Tainted: G S WC O 6.1.25-android14-11-g354e2a7e7cd9 #1
> pstate: 22400005 (nzCv daif +PAN -UAO +TCO -DIT -SSBS BTYPE=--)
> pc : usb_ep_queue+0x7c/0x104
> lr : fsg_main_thread+0x494/0x1b3c
>
> Root cause is mass storage function try to queue request from main thread,
> but other thread may already disable ep when function disable.
>
> As mass storage function have record of ep enable/disable state, let's
> add the state check before queue request to UDC, it maybe avoid warning.
>
> Also use common lock to protect ep state which avoid race between main
> thread and function disable.
This seems like a lot of effort just to avoid a harmless WARNING. How
about instead changing the WARNING to a debug-level message?
Alan Stern
> Cc: <stable@xxxxxxxxxxxxxxx> # 6.1
> Signed-off-by: yuan linyu <yuanlinyu@xxxxxxxxxxx>
> ---
> drivers/usb/gadget/function/f_mass_storage.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index c265a1f62fc1..056083cb68cb 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -520,12 +520,25 @@ static int fsg_setup(struct usb_function *f,
> static int start_transfer(struct fsg_dev *fsg, struct usb_ep *ep,
> struct usb_request *req)
> {
> + unsigned long flags;
> int rc;
>
> - if (ep == fsg->bulk_in)
> + spin_lock_irqsave(&fsg->common->lock, flags);
> + if (ep == fsg->bulk_in) {
> + if (!fsg->bulk_in_enabled) {
> + spin_unlock_irqrestore(&fsg->common->lock, flags);
> + return -ESHUTDOWN;
> + }
> dump_msg(fsg, "bulk-in", req->buf, req->length);
> + } else {
> + if (!fsg->bulk_out_enabled) {
> + spin_unlock_irqrestore(&fsg->common->lock, flags);
> + return -ESHUTDOWN;
> + }
> + }
>
> rc = usb_ep_queue(ep, req, GFP_KERNEL);
> + spin_unlock_irqrestore(&fsg->common->lock, flags);
> if (rc) {
>
> /* We can't do much more than wait for a reset */
> @@ -2406,8 +2419,10 @@ static int fsg_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
> static void fsg_disable(struct usb_function *f)
> {
> struct fsg_dev *fsg = fsg_from_func(f);
> + unsigned long flags;
>
> /* Disable the endpoints */
> + spin_lock_irqsave(&fsg->common->lock, flags);
> if (fsg->bulk_in_enabled) {
> usb_ep_disable(fsg->bulk_in);
> fsg->bulk_in_enabled = 0;
> @@ -2416,6 +2431,7 @@ static void fsg_disable(struct usb_function *f)
> usb_ep_disable(fsg->bulk_out);
> fsg->bulk_out_enabled = 0;
> }
> + spin_unlock_irqrestore(&fsg->common->lock, flags);
>
> __raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE, NULL);
> }
> --
> 2.25.1
>
