diff mbox

REQ_PM vs REQ_TYPE_PM_RESUME

Message ID 52CBB188.2080707@intel.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Aaron Lu Jan. 7, 2014, 7:49 a.m. UTC
On 01/06/2014 10:40 PM, Phillip Susi wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 1/6/2014 4:15 AM, Aaron Lu wrote:
>> My guess why it doesn't work for you is that, when you call 
>> blk_pre_runtime_suspend in sd_resume_work, there are requests left
>> in the queue so that call will simply fail, it's not meant to be
>> used that way.
> 
> There should be no other requests during the system resume callback.

There can be requests left when system resume callback is invoked, but
it's not always the case and depends on what you are doing before system
suspend.

> 
>> It seems you are making use of runtime PM to speed up disk resume,
>> if that is the case, I think we can simply make sure the disk's
>> block queue is put into the same state as runtime suspended and
>> then mark it as runtime suspended during system suspend phase; on
>> system resume, call
> 
> I think I tried that and it didn't work; when it is runtime suspended
> when the system suspends, it's no longer runtime suspended when the
> system resume function was called.  Hence why I'm using the

We can modify the device's system resume callback. To better illustrate
the idea, I just made two patches to do this and I did some quick tests
and didn't find anything wrong.

The two patches are here.

From: Aaron Lu <aaron.lu@intel.com>
Date: Tue, 7 Jan 2014 15:02:13 +0800
Subject: [PATCH 1/2] SCSI: pm: make use of runtime PM for SCSI device

To make system resume fast, modify SCSI PM callbacks so that if
CONFIG_PM_RUNTIME is set, during a system suspend transition, make the
disk device's status exactly the same as runtime suspended, i.e. drain
its request queue and set its request queue's status to RPM_SUSPENDED,
so that during system resume phase, instead of resuming the device
synchronously, we can relay the resume operation to runtime PM framework
by calling pm_request_resume to take advantage of the block layer
runtime PM.

The simplest way to achieve this would be to use the bus' runtime suspend
callback for system suspend callback, but for sr driver, it will refuse
to enter runtime suspend state if there is media inside. This is obviously
not acceptable for system suspend, so instead of using driver's runtime
suspend callback, we keep using driver's system suspend callback(which
is the same for sd and doesn't matter for sr). In addition to drain device's
request queue and set proper runtime status for its request queue, we will
also set the device's runtime status accordingly in its system suspend
callback.

This is part 1, we will also need to do the same thing for the disk's
host, i.e. ATA port on PCs. The next patch will handle just that.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/scsi/scsi_lib.c  | 11 +++++++++
 drivers/scsi/scsi_pm.c   | 60 +++++++++++++++++++++++++++++++++++++++---------
 drivers/scsi/scsi_priv.h |  3 +++
 3 files changed, 63 insertions(+), 11 deletions(-)

Comments

Phillip Susi Jan. 7, 2014, 2:50 p.m. UTC | #1
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 1/7/2014 2:49 AM, Aaron Lu wrote:
> We can modify the device's system resume callback. To better
> illustrate the idea, I just made two patches to do this and I did
> some quick tests and didn't find anything wrong.

That misses one key aspect I was trying to capture: it always leaves
the disk runtime suspended after a resume.  If the disk spins up on
its own, as most ata disks do, then the runtime status doesn't
correctly reflect the actual state of the disk.  This means that
applications that delay activities due to the runtime pm status to
avoid waking a disk won't run, and any runtime autosuspend won't kick
in to actually put the disk back to sleep.

To do that, you need to be able to issue a REQUEST SENSE and
conditionally resume the device.  Of course, you can't do that unless
you first get it into a transitional state, but you can't just request
a resume and then conditionally fail it.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJSzBQiAAoJEI5FoCIzSKrwH9UH/3Z3TfKKKYhNsjr26YsKOge2
EQzbcFCg4H5irnKL/olR5fnRKpZYi1Er2ydV6zsHdcCJLFQoqvzVE4ZInl42STI0
IWQ65n5q+U1OaZY+ttGkOBjixi5VIlkb9izZVbzBTi4n5cLEDwyQsE4Rgd9STwjm
gPkNrmGNGPUoY+4O6bLHu0/WvwTX3L/OgxcSsQd1gsdCX3qZzB+UIzfM31W9/4Yl
3sP/tN8mBYcpqR9jIpw2u7m0XEe9Wc71Nepdv2+6sT5uZJ5knTY6epH5Of4FJsyp
CxslFXf6Jb2FZaey8EJB1ocABnePYluQNHrpdRO4gjY+Au/mtzk15tJjoYx3WXg=
=VlKV
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Stern Jan. 7, 2014, 3:25 p.m. UTC | #2
On Tue, 7 Jan 2014, Aaron Lu wrote:

