[v2] SCSI and block: Simplify resume handling
diff mbox series

Message ID 20200701183718.GA507293@rowland.harvard.edu
State Superseded
Headers show
Series
  • [v2] SCSI and block: Simplify resume handling
Related show

Commit Message

Alan Stern July 1, 2020, 6:37 p.m. UTC
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(-)

Comments

Bart Van Assche July 6, 2020, 2:41 a.m. UTC | #1
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.
Can Guo July 22, 2020, 1:46 a.m. UTC | #2
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);
>  		}
>  	}

Patch
diff mbox series

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);
 		}
 	}