[PATCH 0/7] Runtime PM support for AHCI host controller driver

[PATCH 0/7] Runtime PM support for AHCI host controller driver

[Mika Westerberg]

Hi,

Linux already supports runtime PM of disks (drivers/scsi/sd.c) so that
after certain amount of idle time the disk is suspended automatically. This
series extends the support to AHCI host controllers. Whenever SATA ports
are determined to be idle (all children are runtime suspended) the host
controller is also suspended.

On recent Intel CPUs like Broxton this allows the CPU to go low power idle
states like S0ix runtime (given that all necessary blocks are also in their
correesponding low power states).

Patches [1-2/7] fix a lockup where disk is runtime suspended and the system
is put to sleep. They are independent of the rest of the series.

Patch [3/7] makes it possible for SATA ports to be runtime suspended when
there is not disk connected. For example on Lenovo Yoga 900 there are two
SATA ports which only one of them has disk connected. This patch allows the
host controller to runtime suspend whenever the disk is idle.

Rest of the patches bring runtime PM support for the AHCI driver. By
default runtime PM is blocked and needs to be unblocked from userspace
(following what other PCI drivers do). I've used following script to
unblock runtime PM for the whole stack (with 15 seconds of idle time):

------8<------8<------8<------8<------8<------8<------8<------
#!/bin/sh

TIMEOUT=${1:-15}
HOST=$(lspci -D | grep "SATA controller" | cut -f 1 -d ' ')
DISK=sda

# Enable runtime PM for all SATA ports
for port in /sys/bus/pci/devices/$HOST/ata*; do
	echo auto > $port/power/control
done
# Then for the host controller
echo auto > /sys/bus/pci/devices/$HOST/power/control

# And last for the disk
echo auto > /sys/block/$DISK/device/power/control
echo $(($TIMEOUT * 1000)) > /sys/block/$DISK/device/power/autosuspend_delay_ms
------8<------8<------8<------8<------8<------8<------8<------

Mika Westerberg (7):
  block: Add blk_set_runtime_active()
  scsi: Set request queue runtime PM status back to active on resume
  scsi: Drop runtime PM usage count after host is added
  ahci: Cache host controller version
  ahci: Convert driver to use modern PM hooks
  ahci: Add functions to manage runtime PM of AHCI ports
  ahci: Add runtime PM support for the host controller

 block/blk-core.c       |  24 ++++++++++++
 drivers/ata/ahci.c     | 102 +++++++++++++++++++++++++++++++++++--------------
 drivers/ata/ahci.h     |   1 +
 drivers/ata/libahci.c  |  55 +++++++++++++++++++++++---
 drivers/scsi/hosts.c   |   7 ++++
 drivers/scsi/scsi_pm.c |  10 +++++
 include/linux/blkdev.h |   2 +
 7 files changed, 167 insertions(+), 34 deletions(-)

-- 
2.7.0

[PATCH 1/7] block: Add blk_set_runtime_active()

[Mika Westerberg]

If block device is left runtime suspended during system suspend, resume
hook of the driver typically corrects runtime PM status of the device back
to "active" after it is resumed. However, this is not enough as queue's
runtime PM status is still "suspended". As long as it is in this state
blk_pm_peek_request() returns NULL and thus prevents new requests to be
processed.

Add new function blk_set_runtime_active() that can be used to force the
queue status back to "active" as needed.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 block/blk-core.c       | 24 ++++++++++++++++++++++++
 include/linux/blkdev.h |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/block/blk-core.c b/block/blk-core.c
index b83d29755b5a..d0c6f4c92cb6 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -3529,6 +3529,30 @@ void blk_post_runtime_resume(struct request_queue *q, int err)
 	spin_unlock_irq(q->queue_lock);
 }
 EXPORT_SYMBOL(blk_post_runtime_resume);
+
+/**
+ * blk_set_runtime_active - Force runtime status of the queue to be active
+ * @q: the queue of the device
+ *
+ * If the device is left runtime suspended during system suspend the resume
+ * hook typically resumes the device and corrects runtime status
+ * accordingly. However, that does not affect the queue runtime PM status
+ * which is still "suspended". This prevents processing requests from the
+ * queue.
+ *
+ * This function can be used in driver's resume hook to correct queue
+ * runtime PM status and re-enable peeking requests from the queue. It
+ * should be called before first request is added to the queue.
+ */
+void blk_set_runtime_active(struct request_queue *q)
+{
+	spin_lock_irq(q->queue_lock);
+	q->rpm_status = RPM_ACTIVE;
+	pm_runtime_mark_last_busy(q->dev);
+	pm_request_autosuspend(q->dev);
+	spin_unlock_irq(q->queue_lock);
+}
+EXPORT_SYMBOL(blk_set_runtime_active);
 #endif
 
 int __init blk_dev_init(void)
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 4571ef1a12a9..40c0241a6f28 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1029,6 +1029,7 @@ extern int blk_pre_runtime_suspend(struct request_queue *q);
 extern void blk_post_runtime_suspend(struct request_queue *q, int err);
 extern void blk_pre_runtime_resume(struct request_queue *q);
 extern void blk_post_runtime_resume(struct request_queue *q, int err);