> From: Aaron Lu <aaron.lu@intel.com>
> Date: Tue, 7 Jan 2014 15:02:13 +0800
> Subject: [PATCH 1/2] SCSI: pm: make use of runtime PM for SCSI device
> 
> To make system resume fast, modify SCSI PM callbacks so that if
> CONFIG_PM_RUNTIME is set, during a system suspend transition, make the
> disk device's status exactly the same as runtime suspended, i.e. drain
> its request queue and set its request queue's status to RPM_SUSPENDED,
> so that during system resume phase, instead of resuming the device
> synchronously, we can relay the resume operation to runtime PM framework
> by calling pm_request_resume to take advantage of the block layer
> runtime PM.

This doesn't seem like a good idea.  The way to speed up resumes is to
allow sd's resume routine to return while the disk is still spinning up
(i.e., make the spin-up asynchronous).  There already have been patches
submitted to do this; I don't know what happened to them.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Phillip Susi Jan. 7, 2014, 3:43 p.m. UTC | #3
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 1/7/2014 10:25 AM, Alan Stern wrote:
> This doesn't seem like a good idea.  The way to speed up resumes is
> to allow sd's resume routine to return while the disk is still
> spinning up (i.e., make the spin-up asynchronous).  There already
> have been patches submitted to do this; I don't know what happened
> to them.

Sure, if that is your *only* goal.  I also want the disk to not spin
up *at all* if possible.  There's no sense spinning up all of your
disks every time you resume when you very rarely access some of them.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJSzCCgAAoJEI5FoCIzSKrwkMAH/3Fs/1tdkCUNtj32PjvD/C2O
qgJwkXKtJmThL9+8NK5lNLxWt+buJ9HBozoeKs2AkBIsRpxdXZb6FnutQHhOSIBQ
vqgwKoGvKQUc7bj0sg2WUVckYkZcl2hs54PIZgaAj8VwcJZE7XTNo+BeGlyslyi+
isG+BvGInBkhJ7BsR0nC05Sytrb6F6xYkOkV5hn2PjvPNhxpw9dKlVLGfO9qNFcJ
irazJwVQ2k49y7IGHS+K0NLGv45pBEPRcirwTaxfH0IIVCmDPeQk86SmpmeS+TSO
o3tFgUC7JQjeH2yg0YXkIEGZGGycv57H7Gf+hAOL9mOOD3CD0D4TagbMZP2SOIU=
=gM81
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aaron Lu Jan. 8, 2014, 1:03 a.m. UTC | #4
On 01/07/2014 10:50 PM, Phillip Susi wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 1/7/2014 2:49 AM, Aaron Lu wrote:
>> We can modify the device's system resume callback. To better
>> illustrate the idea, I just made two patches to do this and I did
>> some quick tests and didn't find anything wrong.
> 
> That misses one key aspect I was trying to capture: it always leaves
> the disk runtime suspended after a resume.  If the disk spins up on

You mean you want to leave the disk runtime suspended after a system
resume and in the meantime make sure the disk is indeed not spun up?

-Aaron

> its own, as most ata disks do, then the runtime status doesn't
> correctly reflect the actual state of the disk.  This means that
> applications that delay activities due to the runtime pm status to
> avoid waking a disk won't run, and any runtime autosuspend won't kick
> in to actually put the disk back to sleep.
> 
> To do that, you need to be able to issue a REQUEST SENSE and
> conditionally resume the device.  Of course, you can't do that unless
> you first get it into a transitional state, but you can't just request
> a resume and then conditionally fail it.
> 
> 
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.17 (MingW32)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
> 
> iQEcBAEBAgAGBQJSzBQiAAoJEI5FoCIzSKrwH9UH/3Z3TfKKKYhNsjr26YsKOge2
> EQzbcFCg4H5irnKL/olR5fnRKpZYi1Er2ydV6zsHdcCJLFQoqvzVE4ZInl42STI0
> IWQ65n5q+U1OaZY+ttGkOBjixi5VIlkb9izZVbzBTi4n5cLEDwyQsE4Rgd9STwjm
> gPkNrmGNGPUoY+4O6bLHu0/WvwTX3L/OgxcSsQd1gsdCX3qZzB+UIzfM31W9/4Yl
> 3sP/tN8mBYcpqR9jIpw2u7m0XEe9Wc71Nepdv2+6sT5uZJ5knTY6epH5Of4FJsyp
> CxslFXf6Jb2FZaey8EJB1ocABnePYluQNHrpdRO4gjY+Au/mtzk15tJjoYx3WXg=
> =VlKV
> -----END PGP SIGNATURE-----
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Phillip Susi Jan. 8, 2014, 1:16 a.m. UTC | #5
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 01/07/2014 08:03 PM, Aaron Lu wrote:
> You mean you want to leave the disk runtime suspended after a
> system resume and in the meantime make sure the disk is indeed not
> spun up?

Yep.  If it is spun up, then the runtime status should be updated to
reflect that, otherwise it tricks user space programs into avoiding
doing IO to the disk for fear of waking it, and prevents the runtime
autosuspend timer from kicking in.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBCgAGBQJSzKbjAAoJEI5FoCIzSKrwhEsH/3WXEJm2+uPR2zIeGVvZ/I2X
yu0KpcMto0Ox/v2htII/SjQsmhAPbyphTcbJeZJTmbA5EZmNixZXrZAS1vy3vK06
n2Azmywuf0mihfZV1ORg1GuwjgqSSW9MNRw+IuJ7NkxtOZWvWFRNvpcVdyfcig6d
26CP0drDH/j9VL5L1UFWensESswAWV9l1Ht45FUemQw5QDQK6hwUpOAfhbnurz06
OCOs4QRC+O/zWyxzMVKQtQnCEd5tRnKQq0K6d2ZmCKe368KNUrKZ+sk3SPabYtjI
3CEv1+ytG8VajzxJ0cj4TuePK2O+lcd0TWddARJqxZg7o/JYWAyN4yMFoPsAUPo=
=oDcD
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aaron Lu Jan. 8, 2014, 1:32 a.m. UTC | #6
On 01/08/2014 09:16 AM, Phillip Susi wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
> 
> On 01/07/2014 08:03 PM, Aaron Lu wrote:
>> You mean you want to leave the disk runtime suspended after a
>> system resume and in the meantime make sure the disk is indeed not
>> spun up?
> 
> Yep.  If it is spun up, then the runtime status should be updated to
> reflect that, otherwise it tricks user space programs into avoiding
> doing IO to the disk for fear of waking it, and prevents the runtime
> autosuspend timer from kicking in.

The ATA and SCSI devices are all resumed in my patches, notice there is
a single pm_request_resume call in both ATA and SCSI's system resume
callback, so the runtime status and the disk's state is synced.
The pm_request_resume call is asynchronous to the system resume, so it
doesn't block system resume.

But I see your point, my patch will not achieve that, it can only speed
up S3 for a typical PC with a traditional disk. I can omit the
pm_request_resume call in the system resume callback, but then if the
disk is spun up by itself, then the runtime status indeed doesn't
reflect the actual state. I suppose for SATA controllers that support
Staggered Spin-up wouldn't do this?
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Phillip Susi Jan. 8, 2014, 1:53 a.m. UTC | #7
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 01/07/2014 08:32 PM, Aaron Lu wrote:
> The ATA and SCSI devices are all resumed in my patches, notice
> there is a single pm_request_resume call in both ATA and SCSI's
> system resume callback, so the runtime status and the disk's state
> is synced. The pm_request_resume call is asynchronous to the system
> resume, so it doesn't block system resume.
> 
> But I see your point, my patch will not achieve that, it can only
> speed up S3 for a typical PC with a traditional disk. I can omit
> the pm_request_resume call in the system resume callback, but then
> if the disk is spun up by itself, then the runtime status indeed
> doesn't reflect the actual state. I suppose for SATA controllers
> that support Staggered Spin-up wouldn't do this?

Ahh, yes, the point of my patches was to avoid waking a disk at all if
possible, and avoid blocking on it otherwise.  Todd Brandt's patches
just backgrounded the resume.

