[PATCH v2] SCSI and block: Simplify resume handling

[PATCH v2] SCSI and block: Simplify resume handling

[Alan Stern]

Commit 05d18ae1cc8a ("scsi: pm: Balance pm_only counter of request
queue during system resume") fixed a problem in the block layer's
runtime-PM code: blk_set_runtime_active() failed to call
blk_clear_pm_only().  However, the commit's implementation was
awkward; it forced the SCSI system-resume handler to choose whether to
call blk_post_runtime_resume() or blk_set_runtime_active(), depending
on whether or not the SCSI device had previously been runtime
suspended.

This patch simplifies the situation considerably by adding the missing
function call directly into blk_set_runtime_active() (under the
condition that the queue is not already in the RPM_ACTIVE state).
This allows the SCSI routine to revert back to its original form.
Furthermore, making this change reveals that blk_post_runtime_resume()
(in its success pathway) does exactly the same thing as
blk_set_runtime_active().  The duplicate code is easily removed by
making one routine call the other.

No functional changes are intended.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
CC: Can Guo <cang@codeaurora.org>
CC: Bart Van Assche <bvanassche@acm.org>

---

v2:	Don't call blk_clear_pm_only() if the queue's RPM status was
	already set to RPM_ACTIVE.  This happens during a system resume
	if the device was not in runtime suspend beforehand.

Martin:

Since you merged the oritinal 05d18ae1cc8a commit, I'm submitting this to
you as an update.  If you would prefer to have it go by way of the
block-layer tree, let me know and I'll resend it.


[as1939b]


 block/blk-pm.c         |   30 +++++++++++++++---------------
 drivers/scsi/scsi_pm.c |   10 ++--------
 2 files changed, 17 insertions(+), 23 deletions(-)

Index: usb-devel/block/blk-pm.c
===================================================================
--- usb-devel.orig/block/blk-pm.c
+++ usb-devel/block/blk-pm.c
@@ -164,30 +164,21 @@ EXPORT_SYMBOL(blk_pre_runtime_resume);
  *
  * Description:
  *    Update the queue's runtime status according to the return value of the
- *    device's runtime_resume function. If it is successfully resumed, process
- *    the requests that are queued into the device's queue when it is resuming
- *    and then mark last busy and initiate autosuspend for it.
+ *    device's runtime_resume function. If the resume was successful, call
+ *    blk_set_runtime_active() to do the real work of restarting the queue.
  *
  *    This function should be called near the end of the device's
  *    runtime_resume callback.
  */
 void blk_post_runtime_resume(struct request_queue *q, int err)
 {
-	if (!q->dev)
-		return;
-
-	spin_lock_irq(&q->queue_lock);
 	if (!err) {
-		q->rpm_status = RPM_ACTIVE;
-		pm_runtime_mark_last_busy(q->dev);
-		pm_request_autosuspend(q->dev);
-	} else {
+		blk_set_runtime_active(q);
+	} else if (q->dev) {
+		spin_lock_irq(&q->queue_lock);
 		q->rpm_status = RPM_SUSPENDED;
+		spin_unlock_irq(&q->queue_lock);
 	}
-	spin_unlock_irq(&q->queue_lock);
-
-	if (!err)
-		blk_clear_pm_only(q);
 }
 EXPORT_SYMBOL(blk_post_runtime_resume);
 
@@ -204,15 +195,24 @@ EXPORT_SYMBOL(blk_post_runtime_resume);
  * This function can be used in driver's resume hook to correct queue
  * runtime PM status and re-enable peeking requests from the queue. It
  * should be called before first request is added to the queue.
+ *
+ * This function is also called by blk_post_runtime_resume() for successful
+ * runtime resumes.  It does everything necessary to restart the queue.
  */
 void blk_set_runtime_active(struct request_queue *q)
 {
 	if (q->dev) {
+		int old_status;
+
 		spin_lock_irq(&q->queue_lock);
+		old_status = q->rpm_status;
 		q->rpm_status = RPM_ACTIVE;
 		pm_runtime_mark_last_busy(q->dev);
 		pm_request_autosuspend(q->dev);
 		spin_unlock_irq(&q->queue_lock);
+
+		if (old_status != RPM_ACTIVE)
+			blk_clear_pm_only(q);
 	}
 }
 EXPORT_SYMBOL(blk_set_runtime_active);
Index: usb-devel/drivers/scsi/scsi_pm.c
===================================================================
--- usb-devel.orig/drivers/scsi/scsi_pm.c
+++ usb-devel/drivers/scsi/scsi_pm.c
@@ -80,10 +80,6 @@ static int scsi_dev_type_resume(struct d
 	dev_dbg(dev, "scsi resume: %d\n", err);
 
 	if (err == 0) {
-		bool was_runtime_suspended;
-
-		was_runtime_suspended = pm_runtime_suspended(dev);
-
 		pm_runtime_disable(dev);
 		err = pm_runtime_set_active(dev);
 		pm_runtime_enable(dev);
@@ -97,10 +93,8 @@ static int scsi_dev_type_resume(struct d
 		 */
 		if (!err && scsi_is_sdev_device(dev)) {
 			struct scsi_device *sdev = to_scsi_device(dev);
-			if (was_runtime_suspended)
-				blk_post_runtime_resume(sdev->request_queue, 0);
-			else
-				blk_set_runtime_active(sdev->request_queue);
+
+			blk_set_runtime_active(sdev->request_queue);
 		}
 	}
 

Re: [PATCH v2] SCSI and block: Simplify resume handling

[Bart Van Assche]

On 2020-07-01 11:37, Alan Stern wrote:
>  void blk_post_runtime_resume(struct request_queue *q, int err)
>  {
> -	if (!q->dev)
> -		return;
> -
> -	spin_lock_irq(&q->queue_lock);
>  	if (!err) {
> -		q->rpm_status = RPM_ACTIVE;
> -		pm_runtime_mark_last_busy(q->dev);
> -		pm_request_autosuspend(q->dev);
> -	} else {
> +		blk_set_runtime_active(q);
> +	} else if (q->dev) {
> +		spin_lock_irq(&q->queue_lock);
>  		q->rpm_status = RPM_SUSPENDED;
> +		spin_unlock_irq(&q->queue_lock);
>  	}
> -	spin_unlock_irq(&q->queue_lock);
> -
> -	if (!err)
> -		blk_clear_pm_only(q);
>  }
>  EXPORT_SYMBOL(blk_post_runtime_resume);

I'd like to keep the if (!q->dev) check at the start of the function instead
of moving it to the middle of the function to keep the symmetry with the
existing runtime power management functions in the same source file.

>  void blk_set_runtime_active(struct request_queue *q)
>  {
>  	if (q->dev) {
> +		int old_status;
> +
>  		spin_lock_irq(&q->queue_lock);
> +		old_status = q->rpm_status;
>  		q->rpm_status = RPM_ACTIVE;
>  		pm_runtime_mark_last_busy(q->dev);
>  		pm_request_autosuspend(q->dev);
>  		spin_unlock_irq(&q->queue_lock);
> +
> +		if (old_status != RPM_ACTIVE)
> +			blk_clear_pm_only(q);
>  	}
>  }

Since this function is being modified, please change the if (q->dev) into
if (!q->dev) return since returning early is the recommended kernel coding
style.

Thanks,

Bart.

[PATCH v3] SCSI and block: Simplify resume handling

[Alan Stern]

Commit 05d18ae1cc8a ("scsi: pm: Balance pm_only counter of request
queue during system resume") fixed a problem in the block layer's
runtime-PM code: blk_set_runtime_active() failed to call
blk_clear_pm_only().  However, the commit's implementation was
awkward; it forced the SCSI system-resume handler to choose whether to
call blk_post_runtime_resume() or blk_set_runtime_active(), depending
on whether or not the SCSI device had previously been runtime
suspended.

This patch simplifies the situation considerably by adding the missing
function call directly into blk_set_runtime_active() (under the
condition that the queue is not already in the RPM_ACTIVE state).
This allows the SCSI routine to revert back to its original form.
Furthermore, making this change reveals that blk_post_runtime_resume()
(in its success pathway) does exactly the same thing as
blk_set_runtime_active().  The duplicate code is easily removed by
making one routine call the other.

No functional changes are intended.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
CC: Can Guo <cang@codeaurora.org>
CC: Bart Van Assche <bvanassche@acm.org>

---

v3:	Arrange the code in blk_pm_post_runtime_resume() and
	blk_set_runtime_active() so that the "if (!q->dev)" tests and
	early returns come first, as in the other nearby functions.

v2:	Don't call blk_clear_pm_only() if the queue's RPM status was
	already set to RPM_ACTIVE.  This happens during a system resume
	if the device was not in runtime suspend beforehand.


[as1939c]


 block/blk-pm.c         |   41 ++++++++++++++++++++++-------------------
 drivers/scsi/scsi_pm.c |   10 ++--------
 2 files changed, 24 insertions(+), 27 deletions(-)

Index: usb-devel/block/blk-pm.c
===================================================================
--- usb-devel.orig/block/blk-pm.c
+++ usb-devel/block/blk-pm.c
@@ -164,9 +164,8 @@ EXPORT_SYMBOL(blk_pre_runtime_resume);
  *
  * Description:
  *    Update the queue's runtime status according to the return value of the
- *    device's runtime_resume function. If it is successfully resumed, process
- *    the requests that are queued into the device's queue when it is resuming
- *    and then mark last busy and initiate autosuspend for it.
+ *    device's runtime_resume function. If the resume was successful, call
+ *    blk_set_runtime_active() to do the real work of restarting the queue.
  *
  *    This function should be called near the end of the device's
  *    runtime_resume callback.
@@ -175,19 +174,13 @@ void blk_post_runtime_resume(struct requ
 {
 	if (!q->dev)
 		return;
-
-	spin_lock_irq(&q->queue_lock);
 	if (!err) {
-		q->rpm_status = RPM_ACTIVE;
-		pm_runtime_mark_last_busy(q->dev);
-		pm_request_autosuspend(q->dev);
+		blk_set_runtime_active(q);
 	} else {
+		spin_lock_irq(&q->queue_lock);
 		q->rpm_status = RPM_SUSPENDED;
+		spin_unlock_irq(&q->queue_lock);
 	}
-	spin_unlock_irq(&q->queue_lock);
-
-	if (!err)
-		blk_clear_pm_only(q);
 }
 EXPORT_SYMBOL(blk_post_runtime_resume);
 
@@ -204,15 +197,25 @@ EXPORT_SYMBOL(blk_post_runtime_resume);
  * This function can be used in driver's resume hook to correct queue
  * runtime PM status and re-enable peeking requests from the queue. It
  * should be called before first request is added to the queue.
+ *
+ * This function is also called by blk_post_runtime_resume() for successful
+ * runtime resumes.  It does everything necessary to restart the queue.
  */
 void blk_set_runtime_active(struct request_queue *q)
 {
-	if (q->dev) {
-		spin_lock_irq(&q->queue_lock);
-		q->rpm_status = RPM_ACTIVE;
-		pm_runtime_mark_last_busy(q->dev);
-		pm_request_autosuspend(q->dev);
-		spin_unlock_irq(&q->queue_lock);
-	}
+	int old_status;
+
+	if (!q->dev)
+		return;
+
+	spin_lock_irq(&q->queue_lock);
+	old_status = q->rpm_status;
+	q->rpm_status = RPM_ACTIVE;
+	pm_runtime_mark_last_busy(q->dev);
+	pm_request_autosuspend(q->dev);
+	spin_unlock_irq(&q->queue_lock);
+
+	if (old_status != RPM_ACTIVE)
+		blk_clear_pm_only(q);
 }
 EXPORT_SYMBOL(blk_set_runtime_active);
Index: usb-devel/drivers/scsi/scsi_pm.c
===================================================================
--- usb-devel.orig/drivers/scsi/scsi_pm.c
+++ usb-devel/drivers/scsi/scsi_pm.c
@@ -80,10 +80,6 @@ static int scsi_dev_type_resume(struct d
 	dev_dbg(dev, "scsi resume: %d\n", err);
 
 	if (err == 0) {
-		bool was_runtime_suspended;
-
-		was_runtime_suspended = pm_runtime_suspended(dev);
-
 		pm_runtime_disable(dev);
 		err = pm_runtime_set_active(dev);
 		pm_runtime_enable(dev);
@@ -97,10 +93,8 @@ static int scsi_dev_type_resume(struct d
 		 */
 		if (!err && scsi_is_sdev_device(dev)) {
 			struct scsi_device *sdev = to_scsi_device(dev);
-			if (was_runtime_suspended)
-				blk_post_runtime_resume(sdev->request_queue, 0);
-			else
-				blk_set_runtime_active(sdev->request_queue);
+
+			blk_set_runtime_active(sdev->request_queue);
 		}
 	}
 

Re: [PATCH v3] SCSI and block: Simplify resume handling

[Bart Van Assche]

On 2020-07-06 08:14, Alan Stern wrote:
> Commit 05d18ae1cc8a ("scsi: pm: Balance pm_only counter of request
> queue during system resume") fixed a problem in the block layer's
> runtime-PM code: blk_set_runtime_active() failed to call
> blk_clear_pm_only().  However, the commit's implementation was
> awkward; it forced the SCSI system-resume handler to choose whether to
> call blk_post_runtime_resume() or blk_set_runtime_active(), depending
> on whether or not the SCSI device had previously been runtime
> suspended.
> 
> This patch simplifies the situation considerably by adding the missing
> function call directly into blk_set_runtime_active() (under the
> condition that the queue is not already in the RPM_ACTIVE state).
> This allows the SCSI routine to revert back to its original form.
> Furthermore, making this change reveals that blk_post_runtime_resume()
> (in its success pathway) does exactly the same thing as
> blk_set_runtime_active().  The duplicate code is easily removed by
> making one routine call the other.
> 
> No functional changes are intended.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> CC: Can Guo <cang@codeaurora.org>
> CC: Bart Van Assche <bvanassche@acm.org>

This patch looks good to me. Can, since this patch modifies code in
which you recently fixed a bug, would it be possible to test this patch?

Thanks,

Bart.

Re: [PATCH v3] SCSI and block: Simplify resume handling

[Martin K. Petersen]

> This patch looks good to me. Can, since this patch modifies code in
> which you recently fixed a bug, would it be possible to test this
> patch?

Can: Please verify Alan's patch:

	https://patchwork.kernel.org/patch/11646029/
        
Bart: A proper review tag would be appreciated.

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v3] SCSI and block: Simplify resume handling

[Bart Van Assche]

On 2020-07-06 08:14, Alan Stern wrote:
> Commit 05d18ae1cc8a ("scsi: pm: Balance pm_only counter of request
> queue during system resume") fixed a problem in the block layer's
> runtime-PM code: blk_set_runtime_active() failed to call
> blk_clear_pm_only().  However, the commit's implementation was
> awkward; it forced the SCSI system-resume handler to choose whether to
> call blk_post_runtime_resume() or blk_set_runtime_active(), depending
> on whether or not the SCSI device had previously been runtime
> suspended.
> 
> This patch simplifies the situation considerably by adding the missing
> function call directly into blk_set_runtime_active() (under the
> condition that the queue is not already in the RPM_ACTIVE state).
> This allows the SCSI routine to revert back to its original form.
> Furthermore, making this change reveals that blk_post_runtime_resume()
> (in its success pathway) does exactly the same thing as
> blk_set_runtime_active().  The duplicate code is easily removed by
> making one routine call the other.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH v3] SCSI and block: Simplify resume handling

[Martin K. Petersen]

On Mon, 6 Jul 2020 11:14:36 -0400, Alan Stern wrote:

> Commit 05d18ae1cc8a ("scsi: pm: Balance pm_only counter of request
> queue during system resume") fixed a problem in the block layer's
> runtime-PM code: blk_set_runtime_active() failed to call
> blk_clear_pm_only().  However, the commit's implementation was
> awkward; it forced the SCSI system-resume handler to choose whether to
> call blk_post_runtime_resume() or blk_set_runtime_active(), depending
> on whether or not the SCSI device had previously been runtime
> suspended.
> 
> [...]

Applied to 5.9/scsi-queue, thanks!

[1/1] scsi: block: pm: Simplify resume handling
      https://git.kernel.org/mkp/scsi/c/8f38f8e0a30e

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2] SCSI and block: Simplify resume handling

[Can Guo]

Hi Alan,

On 2020-07-02 02:37, Alan Stern wrote:
> Commit 05d18ae1cc8a ("scsi: pm: Balance pm_only counter of request
> queue during system resume") fixed a problem in the block layer's
> runtime-PM code: blk_set_runtime_active() failed to call
> blk_clear_pm_only().  However, the commit's implementation was
> awkward; it forced the SCSI system-resume handler to choose whether to
> call blk_post_runtime_resume() or blk_set_runtime_active(), depending
> on whether or not the SCSI device had previously been runtime
> suspended.
> 

Yeah, even I thoght it was awkward as I couldn't find a way or a 
existing
API to fix it subtly without changing block layer. I was trying not to
make noise to block layer as I am not an expert of it, so I chose the 
akward
way to keep the fix within driver layer. Anyways, thanks for the change 
and
I will come back after I test it.

Can Guo.

> This patch simplifies the situation considerably by adding the missing
> function call directly into blk_set_runtime_active() (under the
> condition that the queue is not already in the RPM_ACTIVE state).
> This allows the SCSI routine to revert back to its original form.
> Furthermore, making this change reveals that blk_post_runtime_resume()
> (in its success pathway) does exactly the same thing as
> blk_set_runtime_active().  The duplicate code is easily removed by
> making one routine call the other.
> 
> No functional changes are intended.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> CC: Can Guo <cang@codeaurora.org>
> CC: Bart Van Assche <bvanassche@acm.org>
> 
> ---
> 
> v2:	Don't call blk_clear_pm_only() if the queue's RPM status was
> 	already set to RPM_ACTIVE.  This happens during a system resume
> 	if the device was not in runtime suspend beforehand.
> 
> Martin:
> 
> Since you merged the oritinal 05d18ae1cc8a commit, I'm submitting this 
> to
> you as an update.  If you would prefer to have it go by way of the
> block-layer tree, let me know and I'll resend it.
> 
> 
> [as1939b]
> 
> 
>  block/blk-pm.c         |   30 +++++++++++++++---------------
>  drivers/scsi/scsi_pm.c |   10 ++--------
>  2 files changed, 17 insertions(+), 23 deletions(-)
> 
> Index: usb-devel/block/blk-pm.c
> ===================================================================
> --- usb-devel.orig/block/blk-pm.c
> +++ usb-devel/block/blk-pm.c
> @@ -164,30 +164,21 @@ EXPORT_SYMBOL(blk_pre_runtime_resume);
>   *
>   * Description:
>   *    Update the queue's runtime status according to the return value 
> of the
> - *    device's runtime_resume function. If it is successfully resumed, 
> process
> - *    the requests that are queued into the device's queue when it is 
> resuming
> - *    and then mark last busy and initiate autosuspend for it.
> + *    device's runtime_resume function. If the resume was successful, 
> call
> + *    blk_set_runtime_active() to do the real work of restarting the 
> queue.
>   *
>   *    This function should be called near the end of the device's
>   *    runtime_resume callback.
>   */
>  void blk_post_runtime_resume(struct request_queue *q, int err)
>  {
> -	if (!q->dev)
> -		return;
> -
> -	spin_lock_irq(&q->queue_lock);
>  	if (!err) {
> -		q->rpm_status = RPM_ACTIVE;
> -		pm_runtime_mark_last_busy(q->dev);
> -		pm_request_autosuspend(q->dev);
> -	} else {
> +		blk_set_runtime_active(q);
> +	} else if (q->dev) {
> +		spin_lock_irq(&q->queue_lock);
>  		q->rpm_status = RPM_SUSPENDED;
> +		spin_unlock_irq(&q->queue_lock);
>  	}
> -	spin_unlock_irq(&q->queue_lock);
> -
> -	if (!err)
> -		blk_clear_pm_only(q);
>  }
>  EXPORT_SYMBOL(blk_post_runtime_resume);
> 
> @@ -204,15 +195,24 @@ EXPORT_SYMBOL(blk_post_runtime_resume);
>   * This function can be used in driver's resume hook to correct queue
>   * runtime PM status and re-enable peeking requests from the queue. It
>   * should be called before first request is added to the queue.
> + *
> + * This function is also called by blk_post_runtime_resume() for 
> successful
> + * runtime resumes.  It does everything necessary to restart the 
> queue.
>   */
>  void blk_set_runtime_active(struct request_queue *q)
>  {
>  	if (q->dev) {
> +		int old_status;
> +
>  		spin_lock_irq(&q->queue_lock);
> +		old_status = q->rpm_status;
>  		q->rpm_status = RPM_ACTIVE;
>  		pm_runtime_mark_last_busy(q->dev);
>  		pm_request_autosuspend(q->dev);
>  		spin_unlock_irq(&q->queue_lock);
> +
> +		if (old_status != RPM_ACTIVE)
> +			blk_clear_pm_only(q);
>  	}
>  }
>  EXPORT_SYMBOL(blk_set_runtime_active);
> Index: usb-devel/drivers/scsi/scsi_pm.c
> ===================================================================
> --- usb-devel.orig/drivers/scsi/scsi_pm.c
> +++ usb-devel/drivers/scsi/scsi_pm.c
> @@ -80,10 +80,6 @@ static int scsi_dev_type_resume(struct d
>  	dev_dbg(dev, "scsi resume: %d\n", err);
> 
>  	if (err == 0) {
> -		bool was_runtime_suspended;
> -
> -		was_runtime_suspended = pm_runtime_suspended(dev);
> -
>  		pm_runtime_disable(dev);
>  		err = pm_runtime_set_active(dev);
>  		pm_runtime_enable(dev);
> @@ -97,10 +93,8 @@ static int scsi_dev_type_resume(struct d
>  		 */
>  		if (!err && scsi_is_sdev_device(dev)) {
>  			struct scsi_device *sdev = to_scsi_device(dev);
> -			if (was_runtime_suspended)
> -				blk_post_runtime_resume(sdev->request_queue, 0);
> -			else
> -				blk_set_runtime_active(sdev->request_queue);
> +
> +			blk_set_runtime_active(sdev->request_queue);
>  		}
>  	}