Linux-SCSI Archive on lore.kernel.org
 help / color / Atom feed
From: Martin Kepplinger <martin.kepplinger@puri.sm>
To: Bart Van Assche <bvanassche@acm.org>,
	jejb@linux.ibm.com, martin.petersen@oracle.com,
	Alan Stern <stern@rowland.harvard.edu>
Cc: linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org, kernel@puri.sm
Subject: Re: [PATCH] scsi: sd: add runtime pm to open / release
Date: Thu, 25 Jun 2020 10:16:06 +0200
Message-ID: <eccacce9-393c-ca5d-e3b3-09961340e0db@puri.sm> (raw)
In-Reply-To: <ed9ae198-4c68-f82b-04fc-2299ab16df96@acm.org>

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@puri.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

  reply index

Thread overview: 56+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-23 11:10 Martin Kepplinger
2020-06-24 13:33 ` Bart Van Assche
2020-06-25  8:16   ` Martin Kepplinger [this message]
2020-06-25 14:52     ` Alan Stern
2020-06-26  3:53     ` Bart Van Assche
2020-06-26 15:07     ` Bart Van Assche
2020-06-26 15:44       ` Alan Stern
2020-06-28  2:37         ` Bart Van Assche
2020-06-28 13:10           ` Alan Stern
2020-06-29  9:42         ` Martin Kepplinger
2020-06-29 16:15           ` Alan Stern
2020-06-29 16:56             ` Bart Van Assche
2020-06-29 17:40               ` Alan Stern
2020-06-30  3:33             ` Martin Kepplinger
2020-06-30 13:38               ` Alan Stern
2020-06-30 15:59             ` Bart Van Assche
2020-06-30 18:02               ` Alan Stern
2020-06-30 19:23                 ` Bart Van Assche
2020-06-30 19:38                   ` Alan Stern
2020-06-30 23:31                     ` Bart Van Assche
2020-07-01  0:49                       ` Alan Stern
2020-07-06 16:41                         ` Alan Stern
2020-07-28  7:02                           ` Martin Kepplinger
2020-07-28 20:02                             ` Alan Stern
2020-07-29 14:12                               ` Martin Kepplinger
2020-07-29 14:32                                 ` Alan Stern
2020-07-29 14:44                                   ` Martin K. Petersen
2020-07-29 14:56                                     ` Alan Stern
2020-07-29 14:46                                   ` James Bottomley
2020-07-29 14:53                                     ` James Bottomley
2020-07-29 15:40                                       ` Martin Kepplinger
2020-07-29 15:44                                         ` James Bottomley
2020-07-29 16:43                                           ` Martin Kepplinger
2020-07-29 18:25                                             ` Alan Stern
2020-07-29 18:29                                               ` James Bottomley
2020-07-30  8:52                                                 ` Martin Kepplinger
2020-07-30  8:54                                                   ` Martin Kepplinger
2020-07-30 15:10                                                   ` Alan Stern
2020-08-04  9:39                                                     ` Martin Kepplinger
2020-08-07  9:51                                                       ` Martin Kepplinger
2020-08-07 14:30                                                         ` Alan Stern
2020-08-08  6:59                                                           ` Martin Kepplinger
2020-08-08 15:05                                                             ` Alan Stern
2020-08-09  9:20                                                               ` Martin Kepplinger
2020-08-09 15:26                                                                 ` Alan Stern
2020-08-10 12:03                                                                   ` Martin Kepplinger
2020-08-10 14:13                                                                     ` Alan Stern
2020-08-11  7:55                                                                       ` Martin Kepplinger
2020-08-11 13:48                                                                         ` Alan Stern
2020-07-29 15:40                                       ` Alan Stern
2020-07-29 15:49                                         ` James Bottomley
2020-07-29 16:17                                           ` Alan Stern
2020-07-29 15:52                                         ` Martin Kepplinger
2020-07-29 18:10                                   ` Douglas Gilbert
2020-07-30  8:05             ` Martin Kepplinger
2020-07-30 15:14               ` Alan Stern

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=eccacce9-393c-ca5d-e3b3-09961340e0db@puri.sm \
    --to=martin.kepplinger@puri.sm \
    --cc=bvanassche@acm.org \
    --cc=jejb@linux.ibm.com \
    --cc=kernel@puri.sm \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=stern@rowland.harvard.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-SCSI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-scsi/0 linux-scsi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-scsi linux-scsi/ https://lore.kernel.org/linux-scsi \
		linux-scsi@vger.kernel.org
	public-inbox-index linux-scsi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-scsi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git