As far as I can tell, the AHCI staggered spinup feature is only a hint
to the libata driver that it should not probe all disks in parallel.
The way to get an ATA disk to not spin itself up is by enabling the
Power on in Standby feature, either through hdparm, or via a jumper,
and it seems WD drives only support the jumper method.  Once enabled,
a drive may chose to automatically spin up when given a command that
requires it to be spinning, or it can opt to require an explicit SET
FEATURES command to spin up.  libata issues an IDENTIFY DEVICE on
resume to find out of the drive requires this command, and issues it
if so.  One of the other patches in my set fixes libata to avoid doing
this in the suspend path, and defer it to the first time a command is
issued.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBCgAGBQJSzK+MAAoJEI5FoCIzSKrwE5gH/3zpkOR1u+W1/kw3GFLO1YHH
kA2g9VlBMoisiUGLltAuvZYN8zALhWvH3QrTIAvAxq/DjlRQ5ZyBSi3g56swsrHg
ILdx3XW9wuPLSxpWLaiZ/sowTvmrWKSYbyUpxdkDJizCXkg5R3J4LuQ3OpLSSLRh
a6IYMas6l74+xq3wp/eHTE7ofAeoN/jJmT4slUFbzgILMKKEZJQ3wLdjM2uy1d2l
ip3anDOKXHqjrTW4QSkj8piMpR4LBsEpWpMPW9fjYhQe54Hpqv4hwn6vuXEg9SKu
TrwjiH2qb4Ro9twQMUrfF2/r4Ov9swPI1r4EL/bvJ7lJSJ+9c5fRIvObg5Hdaa8=
=woDI
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aaron Lu Jan. 8, 2014, 2:11 a.m. UTC | #8
On 01/08/2014 09:53 AM, Phillip Susi wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
> 
> On 01/07/2014 08:32 PM, Aaron Lu wrote:
>> The ATA and SCSI devices are all resumed in my patches, notice
>> there is a single pm_request_resume call in both ATA and SCSI's
>> system resume callback, so the runtime status and the disk's state
>> is synced. The pm_request_resume call is asynchronous to the system
>> resume, so it doesn't block system resume.
>>
>> But I see your point, my patch will not achieve that, it can only
>> speed up S3 for a typical PC with a traditional disk. I can omit
>> the pm_request_resume call in the system resume callback, but then
>> if the disk is spun up by itself, then the runtime status indeed
>> doesn't reflect the actual state. I suppose for SATA controllers
>> that support Staggered Spin-up wouldn't do this?
> 
> Ahh, yes, the point of my patches was to avoid waking a disk at all if
> possible, and avoid blocking on it otherwise.  Todd Brandt's patches
> just backgrounded the resume.
> 
> As far as I can tell, the AHCI staggered spinup feature is only a hint
> to the libata driver that it should not probe all disks in parallel.

I thought that feature is used to control if a disk should be spun up
once powered from the host side.

> The way to get an ATA disk to not spin itself up is by enabling the
> Power on in Standby feature, either through hdparm, or via a jumper,

Too bad for a jumper, that's beyond our control. And about the hdparm,
does the setting survive a power cycle?
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Phillip Susi Jan. 8, 2014, 2:19 a.m. UTC | #9
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 01/07/2014 09:11 PM, Aaron Lu wrote:
> I thought that feature is used to control if a disk should be spun
> up once powered from the host side.

That *is* what it sounds like, only ATA disks either spin up as soon
as power is applied, or wait for an actual command issued by higher
layers.  There doesn't seem to be any low level SATA mechanism to spin
the drive up that the SSS feature could toggle, so it seems to simply
be a hint to libata, and possibly the bios.

> Too bad for a jumper, that's beyond our control. And about the
> hdparm, does the setting survive a power cycle?

