ata port runtime pm

RFC In review Lin Ming 4 participants 7 messages Oct 28, 2011 – Nov 1, 2011 libata

lore

Share & tools

lore

Lin Ming

author

2011-10-28 03:21 UTC #0 lore raw

RFC 1/1 ata port runtime pm

Hi, all

I send this out early to get feedback how to do ata port runtime pm.

There was some discussion early this year trying to add runtime pm support to sata_mv controller driver.
http://marc.info/?l=linux-ide&m=129403126729115&w=2

Here I focus on ata port runtime pm, not controller.

In below conceptual patch, I did

0. Set autosuspend delay to 3 minutes for ata port
1. Split a new function ata_port_request_pm from ata_host_request_pm
2. Add device_type "ata_port_type" which implements callbacks for runtime pm core
3. Resume port in ata_scsi_queuecmd if needed
4. Request auto suspend in ata_scsi_queuecmd

CAUTION: this patch DOES NOT work at all.
I just threw it out for discussion.

Any idea?
Thanks.

Lin Ming

---
 drivers/ata/libata-core.c |  126 +++++++++++++++++++++++++++++++++------------
 drivers/ata/libata-scsi.c |    6 ++
 2 files changed, 99 insertions(+), 33 deletions(-)

drivers/ata/libata-core.c +93 ~~-33~~

index 4a3a5ae..e0c1a15 100644
@@ -66,6 +66,7 @@
 #include <asm/byteorder.h>
 #include <linux/cdrom.h>
 #include <linux/ratelimit.h>
+#include <linux/pm_runtime.h>
 
 #include "libata.h"
 #include "libata-transport.h"

@@ -5234,51 +5235,62 @@ bool ata_link_offline(struct ata_link *link)
 }
 
 #ifdef CONFIG_PM
