Re: [PATCH v3 06/23] scsi: Do not attempt to rescan suspended devices

[Niklas Cassel <Niklas.Cassel@wdc.com>]

On Fri, Sep 15, 2023 at 05:14:50PM +0900, Damien Le Moal wrote:
> scsi_rescan_device() takes a scsi device lock before executing a device
> handler and device driver rescan methods. Waiting for the completion of
> any command issued to the device by these methods will thus be done with
> the device lock held. As a result, there is a risk of deadlocking within
> the power management code if scsi_rescan_device() is called to handle a
> device resume with the associated scsi device not yet resumed.
> 
> Avoid such situation by checking that the target scsi device is in the
> running state, that is, fully capable of executing commands, before
> proceeding with the rescan and bailout returning -EWOULDBLOCK otherwise.
> With this error return, the caller can retry rescaning the device after
> a delay.
> 
> The state check is done with the device lock held and is thus safe
> against incoming suspend power management operations.
> 
> Fixes: 6aa0365a3c85 ("ata: libata-scsi: Avoid deadlock on rescan after device resume")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/scsi/scsi_scan.c | 18 +++++++++++++++++-
>  include/scsi/scsi_host.h |  2 +-
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 52014b2d39e1..3db4d31a03a1 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1619,12 +1619,24 @@ int scsi_add_device(struct Scsi_Host *host, uint channel,
>  }
>  EXPORT_SYMBOL(scsi_add_device);
>  
> -void scsi_rescan_device(struct scsi_device *sdev)
> +int scsi_rescan_device(struct scsi_device *sdev)
>  {
>  	struct device *dev = &sdev->sdev_gendev;
> +	int ret = 0;
>  
>  	device_lock(dev);
>  
> +	/*
> +	 * Bail out if the device is not running. Otherwise, the rescan may
> +	 * block waiting for commands to be executed, with us holding the
> +	 * device lock. This can result in a potential deadlock in the power
> +	 * management core code when system resume is on-going.
> +	 */
> +	if (sdev->sdev_state != SDEV_RUNNING) {
> +		ret = -EWOULDBLOCK;
> +		goto unlock;
> +	}
> +
>  	scsi_attach_vpd(sdev);
>  	scsi_cdl_check(sdev);
>  
> @@ -1638,7 +1650,11 @@ void scsi_rescan_device(struct scsi_device *sdev)
>  			drv->rescan(dev);
>  		module_put(dev->driver->owner);
>  	}
> +
> +unlock:
>  	device_unlock(dev);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(scsi_rescan_device);
>  
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 49f768d0ff37..4c2dc8150c6d 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -764,7 +764,7 @@ scsi_template_proc_dir(const struct scsi_host_template *sht);
>  #define scsi_template_proc_dir(sht) NULL
>  #endif
>  extern void scsi_scan_host(struct Scsi_Host *);
> -extern void scsi_rescan_device(struct scsi_device *);
> +extern int scsi_rescan_device(struct scsi_device *sdev);
>  extern void scsi_remove_host(struct Scsi_Host *);
>  extern struct Scsi_Host *scsi_host_get(struct Scsi_Host *);
>  extern int scsi_host_busy(struct Scsi_Host *shost);
> -- 
> 2.41.0
> 

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>