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@xxxxxxxxxxxxxxxxxxx>
> > Cc: Douglas Gilbert <dgilbert@xxxxxxxxxxxx>
> > Fixes: bc4f24014de5 ("[SCSI] implement runtime Power Management")
> > Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxx>
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
