[PATCH] scsi: sg: Enable runtime power management

[PATCH] scsi: sg: Enable runtime power management

[Bart Van Assche]

In 2010, runtime power management support was implemented in the SCSI core.
The description of patch "[SCSI] implement runtime Power Management"
mentions that the sg driver is skipped but not why. This patch enables
runtime power management even if an instance of the sg driver is held open.
Enabling runtime PM for the sg driver is safe because all interactions of
the sg driver with the SCSI device pass through the block layer
(blk_execute_rq_nowait()) and the block layer already supports runtime PM.

Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Fixes: bc4f24014de5 ("[SCSI] implement runtime Power Management")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/sg.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index f86be197fedd..84334ab39c81 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -307,10 +307,6 @@ sg_open(struct inode *inode, struct file *filp)
 	if (retval)
 		goto sg_put;
 
-	retval = scsi_autopm_get_device(device);
-	if (retval)
-		goto sdp_put;
-
 	/* scsi_block_when_processing_errors() may block so bypass
 	 * check if O_NONBLOCK. Permits SCSI commands to be issued
 	 * during error recovery. Tread carefully. */
@@ -318,7 +314,7 @@ sg_open(struct inode *inode, struct file *filp)
 	      scsi_block_when_processing_errors(device))) {
 		retval = -ENXIO;
 		/* we are in error recovery for this device */
-		goto error_out;
+		goto sdp_put;
 	}
 
 	mutex_lock(&sdp->open_rel_lock);
@@ -371,8 +367,6 @@ sg_open(struct inode *inode, struct file *filp)
 	}
 error_mutex_locked:
 	mutex_unlock(&sdp->open_rel_lock);
-error_out:
-	scsi_autopm_put_device(device);
 sdp_put:
 	kref_put(&sdp->d_ref, sg_device_destroy);
 	scsi_device_put(device);
@@ -392,7 +386,6 @@ sg_release(struct inode *inode, struct file *filp)
 	SCSI_LOG_TIMEOUT(3, sg_printk(KERN_INFO, sdp, "sg_release\n"));
 
 	mutex_lock(&sdp->open_rel_lock);
-	scsi_autopm_put_device(sdp->device);
 	kref_put(&sfp->f_ref, sg_remove_sfp);
 	sdp->open_cnt--;
 

Re: [PATCH] scsi: sg: Enable runtime power management

[Bart Van Assche]

On 10/30/24 3:03 PM, Bart Van Assche wrote:
> In 2010, runtime power management support was implemented in the SCSI core.
> The description of patch "[SCSI] implement runtime Power Management"
> mentions that the sg driver is skipped but not why. This patch enables
> runtime power management even if an instance of the sg driver is held open.
> Enabling runtime PM for the sg driver is safe because all interactions of
> the sg driver with the SCSI device pass through the block layer
> (blk_execute_rq_nowait()) and the block layer already supports runtime PM.
> 
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Douglas Gilbert <dgilbert@interlog.com>
> Fixes: bc4f24014de5 ("[SCSI] implement runtime Power Management")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/scsi/sg.c | 9 +--------
>   1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index f86be197fedd..84334ab39c81 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -307,10 +307,6 @@ sg_open(struct inode *inode, struct file *filp)
>   	if (retval)
>   		goto sg_put;
>   
> -	retval = scsi_autopm_get_device(device);
> -	if (retval)
> -		goto sdp_put;
> -
>   	/* scsi_block_when_processing_errors() may block so bypass
>   	 * check if O_NONBLOCK. Permits SCSI commands to be issued
>   	 * during error recovery. Tread carefully. */
> @@ -318,7 +314,7 @@ sg_open(struct inode *inode, struct file *filp)
>   	      scsi_block_when_processing_errors(device))) {
>   		retval = -ENXIO;
>   		/* we are in error recovery for this device */
> -		goto error_out;
> +		goto sdp_put;
>   	}
>   
>   	mutex_lock(&sdp->open_rel_lock);
> @@ -371,8 +367,6 @@ sg_open(struct inode *inode, struct file *filp)
>   	}
>   error_mutex_locked:
>   	mutex_unlock(&sdp->open_rel_lock);
> -error_out:
> -	scsi_autopm_put_device(device);
>   sdp_put:
>   	kref_put(&sdp->d_ref, sg_device_destroy);
>   	scsi_device_put(device);
> @@ -392,7 +386,6 @@ sg_release(struct inode *inode, struct file *filp)
>   	SCSI_LOG_TIMEOUT(3, sg_printk(KERN_INFO, sdp, "sg_release\n"));
>   
>   	mutex_lock(&sdp->open_rel_lock);
> -	scsi_autopm_put_device(sdp->device);
>   	kref_put(&sfp->f_ref, sg_remove_sfp);
>   	sdp->open_cnt--;

(replying to my own email)

Can anyone please help with reviewing this patch? This patch is
important for Android. The Android security software uses the sg driver
to communicate with the UFS RPMB so this patch is required to enable
run-time power management in Android devices with UFS storage. See also
https://android.googlesource.com/platform/system/core/+/refs/heads/main/trusty/storage/proxy/rpmb.c

Thanks,

Bart.

Re: [PATCH] scsi: sg: Enable runtime power management

[Martin K. Petersen]

Bart,

>> In 2010, runtime power management support was implemented in the SCSI
>> core. The description of patch "[SCSI] implement runtime Power
>> Management" mentions that the sg driver is skipped but not why. This
>> patch enables runtime power management even if an instance of the sg
>> driver is held open. Enabling runtime PM for the sg driver is safe
>> because all interactions of the sg driver with the SCSI device pass
>> through the block layer (blk_execute_rq_nowait()) and the block layer
>> already supports runtime PM.

Applied to 6.13/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: sg: Enable runtime power management

[Alan Stern]

On Mon, Nov 04, 2024 at 09:25:17AM -0800, Bart Van Assche wrote:
> On 10/30/24 3:03 PM, Bart Van Assche wrote:
> > In 2010, runtime power management support was implemented in the SCSI core.
> > The description of patch "[SCSI] implement runtime Power Management"
> > mentions that the sg driver is skipped but not why. This patch enables
> > runtime power management even if an instance of the sg driver is held open.
> > Enabling runtime PM for the sg driver is safe because all interactions of
> > the sg driver with the SCSI device pass through the block layer
> > (blk_execute_rq_nowait()) and the block layer already supports runtime PM.
> > 
> > Cc: Alan Stern <stern@rowland.harvard.edu>
> > Cc: Douglas Gilbert <dgilbert@interlog.com>
> > Fixes: bc4f24014de5 ("[SCSI] implement runtime Power Management")
> > Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> > ---
> >   drivers/scsi/sg.c | 9 +--------
> >   1 file changed, 1 insertion(+), 8 deletions(-)
> > 
> > diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> > index f86be197fedd..84334ab39c81 100644
> > --- a/drivers/scsi/sg.c
> > +++ b/drivers/scsi/sg.c
> > @@ -307,10 +307,6 @@ sg_open(struct inode *inode, struct file *filp)
> >   	if (retval)
> >   		goto sg_put;
> > -	retval = scsi_autopm_get_device(device);
> > -	if (retval)
> > -		goto sdp_put;
> > -
> >   	/* scsi_block_when_processing_errors() may block so bypass
> >   	 * check if O_NONBLOCK. Permits SCSI commands to be issued
> >   	 * during error recovery. Tread carefully. */
> > @@ -318,7 +314,7 @@ sg_open(struct inode *inode, struct file *filp)
> >   	      scsi_block_when_processing_errors(device))) {
> >   		retval = -ENXIO;
> >   		/* we are in error recovery for this device */
> > -		goto error_out;
> > +		goto sdp_put;
> >   	}
> >   	mutex_lock(&sdp->open_rel_lock);
> > @@ -371,8 +367,6 @@ sg_open(struct inode *inode, struct file *filp)
> >   	}
> >   error_mutex_locked:
> >   	mutex_unlock(&sdp->open_rel_lock);
> > -error_out:
> > -	scsi_autopm_put_device(device);
> >   sdp_put:
> >   	kref_put(&sdp->d_ref, sg_device_destroy);
> >   	scsi_device_put(device);
> > @@ -392,7 +386,6 @@ sg_release(struct inode *inode, struct file *filp)
> >   	SCSI_LOG_TIMEOUT(3, sg_printk(KERN_INFO, sdp, "sg_release\n"));
> >   	mutex_lock(&sdp->open_rel_lock);
> > -	scsi_autopm_put_device(sdp->device);
> >   	kref_put(&sfp->f_ref, sg_remove_sfp);
> >   	sdp->open_cnt--;
> 
> (replying to my own email)
> 
> Can anyone please help with reviewing this patch? This patch is
> important for Android. The Android security software uses the sg driver
> to communicate with the UFS RPMB so this patch is required to enable
> run-time power management in Android devices with UFS storage. See also
> https://android.googlesource.com/platform/system/core/+/refs/heads/main/trusty/storage/proxy/rpmb.c

Acked-by: Alan Stern <stern@rowland.harvard.edu>

The patch itself is innocuous.  However, it does open up the possibility 
of devices unexpectedly going into low-power mode between SG commands, 
if those commands are spaced sufficiently far apart (more than a few 
seconds).  Presumably the system default is to disallow runtime PM for 
SCSI devices, so this shouldn't be a big issue.  But it might cause 
trouble in a few cases.

Alan Stern

Re: [PATCH] scsi: sg: Enable runtime power management

[Martin K. Petersen]

On Wed, 30 Oct 2024 15:03:10 -0700, Bart Van Assche wrote:

> In 2010, runtime power management support was implemented in the SCSI core.
> The description of patch "[SCSI] implement runtime Power Management"
> mentions that the sg driver is skipped but not why. This patch enables
> runtime power management even if an instance of the sg driver is held open.
> Enabling runtime PM for the sg driver is safe because all interactions of
> the sg driver with the SCSI device pass through the block layer
> (blk_execute_rq_nowait()) and the block layer already supports runtime PM.
> 
> [...]

Applied to 6.13/scsi-queue, thanks!

[1/1] scsi: sg: Enable runtime power management
      https://git.kernel.org/mkp/scsi/c/4045de893f69

-- 
Martin K. Petersen	Oracle Linux Engineering