Yes, otherwise there wouldn't be much point to a setting that only
matters at power on ;)


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBCgAGBQJSzLXEAAoJEI5FoCIzSKrwhVwH/2amZwCu1WIFiPswU2ydIZhR
lGL0+0C9hKb4O+FEahkLMYsX/bouxtUs3HOcAJOlmmAFZC5yhczIBijcuIKjZyW5
HAv4Sfw1xIjC6N5pMtNnnG8SUORDXy4cbMDS4+poC0aKIWw47GMNin/Uqav4Wn90
3U9nbUm7sBZ6zSLpDkenB5kZKUYmMaziwEb1IwBvxrRdnS3X5B9L3WuuCjheQoF8
YhSFdc/0OGdRRiY/oaZwizRV7bJO2sg99qRoSVIa8ZnL5v9HzT0MCVq9aUBXNLUI
eTyv0O1rkgW5lFk+4aIhJUPSb52F4lNwBmNVINIzLCehpy/oT1DVZN3sL+ykBeI=
=DJOH
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aaron Lu Jan. 8, 2014, 2:36 a.m. UTC | #10
On 01/08/2014 10:19 AM, Phillip Susi wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
> 
> On 01/07/2014 09:11 PM, Aaron Lu wrote:
>> I thought that feature is used to control if a disk should be spun
>> up once powered from the host side.
> 
> That *is* what it sounds like, only ATA disks either spin up as soon
> as power is applied, or wait for an actual command issued by higher
> layers.  There doesn't seem to be any low level SATA mechanism to spin
> the drive up that the SSS feature could toggle, so it seems to simply
> be a hint to libata, and possibly the bios.

I think the host controller can decide not to power all its ports, only
when a specific reg in the port's memory range is set, it will give
power to that port and thus spin up the drive. Makes sense?

> 
>> Too bad for a jumper, that's beyond our control. And about the
>> hdparm, does the setting survive a power cycle?
> 
> Yes, otherwise there wouldn't be much point to a setting that only
> matters at power on ;)

Oh, of course, my stupid :-)
Then I suddenly think my patches can kind of work - let's say we have
done the hdparm setting thing before suspend and the disk will be spun
up in standby mode next time it is powered. Then during system resume
phase, remove the pm_request_resume call in both SCSI and ATA's system
resume callback,
  - if the disk is powered, it will be in standby mode and its runtime
    status is RPM_SUSPENDED, match its real status(sort of);
  - if the disk is not powered due to some host feature or whatever, it
    will be in unpowered mode and its runtime status is RPM_SUSPENDED,
    still match its real status.

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Phillip Susi Jan. 8, 2014, 5:24 a.m. UTC | #11
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 01/07/2014 09:36 PM, Aaron Lu wrote:
> I think the host controller can decide not to power all its ports,
> only when a specific reg in the port's memory range is set, it will
> give power to that port and thus spin up the drive. Makes sense?

The port doesn't supply power to the drive, it gets it straight from
the system PSU.

> Oh, of course, my stupid :-) Then I suddenly think my patches can
> kind of work - let's say we have done the hdparm setting thing
> before suspend and the disk will be spun up in standby mode next
> time it is powered. Then during system resume phase, remove the
> pm_request_resume call in both SCSI and ATA's system resume
> callback, - if the disk is powered, it will be in standby mode and
> its runtime status is RPM_SUSPENDED, match its real status(sort
> of); - if the disk is not powered due to some host feature or
> whatever, it will be in unpowered mode and its runtime status is
> RPM_SUSPENDED, still match its real status.

Right, but if the disk is a run of the mill ATA disk not configured
for power up in standby, then you end up with runtime pm saying that
it is suspended, when in fact, it spun up on its own and is sitting
there waiting for commands.

The PuiS setting isn't something we can or want to twiggle on our own
during suspend, that's an admin decision that they set more or less
permanently either with hdparm or the hardware jumper.  We just need
to detect what the drive has done and update the runtime pm status to
match.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBCgAGBQJSzOEaAAoJEI5FoCIzSKrwIVoH/1sszQsK1buyg9hDvemd84m6
EMkMUsab4qYZlxGcQpnUUlJbQpGKnhDXjxBstjD8zfnC6WQfOCySTqkqBzZqEXzE
QEt5IV7mWn43tGbu4pyYlw4SrEOmOmmYJxl5yh033MAPNsP/rhToXZoEPOTRCro4
GdkZpxx0A9Y/rnzLN29RoFw41T5G4aG0O7FyTuZGPW/uWhhdUqxpUQt7ACCD+fdD
GaHWf2WInU7vSrDcg6daxvarqQ8GJavc1rafM45EkGMCzGwRhvIR+PCBk8E9t1qA
eB/1b9q8DiBJVCiMxcZVOLY8PY0bm1eBRRqhMef0l7Ppvl8N23f84o7tcN57lWY=
=RCPv
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aaron Lu Jan. 8, 2014, 7 a.m. UTC | #12
On 01/08/2014 01:24 PM, Phillip Susi wrote:
> On 01/07/2014 09:36 PM, Aaron Lu wrote:
>> Oh, of course, my stupid :-) Then I suddenly think my patches can
>> kind of work - let's say we have done the hdparm setting thing
>> before suspend and the disk will be spun up in standby mode next
>> time it is powered. Then during system resume phase, remove the
>> pm_request_resume call in both SCSI and ATA's system resume
>> callback, - if the disk is powered, it will be in standby mode and
>> its runtime status is RPM_SUSPENDED, match its real status(sort
>> of); - if the disk is not powered due to some host feature or
>> whatever, it will be in unpowered mode and its runtime status is
>> RPM_SUSPENDED, still match its real status.
> 
> Right, but if the disk is a run of the mill ATA disk not configured
> for power up in standby, then you end up with runtime pm saying that
> it is suspended, when in fact, it spun up on its own and is sitting
> there waiting for commands.
> 
> The PuiS setting isn't something we can or want to twiggle on our own
> during suspend, that's an admin decision that they set more or less
> permanently either with hdparm or the hardware jumper.  We just need

