Re: [PATCH v1] usb: f_mass_storage: forbid async queue when shutdown happen

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jan 22, 2024 at 06:51:38PM +0800, yuan linyu wrote:
> When write UDC to empty and unbind gadget driver from gadget device, it is
> possible that there are many queue failures for mass storage function.
> 
> The root cause is mass storage main thread alaways try to queue request to
> receive a command from host if running flag is on, on platform like dwc3,
> if pull down called, it will not queue request again and return
> -ESHUTDOWN, but it not affect running flag of mass storage function.
> 
> Check return code from mass storage function and clear running flag if it
> is -ESHUTDOWN, also indicate start in/out transfer failure to break loops.
> 
> Signed-off-by: yuan linyu <yuanlinyu@xxxxxxxxxxx>
> ---
> v1: follow Alan suggestion, only limit change in f_mass_storage
> RFC: https://lore.kernel.org/linux-usb/5f4c9d8b6e0e4e73a5b3b1540a500b6a@xxxxxxxxxxx/T/#t

This is basically okay.  I have a suggestion for improvement, see below.

>  drivers/usb/gadget/function/f_mass_storage.c | 28 +++++++++++++++++---
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index 722a3ab2b337..d5174e066078 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -545,21 +545,41 @@ static int start_transfer(struct fsg_dev *fsg, struct usb_ep *ep,
>  
>  static bool start_in_transfer(struct fsg_common *common, struct fsg_buffhd *bh)
>  {
> +	int rc;
> +
>  	if (!fsg_is_set(common))
>  		return false;
>  	bh->state = BUF_STATE_SENDING;
> -	if (start_transfer(common->fsg, common->fsg->bulk_in, bh->inreq))
> -		bh->state = BUF_STATE_EMPTY;
> +	rc = start_transfer(common->fsg, common->fsg->bulk_in, bh->inreq);
> +	if (!rc)
> +		return true;
> +
> +	bh->state = BUF_STATE_EMPTY;
> +	if (rc == -ESHUTDOWN) {
> +		common->running = 0;
> +		return false;
> +	}
> +
>  	return true;
>  }

This could be written more cleanly as:

	rc = start_transfer(common->fsg, common->fsg->bulk_in, bh->inreq);
	if (rc) {
		bh->state = BUF_STATE_EMPTY;
		if (rc == -ESHUTDOWN) {
			common->running = 0;
			return false;
		}
	}
	return true;

And the same goes for start_out_transfer().

Have you tested this?  Does it do what you want?

Alan Stern




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux