Rework AHCI LPM handling a little

Rework AHCI LPM handling a little

[Matthew Garrett]

This patchset tries to get us closer to having reasonable defaults for link
power management on AHCI. Recent Intel CPUs can't enter deep power saving
states unless the SATA link is in a low power state, appearing to be limited
to PC6 in PARTIAL and PC7 in SLUMBER, and PC2 otherwise. This amounts to
a difference of several Watts in system idle. There appear to be two
components to how this is managed on Windows:

1) Firmware enables a set of power management features on platform init
2) The Intel Rapid Storage Technology AHCI driver enables a set of features

As far as (1) goes, we ignore all firmware config and start from scratch.
As far as (2) goes, we default to not enabling any power management features
because of concerns about reliability and performance.

These patches stash the firmware configuration at kernel init time, add a new
policy that simply reapplies the firmware configuration and changes the
semantics of the medium_power LPM configuration such that it matches the
configuration enabled by the IRST driver on Windows. We can then apply
udev rules that use the firmware_defaults policy in general and the
medium_power policy where we expect that to work, with a view to changing
the in-kernel defaults down the road if this doesn't appear to cause any
problems.

[PATCH 1/3] libata: Stash initial power management configuration

[Matthew Garrett]

The initial configuration for power management settings may be a result of
the firmware using its knowledge of the installed hardware to program
reasonable defaults. Stash as much of it as possible for later use.

Signed-off-by: Matthew Garrett <mjg59@srcf.ucam.org>
---
 drivers/ata/acard-ahci.c       |  3 +++
 drivers/ata/ahci.c             |  3 +++
 drivers/ata/ahci.h             |  7 +++++++
 drivers/ata/libahci.c          | 46 +++++++++++++++++++++++++++++++++++-------
 drivers/ata/libahci_platform.c |  4 ++++
 drivers/ata/libata-core.c      | 19 ++++++++++-------
 drivers/ata/libata-pmp.c       |  2 +-
 drivers/ata/libata.h           |  2 +-
 drivers/ata/sata_highbank.c    |  4 ++++
 include/linux/libata.h         |  3 +++
 10 files changed, 77 insertions(+), 16 deletions(-)