OK, I see.

> to detect what the drive has done and update the runtime pm status to
> match.

Then I would say we just keep the pm_request_resume call in their system
resume callback. For drives that have that cool feature turned on, it
will be in runtime active state while it's actually in standby mode, not
a big deal since it should go to runtime suspended state once the
autosuspend timer is due(the runtime PM core will schedule an idle call
for newly resumed device); For drives that don't have that feature turned
on, it will be in runtime active state while it's actually spun up, no
problem here.

For the PuiS set drive, there is a time window the drive's power mode
doesn't agree with its runtime status, the window is the length of the
autosuspend time interval. Not a big deal I suppose, considering that
time interval isn't eternal? And I wouldn't say the runtime status for
the drive is wrong even in this case, since the runtime status is a
reflection of whether the device is in use or not from kernel's view,
instead of whether it is in a low power mode. But I agree we better
align them.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Phillip Susi Jan. 8, 2014, 7:30 p.m. UTC | #13
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 1/8/2014 2:00 AM, Aaron Lu wrote:
> Then I would say we just keep the pm_request_resume call in their
> system resume callback. For drives that have that cool feature
> turned on, it will be in runtime active state while it's actually
> in standby mode, not

No, it won't since the runtime resume starts the drive.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJSzadoAAoJEI5FoCIzSKrwQWsIAKt/F+eiYYrxxjx58hFCwLxV
3E+CDiuf+pTyUUqPFgcGjyNrfoBO2nWXiCyg5XZ//pjPwbD4GenwSajgUIvKRw/6
0mDw4bb4CyU9/vqVKwHUHfM2jgsAuK9VFn+1fSomODy6B9ZYX6pDPr3jw5S7kDko
nDaYhBX9L2/AE4oPo2qPLpAPMxiYFD9qIi8LcfW/Ha2Av7AneIyTXEvFZzyeDv2K
WoH/KF59snFz9t4wwwfdqu1l1w93b+Tcn2zCpFBaA63TyrJKq4qpx9/W+INgBGol
HKEyv1vh5UkaVSvffiFE/2b1lR4n0hL7LVMe1egazAlNPcsjXdBZv1amYJ7NltI=
=N8WO
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7bd7f0d5f050..2b490813d5ed 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2433,6 +2433,17 @@  void scsi_device_resume(struct scsi_device *sdev)
 }
 EXPORT_SYMBOL(scsi_device_resume);
 
+#ifdef CONFIG_PM_RUNTIME
+void scsi_device_drain_queue(struct scsi_device *sdev)
+{
+	scsi_run_queue(sdev->request_queue);
+	while (sdev->request_queue->nr_pending) {
+		msleep_interruptible(200);
+		scsi_run_queue(sdev->request_queue);
+	}
+}
+#endif
+
 static void
 device_quiesce_fn(struct scsi_device *sdev, void *data)
 {
diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index 001e9ceda4c3..4f3fbd91c396 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -16,6 +16,24 @@ 
 
 #include "scsi_priv.h"
 
+#ifdef CONFIG_PM_RUNTIME
+static int sdev_runtime_suspend_common(struct device *dev,
+				       int (*cb)(struct device *))
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+	int err;
+
+	err = blk_pre_runtime_suspend(sdev->request_queue);
+	if (err)
+		return err;
+	if (cb)
+		err = cb(dev);
+	blk_post_runtime_suspend(sdev->request_queue, err);
+
+	return err;
+}
+#endif
+
 #ifdef CONFIG_PM_SLEEP
 
 static int scsi_dev_type_suspend(struct device *dev, int (*cb)(struct device *))