+extern void blk_set_runtime_active(struct request_queue *q);
 #else
 static inline void blk_pm_runtime_init(struct request_queue *q,
 	struct device *dev) {}
@@ -1039,6 +1040,7 @@ static inline int blk_pre_runtime_suspend(struct request_queue *q)
 static inline void blk_post_runtime_suspend(struct request_queue *q, int err) {}
 static inline void blk_pre_runtime_resume(struct request_queue *q) {}
 static inline void blk_post_runtime_resume(struct request_queue *q, int err) {}
+extern inline void blk_set_runtime_active(struct request_queue *q) {}
 #endif
 
 /*
-- 
2.7.0

[PATCH 2/7] scsi: Set request queue runtime PM status back to active on resume

[Mika Westerberg]

We treat system suspend of SCSI devices pretty much the same as runtime
suspend. If the device is already runtime suspended we leave it to that
state during system suspend. On resume from system sleep we then resume the
device and correct the runtime PM status back to "active".

There is a problem with this because runtime PM status of the request queue
in question is not changed (it will be in "suspended" state). When SCSI
disk driver (sd.c) resumes the disk it sends START message to the device
and because the request queue is still in "suspended" state
blk_pm_peek_request() returns NULL preventing resume of the disk.

The issue can be reproduced with following commands:

  # echo auto > /sys/block/sda/device/power/control
  # echo 15000 > /sys/block/sda/device/power/autosuspend_delay_ms
  [   57.191706] sd 0:0:0:0: [sda] Synchronizing SCSI cache
  [   57.380015] sd 0:0:0:0: [sda] Stopping disk

Now suspend the machine:

  # rtcwake -s10 -mmem

This ends up in soft lockup because resume is not proceeding accordingly
and userspace is never restarted. Also there is nothing printed to the
console.

Fix this by forcing request queue status to "active" before the disk is
resumed.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/scsi/scsi_pm.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index 459abe1dcc87..b44c1bb687a2 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -139,6 +139,16 @@ static int scsi_bus_resume_common(struct device *dev,
 	else
 		fn = NULL;
 
+	/*
+	 * Forcibly set runtime PM status of request queue to "active" to
+	 * make sure we can again get requests from the queue (see also
+	 * blk_pm_peek_request()).
+	 *
+	 * The resume hook will correct runtime PM status of the disk.
+	 */
+	if (scsi_is_sdev_device(dev) && pm_runtime_suspended(dev))
+		blk_set_runtime_active(to_scsi_device(dev)->request_queue);
+
 	if (fn) {
 		async_schedule_domain(fn, dev, &scsi_sd_pm_domain);
 
-- 
2.7.0

[PATCH 3/7] scsi: Drop runtime PM usage count after host is added

[Mika Westerberg]

Runtime PM of the SCSI host is already handled by calls to
scsi_autopm_get_host() and scsi_autopm_put_host() from appropriate places
whenever the host needs to be powered on. This works fine when there is
device connected to the host as once it runtime suspends the host will too.

However, if there is no device connected the host is never runtime
suspended (the usage counter is always 0).

Allow runtime suspend of host even if it has no devices connected by
calling scsi_autopm_put_host() at the end of scsi_add_host_with_dma(). We
temporarily increase runtime PM usage counter first so call to
scsi_autopm_put_host() will result idle request to be scheduled for the
device.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/scsi/hosts.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 82ac1cd818ac..e46bf4d152a0 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -250,6 +250,12 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
 	if (error)
 		goto out_destroy_freelist;
 
+	/*
+	 * Increase usage count temporarily here so that calling
+	 * scsi_autopm_put_host() will trigger runtime idle if there is
+	 * nothing else preventing suspending the device.
+	 */
+	pm_runtime_get_noresume(&shost->shost_gendev);
 	pm_runtime_set_active(&shost->shost_gendev);
 	pm_runtime_enable(&shost->shost_gendev);
 	device_enable_async_suspend(&shost->shost_gendev);
@@ -290,6 +296,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
 		goto out_destroy_host;
 
 	scsi_proc_host_add(shost);
+	scsi_autopm_put_host(shost);
 	return error;
 
  out_destroy_host:
-- 
2.7.0

Re: [PATCH 3/7] scsi: Drop runtime PM usage count after host is added

[Julian Calaby]

Hi Mika,

On Thu, Feb 18, 2016 at 7:54 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> Runtime PM of the SCSI host is already handled by calls to
> scsi_autopm_get_host() and scsi_autopm_put_host() from appropriate places
> whenever the host needs to be powered on. This works fine when there is
> device connected to the host as once it runtime suspends the host will too.
>
> However, if there is no device connected the host is never runtime
> suspended (the usage counter is always 0).
>
> Allow runtime suspend of host even if it has no devices connected by
> calling scsi_autopm_put_host() at the end of scsi_add_host_with_dma(). We
> temporarily increase runtime PM usage counter first so call to
> scsi_autopm_put_host() will result idle request to be scheduled for the
> device.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/scsi/hosts.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 82ac1cd818ac..e46bf4d152a0 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -250,6 +250,12 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
>         if (error)
>                 goto out_destroy_freelist;
>
> +       /*
> +        * Increase usage count temporarily here so that calling
> +        * scsi_autopm_put_host() will trigger runtime idle if there is
> +        * nothing else preventing suspending the device.
> +        */
> +       pm_runtime_get_noresume(&shost->shost_gendev);
>         pm_runtime_set_active(&shost->shost_gendev);
>         pm_runtime_enable(&shost->shost_gendev);
>         device_enable_async_suspend(&shost->shost_gendev);
> @@ -290,6 +296,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
>                 goto out_destroy_host;
>
>         scsi_proc_host_add(shost);
> +       scsi_autopm_put_host(shost);

Would it be cleaner to export the code that runs when the usage
counter decrements and call it here?

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH 3/7] scsi: Drop runtime PM usage count after host is added

[Mika Westerberg]

On Fri, Feb 19, 2016 at 09:50:50AM +1100, Julian Calaby wrote:
> Hi Mika,
> 
> On Thu, Feb 18, 2016 at 7:54 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > Runtime PM of the SCSI host is already handled by calls to
> > scsi_autopm_get_host() and scsi_autopm_put_host() from appropriate places
> > whenever the host needs to be powered on. This works fine when there is
> > device connected to the host as once it runtime suspends the host will too.
> >
> > However, if there is no device connected the host is never runtime
> > suspended (the usage counter is always 0).
> >
> > Allow runtime suspend of host even if it has no devices connected by
> > calling scsi_autopm_put_host() at the end of scsi_add_host_with_dma(). We
> > temporarily increase runtime PM usage counter first so call to
> > scsi_autopm_put_host() will result idle request to be scheduled for the
> > device.
> >
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/scsi/hosts.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> > index 82ac1cd818ac..e46bf4d152a0 100644
> > --- a/drivers/scsi/hosts.c
> > +++ b/drivers/scsi/hosts.c
> > @@ -250,6 +250,12 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
> >         if (error)
> >                 goto out_destroy_freelist;
> >
> > +       /*
> > +        * Increase usage count temporarily here so that calling
> > +        * scsi_autopm_put_host() will trigger runtime idle if there is
> > +        * nothing else preventing suspending the device.
> > +        */
> > +       pm_runtime_get_noresume(&shost->shost_gendev);
> >         pm_runtime_set_active(&shost->shost_gendev);
> >         pm_runtime_enable(&shost->shost_gendev);
> >         device_enable_async_suspend(&shost->shost_gendev);
> > @@ -290,6 +296,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
> >                 goto out_destroy_host;
> >
> >         scsi_proc_host_add(shost);
> > +       scsi_autopm_put_host(shost);
> 
> Would it be cleaner to export the code that runs when the usage
> counter decrements and call it here?

There actually is no code to run. This just ensures that the device
runtime_idle gets called which allows the parent device to runtime
suspend (as it returns 0). Alternative way would be just to call
pm_request_idle() directly here.

[PATCH 4/7] ahci: Cache host controller version

[Mika Westerberg]

This allows sysfs nodes to read the cached value directly instead of
powering up possibly runtime suspended controller.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/ata/ahci.h    | 1 +
 drivers/ata/libahci.c | 7 +++----
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index a44c75d4c284..8138bc967eda 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -336,6 +336,7 @@ struct ahci_host_priv {
 	void __iomem *		mmio;		/* bus-independent mem map */
 	u32			cap;		/* cap to use */
 	u32			cap2;		/* cap2 to use */
+	u32			version;	/* cached version */
 	u32			port_map;	/* port map to use */
 	u32			saved_cap;	/* saved initial cap */
 	u32			saved_cap2;	/* saved initial cap2 */
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 402967902cbe..2ce4c638714e 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -250,9 +250,8 @@ static ssize_t ahci_show_host_version(struct device *dev,
 	struct Scsi_Host *shost = class_to_shost(dev);
 	struct ata_port *ap = ata_shost_to_port(shost);
 	struct ahci_host_priv *hpriv = ap->host->private_data;
-	void __iomem *mmio = hpriv->mmio;
 
-	return sprintf(buf, "%x\n", readl(mmio + HOST_VERSION));
+	return sprintf(buf, "%x\n", hpriv->version);
 }
 
 static ssize_t ahci_show_port_cmd(struct device *dev,
@@ -508,6 +507,7 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
 	/* record values to use during operation */
 	hpriv->cap = cap;
 	hpriv->cap2 = cap2;
+	hpriv->version = readl(mmio + HOST_VERSION);
 	hpriv->port_map = port_map;
 
 	if (!hpriv->start_engine)
@@ -2389,11 +2389,10 @@ static void ahci_port_stop(struct ata_port *ap)
 void ahci_print_info(struct ata_host *host, const char *scc_s)
 {
 	struct ahci_host_priv *hpriv = host->private_data;
-	void __iomem *mmio = hpriv->mmio;
 	u32 vers, cap, cap2, impl, speed;
 	const char *speed_s;
 
-	vers = readl(mmio + HOST_VERSION);
+	vers = hpriv->version;
 	cap = hpriv->cap;
 	cap2 = hpriv->cap2;
 	impl = hpriv->port_map;
-- 
2.7.0

[PATCH 5/7] ahci: Convert driver to use modern PM hooks

[Mika Westerberg]

In order to add support for runtime PM to the ahci driver we first need to
convert the driver to use modern non-legacy system suspend hooks. There
should be no functional changes.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/ata/ahci.c | 49 ++++++++++++++++++++++---------------------------
 1 file changed, 22 insertions(+), 27 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 546a3692774f..44f9d1c09bbe 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -93,9 +93,9 @@ static void ahci_mcp89_apple_enable(struct pci_dev *pdev);
 static bool is_mcp89_apple(struct pci_dev *pdev);
 static int ahci_p5wdh_hardreset(struct ata_link *link, unsigned int *class,
 				unsigned long deadline);
-#ifdef CONFIG_PM
-static int ahci_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg);
-static int ahci_pci_device_resume(struct pci_dev *pdev);
+#ifdef CONFIG_PM_SLEEP
+static int ahci_pci_device_suspend(struct device *dev);
+static int ahci_pci_device_resume(struct device *dev);
 #endif
 
 static struct scsi_host_template ahci_sht = {
@@ -557,16 +557,16 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ }	/* terminate list */
 };
 
+static const struct dev_pm_ops ahci_pci_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(ahci_pci_device_suspend, ahci_pci_device_resume)
+};
 
 static struct pci_driver ahci_pci_driver = {
 	.name			= DRV_NAME,
 	.id_table		= ahci_pci_tbl,
 	.probe			= ahci_init_one,
 	.remove			= ata_pci_remove_one,
-#ifdef CONFIG_PM
-	.suspend		= ahci_pci_device_suspend,
-	.resume			= ahci_pci_device_resume,
-#endif
+	.driver.pm		= &ahci_pci_pm_ops,
 };
 
 #if defined(CONFIG_PATA_MARVELL) || defined(CONFIG_PATA_MARVELL_MODULE)
@@ -794,44 +794,39 @@ static int ahci_avn_hardreset(struct ata_link *link, unsigned int *class,
 }
 
 