diff --git a/drivers/ata/acard-ahci.c b/drivers/ata/acard-ahci.c
index 12489ce..0029229 100644
--- a/drivers/ata/acard-ahci.c
+++ b/drivers/ata/acard-ahci.c
@@ -476,6 +476,9 @@ static int acard_ahci_init_one(struct pci_dev *pdev, const struct pci_device_id
 		ata_port_pbar_desc(ap, AHCI_PCI_BAR,
 				   0x100 + ap->port_no * 0x80, "port");
 
+		rc = ahci_setup_port_privdata(ap);
+		if (rc)
+			return rc;
 		/* set initial link pm policy */
 		/*
 		ap->pm_policy = NOT_AVAILABLE;
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index c7a92a7..0f875e2 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1436,6 +1436,9 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		if (ap->flags & ATA_FLAG_EM)
 			ap->em_message_type = hpriv->em_msg_type;
 
+		rc = ahci_setup_port_privdata(ap);
+		if (rc)
+			return rc;
 
 		/* disabled/not-implemented port */
 		if (!(hpriv->port_map & (1 << i)))
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 71262e0..c1a4b6a 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -313,6 +313,12 @@ struct ahci_port_priv {
 	/* enclosure management info per PM slot */
 	struct ahci_em_priv	em_priv[EM_MAX_SLOTS];
 	char			*irq_desc;	/* desc in /proc/interrupts */
+	bool			init_alpe;	/* alpe enabled by default */
+	bool			init_asp;	/* asp enabled by default */
+	bool			init_devslp;	/* devslp enabled by default */
+	u32			init_dito;	/* initial dito configuration */
+	u32			init_deto;	/* initial deto configuration */
+	u32			init_mdat;	/* initial mdat configuration */
 };
 
 struct ahci_host_priv {
@@ -371,6 +377,7 @@ extern struct ata_port_operations ahci_platform_ops;
 extern struct ata_port_operations ahci_pmp_retry_srst_ops;
 
 unsigned int ahci_dev_classify(struct ata_port *ap);
+int ahci_setup_port_privdata(struct ata_port *ap);
 void ahci_fill_cmd_slot(struct ahci_port_priv *pp, unsigned int tag,
 			u32 opts);
 void ahci_save_initial_config(struct device *dev,
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 61a9c07..f0c7120 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2212,19 +2212,53 @@ static int ahci_port_suspend(struct ata_port *ap, pm_message_t mesg)
 }
 #endif
 
+/*
+ * Allocate port privdata and read back initial power management configuration
+ */
+int ahci_setup_port_privdata(struct ata_port *ap)
+{
+	struct ahci_port_priv *pp;
+	u32 cmd, devslp;
+	void __iomem *port_mmio = ahci_port_base(ap);
+
+	pp = kzalloc(sizeof(*pp), GFP_KERNEL);
+	if (!pp)
+		return -ENOMEM;
+
+	ap->private_data = pp;
+
+	cmd = readl(port_mmio + PORT_CMD);
+
+	if (cmd & PORT_CMD_ALPE)
+		pp->init_alpe = true;
+
+	if (cmd & PORT_CMD_ASP)
+		pp->init_asp = true;
+
+	devslp = readl(port_mmio + PORT_DEVSLP);
+
+	/* devslp unsupported or disabled */
+	if (!(devslp & PORT_DEVSLP_DSP) || !(devslp & PORT_DEVSLP_ADSE))
+		return 0;
+
+	pp->init_devslp = true;
+	pp->init_dito = (devslp >> PORT_DEVSLP_DITO_OFFSET) & 0x3ff;
+	pp->init_deto = (devslp >> PORT_DEVSLP_DETO_OFFSET) & 0xff;
+	pp->init_mdat = (devslp >> PORT_DEVSLP_MDAT_OFFSET) & 0x1f;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(ahci_setup_port_privdata);
+
 static int ahci_port_start(struct ata_port *ap)
 {
 	struct ahci_host_priv *hpriv = ap->host->private_data;
+	struct ahci_port_priv *pp = ap->private_data;
 	struct device *dev = ap->host->dev;
-	struct ahci_port_priv *pp;
 	void *mem;
 	dma_addr_t mem_dma;
 	size_t dma_sz, rx_fis_sz;
 
-	pp = devm_kzalloc(dev, sizeof(*pp), GFP_KERNEL);
-	if (!pp)
-		return -ENOMEM;
-
 	if (ap->host->n_ports > 1) {
 		pp->irq_desc = devm_kzalloc(dev, 8, GFP_KERNEL);
 		if (!pp->irq_desc) {
@@ -2303,8 +2337,6 @@ static int ahci_port_start(struct ata_port *ap)
 		ap->lock = &pp->lock;
 	}
 
-	ap->private_data = pp;
-
 	/* engage engines, captain */
 	return ahci_port_resume(ap);
 }
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index d89305d..39946d4 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -563,6 +563,10 @@ int ahci_platform_init_host(struct platform_device *pdev,
 		if (ap->flags & ATA_FLAG_EM)
 			ap->em_message_type = hpriv->em_msg_type;
 
+		rc = ahci_setup_port_privdata(ap);
+		if (rc)
+			return rc;
+
 		/* disabled/not-implemented port */
 		if (!(hpriv->port_map & (1 << i)))
 			ap->ops = &ata_dummy_port_ops;
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index f6cb1f1..c037f33 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2024,6 +2024,9 @@ retry:
 		}
 	}
 
+	if (id[79] & SATA_DIPM)
+		dev->init_dipm = true;
+
 	*p_class = class;
 
 	return 0;
@@ -5583,11 +5586,11 @@ void ata_link_init(struct ata_port *ap, struct ata_link *link, int pmp)
 }
 
 /**
- *	sata_link_init_spd - Initialize link->sata_spd_limit
- *	@link: Link to configure sata_spd_limit for
+ *	sata_link_init_config - Initialize link->sata_spd_limit and init_lpm
+ *	@link: Link to configure sata_spd_limit and init_lpm for
  *
- *	Initialize @link->[hw_]sata_spd_limit to the currently
- *	configured value.
+ *	Initialize @link->[hw_]sata_spd_limit and @link->init_lpm to the
+ *	currently configured value.
  *
  *	LOCKING:
  *	Kernel thread context (may sleep).
@@ -5595,7 +5598,7 @@ void ata_link_init(struct ata_port *ap, struct ata_link *link, int pmp)
  *	RETURNS:
  *	0 on success, -errno on failure.
  */
-int sata_link_init_spd(struct ata_link *link)
+int sata_link_init_config(struct ata_link *link)
 {
 	u8 spd;
 	int rc;
@@ -5612,6 +5615,8 @@ int sata_link_init_spd(struct ata_link *link)
 
 	link->sata_spd_limit = link->hw_sata_spd_limit;
 
+	link->init_lpm = (link->saved_scontrol >> 8) & 0x7;
+
 	return 0;
 }
 
@@ -6161,9 +6166,9 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht)
 			ap->cbl = ATA_CBL_SATA;
 
 		/* init sata_spd_limit to the current value */
-		sata_link_init_spd(&ap->link);
+		sata_link_init_config(&ap->link);
 		if (ap->slave_link)
-			sata_link_init_spd(ap->slave_link);
+			sata_link_init_config(ap->slave_link);
 
 		/* print per-port info to dmesg */
 		xfer_mask = ata_pack_xfermask(ap->pio_mask, ap->mwdma_mask,
diff --git a/drivers/ata/libata-pmp.c b/drivers/ata/libata-pmp.c
index 7ccc084..b9f7ce8 100644
--- a/drivers/ata/libata-pmp.c
+++ b/drivers/ata/libata-pmp.c
@@ -531,7 +531,7 @@ int sata_pmp_attach(struct ata_device *dev)
 		ap->ops->pmp_attach(ap);
 
 	ata_for_each_link(tlink, ap, EDGE)
-		sata_link_init_spd(tlink);
+		sata_link_init_config(tlink);
 
 	return 0;
 
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index a998a17..35016d6 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -99,7 +99,7 @@ extern bool ata_phys_link_online(struct ata_link *link);
 extern bool ata_phys_link_offline(struct ata_link *link);
 extern void ata_dev_init(struct ata_device *dev);
 extern void ata_link_init(struct ata_port *ap, struct ata_link *link, int pmp);
-extern int sata_link_init_spd(struct ata_link *link);
+extern int sata_link_init_config(struct ata_link *link);
 extern int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg);
 extern int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg);
 extern struct ata_port *ata_port_alloc(struct ata_host *host);
