[RFC][PATCH 0/4] ahci controller runtime PM support

[RFC][PATCH 0/4] ahci controller runtime PM support

[Lin Ming]

Hi all,

The ata port runtime PM support patches have been merged.
Now I move on to ahci controller runtime PM support.

My test enviroment is:
1. use nfsroot, so disk is not mounted. AHCI controller can be runtime suspended.
2. use eSATA to test if hot plug works with port/controller runtime suspended.

I send these RFC patches to get feedbacks.
Any comment is much appriciated.

[RFC][PATCH 1/4] ahci: port legacy pm interface to struct dev_pm_ops
[RFC][PATCH 2/4] ahci: add runtime PM callbacks
[RFC][PATCH 3/4] ata: runtime suspend port if no device attached
[RFC][PATCH 4/4] ahci: support hot plug when port/controller is runtime suspended

Thanks,
Lin Ming

[RFC][PATCH 1/4] ahci: port legacy pm interface to struct dev_pm_ops

[Lin Ming]

So we can add runtime pm callbacks later.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/ata/ahci.c |   61 +++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index cf26222..c745603 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -84,8 +84,9 @@ static int ahci_vt8251_hardreset(struct ata_link *link, unsigned int *class,
 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);
+static int ahci_pci_device_suspend(struct device *dev);
+static int ahci_pci_device_hibernate(struct device *dev);
+static int ahci_pci_device_resume(struct device *dev);
 #endif
 
 static struct scsi_host_template ahci_sht = {
@@ -400,6 +401,11 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ }	/* terminate list */
 };
 
+static struct dev_pm_ops ahci_pci_pm_ops = {
+	.suspend		= ahci_pci_device_suspend,
+	.resume			= ahci_pci_device_resume,
+	.poweroff		= ahci_pci_device_hibernate,
+};
 
 static struct pci_driver ahci_pci_driver = {
 	.name			= DRV_NAME,
@@ -407,8 +413,9 @@ static struct pci_driver ahci_pci_driver = {
 	.probe			= ahci_init_one,
 	.remove			= ata_pci_remove_one,
 #ifdef CONFIG_PM
-	.suspend		= ahci_pci_device_suspend,
-	.resume			= ahci_pci_device_resume,
+	.driver			= {
+		.pm = &ahci_pci_pm_ops
+	},
 #endif
 };
 
@@ -567,37 +574,51 @@ static int ahci_p5wdh_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)
+static int ahci_pci_device_suspend_common(struct pci_dev *pdev, pm_message_t mesg)
 {
 	struct ata_host *host = dev_get_drvdata(&pdev->dev);
 	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) {
+	/* 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);
+}
+
+static int ahci_pci_device_suspend(struct device *dev)
+{
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	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 */
-	}
+	return ahci_pci_device_suspend_common(pdev, PMSG_SUSPEND);
+}
 
-	return ata_pci_device_suspend(pdev, mesg);
+static int ahci_pci_device_hibernate(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return ahci_pci_device_suspend_common(pdev, PMSG_HIBERNATE);
 }
 
-static int ahci_pci_device_resume(struct pci_dev *pdev)
+static int ahci_pci_device_resume(struct device *dev)
 {
-	struct ata_host *host = dev_get_drvdata(&pdev->dev);
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct pci_dev *pdev = to_pci_dev(dev);
 	int rc;
 
 	rc = ata_pci_device_do_resume(pdev);
-- 
1.7.2.5

Re: [RFC][PATCH 1/4] ahci: port legacy pm interface to struct dev_pm_ops

[Rafael J. Wysocki]

On Thursday, December 15, 2011, Lin Ming wrote:
> So we can add runtime pm callbacks later.
> 
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> ---
>  drivers/ata/ahci.c |   61 +++++++++++++++++++++++++++++++++++-----------------
>  1 files changed, 41 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index cf26222..c745603 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -84,8 +84,9 @@ static int ahci_vt8251_hardreset(struct ata_link *link, unsigned int *class,
>  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);
> +static int ahci_pci_device_suspend(struct device *dev);
> +static int ahci_pci_device_hibernate(struct device *dev);
> +static int ahci_pci_device_resume(struct device *dev);
>  #endif
>  
>  static struct scsi_host_template ahci_sht = {
> @@ -400,6 +401,11 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>  	{ }	/* terminate list */
>  };
>  
> +static struct dev_pm_ops ahci_pci_pm_ops = {
> +	.suspend		= ahci_pci_device_suspend,
> +	.resume			= ahci_pci_device_resume,
> +	.poweroff		= ahci_pci_device_hibernate,
> +};

So seem to have forgotten about .restore().  Please define it at least or
resume from hibernation will be broken.

Also, are .freeze() and .thaw() callbacks really unnecessary?

Rafael


>  static struct pci_driver ahci_pci_driver = {
>  	.name			= DRV_NAME,
> @@ -407,8 +413,9 @@ static struct pci_driver ahci_pci_driver = {
>  	.probe			= ahci_init_one,
>  	.remove			= ata_pci_remove_one,
>  #ifdef CONFIG_PM
> -	.suspend		= ahci_pci_device_suspend,
> -	.resume			= ahci_pci_device_resume,
> +	.driver			= {
> +		.pm = &ahci_pci_pm_ops
> +	},
>  #endif
>  };
>  
> @@ -567,37 +574,51 @@ static int ahci_p5wdh_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)
> +static int ahci_pci_device_suspend_common(struct pci_dev *pdev, pm_message_t mesg)
>  {
>  	struct ata_host *host = dev_get_drvdata(&pdev->dev);
>  	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) {
> +	/* 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);
> +}
> +
> +static int ahci_pci_device_suspend(struct device *dev)
> +{
> +	struct ata_host *host = dev_get_drvdata(dev);
> +	struct ahci_host_priv *hpriv = host->private_data;
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	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 */
> -	}
> +	return ahci_pci_device_suspend_common(pdev, PMSG_SUSPEND);
> +}
>  
> -	return ata_pci_device_suspend(pdev, mesg);
> +static int ahci_pci_device_hibernate(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	return ahci_pci_device_suspend_common(pdev, PMSG_HIBERNATE);
>  }
>  
> -static int ahci_pci_device_resume(struct pci_dev *pdev)
> +static int ahci_pci_device_resume(struct device *dev)
>  {
> -	struct ata_host *host = dev_get_drvdata(&pdev->dev);
> +	struct ata_host *host = dev_get_drvdata(dev);
> +	struct pci_dev *pdev = to_pci_dev(dev);
>  	int rc;
>  
>  	rc = ata_pci_device_do_resume(pdev);
> 

Re: [RFC][PATCH 1/4] ahci: port legacy pm interface to struct dev_pm_ops

[Lin Ming]

2011/12/16 Rafael J. Wysocki <rjw@sisk.pl>:
> On Thursday, December 15, 2011, Lin Ming wrote:
>> So we can add runtime pm callbacks later.
>>
>> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
>> ---
>>  drivers/ata/ahci.c |   61 +++++++++++++++++++++++++++++++++++-----------------
>>  1 files changed, 41 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>> index cf26222..c745603 100644
>> --- a/drivers/ata/ahci.c
>> +++ b/drivers/ata/ahci.c
>> @@ -84,8 +84,9 @@ static int ahci_vt8251_hardreset(struct ata_link *link, unsigned int *class,
>>  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);
>> +static int ahci_pci_device_suspend(struct device *dev);
>> +static int ahci_pci_device_hibernate(struct device *dev);
>> +static int ahci_pci_device_resume(struct device *dev);
>>  #endif
>>
>>  static struct scsi_host_template ahci_sht = {
>> @@ -400,6 +401,11 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>>       { }     /* terminate list */
>>  };
>>
>> +static struct dev_pm_ops ahci_pci_pm_ops = {
>> +     .suspend                = ahci_pci_device_suspend,
>> +     .resume                 = ahci_pci_device_resume,
>> +     .poweroff               = ahci_pci_device_hibernate,
>> +};
>
> So seem to have forgotten about .restore().  Please define it at least or
> resume from hibernation will be broken.

Will add it.

>
> Also, are .freeze() and .thaw() callbacks really unnecessary?

I'm not sure for now.
Will test hibernation.

Thanks for the comments.

Lin Ming

>
> Rafael

[RFC][PATCH 2/4] ahci: add runtime PM callbacks

[Lin Ming]

Add ahci controller runtime PM callbacks.

Call pm_runtime_put_noidle() in its probe routine and 
pm_runtime_get_noresume() in its remove routine.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/ata/ahci.c |   24 +++++++++++++++++++++++-
 1 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index c745603..7ab9b0d 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -46,6 +46,7 @@
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_cmnd.h>
 #include <linux/libata.h>
+#include <linux/pm_runtime.h>
 #include "ahci.h"
 
 #define DRV_NAME	"ahci"
@@ -79,6 +80,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 *pdev);
 static int ahci_vt8251_hardreset(struct ata_link *link, unsigned int *class,
 				 unsigned long deadline);
 static int ahci_p5wdh_hardreset(struct ata_link *link, unsigned int *class,
@@ -87,6 +89,7 @@ static int ahci_p5wdh_hardreset(struct ata_link *link, unsigned int *class,
 static int ahci_pci_device_suspend(struct device *dev);
 static int ahci_pci_device_hibernate(struct device *dev);
 static int ahci_pci_device_resume(struct device *dev);
+static int ahci_pci_device_runtime_idle(struct device *dev);
 #endif
 
 static struct scsi_host_template ahci_sht = {
@@ -405,13 +408,16 @@ static struct dev_pm_ops ahci_pci_pm_ops = {
 	.suspend		= ahci_pci_device_suspend,
 	.resume			= ahci_pci_device_resume,
 	.poweroff		= ahci_pci_device_hibernate,
+	.runtime_suspend	= ahci_pci_device_suspend,
+	.runtime_resume		= ahci_pci_device_resume,
+	.runtime_idle		= ahci_pci_device_runtime_idle,
 };
 
 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,
 #ifdef CONFIG_PM
 	.driver			= {
 		.pm = &ahci_pci_pm_ops
@@ -637,6 +643,12 @@ static int ahci_pci_device_resume(struct device *dev)
 
 	return 0;
 }
+
+static int ahci_pci_device_runtime_idle(struct device *dev)
+{
+	return pm_runtime_suspend(dev);
+}
+
 #endif
 
 static int ahci_configure_dma_masks(struct pci_dev *pdev, int using_dac)
@@ -1226,10 +1238,20 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	ahci_pci_print_info(host);
 
 	pci_set_master(pdev);
+	pm_runtime_put_noidle(dev);
+	pm_runtime_allow(dev);
 	return ata_host_activate(host, pdev->irq, ahci_interrupt, IRQF_SHARED,
 				 &ahci_sht);
 }
 
+static void ahci_remove_one(struct pci_dev *pdev)
+{
+	pm_runtime_forbid(&pdev->dev);
+	pm_runtime_get_noresume(&pdev->dev);
+
+	ata_pci_remove_one(pdev);
+}
+
 static int __init ahci_init(void)
 {
 	return pci_register_driver(&ahci_pci_driver);
-- 
1.7.2.5

[RFC][PATCH 3/4] ata: runtime suspend port if no device attached

[Lin Ming]

Add a new function ata_device_probed(...) to check if device was probed.
Runtime suspend scsi_host--->ata port if no device was probed.

Controller will be runtime suspended if all port are suspended already.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/ata/libata-core.c |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 8996758..4bbbd55 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5934,6 +5934,21 @@ int ata_port_probe(struct ata_port *ap)
 	return rc;
 }
 
+/**
+ *	ata_device_probed - Check if device is probed
+ *	@ap:	port to check
+ */
+static bool ata_device_probed(struct ata_port *ap)
+{
+	struct ata_link *link;
+	struct ata_device *dev;
+
+	ata_for_each_link(link, ap, EDGE)
+		ata_for_each_dev(dev, link, ENABLED)
+			return true;
+
+	return false;
+}
 
 static void async_port_probe(void *data, async_cookie_t cookie)
 {
@@ -5954,7 +5969,11 @@ static void async_port_probe(void *data, async_cookie_t cookie)
 	/* in order to keep device order, we need to synchronize at this point */
 	async_synchronize_cookie(cookie);
 
-	ata_scsi_scan_host(ap, 1);
+	if (ata_device_probed(ap))
+		ata_scsi_scan_host(ap, 1);
+	else
+		/* Runtime suspend it if no device is attached */
+		pm_runtime_idle(&ap->scsi_host->shost_gendev);
 }
 
 /**
-- 
1.7.2.5

[RFC][PATCH 4/4] ahci: support hot plug when port/controller is runtime suspended

[Lin Ming]

This is an empty patch now :)

I use eSATA to test disk hot plug.
With previous 3 patches applied, disk hot plug does not wok
when ahci port/controller is runtime suspended.

There are 2 cases we need to handle for hot plug:

1. port suspended, controller active
   Is IRQ need to be enabled for port to detect hot plug? 

2. port suspended, controller suspended
   Will controller get a PME when hot plug happens?

I'm still investigating hot plug support.

Anyone has comment?

Thanks,
Lin Ming

Re: [RFC][PATCH 4/4] ahci: support hot plug when port/controller is runtime suspended

[Tejun Heo]

Hello,

On Thu, Dec 15, 2011 at 09:12:49PM +0800, Lin Ming wrote:
> I use eSATA to test disk hot plug.
> With previous 3 patches applied, disk hot plug does not wok
> when ahci port/controller is runtime suspended.
> 
> There are 2 cases we need to handle for hot plug:
> 
> 1. port suspended, controller active
>    Is IRQ need to be enabled for port to detect hot plug? 
> 
> 2. port suspended, controller suspended
>    Will controller get a PME when hot plug happens?
> 
> I'm still investigating hot plug support.
> 
> Anyone has comment?

SATA link detection requires hot wire and keeping wire hot takes
power.  I think it's fair tradeoff to not support hotplug while
powersaving is on.  We have warm plug mechanism (the SCSI rescan
trigger via sysfs) after all.  It would be nice if things like that is
somehow exported to userland in easy way tho (cc'ing Kay), but I
frankly don't have much idea where that would fit.

Thanks.

-- 
tejun

Re: [RFC][PATCH 4/4] ahci: support hot plug when port/controller is runtime suspended

[Kay Sievers]

On Thu, Dec 15, 2011 at 20:21, Tejun Heo <tj@kernel.org> wrote:
> On Thu, Dec 15, 2011 at 09:12:49PM +0800, Lin Ming wrote:
>> I use eSATA to test disk hot plug.
>> With previous 3 patches applied, disk hot plug does not wok
>> when ahci port/controller is runtime suspended.
>>
>> There are 2 cases we need to handle for hot plug:
>>
>> 1. port suspended, controller active
>>    Is IRQ need to be enabled for port to detect hot plug?
>>
>> 2. port suspended, controller suspended
>>    Will controller get a PME when hot plug happens?
>>
>> I'm still investigating hot plug support.
>>
>> Anyone has comment?
>
> SATA link detection requires hot wire and keeping wire hot takes
> power.

Oh, hardware in general just sucks. :)

> I think it's fair tradeoff to not support hotplug while
> powersaving is on.  We have warm plug mechanism (the SCSI rescan
> trigger via sysfs) after all.  It would be nice if things like that is
> somehow exported to userland in easy way tho (cc'ing Kay), but I
> frankly don't have much idea where that would fit.

Getting David in the loop. He might have an idea.

Kay

Re: [RFC][PATCH 4/4] ahci: support hot plug when port/controller is runtime suspended

[Matthew Garrett]

On Thu, Dec 15, 2011 at 11:21:31AM -0800, Tejun Heo wrote:

> SATA link detection requires hot wire and keeping wire hot takes
> power.  I think it's fair tradeoff to not support hotplug while
> powersaving is on.  We have warm plug mechanism (the SCSI rescan
> trigger via sysfs) after all.  It would be nice if things like that is
> somehow exported to userland in easy way tho (cc'ing Kay), but I
> frankly don't have much idea where that would fit.

We expose the ahci_port_cmd field in sysfs, which lets us know whether a 
port is flagged as hotpluggable or external. Userspace could use that to 
identify whether a given port can be safely powered off or not. It's not 
guaranteed - desktop boards with esata ports will typically not have 
this information available (because the esata port could be plugged into 
any on-board port), so I think leaving it up to userspace to set the 
policy makes sense.

-- 
Matthew Garrett | mjg59@srcf.ucam.org