-#ifdef CONFIG_PM
-static int ahci_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg)
+#ifdef CONFIG_PM_SLEEP
+static int ahci_pci_device_suspend(struct device *dev)
 {
+	struct pci_dev *pdev = to_pci_dev(dev);
 	struct ata_host *host = pci_get_drvdata(pdev);
 	struct ahci_host_priv *hpriv = host->private_data;
 	void __iomem *mmio = hpriv->mmio;
 	u32 ctl;
 
-	if (mesg.event & PM_EVENT_SUSPEND &&
-	    hpriv->flags & AHCI_HFLAG_NO_SUSPEND) {
+	if (hpriv->flags & AHCI_HFLAG_NO_SUSPEND) {
 		dev_err(&pdev->dev,
 			"BIOS update required for suspend/resume\n");
 		return -EIO;
 	}
 
-	if (mesg.event & PM_EVENT_SLEEP) {
-		/* AHCI spec rev1.1 section 8.3.3:
-		 * Software must disable interrupts prior to requesting a
-		 * transition of the HBA to D3 state.
-		 */
-		ctl = readl(mmio + HOST_CTL);
-		ctl &= ~HOST_IRQ_EN;
-		writel(ctl, mmio + HOST_CTL);
-		readl(mmio + HOST_CTL); /* flush */
-	}
+	/* AHCI spec rev1.1 section 8.3.3:
+	 * Software must disable interrupts prior to requesting a
+	 * transition of the HBA to D3 state.
+	 */
+	ctl = readl(mmio + HOST_CTL);
+	ctl &= ~HOST_IRQ_EN;
+	writel(ctl, mmio + HOST_CTL);
+	readl(mmio + HOST_CTL); /* flush */
 
-	return ata_pci_device_suspend(pdev, mesg);
+	return ata_host_suspend(host, PMSG_SUSPEND);
 }
 
-static int ahci_pci_device_resume(struct pci_dev *pdev)
+static int ahci_pci_device_resume(struct device *dev)
 {
+	struct pci_dev *pdev = to_pci_dev(dev);
 	struct ata_host *host = pci_get_drvdata(pdev);
 	int rc;
 
-	rc = ata_pci_device_do_resume(pdev);
-	if (rc)
-		return rc;
-
 	/* Apple BIOS helpfully mangles the registers on resume */
 	if (is_mcp89_apple(pdev))
 		ahci_mcp89_apple_enable(pdev);
-- 
2.7.0

Re: [PATCH 5/7] ahci: Convert driver to use modern PM hooks

[Andy Shevchenko]

On Thu, Feb 18, 2016 at 10:54 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> In order to add support for runtime PM to the ahci driver we first need to
> convert the driver to use modern non-legacy system suspend hooks. There
> should be no functional changes.
>

One comment below, otherwise:
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/ata/ahci.c | 49 ++++++++++++++++++++++---------------------------
>  1 file changed, 22 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 546a3692774f..44f9d1c09bbe 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -93,9 +93,9 @@ static void ahci_mcp89_apple_enable(struct pci_dev *pdev);
>  static bool is_mcp89_apple(struct pci_dev *pdev);
>  static int ahci_p5wdh_hardreset(struct ata_link *link, unsigned int *class,
>                                 unsigned long deadline);
> -#ifdef CONFIG_PM
> -static int ahci_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg);
> -static int ahci_pci_device_resume(struct pci_dev *pdev);
> +#ifdef CONFIG_PM_SLEEP
> +static int ahci_pci_device_suspend(struct device *dev);
> +static int ahci_pci_device_resume(struct device *dev);
>  #endif
>
>  static struct scsi_host_template ahci_sht = {
> @@ -557,16 +557,16 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>         { }     /* terminate list */
>  };
>
> +static const struct dev_pm_ops ahci_pci_pm_ops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(ahci_pci_device_suspend, ahci_pci_device_resume)
> +};
>
>  static struct pci_driver ahci_pci_driver = {
>         .name                   = DRV_NAME,
>         .id_table               = ahci_pci_tbl,
>         .probe                  = ahci_init_one,
>         .remove                 = ata_pci_remove_one,
> -#ifdef CONFIG_PM
> -       .suspend                = ahci_pci_device_suspend,
> -       .resume                 = ahci_pci_device_resume,
> -#endif
> +       .driver.pm              = &ahci_pci_pm_ops,

Please, do this in several assignments, since to support older compilers.

>  };
>
>  #if defined(CONFIG_PATA_MARVELL) || defined(CONFIG_PATA_MARVELL_MODULE)
> @@ -794,44 +794,39 @@ static int ahci_avn_hardreset(struct ata_link *link, unsigned int *class,
>  }
>
>
> -#ifdef CONFIG_PM
> -static int ahci_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg)
> +#ifdef CONFIG_PM_SLEEP
> +static int ahci_pci_device_suspend(struct device *dev)
>  {
> +       struct pci_dev *pdev = to_pci_dev(dev);
>         struct ata_host *host = pci_get_drvdata(pdev);
>         struct ahci_host_priv *hpriv = host->private_data;
>         void __iomem *mmio = hpriv->mmio;
>         u32 ctl;
>
> -       if (mesg.event & PM_EVENT_SUSPEND &&
> -           hpriv->flags & AHCI_HFLAG_NO_SUSPEND) {
> +       if (hpriv->flags & AHCI_HFLAG_NO_SUSPEND) {
>                 dev_err(&pdev->dev,
>                         "BIOS update required for suspend/resume\n");
>                 return -EIO;
>         }
>
> -       if (mesg.event & PM_EVENT_SLEEP) {
> -               /* AHCI spec rev1.1 section 8.3.3:
> -                * Software must disable interrupts prior to requesting a
> -                * transition of the HBA to D3 state.
> -                */
> -               ctl = readl(mmio + HOST_CTL);
> -               ctl &= ~HOST_IRQ_EN;
> -               writel(ctl, mmio + HOST_CTL);
> -               readl(mmio + HOST_CTL); /* flush */
> -       }
> +       /* AHCI spec rev1.1 section 8.3.3:
> +        * Software must disable interrupts prior to requesting a
> +        * transition of the HBA to D3 state.
> +        */
> +       ctl = readl(mmio + HOST_CTL);
> +       ctl &= ~HOST_IRQ_EN;
> +       writel(ctl, mmio + HOST_CTL);
> +       readl(mmio + HOST_CTL); /* flush */
>
> -       return ata_pci_device_suspend(pdev, mesg);
> +       return ata_host_suspend(host, PMSG_SUSPEND);
>  }
>
> -static int ahci_pci_device_resume(struct pci_dev *pdev)
> +static int ahci_pci_device_resume(struct device *dev)
>  {
> +       struct pci_dev *pdev = to_pci_dev(dev);
>         struct ata_host *host = pci_get_drvdata(pdev);
>         int rc;
>
> -       rc = ata_pci_device_do_resume(pdev);
> -       if (rc)
> -               return rc;
> -
>         /* Apple BIOS helpfully mangles the registers on resume */
>         if (is_mcp89_apple(pdev))
>                 ahci_mcp89_apple_enable(pdev);
> --
> 2.7.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/7] ahci: Convert driver to use modern PM hooks

[Tejun Heo]

On Thu, Feb 18, 2016 at 12:45:05PM +0200, Andy Shevchenko wrote:
> > +       .driver.pm              = &ahci_pci_pm_ops,
> 
> Please, do this in several assignments, since to support older compilers.

Can you please elaborate?  That's a single assignment.  Are there
compilers which can't do that?

Thanks.

-- 
tejun

Re: [PATCH 5/7] ahci: Convert driver to use modern PM hooks

[Christoph Hellwig]

On Thu, Feb 18, 2016 at 08:12:46AM -0500, Tejun Heo wrote:
> On Thu, Feb 18, 2016 at 12:45:05PM +0200, Andy Shevchenko wrote:
> > > +       .driver.pm              = &ahci_pci_pm_ops,
> > 
> > Please, do this in several assignments, since to support older compilers.
> 
> Can you please elaborate?  That's a single assignment.  Are there
> compilers which can't do that?

Old gcc (I think 4.4 is the last one) needs:

	.driver = {
		.pm		= &ahci_pci_pm_ops,
	},

[PATCH 6/7] ahci: Add functions to manage runtime PM of AHCI ports

[Mika Westerberg]

Add new functions ahci_rpm_get_port()/ahci_rpm_put_port() that change
runtime PM status of AHCI ports. Depending if the AHCI host has runtime PM
enabled or disabled calling these may trigger runtime suspend/resume of the
host controller.

We also call these functions in appropriate places to make sure host
controller registers are available before using them.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/ata/libahci.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 2ce4c638714e..a5dc80810a5e 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -224,6 +224,31 @@ static void ahci_enable_ahci(void __iomem *mmio)
 	WARN_ON(1);
 }
 