diff --git a/drivers/ata/sata_highbank.c b/drivers/ata/sata_highbank.c
index 24e311f..a2adf3f 100644
--- a/drivers/ata/sata_highbank.c
+++ b/drivers/ata/sata_highbank.c
@@ -556,6 +556,10 @@ static int ahci_highbank_probe(struct platform_device *pdev)
 		if (ap->flags & ATA_FLAG_EM)
 			ap->em_message_type = hpriv->em_msg_type;
 
+		rc = ahci_setup_port_privdata(ap);
+		if (rc)
+			goto err0;
+
 		/* disabled/not-implemented port */
 		if (!(hpriv->port_map & (1 << i)))
 			ap->ops = &ata_dummy_port_ops;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 8dad4a3..31c149b 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -719,6 +719,8 @@ struct ata_device {
 	int			spdn_cnt;
 	/* ering is CLEAR_END, read comment above CLEAR_END */
 	struct ata_ering	ering;
+	/* Initial DIPM configuration */
+	bool			init_dipm;
 };
 
 /* Fields between ATA_DEVICE_CLEAR_BEGIN and ATA_DEVICE_CLEAR_END are
@@ -788,6 +790,7 @@ struct ata_link {
 	struct ata_eh_context	eh_context;
 
 	struct ata_device	device[ATA_MAX_DEVICES];
+	u8			init_lpm; /* initial lpm configuration */
 };
 #define ATA_LINK_CLEAR_BEGIN		offsetof(struct ata_link, active_tag)
 #define ATA_LINK_CLEAR_END		offsetof(struct ata_link, device[0])
-- 
2.3.5

Re: [PATCH 1/3] libata: Stash initial power management configuration

[Tejun Heo]

Hello, Matthew.

On Sat, Apr 18, 2015 at 08:26:34AM -0700, Matthew Garrett wrote:
> @@ -313,6 +313,12 @@ struct ahci_port_priv {
>  	/* enclosure management info per PM slot */
>  	struct ahci_em_priv	em_priv[EM_MAX_SLOTS];
>  	char			*irq_desc;	/* desc in /proc/interrupts */

Can you please add a comment here referring to
ahci_setup_port_privdata()?

> +	bool			init_alpe;	/* alpe enabled by default */
> +	bool			init_asp;	/* asp enabled by default */
> +	bool			init_devslp;	/* devslp enabled by default */
> +	u32			init_dito;	/* initial dito configuration */
> +	u32			init_deto;	/* initial deto configuration */
> +	u32			init_mdat;	/* initial mdat configuration */
>  };
...
> +/*
> + * Allocate port privdata and read back initial power management configuration
> + */

And convert this to proper function comment which explains where it's
supposed to be called from and why it does what it does there?

> +int ahci_setup_port_privdata(struct ata_port *ap)
> +{
...
> @@ -5583,11 +5586,11 @@ void ata_link_init(struct ata_port *ap, struct ata_link *link, int pmp)
>  }
>  
>  /**
> - *	sata_link_init_spd - Initialize link->sata_spd_limit
> - *	@link: Link to configure sata_spd_limit for
> + *	sata_link_init_config - Initialize link->sata_spd_limit and init_lpm
> + *	@link: Link to configure sata_spd_limit and init_lpm for
>   *
> - *	Initialize @link->[hw_]sata_spd_limit to the currently
> - *	configured value.
> + *	Initialize @link->[hw_]sata_spd_limit and @link->init_lpm to the
> + *	currently configured value.

And I think it probably would be a better idea to make the libata core
dipm part a separate patch.

Other than that, looks good to me.

Thanks.

-- 
tejun

[PATCH 2/3] libata: Add firmware_default LPM policy

[Matthew Garrett]

System vendors may configure SATA link power management appropriately for
their systems in firmware, based on their knowledge of the hardware
installed. Add an additional LPM policy designed to inherit the
configuration provided by the firmware.

Signed-off-by: Matthew Garrett <mjg59@coreos.com>
---
 .../scsi/link_power_management_policy.txt          |  5 +-
 drivers/ata/libahci.c                              | 62 +++++++++++++++-------
 drivers/ata/libata-core.c                          |  7 ++-
 drivers/ata/libata-eh.c                            | 15 +++---
 drivers/ata/libata-scsi.c                          |  1 +
 include/linux/libata.h                             |  1 +
 6 files changed, 63 insertions(+), 28 deletions(-)

diff --git a/Documentation/scsi/link_power_management_policy.txt b/Documentation/scsi/link_power_management_policy.txt
index d18993d..0285601 100644
--- a/Documentation/scsi/link_power_management_policy.txt
+++ b/Documentation/scsi/link_power_management_policy.txt
@@ -1,8 +1,11 @@
 This parameter allows the user to set the link (interface) power management.
-There are 3 possible options:
+There are 4 possible options:
 
 Value			Effect
 ----------------------------------------------------------------------------
+firmware_defaults	Inherit configuration from the state programmed by
+			the firmware during system init.
+
 min_power		Tell the controller to try to make the link use the
 			least possible power when possible.  This may
 			sacrifice some performance due to increased latency
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index f0c7120..fabcff4 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -684,6 +684,7 @@ static int ahci_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 {
 	struct ata_port *ap = link->ap;
 	struct ahci_host_priv *hpriv = ap->host->private_data;
+	struct ahci_port_priv *ppriv = ap->private_data;
 	struct ahci_port_priv *pp = ap->private_data;
 	void __iomem *port_mmio = ahci_port_base(ap);
 
@@ -701,9 +702,9 @@ static int ahci_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 
 	if (hpriv->cap & HOST_CAP_ALPM) {
 		u32 cmd = readl(port_mmio + PORT_CMD);
+		cmd &= ~(PORT_CMD_ASP | PORT_CMD_ALPE);
 
 		if (policy == ATA_LPM_MAX_POWER || !(hints & ATA_LPM_HIPM)) {
-			cmd &= ~(PORT_CMD_ASP | PORT_CMD_ALPE);
 			cmd |= PORT_CMD_ICC_ACTIVE;
 
 			writel(cmd, port_mmio + PORT_CMD);
@@ -711,6 +712,13 @@ static int ahci_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 
 			/* wait 10ms to be sure we've come out of LPM state */
 			ata_msleep(ap, 10);
