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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <eccacce9-393c-ca5d-e3b3-09961340e0db@puri.sm>
Date:   Thu, 25 Jun 2020 10:16:06 +0200
From:   Martin Kepplinger <martin.kepplinger@...i.sm>
To:     Bart Van Assche <bvanassche@....org>, jejb@...ux.ibm.com,
        martin.petersen@...cle.com, Alan Stern <stern@...land.harvard.edu>
Cc:     linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel@...i.sm
Subject: Re: [PATCH] scsi: sd: add runtime pm to open / release

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@...i.sm>
>> ---
>>  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@puri.sm/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