-static int ata_host_request_pm(struct ata_host *host, pm_message_t mesg,
+static int ata_port_request_pm(struct ata_port *ap, pm_message_t mesg,
 			       unsigned int action, unsigned int ehi_flags,
 			       int wait)
 {
+	struct ata_link *link;
 	unsigned long flags;
-	int i, rc;
+	int rc;
 
-	for (i = 0; i < host->n_ports; i++) {
-		struct ata_port *ap = host->ports[i];
-		struct ata_link *link;
+	/* Previous resume operation might still be in
+	 * progress.  Wait for PM_PENDING to clear.
+	 */
+	if (ap->pflags & ATA_PFLAG_PM_PENDING) {
+		ata_port_wait_eh(ap);
+		WARN_ON(ap->pflags & ATA_PFLAG_PM_PENDING);
+	}
 
-		/* Previous resume operation might still be in
-		 * progress.  Wait for PM_PENDING to clear.
-		 */
-		if (ap->pflags & ATA_PFLAG_PM_PENDING) {
-			ata_port_wait_eh(ap);
-			WARN_ON(ap->pflags & ATA_PFLAG_PM_PENDING);
-		}
+	/* request PM ops to EH */
+	spin_lock_irqsave(ap->lock, flags);
 
-		/* request PM ops to EH */
-		spin_lock_irqsave(ap->lock, flags);
+	ap->pm_mesg = mesg;
+	if (wait) {
+		rc = 0;
+		ap->pm_result = &rc;
+	}
 
-		ap->pm_mesg = mesg;
-		if (wait) {
-			rc = 0;
-			ap->pm_result = &rc;
-		}
+	ap->pflags |= ATA_PFLAG_PM_PENDING;
+	ata_for_each_link(link, ap, HOST_FIRST) {
+		link->eh_info.action |= action;
+		link->eh_info.flags |= ehi_flags;
+	}
 
-		ap->pflags |= ATA_PFLAG_PM_PENDING;
-		ata_for_each_link(link, ap, HOST_FIRST) {
-			link->eh_info.action |= action;
-			link->eh_info.flags |= ehi_flags;
-		}
+	ata_port_schedule_eh(ap);
 
-		ata_port_schedule_eh(ap);
+	spin_unlock_irqrestore(ap->lock, flags);
 
-		spin_unlock_irqrestore(ap->lock, flags);
+	/* wait and check result */
+	if (wait) {
+		ata_port_wait_eh(ap);
+		WARN_ON(ap->pflags & ATA_PFLAG_PM_PENDING);
+	}
 
-		/* wait and check result */
-		if (wait) {
-			ata_port_wait_eh(ap);
-			WARN_ON(ap->pflags & ATA_PFLAG_PM_PENDING);
-			if (rc)
-				return rc;
-		}
+	return rc;
+}
+
+static int ata_host_request_pm(struct ata_host *host, pm_message_t mesg,
+			       unsigned int action, unsigned int ehi_flags,
+			       int wait)
+{
+	int i, rc;
+
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap = host->ports[i];
+
+		rc = ata_port_request_pm(ap, mesg, action, ehi_flags, wait);
+		if (rc)
+			return rc;
 	}
 
 	return 0;

@@ -5874,6 +5886,45 @@ void ata_host_init(struct ata_host *host, struct device *dev,
 	host->ops = ops;
 }
 
+#define to_ata_port(d) container_of(d, struct ata_port, tdev)
+
+static int ata_port_runtime_suspend(struct device *dev)
+{
+	struct ata_port *ap = to_ata_port(dev);
+	struct Scsi_Host *shost = ap->scsi_host;
+	int rc;
+
+	/* TODO: sync with hardware access */
+
+	if (shost->host_busy)
+		return -EBUSY;
+
+	rc = ata_port_request_pm(ap, PMSG_SUSPEND, 0, ATA_EHI_QUIET, 1);
+	return rc;
+}
+
+static int ata_port_runtime_resume(struct device *dev)
+{
+	struct ata_port *ap = to_ata_port(dev);
+	int rc;
+
+	rc = ata_port_request_pm(ap, PMSG_ON, ATA_EH_RESET,
+		ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET, 1);
+	return rc;
+}
+
+static const struct dev_pm_ops ata_port_pm_ops = {
+	.runtime_suspend = ata_port_runtime_suspend,
+	.runtime_resume = ata_port_runtime_resume,
+};
+
+static struct device_type ata_port_type = {
+	.name = "ata_port",
+#ifdef CONFIG_PM
+	.pm = &ata_port_pm_ops,
+#endif
+};
+
 int ata_port_probe(struct ata_port *ap)
 {
 	int rc = 0;

@@ -5903,6 +5954,15 @@ int ata_port_probe(struct ata_port *ap)
 		rc = ata_bus_probe(ap);
 		DPRINTK("ata%u: bus probe end\n", ap->print_id);
 	}
+
+	ap->tdev.type = &ata_port_type;
+	pm_runtime_set_active(&ap->tdev);
+	pm_runtime_use_autosuspend(&ap->tdev);
+	/* 3 minutes idle to auto suspend */
+	pm_runtime_set_autosuspend_delay(&ap->tdev, 180*1000);
+	pm_runtime_enable(&ap->tdev);
+	pm_request_autosuspend(&ap->tdev);
+
 	return rc;
 }

drivers/ata/libata-scsi.c +6 -0

index 46d087f..88fc7fe 100644
@@ -48,6 +48,7 @@
 #include <linux/hdreg.h>
 #include <linux/uaccess.h>
 #include <linux/suspend.h>
+#include <linux/pm_runtime.h>
 #include <asm/unaligned.h>
 
 #include "libata.h"

@@ -3208,6 +3209,11 @@ int ata_scsi_queuecmd(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
 
 	ap = ata_shost_to_port(shost);
 
+	if (pm_runtime_suspended(&ap->tdev))
+		pm_runtime_resume(&ap->tdev);
+	pm_runtime_mark_last_busy(&ap->tdev);
+	pm_request_autosuspend(&ap->tdev);
+
 	spin_lock_irqsave(ap->lock, irq_flags);
 
 	ata_scsi_dump_cdb(ap, cmd);

Jeff Garzik

2011-10-28 03:37 UTC #1 lore raw

drivers/ata/libata-scsi.c int ata_scsi_queuecmd(struct Scsi_Host *shost, struct scsi_cmnd *cmd) · line 3218

Quoted 11 lines · Lin Ming

drivers/ata/libata-scsi.c +5 -0

@@ -3208,6 +3209,11 @@ int ata_scsi_queuecmd(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
 
  	ap = ata_shost_to_port(shost);
 
+	if (pm_runtime_suspended(&ap->tdev))
+		pm_runtime_resume(&ap->tdev);
+	pm_runtime_mark_last_busy(&ap->tdev);
+	pm_request_autosuspend(&ap->tdev);
+
  	spin_lock_irqsave(ap->lock, irq_flags);

Putting this into the core command dispatch fast-path is rather disappointing. That's at least one additional lock, plus some atomic instructions and tests.

Jeff

Lin Ming

author

2011-10-28 05:36 UTC #2 lore raw

Quoted 5 lines · Jeff Garzik

Quoted 4 lines · Lin Ming

drivers/ata/libata-scsi.c +0 -0

@@ -3208,6 +3209,11 @@ int ata_scsi_queuecmd(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
 
  	ap = ata_shost_to_port(shost);

Quoted 12 lines

drivers/ata/libata-scsi.c int ata_scsi_queuecmd(struct Scsi_Host *shost, struct scsi_cmnd *cmd) · line 3218
Quoted 7 lines
drivers/ata/libata-scsi.c +5 -0
+	if (pm_runtime_suspended(&ap->tdev))
+		pm_runtime_resume(&ap->tdev);
+	pm_runtime_mark_last_busy(&ap->tdev);
+	pm_request_autosuspend(&ap->tdev);
+
  	spin_lock_irqsave(ap->lock, irq_flags);
 
Putting this into the core command dispatch fast-path is rather disappointing. That's at least one additional lock, plus some atomic instructions and tests.

Maybe move suspend request to the resume function, as below.

static int ata_port_runtime_resume(struct device *dev)

{
        struct ata_port *ap = to_ata_port(dev);
        int rc;

        rc = ata_port_request_pm(ap, PMSG_ON, ATA_EH_RESET,
                ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET, 1);

        pm_runtime_mark_last_busy(dev);
        pm_request_autosuspend(dev);

        return rc;
}

Then the fast-path looks like,

@@ -3208,6 +3209,9 @@ int ata_scsi_queuecmd(struct Scsi_Host *shost, struct scsi_cmnd *cmd)

        ap = ata_shost_to_port(shost);

+ if (pm_runtime_suspended(&ap->tdev))
+ pm_runtime_resume(&ap->tdev);
+

        spin_lock_irqsave(ap->lock, irq_flags);

        ata_scsi_dump_cdb(ap, cmd);

What do you think?

Thanks,
Lin Ming

Rafael J. Wysocki

2011-10-28 17:29 UTC #3 lore raw

Please CC PM-related patches to [email protected] (now added).

Thanks,
Rafael

Quoted 56 lines · Lin Ming

Quoted 5 lines · Jeff Garzik
Quoted 4 lines · Lin Ming
drivers/ata/libata-scsi.c +0 -0
@@ -3208,6 +3209,11 @@ int ata_scsi_queuecmd(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
 
  	ap = ata_shost_to_port(shost);
 
Quoted 12 lines
drivers/ata/libata-scsi.c int ata_scsi_queuecmd(struct Scsi_Host *shost, struct scsi_cmnd *cmd) · line 3218
Quoted 7 lines
drivers/ata/libata-scsi.c +5 -0
+	if (pm_runtime_suspended(&ap->tdev))
+		pm_runtime_resume(&ap->tdev);
+	pm_runtime_mark_last_busy(&ap->tdev);
+	pm_request_autosuspend(&ap->tdev);
+
  	spin_lock_irqsave(ap->lock, irq_flags);
 
Putting this into the core command dispatch fast-path is rather disappointing. That's at least one additional lock, plus some atomic instructions and tests.
Maybe move suspend request to the resume function, as below.
static int ata_port_runtime_resume(struct device *dev)
{
        struct ata_port *ap = to_ata_port(dev);
        int rc;

        rc = ata_port_request_pm(ap, PMSG_ON, ATA_EH_RESET,
                ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET, 1);

        pm_runtime_mark_last_busy(dev);
        pm_request_autosuspend(dev);

        return rc;
}
Then the fast-path looks like,
drivers/ata/libata-scsi.c +3 -0
@@ -3208,6 +3209,9 @@ int ata_scsi_queuecmd(struct Scsi_Host *shost,
 struct scsi_cmnd *cmd)
 
        ap = ata_shost_to_port(shost);
 
+       if (pm_runtime_suspended(&ap->tdev))
+               pm_runtime_resume(&ap->tdev);
+
        spin_lock_irqsave(ap->lock, irq_flags);
 
        ata_scsi_dump_cdb(ap, cmd);
What do you think?
Thanks,
Lin Ming

Alan Stern

2011-10-28 18:51 UTC #4 lore raw

Quoted 7 lines · Rafael J. Wysocki

Quoted 6 lines · Lin Ming
Quoted 5 lines · Jeff Garzik
Quoted 4 lines · Lin Ming
drivers/ata/libata-scsi.c +0 -0
@@ -3208,6 +3209,11 @@ int ata_scsi_queuecmd(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
 
  	ap = ata_shost_to_port(shost);
 

Quoted 12 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/ata/libata-scsi.c +4 -0
+	if (pm_runtime_suspended(&ap->tdev))
+		pm_runtime_resume(&ap->tdev);
+	pm_runtime_mark_last_busy(&ap->tdev);
+	pm_request_autosuspend(&ap->tdev);
Quoted 8 lines
Quoted 8 lines
drivers/ata/libata-scsi.c int ata_scsi_queuecmd(struct Scsi_Host *shost, struct scsi_cmnd *cmd) · line 3218
Quoted 3 lines
drivers/ata/libata-scsi.c +1 -0
+
  	spin_lock_irqsave(ap->lock, irq_flags);
 
Putting this into the core command dispatch fast-path is rather disappointing. That's at least one additional lock, plus some atomic instructions and tests.

And it calls pm_runtime_resume(), which requires process context, from within a SCSI queuecmd routine, which runs in interrupt context.

Quoted 18 lines

Quoted 18 lines
Maybe move suspend request to the resume function, as below.
static int ata_port_runtime_resume(struct device *dev)
{
        struct ata_port *ap = to_ata_port(dev);
        int rc;

        rc = ata_port_request_pm(ap, PMSG_ON, ATA_EH_RESET,
                ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET, 1);

        pm_runtime_mark_last_busy(dev);
        pm_request_autosuspend(dev);

        return rc;
}
Then the fast-path looks like,

Quoted 8 lines

drivers/ata/libata-scsi.c int ata_scsi_queuecmd(struct Scsi_Host *shost, · line 3215

Quoted 8 lines

drivers/ata/libata-scsi.c +3 -0

@@ -3208,6 +3209,9 @@ int ata_scsi_queuecmd(struct Scsi_Host *shost,
 struct scsi_cmnd *cmd)
 
        ap = ata_shost_to_port(shost);
 
+       if (pm_runtime_suspended(&ap->tdev))
+               pm_runtime_resume(&ap->tdev);
+

Unfortunately that won't work. It's got a race; the device might be active when the pm_runtime_suspended() test runs and then it might suspend immediately after.

Quoted 5 lines

Quoted 5 lines
drivers/ata/libata-scsi.c +0 -0
        spin_lock_irqsave(ap->lock, irq_flags);
 
        ata_scsi_dump_cdb(ap, cmd);
What do you think?

See the example code in section 9 of Documentation/power/runtime_pm.txt for the outline of a general approach.

Alan Stern

Lin Ming

author

2011-11-01 08:12 UTC #5 lore raw

Quoted 22 lines · Alan Stern

Quoted 7 lines · Rafael J. Wysocki
Quoted 6 lines · Lin Ming
Quoted 5 lines · Jeff Garzik
Quoted 4 lines · Lin Ming
drivers/ata/libata-scsi.c +0 -0
@@ -3208,6 +3209,11 @@ int ata_scsi_queuecmd(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
 
  	ap = ata_shost_to_port(shost);
 
Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/ata/libata-scsi.c +4 -0
+	if (pm_runtime_suspended(&ap->tdev))
+		pm_runtime_resume(&ap->tdev);
+	pm_runtime_mark_last_busy(&ap->tdev);
+	pm_request_autosuspend(&ap->tdev);
Quoted 8 lines
Quoted 8 lines
drivers/ata/libata-scsi.c int ata_scsi_queuecmd(struct Scsi_Host *shost, struct scsi_cmnd *cmd) · line 3218
Quoted 3 lines
drivers/ata/libata-scsi.c +1 -0
+
  	spin_lock_irqsave(ap->lock, irq_flags);
 
Putting this into the core command dispatch fast-path is rather disappointing. That's at least one additional lock, plus some atomic instructions and tests.

Quoted 2 lines

And it calls pm_runtime_resume(), which requires process context, from within a SCSI queuecmd routine, which runs in interrupt context.

Hi,

Thanks to point this out. I change the code to do ata port runtime suspend/resume through scsi layer.

scsi host runtime suspend/resume framework is already there(scsi_pm.c). So I only need to insert hooks for ata port in scsi_runtime_suspend/resume(...).

But I found a live lock when testing my patch.

<scsi host runtime suspend>
scsi_autopm_put_host
pm_runtime_put_sync
<scsi_host runtime pm status updated to RPM_SUSPENDING>
......
<call libata hook to do suspend>
<wake up scsi EH to handle suspend>
<wait for scsi EH ...>

<scsi EH wake up>
scsi_error_handler
<resume scsi host>
scsi_autopm_get_host
pm_runtime_get_sync
.....
<sleep to wait for the ongoing scsi host suspend>

libata schedules scsi EH to handle suspend, then dead lock happens because scsi EH in turn waits for the ongoing suspend.

Any idea how to resolve this dead lock?

Thanks,
Lin Ming

Alan Stern

2011-11-01 19:34 UTC #6 lore raw

Quoted 54 lines · Lin Ming

Quoted 22 lines · Alan Stern
Quoted 7 lines · Rafael J. Wysocki
Quoted 6 lines · Lin Ming
Quoted 5 lines · Jeff Garzik
Quoted 4 lines · Lin Ming
drivers/ata/libata-scsi.c +0 -0
@@ -3208,6 +3209,11 @@ int ata_scsi_queuecmd(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
 
  	ap = ata_shost_to_port(shost);
 
Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/ata/libata-scsi.c +4 -0
+	if (pm_runtime_suspended(&ap->tdev))
+		pm_runtime_resume(&ap->tdev);
+	pm_runtime_mark_last_busy(&ap->tdev);
+	pm_request_autosuspend(&ap->tdev);
Quoted 8 lines
Quoted 8 lines
drivers/ata/libata-scsi.c int ata_scsi_queuecmd(struct Scsi_Host *shost, struct scsi_cmnd *cmd) · line 3218
Quoted 3 lines
drivers/ata/libata-scsi.c +1 -0
+
  	spin_lock_irqsave(ap->lock, irq_flags);
 
Putting this into the core command dispatch fast-path is rather disappointing. That's at least one additional lock, plus some atomic instructions and tests.
Quoted 2 lines
And it calls pm_runtime_resume(), which requires process context, from within a SCSI queuecmd routine, which runs in interrupt context.
Hi,
Thanks to point this out. I change the code to do ata port runtime suspend/resume through scsi layer.
scsi host runtime suspend/resume framework is already there(scsi_pm.c). So I only need to insert hooks for ata port in scsi_runtime_suspend/resume(...).
But I found a live lock when testing my patch.
<scsi host runtime suspend>
scsi_autopm_put_host
pm_runtime_put_sync
<scsi_host runtime pm status updated to RPM_SUSPENDING>
......
<call libata hook to do suspend>
<wake up scsi EH to handle suspend>
<wait for scsi EH ...>
<scsi EH wake up>
scsi_error_handler
<resume scsi host>
scsi_autopm_get_host
pm_runtime_get_sync
.....
<sleep to wait for the ongoing scsi host suspend>

Quoted 4 lines

libata schedules scsi EH to handle suspend, then dead lock happens because scsi EH in turn waits for the ongoing suspend.
Any idea how to resolve this dead lock?

This is a nasty problem. I've known for a long time that the scsi_autopm_get_host() call in the error handler was going to lead to problems.

For now, it seems best to assume that when the error handler starts, the device will still be active. Therefore the scsi_autopm_get_host() should be replaced by something that calls pm_runtime_get_noresume() instead of pm_runtime_get_sync().

You can try replacing one function call with the other, or you can define a new scsi_autopm_get_host_noresume() routine.

Alan Stern