diff mbox series

[v6,19/23] ata: libata-core: Do not resume runtime suspended ports

Message ID 20230923002932.1082348-20-dlemoal@kernel.org
State New
Headers show
Series Fix libata suspend/resume handling and code cleanup | expand

Commit Message

Damien Le Moal Sept. 23, 2023, 12:29 a.m. UTC 
The scsi disk driver does not resume disks that have been runtime
suspended by the user. To be consistent with this behavior, do the same
for ata ports and skip the PM request in ata_port_pm_resume() if the
port was already runtime suspended. With this change, it is no longer
necessary to force the PM state of the port to ACTIVE as the PM core
code will take care of that when handling runtime resume.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Tested-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/ata/libata-core.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Phillip Susi Sept. 25, 2023, 5:26 p.m. UTC | #1 
Damien Le Moal <dlemoal@kernel.org> writes:

> The scsi disk driver does not resume disks that have been runtime
> suspended by the user. To be consistent with this behavior, do the same
> for ata ports and skip the PM request in ata_port_pm_resume() if the
> port was already runtime suspended. With this change, it is no longer
> necessary to force the PM state of the port to ACTIVE as the PM core
> code will take care of that when handling runtime resume.

The problem with this is that ATA disks normally spin up on their own
after system resume.  As a result, if the disk was put to sleep with
runtime pm before the system suspend, then after resume, the kernel will
still show that it is runtime suspended, even though it is not.  Then
the disk will keep spinning forever.

We need to check the drive on system resume to see if it is in standby
or not, and force the runtime pm state to match.  I couldn't quite work
out how to do that properly before.  I dug up my old patch series and
have been reviewing it.  If you are interested, it can be found here:

https://lore.kernel.org/all/1387236657-4852-5-git-send-email-psusi@ubuntu.com/
Damien Le Moal Sept. 26, 2023, 6:27 a.m. UTC | #2 
On 2023/09/25 19:26, Phillip Susi wrote:
> 
> Damien Le Moal <dlemoal@kernel.org> writes:
> 
>> The scsi disk driver does not resume disks that have been runtime
>> suspended by the user. To be consistent with this behavior, do the same
>> for ata ports and skip the PM request in ata_port_pm_resume() if the
>> port was already runtime suspended. With this change, it is no longer
>> necessary to force the PM state of the port to ACTIVE as the PM core
>> code will take care of that when handling runtime resume.
> 
> The problem with this is that ATA disks normally spin up on their own
> after system resume.  As a result, if the disk was put to sleep with
> runtime pm before the system suspend, then after resume, the kernel will
> still show that it is runtime suspended, even though it is not.  Then
> the disk will keep spinning forever.

I suspect you are talking about resume from hybernation here, where the drive
may have been completely powered off... Yes, in such case, the drive will
spinup, unless you have PUIS and enabled it.

> We need to check the drive on system resume to see if it is in standby
> or not, and force the runtime pm state to match.  I couldn't quite work
> out how to do that properly before.  I dug up my old patch series and
> have been reviewing it.  If you are interested, it can be found here:
> 
> https://lore.kernel.org/all/1387236657-4852-5-git-send-email-psusi@ubuntu.com/

Sure, but please do not have this delay this patch series. The problem you are
describing above exists today already. This patch series is not making it worse,
nor is it trying to solve it. And note that this issue is not just for ATA. SCSI
devices locally attached to a machine that you hybernate will end up doing the
same and spinup when power is restored...
Phillip Susi Sept. 26, 2023, 3:01 p.m. UTC | #3 
Damien Le Moal <dlemoal@kernel.org> writes:

> I suspect you are talking about resume from hybernation here, where the drive
> may have been completely powered off... Yes, in such case, the drive will
> spinup, unless you have PUIS and enabled it.

The same thing happens in suspend / S3.

> Sure, but please do not have this delay this patch series. The problem you are
> describing above exists today already. This patch series is not making it worse,
> nor is it trying to solve it. And note that this issue is not just for ATA. SCSI
> devices locally attached to a machine that you hybernate will end up doing the
> same and spinup when power is restored...

You are saying that right now, the sd driver issues a START UNIT command
on system resume ( it looks like there's a flag you can set now to prevent
that ), then leaves the runtime pm state looking like the drive is still
suspended?  I thought it handled that correctly but I don't see any code
doing so right now.

If that's the case, then I suppose this series at least does not make
things worse...
diff mbox series

Patch

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index df6ed386e6fc..58f03031a259 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5230,10 +5230,8 @@  static void ata_port_resume(struct ata_port *ap, pm_message_t mesg,
 
 static int ata_port_pm_resume(struct device *dev)
 {
-	ata_port_resume(to_ata_port(dev), PMSG_RESUME, true);
-	pm_runtime_disable(dev);
-	pm_runtime_set_active(dev);
-	pm_runtime_enable(dev);
+	if (!pm_runtime_suspended(dev))
+		ata_port_resume(to_ata_port(dev), PMSG_RESUME, true);
 	return 0;
 }