[PATCH v7] scsi: ufs: Quiesce all scsi devices before shutdown

[PATCH v7] scsi: ufs: Quiesce all scsi devices before shutdown

[Stanley Chu]

Currently I/O request could be still submitted to UFS device while
UFS is working on shutdown flow. This may lead to racing as below
scenarios and finally system may crash due to unclocked register
accesses.

To fix this kind of issues, in ufshcd_shutdown(),

1. Use pm_runtime_get_sync() instead of resuming UFS device by
   ufshcd_runtime_resume() "internally" to let runtime PM framework
   manage and prevent concurrent runtime operations by incoming I/O
   requests.

2. Specifically quiesce all SCSI devices to block all I/O requests
   after device is resumed.

Example of racing scenario: While UFS device is runtime-suspended

Thread #1: Executing UFS shutdown flow, e.g.,
           ufshcd_suspend(UFS_SHUTDOWN_PM)

Thread #2: Executing runtime resume flow triggered by I/O request,
           e.g., ufshcd_resume(UFS_RUNTIME_PM)

This breaks the assumption that UFS PM flows can not be running
concurrently and some unexpected racing behavior may happen.

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
Changes:
  - Since v6:
	- Do quiesce to all SCSI devices.
  - Since v4:
	- Use pm_runtime_get_sync() instead of resuming UFS device by ufshcd_runtime_resume() "internally".