+		} else if (policy == ATA_LPM_FIRMWARE_DEFAULTS) {
+			if (ppriv->init_alpe)
+				cmd |= PORT_CMD_ALPE;
+			if (ppriv->init_asp)
+				cmd |= PORT_CMD_ASP;
+
+			writel(cmd, port_mmio + PORT_CMD);
 		} else {
 			cmd |= PORT_CMD_ALPE;
 			if (policy == ATA_LPM_MIN_POWER)
@@ -725,10 +733,17 @@ static int ahci_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 	if ((hpriv->cap2 & HOST_CAP2_SDS) &&
 	    (hpriv->cap2 & HOST_CAP2_SADM) &&
 	    (link->device->flags & ATA_DFLAG_DEVSLP)) {
-		if (policy == ATA_LPM_MIN_POWER)
+		switch (policy) {
+		case ATA_LPM_MIN_POWER:
 			ahci_set_aggressive_devslp(ap, true);
-		else
+			break;
+		case ATA_LPM_FIRMWARE_DEFAULTS:
+			ahci_set_aggressive_devslp(ap, ppriv->init_devslp);
+			break;
+		default:
 			ahci_set_aggressive_devslp(ap, false);
+			break;
+		}
 	}
 
 	if (policy == ATA_LPM_MAX_POWER) {
@@ -1995,6 +2010,7 @@ static void ahci_post_internal_cmd(struct ata_queued_cmd *qc)
 static void ahci_set_aggressive_devslp(struct ata_port *ap, bool sleep)
 {
 	struct ahci_host_priv *hpriv = ap->host->private_data;
+	struct ahci_port_priv *ppriv = ap->private_data;
 	void __iomem *port_mmio = ahci_port_base(ap);
 	struct ata_device *dev = ap->link.device;
 	u32 devslp, dm, dito, mdat, deto;
@@ -2030,26 +2046,32 @@ static void ahci_set_aggressive_devslp(struct ata_port *ap, bool sleep)
 	if (rc)
 		return;
 
-	dm = (devslp & PORT_DEVSLP_DM_MASK) >> PORT_DEVSLP_DM_OFFSET;
-	dito = devslp_idle_timeout / (dm + 1);
-	if (dito > 0x3ff)
-		dito = 0x3ff;
+	if (ppriv->init_devslp) {
+		dito = ppriv->init_dito;
+		deto = ppriv->init_deto;
+		mdat = ppriv->init_mdat;
+	} else {
+		dm = (devslp & PORT_DEVSLP_DM_MASK) >> PORT_DEVSLP_DM_OFFSET;
+		dito = devslp_idle_timeout / (dm + 1);
+		if (dito > 0x3ff)
+			dito = 0x3ff;
 
-	/* Use the nominal value 10 ms if the read MDAT is zero,
-	 * the nominal value of DETO is 20 ms.
-	 */
-	if (dev->devslp_timing[ATA_LOG_DEVSLP_VALID] &
-	    ATA_LOG_DEVSLP_VALID_MASK) {
-		mdat = dev->devslp_timing[ATA_LOG_DEVSLP_MDAT] &
-		       ATA_LOG_DEVSLP_MDAT_MASK;
-		if (!mdat)
+		/* Use the nominal value 10 ms if the read MDAT is zero,
+		 * the nominal value of DETO is 20 ms.
+		 */
+		if (dev->devslp_timing[ATA_LOG_DEVSLP_VALID] &
+		    ATA_LOG_DEVSLP_VALID_MASK) {
+			mdat = dev->devslp_timing[ATA_LOG_DEVSLP_MDAT] &
+				ATA_LOG_DEVSLP_MDAT_MASK;
+			if (!mdat)
+				mdat = 10;
+			deto = dev->devslp_timing[ATA_LOG_DEVSLP_DETO];
+			if (!deto)
+				deto = 20;
+		} else {
 			mdat = 10;
-		deto = dev->devslp_timing[ATA_LOG_DEVSLP_DETO];
-		if (!deto)
 			deto = 20;
-	} else {
-		mdat = 10;
-		deto = 20;
+		}
 	}
 
 	devslp |= ((dito << PORT_DEVSLP_DITO_OFFSET) |
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index c037f33..0a78f01 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2024,7 +2024,7 @@ retry:
 		}
 	}
 
-	if (id[79] & SATA_DIPM)
+	if (id[79] & (1 << SATA_DIPM))
 		dev->init_dipm = true;
 
 	*p_class = class;
@@ -3672,6 +3672,11 @@ int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 		return rc;
 
 	switch (policy) {
+	case ATA_LPM_FIRMWARE_DEFAULTS:
+		/* use the values we read at probe */
+		scontrol &= ~(0x7 << 8);
+		scontrol |= (link->init_lpm << 8);
+		break;
 	case ATA_LPM_MAX_POWER:
 		/* disable all LPM transitions */
 		scontrol |= (0x7 << 8);
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 07f41be..c36fa56 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -3519,9 +3519,9 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 		return 0;
 
 	/*
-	 * DIPM is enabled only for MIN_POWER as some devices
-	 * misbehave when the host NACKs transition to SLUMBER.  Order
-	 * device and link configurations such that the host always
+	 * DIPM is enabled only for MIN_POWER and FIRMWARE_DEFAULT as some
+	 * devices misbehave when the host NACKs transition to SLUMBER.
+	 * Order device and link configurations such that the host always
 	 * allows DIPM requests.
 	 */
 	ata_for_each_dev(dev, link, ENABLED) {
@@ -3581,10 +3581,13 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 	if (ap && ap->slave_link)
 		ap->slave_link->lpm_policy = policy;
 
-	/* host config updated, enable DIPM if transitioning to MIN_POWER */
+	/* host config updated, enable DIPM if transitioning to MIN_POWER or
+	 * FIRMWARE_DEFAULT when enabled by firmware
+	 */
 	ata_for_each_dev(dev, link, ENABLED) {
-		if (policy == ATA_LPM_MIN_POWER && !no_dipm &&
-		    ata_id_has_dipm(dev->id)) {
+		if ((policy == ATA_LPM_MIN_POWER && !no_dipm &&
+		     ata_id_has_dipm(dev->id)) ||
+		    (policy == ATA_LPM_FIRMWARE_DEFAULTS && dev->init_dipm)) {
 			err_mask = ata_dev_set_feature(dev,
 					SETFEATURES_SATA_ENABLE, SATA_DIPM);
 			if (err_mask && err_mask != AC_ERR_DEV) {
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 3131adc..f1ea052 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -107,6 +107,7 @@ static const u8 def_control_mpage[CONTROL_MPAGE_LEN] = {
 static const char *ata_lpm_policy_names[] = {
 	[ATA_LPM_UNKNOWN]	= "max_performance",
 	[ATA_LPM_MAX_POWER]	= "max_performance",
+	[ATA_LPM_FIRMWARE_DEFAULTS] = "firmware_defaults",
 	[ATA_LPM_MED_POWER]	= "medium_power",
 	[ATA_LPM_MIN_POWER]	= "min_power",
 };
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 31c149b..57b465d 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -507,6 +507,7 @@ enum ata_completion_errors {
 enum ata_lpm_policy {
 	ATA_LPM_UNKNOWN,
 	ATA_LPM_MAX_POWER,
+	ATA_LPM_FIRMWARE_DEFAULTS,
 	ATA_LPM_MED_POWER,
 	ATA_LPM_MIN_POWER,
 };
-- 
2.3.5

Re: [PATCH 2/3] libata: Add firmware_default LPM policy

[Tejun Heo]

Hello, Matthew.

On Sat, Apr 18, 2015 at 08:26:35AM -0700, Matthew Garrett wrote:
> +firmware_defaults	Inherit configuration from the state programmed by
> +			the firmware during system init.

Do we lose anything by naming the policy just "firmware"?

> @@ -701,9 +702,9 @@ static int ahci_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
>  
>  	if (hpriv->cap & HOST_CAP_ALPM) {
>  		u32 cmd = readl(port_mmio + PORT_CMD);
> +		cmd &= ~(PORT_CMD_ASP | PORT_CMD_ALPE);

Prolly worth nothing in the changelog?

>  
>  		if (policy == ATA_LPM_MAX_POWER || !(hints & ATA_LPM_HIPM)) {
> -			cmd &= ~(PORT_CMD_ASP | PORT_CMD_ALPE);
>  			cmd |= PORT_CMD_ICC_ACTIVE;
>  
>  			writel(cmd, port_mmio + PORT_CMD);
...
> @@ -2024,7 +2024,7 @@ retry:
>  		}
>  	}
>  
> -	if (id[79] & SATA_DIPM)
> +	if (id[79] & (1 << SATA_DIPM))
>  		dev->init_dipm = true;

Does this chunk belong here?

Thanks.

-- 
tejun

Re: [PATCH 2/3] libata: Add firmware_default LPM policy

[Matthew Garrett]

Ok, I've had one person testing this report the following:

[ 1896.902726] ata1.00: exception Emask 0x0 SAct 0x800 SErr 0x40000 action 0x0
[ 1896.902735] ata1.00: irq_stat 0x40000008
[ 1896.902741] ata1: SError: { CommWake }
[ 1896.902748] ata1.00: failed command: WRITE FPDMA QUEUED
[ 1896.902758] ata1.00: cmd 61/18:58:88:d5:5d/00:00:0d:00:00/40 tag 11
ncq 12288 out
         res 41/04:00:9f:d5:5d/00:00:0d:00:00/40 Emask 0x401 (device error) <F>
[ 1896.902764] ata1.00: status: { DRDY ERR }
[ 1896.902768] ata1.00: error: { ABRT }
[ 1896.912725] ata1.00: supports DRM functions and may not be fully accessible
[ 1896.932716] ata1.00: supports DRM functions and may not be fully accessible
[ 1896.942783] ata1.00: configured for UDMA/133
[ 1896.942815] ata1: EH complete

which seems to occur in batches. I'm a little confused - isn't it
legitimate for the phy to report comwake here?
-- 
Matthew Garrett | matthew.garrett@coreos.com

Re: [PATCH 2/3] libata: Add firmware_default LPM policy

[Tejun Heo]

Hello, Matthew.

On Mon, May 11, 2015 at 01:24:18PM -0700, Matthew Garrett wrote:
> Ok, I've had one person testing this report the following:
> 
> [ 1896.902726] ata1.00: exception Emask 0x0 SAct 0x800 SErr 0x40000 action 0x0
> [ 1896.902735] ata1.00: irq_stat 0x40000008
> [ 1896.902741] ata1: SError: { CommWake }
> [ 1896.902748] ata1.00: failed command: WRITE FPDMA QUEUED
> [ 1896.902758] ata1.00: cmd 61/18:58:88:d5:5d/00:00:0d:00:00/40 tag 11
> ncq 12288 out
>          res 41/04:00:9f:d5:5d/00:00:0d:00:00/40 Emask 0x401 (device error) <F>
> [ 1896.902764] ata1.00: status: { DRDY ERR }
> [ 1896.902768] ata1.00: error: { ABRT }
> [ 1896.912725] ata1.00: supports DRM functions and may not be fully accessible
> [ 1896.932716] ata1.00: supports DRM functions and may not be fully accessible
> [ 1896.942783] ata1.00: configured for UDMA/133
> [ 1896.942815] ata1: EH complete
> 
> which seems to occur in batches. I'm a little confused - isn't it
> legitimate for the phy to report comwake here?

CommWake isn't the problem here.  SError is being dumped just for
information.  The disk is reporting failure on a write command which
is diagnosed as "device error" and thus the link is not reset.  It's
really the device actively reporting command failure.

Thanks.

-- 
tejun

Re: [PATCH 2/3] libata: Add firmware_default LPM policy

[Matthew Garrett]

On Mon, May 11, 2015 at 1:28 PM, Tejun Heo <tj@kernel.org> wrote:

> CommWake isn't the problem here.  SError is being dumped just for
> information.  The disk is reporting failure on a write command which
> is diagnosed as "device error" and thus the link is not reset.  It's
> really the device actively reporting command failure.

Ok, that makes sense. Is there any practical way for us to identify
why the device might be doing that? It seems to be limited to the LPM
case, but this is (theoretically) in the same configuration that the
firmware programmed, so it's a little surprising.

-- 
Matthew Garrett | matthew.garrett@coreos.com

Re: [PATCH 2/3] libata: Add firmware_default LPM policy

[Tejun Heo]

Hello, Matthew.

On Mon, May 11, 2015 at 01:34:40PM -0700, Matthew Garrett wrote:
> On Mon, May 11, 2015 at 1:28 PM, Tejun Heo <tj@kernel.org> wrote:
> 
> > CommWake isn't the problem here.  SError is being dumped just for
> > information.  The disk is reporting failure on a write command which
> > is diagnosed as "device error" and thus the link is not reset.  It's
> > really the device actively reporting command failure.
> 
> Ok, that makes sense. Is there any practical way for us to identify
> why the device might be doing that? It seems to be limited to the LPM
> case, but this is (theoretically) in the same configuration that the
> firmware programmed, so it's a little surprising.

Modern ATA spec do implement extended error reporting and Hannes
recently (not mainline yet) added support for it and the kernel will
print out sense codes if the device reports it ("NCQ Autosense
xx/xx/xx").  Another vector could be SMART error log which is
accessible through smartctl, but it's quite possible that the ABRT bit
is the only thing the device is exposing at all.

Thanks.

-- 
tejun

[PATCH 3/3] libata: Change medium_power LPM policy to match Intel recommendations

[Matthew Garrett]

Intel publish a document on designing energy efficient SATA devices at
http://www.intel.com/content/dam/doc/reference-guide/sata-devices-implementation-recommendations.pdf
which recommends that ALPE be set, ASPE be cleared and that DIPM be enabled
on the device. Right now we have no policy that matches that - medium_power
does not enable DIPM and min_power sets ASPE. Change medium_power to
implement these recommendations, with the addition of devslp state being
inherited from the initial configuration. With luck this will provide
reasonable power savings without causing the device breakages we
occasionally see with the min_power policy.

Signed-off-by: Matthew Garrett <mjg59@coreos.com>
---
 drivers/ata/libahci.c     |  1 +
 drivers/ata/libata-core.c |  4 ----
 drivers/ata/libata-eh.c   | 10 ++++------
 3 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index fabcff4..8efacb9 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -738,6 +738,7 @@ static int ahci_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 			ahci_set_aggressive_devslp(ap, true);
 			break;
 		case ATA_LPM_FIRMWARE_DEFAULTS:
+		case ATA_LPM_MED_POWER:
 			ahci_set_aggressive_devslp(ap, ppriv->init_devslp);
 			break;
 		default:
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 0a78f01..99a7b8f 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3687,10 +3687,6 @@ int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 		}
 		break;
 	case ATA_LPM_MED_POWER:
-		/* allow LPM to PARTIAL */
-		scontrol &= ~(0x1 << 8);
-		scontrol |= (0x6 << 8);
-		break;
 	case ATA_LPM_MIN_POWER:
 		if (ata_link_nr_enabled(link) > 0)
 			/* no restrictions on LPM transitions */
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index c36fa56..25d5f37 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -3519,8 +3519,6 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 		return 0;
 
 	/*
-	 * DIPM is enabled only for MIN_POWER and FIRMWARE_DEFAULT as some
-	 * devices misbehave when the host NACKs transition to SLUMBER.
 	 * Order device and link configurations such that the host always
 	 * allows DIPM requests.
 	 */
@@ -3540,7 +3538,7 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 			hints &= ~ATA_LPM_HIPM;
 
 		/* disable DIPM before changing link config */
-		if (policy != ATA_LPM_MIN_POWER && dipm) {
+		if (policy < ATA_LPM_MED_POWER && dipm) {
 			err_mask = ata_dev_set_feature(dev,
 					SETFEATURES_SATA_DISABLE, SATA_DIPM);
 			if (err_mask && err_mask != AC_ERR_DEV) {
@@ -3581,11 +3579,11 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 	if (ap && ap->slave_link)
 		ap->slave_link->lpm_policy = policy;
 
-	/* host config updated, enable DIPM if transitioning to MIN_POWER or
-	 * FIRMWARE_DEFAULT when enabled by firmware
+	/* host config updated, enable DIPM if transitioning to MED_POWER,
+	 * MIN_POWER or FIRMWARE_DEFAULT when enabled by firmware
 	 */
 	ata_for_each_dev(dev, link, ENABLED) {
-		if ((policy == ATA_LPM_MIN_POWER && !no_dipm &&
+		if ((policy >= ATA_LPM_MED_POWER && !no_dipm &&
 		     ata_id_has_dipm(dev->id)) ||
 		    (policy == ATA_LPM_FIRMWARE_DEFAULTS && dev->init_dipm)) {
 			err_mask = ata_dev_set_feature(dev,
-- 
2.3.5

Re: [PATCH 3/3] libata: Change medium_power LPM policy to match Intel recommendations

[Tejun Heo]

On Sat, Apr 18, 2015 at 08:26:36AM -0700, Matthew Garrett wrote:
> Intel publish a document on designing energy efficient SATA devices at
> http://www.intel.com/content/dam/doc/reference-guide/sata-devices-implementation-recommendations.pdf
> which recommends that ALPE be set, ASPE be cleared and that DIPM be enabled
> on the device. Right now we have no policy that matches that - medium_power
> does not enable DIPM and min_power sets ASPE. Change medium_power to
> implement these recommendations, with the addition of devslp state being
> inherited from the initial configuration. With luck this will provide
> reasonable power savings without causing the device breakages we
> occasionally see with the min_power policy.

Can we add comment explaining what med_power implements and link to
the above?

Thanks.

-- 
tejun