+/**
+ *	ahci_rpm_get_port - Make sure the port is powered on
+ *	@ap: Port to power on
+ *
+ *	Whenever there is need to access the AHCI host registers outside of
+ *	normal execution paths, call this function to make sure the host is
+ *	actually powered on.
+ */
+static int ahci_rpm_get_port(struct ata_port *ap)
+{
+	return pm_runtime_get_sync(ap->dev);
+}
+
+/**
+ *	ahci_rpm_put_port - Undoes ahci_rpm_get_port()
+ *	@ap: Port to power down
+ *
+ *	Undoes ahci_rpm_get_port() and possibly powers down the AHCI host
+ *	if it has no more active users.
+ */
+static void ahci_rpm_put_port(struct ata_port *ap)
+{
+	pm_runtime_put(ap->dev);
+}
+
 static ssize_t ahci_show_host_caps(struct device *dev,
 				   struct device_attribute *attr, char *buf)
 {
@@ -260,8 +285,13 @@ static ssize_t ahci_show_port_cmd(struct device *dev,
 	struct Scsi_Host *shost = class_to_shost(dev);
 	struct ata_port *ap = ata_shost_to_port(shost);
 	void __iomem *port_mmio = ahci_port_base(ap);
+	ssize_t ret;
 
-	return sprintf(buf, "%x\n", readl(port_mmio + PORT_CMD));
+	ahci_rpm_get_port(ap);
+	ret = sprintf(buf, "%x\n", readl(port_mmio + PORT_CMD));
+	ahci_rpm_put_port(ap);
+
+	return ret;
 }
 
 static ssize_t ahci_read_em_buffer(struct device *dev,
@@ -277,17 +307,20 @@ static ssize_t ahci_read_em_buffer(struct device *dev,
 	size_t count;
 	int i;
 
+	ahci_rpm_get_port(ap);
 	spin_lock_irqsave(ap->lock, flags);
 
 	em_ctl = readl(mmio + HOST_EM_CTL);
 	if (!(ap->flags & ATA_FLAG_EM) || em_ctl & EM_CTL_XMT ||
 	    !(hpriv->em_msg_type & EM_MSG_TYPE_SGPIO)) {
 		spin_unlock_irqrestore(ap->lock, flags);
+		ahci_rpm_put_port(ap);
 		return -EINVAL;
 	}
 
 	if (!(em_ctl & EM_CTL_MR)) {
 		spin_unlock_irqrestore(ap->lock, flags);
+		ahci_rpm_put_port(ap);
 		return -EAGAIN;
 	}
 
@@ -315,6 +348,7 @@ static ssize_t ahci_read_em_buffer(struct device *dev,
 	}
 
 	spin_unlock_irqrestore(ap->lock, flags);
+	ahci_rpm_put_port(ap);
 
 	return i;
 }
@@ -339,11 +373,13 @@ static ssize_t ahci_store_em_buffer(struct device *dev,
 	    size % 4 || size > hpriv->em_buf_sz)
 		return -EINVAL;
 
+	ahci_rpm_get_port(ap);
 	spin_lock_irqsave(ap->lock, flags);
 
 	em_ctl = readl(mmio + HOST_EM_CTL);
 	if (em_ctl & EM_CTL_TM) {
 		spin_unlock_irqrestore(ap->lock, flags);
+		ahci_rpm_put_port(ap);
 		return -EBUSY;
 	}
 
@@ -356,6 +392,7 @@ static ssize_t ahci_store_em_buffer(struct device *dev,
 	writel(em_ctl | EM_CTL_TM, mmio + HOST_EM_CTL);
 
 	spin_unlock_irqrestore(ap->lock, flags);
+	ahci_rpm_put_port(ap);
 
 	return size;
 }
@@ -369,7 +406,9 @@ static ssize_t ahci_show_em_supported(struct device *dev,
 	void __iomem *mmio = hpriv->mmio;
 	u32 em_ctl;
 
+	ahci_rpm_get_port(ap);
 	em_ctl = readl(mmio + HOST_EM_CTL);
+	ahci_rpm_put_port(ap);
 
 	return sprintf(buf, "%s%s%s%s\n",
 		       em_ctl & EM_CTL_LED ? "led " : "",
@@ -1010,6 +1049,7 @@ static ssize_t ahci_transmit_led_message(struct ata_port *ap, u32 state,
 	else
 		return -EINVAL;
 
+	ahci_rpm_get_port(ap);
 	spin_lock_irqsave(ap->lock, flags);
 
 	/*
@@ -1019,6 +1059,7 @@ static ssize_t ahci_transmit_led_message(struct ata_port *ap, u32 state,
 	em_ctl = readl(mmio + HOST_EM_CTL);
 	if (em_ctl & EM_CTL_TM) {
 		spin_unlock_irqrestore(ap->lock, flags);
+		ahci_rpm_put_port(ap);
 		return -EBUSY;
 	}
 
@@ -1046,6 +1087,8 @@ static ssize_t ahci_transmit_led_message(struct ata_port *ap, u32 state,
 	emp->led_state = state;
 
 	spin_unlock_irqrestore(ap->lock, flags);
+	ahci_rpm_put_port(ap);
+
 	return size;
 }
 
@@ -2248,6 +2291,8 @@ static void ahci_pmp_detach(struct ata_port *ap)
 
 int ahci_port_resume(struct ata_port *ap)
 {
+	ahci_rpm_get_port(ap);
+
 	ahci_power_up(ap);
 	ahci_start_port(ap);
 
@@ -2274,6 +2319,7 @@ static int ahci_port_suspend(struct ata_port *ap, pm_message_t mesg)
 		ata_port_freeze(ap);
 	}
 
+	ahci_rpm_put_port(ap);
 	return rc;
 }
 #endif
-- 
2.7.0

[PATCH 7/7] ahci: Add runtime PM support for the host controller

[Mika Westerberg]

This patch adds runtime PM support for the AHCI host controller driver so
that the host controller is powered down when all SATA ports are runtime
suspended. Powering down the AHCI host controller can reduce power
consumption and possibly allow the CPU to enter lower power idle states
(S0ix) during runtime.

Runtime PM is blocked by default and needs to be unblocked from userspace
as needed (via power/* sysfs nodes).

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/ata/ahci.c | 73 +++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 61 insertions(+), 12 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 44f9d1c09bbe..fb0c23ae3ce9 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -85,6 +85,7 @@ enum board_ids {
 };
 
 static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent);
+static void ahci_remove_one(struct pci_dev *dev);
 static int ahci_vt8251_hardreset(struct ata_link *link, unsigned int *class,
 				 unsigned long deadline);
 static int ahci_avn_hardreset(struct ata_link *link, unsigned int *class,
@@ -93,10 +94,14 @@ static void ahci_mcp89_apple_enable(struct pci_dev *pdev);
 static bool is_mcp89_apple(struct pci_dev *pdev);
 static int ahci_p5wdh_hardreset(struct ata_link *link, unsigned int *class,
 				unsigned long deadline);
+#ifdef CONFIG_PM
+static int ahci_pci_device_runtime_suspend(struct device *dev);
+static int ahci_pci_device_runtime_resume(struct device *dev);
 #ifdef CONFIG_PM_SLEEP
 static int ahci_pci_device_suspend(struct device *dev);
 static int ahci_pci_device_resume(struct device *dev);
 #endif
+#endif /* CONFIG_PM */
 
 static struct scsi_host_template ahci_sht = {
 	AHCI_SHT("ahci"),
@@ -559,13 +564,15 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 
 static const struct dev_pm_ops ahci_pci_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(ahci_pci_device_suspend, ahci_pci_device_resume)
+	SET_RUNTIME_PM_OPS(ahci_pci_device_runtime_suspend,
+			   ahci_pci_device_runtime_resume, NULL)
 };
 
 static struct pci_driver ahci_pci_driver = {
 	.name			= DRV_NAME,
 	.id_table		= ahci_pci_tbl,
 	.probe			= ahci_init_one,
-	.remove			= ata_pci_remove_one,
+	.remove			= ahci_remove_one,
 	.driver.pm		= &ahci_pci_pm_ops,
 };
 
@@ -794,21 +801,13 @@ static int ahci_avn_hardreset(struct ata_link *link, unsigned int *class,
 }
 
 
-#ifdef CONFIG_PM_SLEEP
-static int ahci_pci_device_suspend(struct device *dev)
+#ifdef CONFIG_PM
+static void ahci_pci_disable_interrupts(struct ata_host *host)
 {
-	struct pci_dev *pdev = to_pci_dev(dev);
-	struct ata_host *host = pci_get_drvdata(pdev);
 	struct ahci_host_priv *hpriv = host->private_data;
 	void __iomem *mmio = hpriv->mmio;
 	u32 ctl;
 
-	if (hpriv->flags & AHCI_HFLAG_NO_SUSPEND) {
-		dev_err(&pdev->dev,
-			"BIOS update required for suspend/resume\n");
-		return -EIO;
-	}
-
 	/* AHCI spec rev1.1 section 8.3.3:
 	 * Software must disable interrupts prior to requesting a
 	 * transition of the HBA to D3 state.
@@ -817,7 +816,44 @@ static int ahci_pci_device_suspend(struct device *dev)
 	ctl &= ~HOST_IRQ_EN;
 	writel(ctl, mmio + HOST_CTL);
 	readl(mmio + HOST_CTL); /* flush */
+}
+
+static int ahci_pci_device_runtime_suspend(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct ata_host *host = pci_get_drvdata(pdev);
 
+	ahci_pci_disable_interrupts(host);
+	return 0;
+}
+
+static int ahci_pci_device_runtime_resume(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct ata_host *host = pci_get_drvdata(pdev);
+	int rc;
+
+	rc = ahci_pci_reset_controller(host);
+	if (rc)
+		return rc;
+	ahci_pci_init_controller(host);
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int ahci_pci_device_suspend(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct ata_host *host = pci_get_drvdata(pdev);
+	struct ahci_host_priv *hpriv = host->private_data;
+
+	if (hpriv->flags & AHCI_HFLAG_NO_SUSPEND) {
+		dev_err(&pdev->dev,
+			"BIOS update required for suspend/resume\n");
+		return -EIO;
+	}
+
+	ahci_pci_disable_interrupts(host);
 	return ata_host_suspend(host, PMSG_SUSPEND);
 }
 
@@ -845,6 +881,8 @@ static int ahci_pci_device_resume(struct device *dev)
 }
 #endif
 
+#endif /* CONFIG_PM */
+
 static int ahci_configure_dma_masks(struct pci_dev *pdev, int using_dac)
 {
 	int rc;
@@ -1664,7 +1702,18 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	pci_set_master(pdev);
 
-	return ahci_host_activate(host, &ahci_sht);
+	rc = ahci_host_activate(host, &ahci_sht);
+	if (rc)
+		return rc;
+
+	pm_runtime_put_noidle(&pdev->dev);
+	return 0;
+}
+
+static void ahci_remove_one(struct pci_dev *pdev)
+{
+	pm_runtime_get_noresume(&pdev->dev);
+	ata_pci_remove_one(pdev);
 }
 
 module_pci_driver(ahci_pci_driver);
-- 
2.7.0

Re: [PATCH 0/7] Runtime PM support for AHCI host controller driver

[Tejun Heo]

Hello,

Patchset looks good to me.  Once Jens acks the first patch, I'll apply
the whole series to libata/for-4.6 w/ the field initialization
updated.

Thanks.

-- 
tejun

Re: [PATCH 0/7] Runtime PM support for AHCI host controller driver

[Mika Westerberg]

On Thu, Feb 18, 2016 at 11:40:29AM -0500, Tejun Heo wrote:
> Hello,
> 
> Patchset looks good to me.  Once Jens acks the first patch, I'll apply
> the whole series to libata/for-4.6 w/ the field initialization
> updated.

Thanks!

Re: [PATCH 0/7] Runtime PM support for AHCI host controller driver

[Jens Axboe]

On 02/18/2016 09:40 AM, Tejun Heo wrote:
> Hello,
>
> Patchset looks good to me.  Once Jens acks the first patch, I'll apply
> the whole series to libata/for-4.6 w/ the field initialization
> updated.

You can add my acked-by to that one, looks good to me.

-- 
Jens Axboe

Re: [PATCH 0/7] Runtime PM support for AHCI host controller driver

[Tejun Heo]

On Thu, Feb 18, 2016 at 10:54:10AM +0200, Mika Westerberg wrote:
> Linux already supports runtime PM of disks (drivers/scsi/sd.c) so that
> after certain amount of idle time the disk is suspended automatically. This
> series extends the support to AHCI host controllers. Whenever SATA ports
> are determined to be idle (all children are runtime suspended) the host
> controller is also suspended.
> 
> On recent Intel CPUs like Broxton this allows the CPU to go low power idle
> states like S0ix runtime (given that all necessary blocks are also in their
> correesponding low power states).
> 
> Patches [1-2/7] fix a lockup where disk is runtime suspended and the system
> is put to sleep. They are independent of the rest of the series.
> 
> Patch [3/7] makes it possible for SATA ports to be runtime suspended when
> there is not disk connected. For example on Lenovo Yoga 900 there are two
> SATA ports which only one of them has disk connected. This patch allows the
> host controller to runtime suspend whenever the disk is idle.

Applied to libata/for-4.6.

Thanks.

-- 
tejun