---
 drivers/scsi/ufs/ufshcd.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 307622284239..7cb220b3fde0 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -8640,6 +8640,7 @@ EXPORT_SYMBOL(ufshcd_runtime_idle);
 int ufshcd_shutdown(struct ufs_hba *hba)
 {
 	int ret = 0;
+	struct scsi_target *starget;
 
 	if (!hba->is_powered)
 		goto out;
@@ -8647,11 +8648,27 @@ int ufshcd_shutdown(struct ufs_hba *hba)
 	if (ufshcd_is_ufs_dev_poweroff(hba) && ufshcd_is_link_off(hba))
 		goto out;
 
-	if (pm_runtime_suspended(hba->dev)) {
-		ret = ufshcd_runtime_resume(hba);
-		if (ret)
-			goto out;
-	}
+	/*
+	 * Let runtime PM framework manage and prevent concurrent runtime
+	 * operations with shutdown flow.
+	 */
+	pm_runtime_get_sync(hba->dev);
+
+	/*
+	 * Quiesce all SCSI devices to prevent any non-PM requests sending
+	 * from block layer during and after shutdown.
+	 *
+	 * Here we can not use blk_cleanup_queue() since PM requests
+	 * (with BLK_MQ_REQ_PREEMPT flag) are still required to be sent
+	 * through block layer. Therefore SCSI command queued after the
+	 * scsi_target_quiesce() call returned will block until
+	 * blk_cleanup_queue() is called.
+	 *
+	 * Besides, scsi_target_"un"quiesce (e.g., scsi_target_resume) can
+	 * be ignored since shutdown is one-way flow.
+	 */
+	list_for_each_entry(starget, &hba->host->__targets, siblings)
+		scsi_target_quiesce(starget);
 
 	ret = ufshcd_suspend(hba, UFS_SHUTDOWN_PM);
 out:
-- 
2.18.0

Re: [PATCH v7] scsi: ufs: Quiesce all scsi devices before shutdown

[Can Guo]

Hi Stanley,

On 2020-08-03 18:04, Stanley Chu wrote:
> Currently I/O request could be still submitted to UFS device while
> UFS is working on shutdown flow. This may lead to racing as below
> scenarios and finally system may crash due to unclocked register
> accesses.
> 
> To fix this kind of issues, in ufshcd_shutdown(),
> 
> 1. Use pm_runtime_get_sync() instead of resuming UFS device by
>    ufshcd_runtime_resume() "internally" to let runtime PM framework
>    manage and prevent concurrent runtime operations by incoming I/O
>    requests.
> 
> 2. Specifically quiesce all SCSI devices to block all I/O requests
>    after device is resumed.
> 
> Example of racing scenario: While UFS device is runtime-suspended
> 
> Thread #1: Executing UFS shutdown flow, e.g.,
>            ufshcd_suspend(UFS_SHUTDOWN_PM)
> 
> Thread #2: Executing runtime resume flow triggered by I/O request,
>            e.g., ufshcd_resume(UFS_RUNTIME_PM)
> 
> This breaks the assumption that UFS PM flows can not be running
> concurrently and some unexpected racing behavior may happen.
> 
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> ---
> Changes:
>   - Since v6:
> 	- Do quiesce to all SCSI devices.
>   - Since v4:
> 	- Use pm_runtime_get_sync() instead of resuming UFS device by
> ufshcd_runtime_resume() "internally".
> ---
>  drivers/scsi/ufs/ufshcd.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 307622284239..7cb220b3fde0 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -8640,6 +8640,7 @@ EXPORT_SYMBOL(ufshcd_runtime_idle);
>  int ufshcd_shutdown(struct ufs_hba *hba)
>  {
>  	int ret = 0;
> +	struct scsi_target *starget;
> 
>  	if (!hba->is_powered)
>  		goto out;
> @@ -8647,11 +8648,27 @@ int ufshcd_shutdown(struct ufs_hba *hba)
>  	if (ufshcd_is_ufs_dev_poweroff(hba) && ufshcd_is_link_off(hba))
>  		goto out;
> 
> -	if (pm_runtime_suspended(hba->dev)) {
> -		ret = ufshcd_runtime_resume(hba);
> -		if (ret)
> -			goto out;
> -	}
> +	/*
> +	 * Let runtime PM framework manage and prevent concurrent runtime
> +	 * operations with shutdown flow.
> +	 */
> +	pm_runtime_get_sync(hba->dev);
> +
> +	/*
> +	 * Quiesce all SCSI devices to prevent any non-PM requests sending
> +	 * from block layer during and after shutdown.
> +	 *
> +	 * Here we can not use blk_cleanup_queue() since PM requests
> +	 * (with BLK_MQ_REQ_PREEMPT flag) are still required to be sent
> +	 * through block layer. Therefore SCSI command queued after the
> +	 * scsi_target_quiesce() call returned will block until
> +	 * blk_cleanup_queue() is called.
> +	 *
> +	 * Besides, scsi_target_"un"quiesce (e.g., scsi_target_resume) can
> +	 * be ignored since shutdown is one-way flow.
> +	 */
> +	list_for_each_entry(starget, &hba->host->__targets, siblings)
> +		scsi_target_quiesce(starget);
> 

Sorry for misleading you to scsi_target_quiesce(), maybe below is 
better.

     shost_for_each_device(sdev, hba->host)
         scsi_device_quiesce(sdev);

We may need to discuss more about this quiesce part since I missed 
something.

After we quiesce the scsi devices, only PM requests are allowed, but it
is still not safe: [1] PM requests can still pass through, [2] there can
be tasks/reqs present in doorbells before the devices are quiesced. So,
these tasks/reqs in [1] and [2] can still be flying in parallel while
ufshcd_suspend is running.

How about only quiescing the UFS device well known scsi device but using
freeze_queue to the other scsi devices? blk_mq_freeze_queue can 
eliminate
the risks mentioned in [1] and [2].

      shost_for_each_device(sdev, hba->host) {
          if (sdev == hba->sdev_ufs_device)
               scsi_device_quiesce(sdev);
          else
               blk_mq_freeze_queue(sdev->request_queue);
      }

IF blk_mq_freeze_queue is not allowed to be used by LLD (I think we can
use it as I recalled Bart used to use it in one of his changes to UFS 
scaling),
we may need to make changes like below. [1] is to make sure no more PM 
requests
sent to scsi devices, [2] is make sure doorbells are cleared before 
invoke
ufshcd_suspend.

     shost_for_each_device(sdev, hba->host) {
         scsi_autopm_get_device(sdev); [1]
         scsi_device_quiesce(sdev);
     }

     ufshcd_wait_for_doorbell_clr(hba, U64_MAX); [2]

Please let me know your ideas, thanks!

Regards,

Can Guo.

>  	ret = ufshcd_suspend(hba, UFS_SHUTDOWN_PM);
>  out:

Re: [PATCH v7] scsi: ufs: Quiesce all scsi devices before shutdown

[Can Guo]

Slightly updated my comments

On 2020-08-03 19:50, Can Guo wrote:
> Hi Stanley,
> 
> On 2020-08-03 18:04, Stanley Chu wrote:
>> Currently I/O request could be still submitted to UFS device while
>> UFS is working on shutdown flow. This may lead to racing as below
>> scenarios and finally system may crash due to unclocked register
>> accesses.
>> 
>> To fix this kind of issues, in ufshcd_shutdown(),
>> 
>> 1. Use pm_runtime_get_sync() instead of resuming UFS device by
>>    ufshcd_runtime_resume() "internally" to let runtime PM framework
>>    manage and prevent concurrent runtime operations by incoming I/O
>>    requests.
>> 
>> 2. Specifically quiesce all SCSI devices to block all I/O requests
>>    after device is resumed.
>> 
>> Example of racing scenario: While UFS device is runtime-suspended
>> 
>> Thread #1: Executing UFS shutdown flow, e.g.,
>>            ufshcd_suspend(UFS_SHUTDOWN_PM)
>> 
>> Thread #2: Executing runtime resume flow triggered by I/O request,
>>            e.g., ufshcd_resume(UFS_RUNTIME_PM)
>> 
>> This breaks the assumption that UFS PM flows can not be running
>> concurrently and some unexpected racing behavior may happen.
>> 
>> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
>> ---
>> Changes:
>>   - Since v6:
>> 	- Do quiesce to all SCSI devices.
>>   - Since v4:
>> 	- Use pm_runtime_get_sync() instead of resuming UFS device by
>> ufshcd_runtime_resume() "internally".
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 27 ++++++++++++++++++++++-----
>>  1 file changed, 22 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 307622284239..7cb220b3fde0 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -8640,6 +8640,7 @@ EXPORT_SYMBOL(ufshcd_runtime_idle);
>>  int ufshcd_shutdown(struct ufs_hba *hba)
>>  {
>>  	int ret = 0;
>> +	struct scsi_target *starget;
>> 
>>  	if (!hba->is_powered)
>>  		goto out;
>> @@ -8647,11 +8648,27 @@ int ufshcd_shutdown(struct ufs_hba *hba)
>>  	if (ufshcd_is_ufs_dev_poweroff(hba) && ufshcd_is_link_off(hba))
>>  		goto out;
>> 
>> -	if (pm_runtime_suspended(hba->dev)) {
>> -		ret = ufshcd_runtime_resume(hba);
>> -		if (ret)
>> -			goto out;
>> -	}
>> +	/*
>> +	 * Let runtime PM framework manage and prevent concurrent runtime
>> +	 * operations with shutdown flow.
>> +	 */
>> +	pm_runtime_get_sync(hba->dev);
>> +
>> +	/*
>> +	 * Quiesce all SCSI devices to prevent any non-PM requests sending
>> +	 * from block layer during and after shutdown.
>> +	 *
>> +	 * Here we can not use blk_cleanup_queue() since PM requests
>> +	 * (with BLK_MQ_REQ_PREEMPT flag) are still required to be sent
>> +	 * through block layer. Therefore SCSI command queued after the
>> +	 * scsi_target_quiesce() call returned will block until
>> +	 * blk_cleanup_queue() is called.
>> +	 *
>> +	 * Besides, scsi_target_"un"quiesce (e.g., scsi_target_resume) can
>> +	 * be ignored since shutdown is one-way flow.
>> +	 */
>> +	list_for_each_entry(starget, &hba->host->__targets, siblings)
>> +		scsi_target_quiesce(starget);
>> 
> 
> Sorry for misleading you to scsi_target_quiesce(), maybe below is 
> better.
> 
>     shost_for_each_device(sdev, hba->host)
>         scsi_device_quiesce(sdev);
> 
> We may need to discuss more about this quiesce part since I missed 
> something.
> 
> After we quiesce the scsi devices, only PM requests are allowed, but it
> is still not safe: [1] PM requests can still pass through, [2] there 
> can
> be tasks/reqs present in doorbells before the devices are quiesced. So,
> these tasks/reqs in [1] and [2] can still be flying in parallel while
> ufshcd_suspend is running.
> 
> How about only quiescing the UFS device well known scsi device but 
> using
> freeze_queue to the other scsi devices? blk_mq_freeze_queue can 
> eliminate
> the risks mentioned in [1] and [2].
> 
>      shost_for_each_device(sdev, hba->host) {
>          if (sdev == hba->sdev_ufs_device)
>               scsi_device_quiesce(sdev);
>          else
>               blk_mq_freeze_queue(sdev->request_queue);
>      }
> 
> IF blk_mq_freeze_queue is not allowed to be used by LLD (I think we can
> use it as I recalled Bart used to use it in one of his changes to UFS 
> scaling),
> we can use scsi_remove_device instead, it changes scsi device's state 
> to
> SDEV_DEL and calls blk_cleanup_queue.
> 
> We can also make changes like below. [1] is to make sure no more PM 
> requests
> sent to scsi devices, [2] is make sure doorbells are cleared before 
> invoke
> ufshcd_suspend.
> 
>     shost_for_each_device(sdev, hba->host) {
>         scsi_autopm_get_device(sdev); [1]
>         scsi_device_quiesce(sdev);
>     }
> 
>     ufshcd_wait_for_doorbell_clr(hba, U64_MAX); [2]
> 
> Please let me know which one you prefer or if you have better idea, 
> thanks!
> 
> Regards,
> 
> Can Guo.
> 
>>  	ret = ufshcd_suspend(hba, UFS_SHUTDOWN_PM);
>>  out:

Re: [PATCH v7] scsi: ufs: Quiesce all scsi devices before shutdown

[Can Guo]

Hi Stanley,

Sorry for the noises, please ignore my previous 2 mails and let's
focus on this one.

On 2020-08-03 18:04, Stanley Chu wrote:
> Currently I/O request could be still submitted to UFS device while
> UFS is working on shutdown flow. This may lead to racing as below
> scenarios and finally system may crash due to unclocked register
> accesses.
> 
> To fix this kind of issues, in ufshcd_shutdown(),
> 
> 1. Use pm_runtime_get_sync() instead of resuming UFS device by
>    ufshcd_runtime_resume() "internally" to let runtime PM framework
>    manage and prevent concurrent runtime operations by incoming I/O
>    requests.
> 
> 2. Specifically quiesce all SCSI devices to block all I/O requests
>    after device is resumed.
> 
> Example of racing scenario: While UFS device is runtime-suspended
> 
> Thread #1: Executing UFS shutdown flow, e.g.,
>            ufshcd_suspend(UFS_SHUTDOWN_PM)
> 
> Thread #2: Executing runtime resume flow triggered by I/O request,
>            e.g., ufshcd_resume(UFS_RUNTIME_PM)
> 
> This breaks the assumption that UFS PM flows can not be running
> concurrently and some unexpected racing behavior may happen.
> 
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> ---
> Changes:
>   - Since v6:
> 	- Do quiesce to all SCSI devices.
>   - Since v4:
> 	- Use pm_runtime_get_sync() instead of resuming UFS device by
> ufshcd_runtime_resume() "internally".
> ---
>  drivers/scsi/ufs/ufshcd.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 307622284239..7cb220b3fde0 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -8640,6 +8640,7 @@ EXPORT_SYMBOL(ufshcd_runtime_idle);
>  int ufshcd_shutdown(struct ufs_hba *hba)
>  {
>  	int ret = 0;
> +	struct scsi_target *starget;
> 
>  	if (!hba->is_powered)
>  		goto out;
> @@ -8647,11 +8648,27 @@ int ufshcd_shutdown(struct ufs_hba *hba)
>  	if (ufshcd_is_ufs_dev_poweroff(hba) && ufshcd_is_link_off(hba))
>  		goto out;
> 
> -	if (pm_runtime_suspended(hba->dev)) {
> -		ret = ufshcd_runtime_resume(hba);
> -		if (ret)
> -			goto out;
> -	}
> +	/*
> +	 * Let runtime PM framework manage and prevent concurrent runtime
> +	 * operations with shutdown flow.
> +	 */
> +	pm_runtime_get_sync(hba->dev);
> +
> +	/*
> +	 * Quiesce all SCSI devices to prevent any non-PM requests sending
> +	 * from block layer during and after shutdown.
> +	 *
> +	 * Here we can not use blk_cleanup_queue() since PM requests
> +	 * (with BLK_MQ_REQ_PREEMPT flag) are still required to be sent
> +	 * through block layer. Therefore SCSI command queued after the
> +	 * scsi_target_quiesce() call returned will block until
> +	 * blk_cleanup_queue() is called.
> +	 *
> +	 * Besides, scsi_target_"un"quiesce (e.g., scsi_target_resume) can
> +	 * be ignored since shutdown is one-way flow.
> +	 */
> +	list_for_each_entry(starget, &hba->host->__targets, siblings)
> +		scsi_target_quiesce(starget);
> 

Sorry for misleading you to scsi_target_quiesce(), maybe below is 
better.

     shost_for_each_device(sdev, hba->host)
         scsi_device_quiesce(sdev);

We may need to discuss more about this quiesce part since I missed 
something.

After we quiesce the scsi devices, only PM requests are allowed, but it
is still not safe - PM requests can still pass through.

How about only quiescing the UFS device well known scsi device but using
freeze_queue to the other scsi devices? blk_mq_freeze_queue can 
eliminate
the risk.

      shost_for_each_device(sdev, hba->host) {
          if (sdev == hba->sdev_ufs_device)
               scsi_device_quiesce(sdev);
          else
               blk_mq_freeze_queue(sdev->request_queue);
      }

IF blk_mq_freeze_queue is not allowed to be used by LLD (I think we can
use it as I recalled Bart used to use it in one of his changes to UFS 
scaling),
we can use scsi_remove_device instead, it changes scsi device's state to
SDEV_DEL and calls blk_cleanup_queue.

We can also use scsi_autopm_get_device like below. It is to make sure
no more PM requests sent to scsi devices (since PM requests are only 
sent
during PM ops).

     shost_for_each_device(sdev, hba->host) {
         scsi_autopm_get_device(sdev);
         scsi_device_quiesce(sdev);
     }

Please let me know which one do you prefer or if you have better ideas, 
thanks!

Regards,

Can Guo.

>  	ret = ufshcd_suspend(hba, UFS_SHUTDOWN_PM);
>  out:

Re: [PATCH v7] scsi: ufs: Quiesce all scsi devices before shutdown

[Bart Van Assche]

On 2020-08-03 03:04, Stanley Chu wrote:
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 307622284239..7cb220b3fde0 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -8640,6 +8640,7 @@ EXPORT_SYMBOL(ufshcd_runtime_idle);
>  int ufshcd_shutdown(struct ufs_hba *hba)
>  {
>  	int ret = 0;
> +	struct scsi_target *starget;
>  
>  	if (!hba->is_powered)
>  		goto out;
> @@ -8647,11 +8648,27 @@ int ufshcd_shutdown(struct ufs_hba *hba)
>  	if (ufshcd_is_ufs_dev_poweroff(hba) && ufshcd_is_link_off(hba))
>  		goto out;
>  
> -	if (pm_runtime_suspended(hba->dev)) {
> -		ret = ufshcd_runtime_resume(hba);
> -		if (ret)
> -			goto out;
> -	}
> +	/*
> +	 * Let runtime PM framework manage and prevent concurrent runtime
> +	 * operations with shutdown flow.
> +	 */
> +	pm_runtime_get_sync(hba->dev);
> +
> +	/*
> +	 * Quiesce all SCSI devices to prevent any non-PM requests sending
> +	 * from block layer during and after shutdown.
> +	 *
> +	 * Here we can not use blk_cleanup_queue() since PM requests
> +	 * (with BLK_MQ_REQ_PREEMPT flag) are still required to be sent
> +	 * through block layer. Therefore SCSI command queued after the
> +	 * scsi_target_quiesce() call returned will block until
> +	 * blk_cleanup_queue() is called.
> +	 *
> +	 * Besides, scsi_target_"un"quiesce (e.g., scsi_target_resume) can
> +	 * be ignored since shutdown is one-way flow.
> +	 */
> +	list_for_each_entry(starget, &hba->host->__targets, siblings)
> +		scsi_target_quiesce(starget);
>  
>  	ret = ufshcd_suspend(hba, UFS_SHUTDOWN_PM);
>  out:

This seems wrong to me. Since ufshcd_shutdown() shuts down the link I think
it should call scsi_remove_device() instead of scsi_target_quiesce().

Thanks,

Bart.

Re: [PATCH v7] scsi: ufs: Quiesce all scsi devices before shutdown

[Stanley Chu]

Hi Bart, Can, Chaotian,

Very appreciate your comments and suggestions, please see update below,

On Tue, 2020-08-04 at 00:04 +0800, Bart Van Assche wrote:
> On 2020-08-03 03:04, Stanley Chu wrote:
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 307622284239..7cb220b3fde0 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -8640,6 +8640,7 @@ EXPORT_SYMBOL(ufshcd_runtime_idle);
> >  int ufshcd_shutdown(struct ufs_hba *hba)
> >  {
> >  	int ret = 0;
> > +	struct scsi_target *starget;
> >  
> >  	if (!hba->is_powered)
> >  		goto out;
> > @@ -8647,11 +8648,27 @@ int ufshcd_shutdown(struct ufs_hba *hba)
> >  	if (ufshcd_is_ufs_dev_poweroff(hba) && ufshcd_is_link_off(hba))
> >  		goto out;
> >  
> > -	if (pm_runtime_suspended(hba->dev)) {
> > -		ret = ufshcd_runtime_resume(hba);
> > -		if (ret)
> > -			goto out;
> > -	}
> > +	/*
> > +	 * Let runtime PM framework manage and prevent concurrent runtime
> > +	 * operations with shutdown flow.
> > +	 */
> > +	pm_runtime_get_sync(hba->dev);
> > +
> > +	/*
> > +	 * Quiesce all SCSI devices to prevent any non-PM requests sending
> > +	 * from block layer during and after shutdown.
> > +	 *
> > +	 * Here we can not use blk_cleanup_queue() since PM requests
> > +	 * (with BLK_MQ_REQ_PREEMPT flag) are still required to be sent
> > +	 * through block layer. Therefore SCSI command queued after the
> > +	 * scsi_target_quiesce() call returned will block until
> > +	 * blk_cleanup_queue() is called.
> > +	 *
> > +	 * Besides, scsi_target_"un"quiesce (e.g., scsi_target_resume) can
> > +	 * be ignored since shutdown is one-way flow.
> > +	 */
> > +	list_for_each_entry(starget, &hba->host->__targets, siblings)
> > +		scsi_target_quiesce(starget);
> >  
> >  	ret = ufshcd_suspend(hba, UFS_SHUTDOWN_PM);
> >  out:
> 
> This seems wrong to me. Since ufshcd_shutdown() shuts down the link I think
> it should call scsi_remove_device() instead of scsi_target_quiesce().

I tried many ways to come out the final solution. Currently two options
are considered,

== Option 1 ==
	pm_runtime_get_sync(hba->dev);

	shost_for_each_device(sdev, hba->host) {
		scsi_autopm_get_device(sdev);
		if (sdev == hba->sdev_ufs_device)
			scsi_device_quiesce(sdev);
		else
			scsi_remove_device(sdev);
	}

	ret = ufshcd_suspend(hba, UFS_SHUTDOWN_PM);

	scsi_remove_device(hba->sdev_ufs_device);

Note. Using scsi_autopm_get_device() instead of pm_runtime_disable()
is to prevent noisy message by below checking,

	WARN_ON_ONCE(sdev->quiesced_by && sdev->quiesced_by != current);

in
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/scsi/scsi_lib.c#n2515

This warning shows up if we try to quiesce a runtime-suspended SCSI
device. This is possible during our new shutdown flow. Using
scsi_autopm_get_device() to resume all SCSI devices first can prevent
it.

In addition, normally sd_shutdown() would be executed prior than
ufshcd_shutdown(). If scsi_remove_device() is invoked by
ufshcd_shutdown(), sd_shutdown() will be executed again for a SCSI disk
by

[  131.398977]  sd_shutdown+0x44/0x118
[  131.399416]  sd_remove+0x5c/0xc4
[  131.399824]  device_release_driver_internal+0x1c4/0x2e4
[  131.400481]  device_release_driver+0x18/0x24
[  131.401018]  bus_remove_device+0x108/0x134
[  131.401533]  device_del+0x2dc/0x630
[  131.401973]  __scsi_remove_device+0xc0/0x174
[  131.402510]  scsi_remove_device+0x30/0x48
[  131.403014]  ufshcd_shutdown+0xc8/0x138

In this case, we could see SYNCHRONIZE_CACHE command will be sent to the
same SCSI device twice. This is kind of wired during shutdown flow.

Moreover, in consideration of performance of ufshcd_shutdown(), Option 1
obviously degrades the latency a lot by scsi_remove_device(). Please see
the "Performance Measurement" data below.

Compared Option 2, this way is simpler and also effective. This way may
be a better compromise.

== Option 2  ==
	pm_runtime_get_sync(hba->dev);

	shost_for_each_device(sdev, hba->host) {
		scsi_autopm_get_device(sdev);
		scsi_device_quiesce(sdev);
	}

== Performance Measurement ==
As-Is: < 5 ms
Option 1: 850 ms
Option 2: 60 ms

What would you prefer? Or would you have any further suggestions?

Thanks,

Stanley Chu

Re: [PATCH v7] scsi: ufs: Quiesce all scsi devices before shutdown

[Bart Van Assche]

On 2020-08-13 01:55, Stanley Chu wrote:
> I tried many ways to come out the final solution. Currently two options
> are considered,
> 
> == Option 1 ==
> 	pm_runtime_get_sync(hba->dev);
> 
> 	shost_for_each_device(sdev, hba->host) {
> 		scsi_autopm_get_device(sdev);
> 		if (sdev == hba->sdev_ufs_device)
> 			scsi_device_quiesce(sdev);
> 		else
> 			scsi_remove_device(sdev);
> 	}
> 
> 	ret = ufshcd_suspend(hba, UFS_SHUTDOWN_PM);
> 
> 	scsi_remove_device(hba->sdev_ufs_device);
> 
> Note. Using scsi_autopm_get_device() instead of pm_runtime_disable()
> is to prevent noisy message by below checking,
> 
> 	WARN_ON_ONCE(sdev->quiesced_by && sdev->quiesced_by != current);
> 
> in
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/scsi/scsi_lib.c#n2515
> 
> This warning shows up if we try to quiesce a runtime-suspended SCSI
> device. This is possible during our new shutdown flow. Using
> scsi_autopm_get_device() to resume all SCSI devices first can prevent
> it.
> 
> In addition, normally sd_shutdown() would be executed prior than
> ufshcd_shutdown(). If scsi_remove_device() is invoked by
> ufshcd_shutdown(), sd_shutdown() will be executed again for a SCSI disk
> by
> 
> [  131.398977]  sd_shutdown+0x44/0x118
> [  131.399416]  sd_remove+0x5c/0xc4
> [  131.399824]  device_release_driver_internal+0x1c4/0x2e4
> [  131.400481]  device_release_driver+0x18/0x24
> [  131.401018]  bus_remove_device+0x108/0x134
> [  131.401533]  device_del+0x2dc/0x630
> [  131.401973]  __scsi_remove_device+0xc0/0x174
> [  131.402510]  scsi_remove_device+0x30/0x48
> [  131.403014]  ufshcd_shutdown+0xc8/0x138
> 
> In this case, we could see SYNCHRONIZE_CACHE command will be sent to the
> same SCSI device twice. This is kind of wired during shutdown flow.
> 
> Moreover, in consideration of performance of ufshcd_shutdown(), Option 1
> obviously degrades the latency a lot by scsi_remove_device(). Please see
> the "Performance Measurement" data below.
> 
> Compared Option 2, this way is simpler and also effective. This way may
> be a better compromise.
> 
> == Option 2  ==
> 	pm_runtime_get_sync(hba->dev);
> 
> 	shost_for_each_device(sdev, hba->host) {
> 		scsi_autopm_get_device(sdev);
> 		scsi_device_quiesce(sdev);
> 	}
> 
> == Performance Measurement ==
> As-Is: < 5 ms
> Option 1: 850 ms
> Option 2: 60 ms
> 
> What would you prefer? Or would you have any further suggestions?

Hi Stanley,

Thanks for the detailed report and also for having shared timing information.

The approach of option 2 seems wrong to me because the SCSI devices are not
removed. My concern is that option (2) could cause the sd driver to send SYNC
and/or STOP commands to the device after its PCIe resources have been freed,
resulting in a crash.

Please take a look at the output of the following command:

$ git grep -nHA10 'struct pci_driver.* = {$' */scsi |
  sed -e 's/-/:/' -e 's/-/:/' |
  grep ':[[:blank:]]*\.remove'

It seems to me that other SCSI LLDs do at least the following in their PCIe
removal callback:

1. Call scsi_remove_host()
2. Call scsi_host_put()
3. Call pci_disable_device()

Would that approach work for UFS? Would offlining the UFS LUNs (SDEV_OFFLINE)
before calling the above functions make SCSI host removal faster? See also
scsi_prep_state_check().

Thanks,

Bart.