Re: Reproducible deadlock when usb-storage scsi command timeouts twice

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

 



On Thu, May 04, 2023 at 04:41:11PM +0200, Maxime Bizon wrote:
> 
> On Thursday 04 May 2023 à 10:05:38 (-0400), Alan Stern wrote:
> 
> > Maxime, would you like to submit a revised version of your patch?  The 
> > key difference is that it should abort the currently executing command 
> > (if there is one), regardless of whether the srb value matches.
> 
> Yes
> 
> before I do a format submission, is this what you have in mind ?

Yes, except that I would not make command_abort_any() a separate 
routine.  Just put it inline in device_reset().

Alan Stern

> 
> diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
> index 8931df5a85fd..380b04273969 100644
> --- a/drivers/usb/storage/scsiglue.c
> +++ b/drivers/usb/storage/scsiglue.c
> @@ -406,10 +406,8 @@ static DEF_SCSI_QCMD(queuecommand)
>   ***********************************************************************/
>  
>  /* Command timeout and abort */
> -static int command_abort(struct scsi_cmnd *srb)
> +static int command_abort_matching(struct us_data *us, struct scsi_cmnd *srb_match)
>  {
> -	struct us_data *us = host_to_us(srb->device->host);
> -
>  	usb_stor_dbg(us, "%s called\n", __func__);
>  
>  	/*
> @@ -418,10 +416,17 @@ static int command_abort(struct scsi_cmnd *srb)
>  	 */
>  	scsi_lock(us_to_host(us));
>  
> -	/* Is this command still active? */
> -	if (us->srb != srb) {
> +	/* is there any active pending command to abort ? */
> +	if (!us->srb) {
>  		scsi_unlock(us_to_host(us));
>  		usb_stor_dbg(us, "-- nothing to abort\n");
> +		return SUCCESS;
> +	}
> +
> +	/* Does the command match the passed srb if any ? */
> +	if (srb_match && us->srb != srb_match) {
> +		scsi_unlock(us_to_host(us));
> +		usb_stor_dbg(us, "-- pending command mismatch\n");
>  		return FAILED;
>  	}
>  
> @@ -444,6 +449,16 @@ static int command_abort(struct scsi_cmnd *srb)
>  	return SUCCESS;
>  }
>  
> +static int command_abort_any(struct us_data *us)
> +{
> +	return command_abort_matching(us, NULL);
> +}
> +
> +static int command_abort(struct scsi_cmnd *srb)
> +{
> +	return command_abort_matching(host_to_us(srb->device->host), srb);
> +}
> +
>  /*
>   * This invokes the transport reset mechanism to reset the state of the
>   * device
> @@ -455,6 +470,8 @@ static int device_reset(struct scsi_cmnd *srb)
>  
>  	usb_stor_dbg(us, "%s called\n", __func__);
>  
> +	command_abort_any(us);
> +
>  	/* lock the device pointers and do the reset */
>  	mutex_lock(&(us->dev_mutex));
>  	result = us->transport_reset(us);
> 
> 
> -- 
> Maxime



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

  Powered by Linux