Re: [PATCH] scsi: sd: add runtime pm to open / release

[Martin Kepplinger]

On 24.06.20 15:33, Bart Van Assche wrote:
> On 2020-06-23 04:10, Martin Kepplinger wrote:
>> This add a very conservative but simple implementation for runtime PM
>> to the sd scsi driver:
>> Resume when opened (mounted) and suspend when released (unmounted).
>>
>> Improvements that allow suspending while a device is "open" can
>> be added later, but now we save power when no filesystem is mounted
>> and runtime PM is enabled.
>>
>> Signed-off-by: Martin Kepplinger <martin.kepplinger@xxxxxxx>
>> ---
>>  drivers/scsi/sd.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index d90fefffe31b..fe4cb7c50ec1 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -1372,6 +1372,7 @@ static int sd_open(struct block_device *bdev, fmode_t mode)
>>  	SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp, "sd_open\n"));
>>  
>>  	sdev = sdkp->device;
>> +	scsi_autopm_get_device(sdev);
>>  
>>  	/*
>>  	 * If the device is in error recovery, wait until it is done.
>> @@ -1418,6 +1419,9 @@ static int sd_open(struct block_device *bdev, fmode_t mode)
>>  
>>  error_out:
>>  	scsi_disk_put(sdkp);
>> +
>> +	scsi_autopm_put_device(sdev);
>> +
>>  	return retval;	
>>  }
>>  
>> @@ -1441,6 +1445,8 @@ static void sd_release(struct gendisk *disk, fmode_t mode)
>>  
>>  	SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp, "sd_release\n"));
>>  
>> +	scsi_autopm_put_device(sdev);
>> +
>>  	if (atomic_dec_return(&sdkp->openers) == 0 && sdev->removable) {
>>  		if (scsi_block_when_processing_errors(sdev))
>>  			scsi_set_medium_removal(sdev, SCSI_REMOVAL_ALLOW);
> 
> My understanding of the above patch is that it introduces a regression,
> namely by disabling runtime suspend as long as an sd device is held open.
> 
> Bart.
> 
> 

hi Bart,

Alan says the same (on block request, the block layer should initiate a
runtime resume), so merging with the thread from
https://lore.kernel.org/linux-usb/8738e4d3-62b1-0144-107d-ff42000ed6c6@xxxxxxx/T/
now and answer to both Bart and Alan here:]

I see scsi-pm.c using the blk-pm.c API but I'm not sure how the block
layer would itself resume the scsi device (I use it via usb_storage, so
that usb_stor_resume() follows in my case but I guess that doesn't
matter here):

my understanding of "sd" is: enable runtime pm in probe(), so *allow*
the device to be suspended (if enabled by the user), but never
resume(?). Also, why isn't "autopm" used in its ioctl() implementation
(as opposed to in "sr")?

here's roughly what happens when enabling runtime PM in sysfs (again,
because sd_probe() calls autopm_put() and thus allows it:

[   27.384446] sd 0:0:0:0: scsi_runtime_suspend
[   27.432282] blk_pre_runtime_suspend
[   27.435783] sd_suspend_common
[   27.438782] blk_post_runtime_suspend
[   27.442427] scsi target0:0:0: scsi_runtime_suspend
[   27.447303] scsi host0: scsi_runtime_suspend

then I "mount /dev/sda1 /mnt" and none of the resume() functions get
called. To me it looks like the sd driver should initiate resuming, and
that's not implemented.

what am I doing wrong or overlooking? how exactly does (or should) the
block layer initiate resume here?

thanks again for your time,

                               martin