@@ -95,6 +113,35 @@  static int scsi_bus_prepare(struct device *dev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_RUNTIME
+static int sdev_system_suspend(struct device *dev, int (*cb)(struct device *))
+{
+	scsi_device_drain_queue(to_scsi_device(dev));
+	return sdev_runtime_suspend_common(dev, cb);
+}
+
+static int scsi_bus_suspend(struct device *dev)
+{
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	int err = 0;
+
+	if (scsi_is_sdev_device(dev))
+		err = sdev_system_suspend(dev, pm ? pm->suspend : NULL);
+
+	if (!err) {
+		__pm_runtime_disable(dev, false);
+		pm_runtime_set_suspended(dev);
+		pm_runtime_enable(dev);
+	}
+
+	return err;
+}
+
+static int scsi_bus_resume(struct device *dev)
+{
+	return pm_request_resume(dev);
+}
+#else
 static int scsi_bus_suspend(struct device *dev)
 {
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
@@ -106,6 +153,7 @@  static int scsi_bus_resume(struct device *dev)
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 	return scsi_bus_resume_common(dev, pm ? pm->resume : NULL);
 }
+#endif
 
 static int scsi_bus_freeze(struct device *dev)
 {
@@ -148,17 +196,7 @@  static int scsi_bus_restore(struct device *dev)
 static int sdev_runtime_suspend(struct device *dev)
 {
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
-	struct scsi_device *sdev = to_scsi_device(dev);
-	int err;
-
-	err = blk_pre_runtime_suspend(sdev->request_queue);
-	if (err)
-		return err;
-	if (pm && pm->runtime_suspend)
-		err = pm->runtime_suspend(dev);
-	blk_post_runtime_suspend(sdev->request_queue, err);
-
-	return err;
+	return sdev_runtime_suspend_common(dev, pm->runtime_suspend);
 }
 
 static int scsi_runtime_suspend(struct device *dev)
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index f079a598bed4..fdd3a3a04eb4 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -90,6 +90,9 @@  extern void scsi_run_host_queues(struct Scsi_Host *shost);
 extern struct request_queue *scsi_alloc_queue(struct scsi_device *sdev);
 extern int scsi_init_queue(void);
 extern void scsi_exit_queue(void);
+#ifdef CONFIG_PM_RUNTIME
+extern void scsi_device_drain_queue(struct scsi_device *sdev);
+#endif
 struct request_queue;
 struct request;
 extern struct kmem_cache *scsi_sdb_cache;
-- 
1.8.4.2


From: Aaron Lu <aaron.lu@intel.com>
Date: Tue, 7 Jan 2014 15:14:09 +0800
Subject: [PATCH 2/2] ata: pm: make use of runtime PM for ata port

To realize fast resume for hard disks, the ata port's device will also
need to make use of runtime PM in its system resume callback.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/ata/libata-core.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 1393a5890ed5..4f92a7834dd1 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5396,10 +5396,18 @@  static int ata_port_suspend_common(struct device *dev, pm_message_t mesg)
 
 static int ata_port_suspend(struct device *dev)
 {
+	int err;
+
 	if (pm_runtime_suspended(dev))
 		return 0;
 
-	return ata_port_suspend_common(dev, PMSG_SUSPEND);
+	err = ata_port_suspend_common(dev, PMSG_SUSPEND);
+	if (!err) {
+		__pm_runtime_disable(dev, false);
+		pm_runtime_set_suspended(dev);
+		pm_runtime_enable(dev);
+	}
+	return err;
 }
 
 static int ata_port_do_freeze(struct device *dev)
@@ -5432,6 +5440,12 @@  static int ata_port_resume_common(struct device *dev, pm_message_t mesg)
 	return __ata_port_resume_common(ap, mesg, NULL);
 }
 
+#ifdef CONFIG_PM_RUNTIME
+static int ata_port_resume(struct device *dev)
+{
+	return pm_request_resume(dev);
+}
+#else
 static int ata_port_resume(struct device *dev)
 {
 	int rc;
@@ -5445,6 +5459,7 @@  static int ata_port_resume(struct device *dev)
 
 	return rc;
 }
+#endif
 
 /*
  * For ODDs, the upper layer will poll for media change every few seconds,