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@xxxxxxxxxxxxxxx
> Signed-off-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
> ---
> 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@xxxxxxx>
