[PATCH v2 1/2] ata: ahci: Drop low power policy board type

[PATCH v2 1/2] ata: ahci: Drop low power policy board type

[Mario Limonciello]

The low power policy board type was introduced to allow systems
to get into deep states reliably.  Before it was introduced `min_power`
was causing problems for a number of drives.  New power policies
`min_power_with_partial` and `med_power_with_dipm` have been introduced
which provide a more stable baseline for systems.

As discussed in the RFC, if this patch introduces regressions they need
to be examined closely to decide if reverting makes sense. Some things
that make sense to look at:

1. Check what modes are set by the disk
2. Check what policy is set by the user in kernel config
   (or module parameter)
3. Verify the firmware version of the system and check whether updating
   it helps
4. Check the firmware version of the disk and whether updating it helps

The outputs from these can be used for building a disallow list rather
than a flat revert of this commit.  If that list grows too large, it might
make sense to revert however.

Suggested-by: Christoph Hellwig <hch@infradead.org>
Acked-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
Changes from v1->v2:
 * Add Chris' tag
 drivers/ata/Kconfig |   2 +-
 drivers/ata/ahci.c  | 105 +++++++++++++++++++-------------------------
 drivers/ata/ahci.h  |   9 ++--
 3 files changed, 50 insertions(+), 66 deletions(-)

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index cb9e71b4ca4d..3624d56d32b4 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -116,7 +116,7 @@ config SATA_AHCI
 	  If unsure, say N.
 
 config SATA_LPM_POLICY
-	int "Default SATA Link Power Management policy for low power chipsets"
+	int "Default SATA Link Power Management policy"
 	range 0 4
 	default 0
 	depends on SATA_AHCI
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index ffaad9eaa79d..17d757ad7111 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -50,7 +50,6 @@ enum board_ids {
 	/* board IDs by feature in alphabetical order */
 	board_ahci,
 	board_ahci_ign_iferr,
-	board_ahci_low_power,
 	board_ahci_no_debounce_delay,
 	board_ahci_nomsi,
 	board_ahci_noncq,
@@ -135,13 +134,6 @@ static const struct ata_port_info ahci_port_info[] = {
 		.udma_mask	= ATA_UDMA6,
 		.port_ops	= &ahci_ops,
 	},
-	[board_ahci_low_power] = {
-		AHCI_HFLAGS	(AHCI_HFLAG_USE_LPM_POLICY),
-		.flags		= AHCI_FLAG_COMMON,
-		.pio_mask	= ATA_PIO4,
-		.udma_mask	= ATA_UDMA6,
-		.port_ops	= &ahci_ops,
-	},
 	[board_ahci_no_debounce_delay] = {
 		.flags		= AHCI_FLAG_COMMON,
 		.link_flags	= ATA_LFLAG_NO_DEBOUNCE_DELAY,
@@ -275,13 +267,13 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ PCI_VDEVICE(INTEL, 0x2924), board_ahci }, /* ICH9 */
 	{ PCI_VDEVICE(INTEL, 0x2925), board_ahci }, /* ICH9 */
 	{ PCI_VDEVICE(INTEL, 0x2927), board_ahci }, /* ICH9 */
-	{ PCI_VDEVICE(INTEL, 0x2929), board_ahci_low_power }, /* ICH9M */
-	{ PCI_VDEVICE(INTEL, 0x292a), board_ahci_low_power }, /* ICH9M */
-	{ PCI_VDEVICE(INTEL, 0x292b), board_ahci_low_power }, /* ICH9M */
-	{ PCI_VDEVICE(INTEL, 0x292c), board_ahci_low_power }, /* ICH9M */
-	{ PCI_VDEVICE(INTEL, 0x292f), board_ahci_low_power }, /* ICH9M */
+	{ PCI_VDEVICE(INTEL, 0x2929), board_ahci }, /* ICH9M */
+	{ PCI_VDEVICE(INTEL, 0x292a), board_ahci }, /* ICH9M */
+	{ PCI_VDEVICE(INTEL, 0x292b), board_ahci }, /* ICH9M */
+	{ PCI_VDEVICE(INTEL, 0x292c), board_ahci }, /* ICH9M */
+	{ PCI_VDEVICE(INTEL, 0x292f), board_ahci }, /* ICH9M */
 	{ PCI_VDEVICE(INTEL, 0x294d), board_ahci }, /* ICH9 */
-	{ PCI_VDEVICE(INTEL, 0x294e), board_ahci_low_power }, /* ICH9M */
+	{ PCI_VDEVICE(INTEL, 0x294e), board_ahci }, /* ICH9M */
 	{ PCI_VDEVICE(INTEL, 0x502a), board_ahci }, /* Tolapai */
 	{ PCI_VDEVICE(INTEL, 0x502b), board_ahci }, /* Tolapai */
 	{ PCI_VDEVICE(INTEL, 0x3a05), board_ahci }, /* ICH10 */
@@ -291,9 +283,9 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ PCI_VDEVICE(INTEL, 0x3b23), board_ahci }, /* PCH AHCI */
 	{ PCI_VDEVICE(INTEL, 0x3b24), board_ahci }, /* PCH RAID */
 	{ PCI_VDEVICE(INTEL, 0x3b25), board_ahci }, /* PCH RAID */
-	{ PCI_VDEVICE(INTEL, 0x3b29), board_ahci_low_power }, /* PCH M AHCI */
+	{ PCI_VDEVICE(INTEL, 0x3b29), board_ahci }, /* PCH M AHCI */
 	{ PCI_VDEVICE(INTEL, 0x3b2b), board_ahci }, /* PCH RAID */
-	{ PCI_VDEVICE(INTEL, 0x3b2c), board_ahci_low_power }, /* PCH M RAID */
+	{ PCI_VDEVICE(INTEL, 0x3b2c), board_ahci }, /* PCH M RAID */
 	{ PCI_VDEVICE(INTEL, 0x3b2f), board_ahci }, /* PCH AHCI */
 	{ PCI_VDEVICE(INTEL, 0x19b0), board_ahci_pcs7 }, /* DNV AHCI */
 	{ PCI_VDEVICE(INTEL, 0x19b1), board_ahci_pcs7 }, /* DNV AHCI */
@@ -316,9 +308,9 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ PCI_VDEVICE(INTEL, 0x19cE), board_ahci_pcs7 }, /* DNV AHCI */
 	{ PCI_VDEVICE(INTEL, 0x19cF), board_ahci_pcs7 }, /* DNV AHCI */
 	{ PCI_VDEVICE(INTEL, 0x1c02), board_ahci }, /* CPT AHCI */
-	{ PCI_VDEVICE(INTEL, 0x1c03), board_ahci_low_power }, /* CPT M AHCI */
+	{ PCI_VDEVICE(INTEL, 0x1c03), board_ahci }, /* CPT M AHCI */
 	{ PCI_VDEVICE(INTEL, 0x1c04), board_ahci }, /* CPT RAID */
-	{ PCI_VDEVICE(INTEL, 0x1c05), board_ahci_low_power }, /* CPT M RAID */
+	{ PCI_VDEVICE(INTEL, 0x1c05), board_ahci }, /* CPT M RAID */
 	{ PCI_VDEVICE(INTEL, 0x1c06), board_ahci }, /* CPT RAID */
 	{ PCI_VDEVICE(INTEL, 0x1c07), board_ahci }, /* CPT RAID */
 	{ PCI_VDEVICE(INTEL, 0x1d02), board_ahci }, /* PBG AHCI */
@@ -327,29 +319,29 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ PCI_VDEVICE(INTEL, 0x2826), board_ahci }, /* PBG/Lewisburg RAID*/
 	{ PCI_VDEVICE(INTEL, 0x2323), board_ahci }, /* DH89xxCC AHCI */
 	{ PCI_VDEVICE(INTEL, 0x1e02), board_ahci }, /* Panther Point AHCI */
-	{ PCI_VDEVICE(INTEL, 0x1e03), board_ahci_low_power }, /* Panther M AHCI */
+	{ PCI_VDEVICE(INTEL, 0x1e03), board_ahci }, /* Panther M AHCI */
 	{ PCI_VDEVICE(INTEL, 0x1e04), board_ahci }, /* Panther Point RAID */
 	{ PCI_VDEVICE(INTEL, 0x1e05), board_ahci }, /* Panther Point RAID */
 	{ PCI_VDEVICE(INTEL, 0x1e06), board_ahci }, /* Panther Point RAID */
-	{ PCI_VDEVICE(INTEL, 0x1e07), board_ahci_low_power }, /* Panther M RAID */
+	{ PCI_VDEVICE(INTEL, 0x1e07), board_ahci }, /* Panther M RAID */
 	{ PCI_VDEVICE(INTEL, 0x1e0e), board_ahci }, /* Panther Point RAID */
 	{ PCI_VDEVICE(INTEL, 0x8c02), board_ahci }, /* Lynx Point AHCI */
-	{ PCI_VDEVICE(INTEL, 0x8c03), board_ahci_low_power }, /* Lynx M AHCI */
+	{ PCI_VDEVICE(INTEL, 0x8c03), board_ahci }, /* Lynx M AHCI */
 	{ PCI_VDEVICE(INTEL, 0x8c04), board_ahci }, /* Lynx Point RAID */
-	{ PCI_VDEVICE(INTEL, 0x8c05), board_ahci_low_power }, /* Lynx M RAID */
+	{ PCI_VDEVICE(INTEL, 0x8c05), board_ahci }, /* Lynx M RAID */
 	{ PCI_VDEVICE(INTEL, 0x8c06), board_ahci }, /* Lynx Point RAID */
-	{ PCI_VDEVICE(INTEL, 0x8c07), board_ahci_low_power }, /* Lynx M RAID */
+	{ PCI_VDEVICE(INTEL, 0x8c07), board_ahci }, /* Lynx M RAID */
 	{ PCI_VDEVICE(INTEL, 0x8c0e), board_ahci }, /* Lynx Point RAID */
-	{ PCI_VDEVICE(INTEL, 0x8c0f), board_ahci_low_power }, /* Lynx M RAID */
-	{ PCI_VDEVICE(INTEL, 0x9c02), board_ahci_low_power }, /* Lynx LP AHCI */
-	{ PCI_VDEVICE(INTEL, 0x9c03), board_ahci_low_power }, /* Lynx LP AHCI */
-	{ PCI_VDEVICE(INTEL, 0x9c04), board_ahci_low_power }, /* Lynx LP RAID */
-	{ PCI_VDEVICE(INTEL, 0x9c05), board_ahci_low_power }, /* Lynx LP RAID */
-	{ PCI_VDEVICE(INTEL, 0x9c06), board_ahci_low_power }, /* Lynx LP RAID */
-	{ PCI_VDEVICE(INTEL, 0x9c07), board_ahci_low_power }, /* Lynx LP RAID */
-	{ PCI_VDEVICE(INTEL, 0x9c0e), board_ahci_low_power }, /* Lynx LP RAID */
-	{ PCI_VDEVICE(INTEL, 0x9c0f), board_ahci_low_power }, /* Lynx LP RAID */
-	{ PCI_VDEVICE(INTEL, 0x9dd3), board_ahci_low_power }, /* Cannon Lake PCH-LP AHCI */
+	{ PCI_VDEVICE(INTEL, 0x8c0f), board_ahci }, /* Lynx M RAID */
+	{ PCI_VDEVICE(INTEL, 0x9c02), board_ahci }, /* Lynx LP AHCI */
+	{ PCI_VDEVICE(INTEL, 0x9c03), board_ahci }, /* Lynx LP AHCI */
+	{ PCI_VDEVICE(INTEL, 0x9c04), board_ahci }, /* Lynx LP RAID */
+	{ PCI_VDEVICE(INTEL, 0x9c05), board_ahci }, /* Lynx LP RAID */
+	{ PCI_VDEVICE(INTEL, 0x9c06), board_ahci }, /* Lynx LP RAID */
+	{ PCI_VDEVICE(INTEL, 0x9c07), board_ahci }, /* Lynx LP RAID */
+	{ PCI_VDEVICE(INTEL, 0x9c0e), board_ahci }, /* Lynx LP RAID */
+	{ PCI_VDEVICE(INTEL, 0x9c0f), board_ahci }, /* Lynx LP RAID */
+	{ PCI_VDEVICE(INTEL, 0x9dd3), board_ahci }, /* Cannon Lake PCH-LP AHCI */
 	{ PCI_VDEVICE(INTEL, 0x1f22), board_ahci }, /* Avoton AHCI */
 	{ PCI_VDEVICE(INTEL, 0x1f23), board_ahci }, /* Avoton AHCI */
 	{ PCI_VDEVICE(INTEL, 0x1f24), board_ahci }, /* Avoton RAID */
@@ -381,26 +373,26 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ PCI_VDEVICE(INTEL, 0x8d66), board_ahci }, /* Wellsburg RAID */
 	{ PCI_VDEVICE(INTEL, 0x8d6e), board_ahci }, /* Wellsburg RAID */
 	{ PCI_VDEVICE(INTEL, 0x23a3), board_ahci }, /* Coleto Creek AHCI */
-	{ PCI_VDEVICE(INTEL, 0x9c83), board_ahci_low_power }, /* Wildcat LP AHCI */
-	{ PCI_VDEVICE(INTEL, 0x9c85), board_ahci_low_power }, /* Wildcat LP RAID */
-	{ PCI_VDEVICE(INTEL, 0x9c87), board_ahci_low_power }, /* Wildcat LP RAID */
-	{ PCI_VDEVICE(INTEL, 0x9c8f), board_ahci_low_power }, /* Wildcat LP RAID */
+	{ PCI_VDEVICE(INTEL, 0x9c83), board_ahci }, /* Wildcat LP AHCI */
+	{ PCI_VDEVICE(INTEL, 0x9c85), board_ahci }, /* Wildcat LP RAID */
+	{ PCI_VDEVICE(INTEL, 0x9c87), board_ahci }, /* Wildcat LP RAID */
+	{ PCI_VDEVICE(INTEL, 0x9c8f), board_ahci }, /* Wildcat LP RAID */
 	{ PCI_VDEVICE(INTEL, 0x8c82), board_ahci }, /* 9 Series AHCI */
-	{ PCI_VDEVICE(INTEL, 0x8c83), board_ahci_low_power }, /* 9 Series M AHCI */
+	{ PCI_VDEVICE(INTEL, 0x8c83), board_ahci }, /* 9 Series M AHCI */
 	{ PCI_VDEVICE(INTEL, 0x8c84), board_ahci }, /* 9 Series RAID */
-	{ PCI_VDEVICE(INTEL, 0x8c85), board_ahci_low_power }, /* 9 Series M RAID */
+	{ PCI_VDEVICE(INTEL, 0x8c85), board_ahci }, /* 9 Series M RAID */
 	{ PCI_VDEVICE(INTEL, 0x8c86), board_ahci }, /* 9 Series RAID */
-	{ PCI_VDEVICE(INTEL, 0x8c87), board_ahci_low_power }, /* 9 Series M RAID */
+	{ PCI_VDEVICE(INTEL, 0x8c87), board_ahci }, /* 9 Series M RAID */
 	{ PCI_VDEVICE(INTEL, 0x8c8e), board_ahci }, /* 9 Series RAID */
-	{ PCI_VDEVICE(INTEL, 0x8c8f), board_ahci_low_power }, /* 9 Series M RAID */
-	{ PCI_VDEVICE(INTEL, 0x9d03), board_ahci_low_power }, /* Sunrise LP AHCI */
-	{ PCI_VDEVICE(INTEL, 0x9d05), board_ahci_low_power }, /* Sunrise LP RAID */
-	{ PCI_VDEVICE(INTEL, 0x9d07), board_ahci_low_power }, /* Sunrise LP RAID */
+	{ PCI_VDEVICE(INTEL, 0x8c8f), board_ahci }, /* 9 Series M RAID */
+	{ PCI_VDEVICE(INTEL, 0x9d03), board_ahci }, /* Sunrise LP AHCI */
+	{ PCI_VDEVICE(INTEL, 0x9d05), board_ahci }, /* Sunrise LP RAID */
+	{ PCI_VDEVICE(INTEL, 0x9d07), board_ahci }, /* Sunrise LP RAID */
 	{ PCI_VDEVICE(INTEL, 0xa102), board_ahci }, /* Sunrise Point-H AHCI */
-	{ PCI_VDEVICE(INTEL, 0xa103), board_ahci_low_power }, /* Sunrise M AHCI */
+	{ PCI_VDEVICE(INTEL, 0xa103), board_ahci }, /* Sunrise M AHCI */
 	{ PCI_VDEVICE(INTEL, 0xa105), board_ahci }, /* Sunrise Point-H RAID */
 	{ PCI_VDEVICE(INTEL, 0xa106), board_ahci }, /* Sunrise Point-H RAID */
-	{ PCI_VDEVICE(INTEL, 0xa107), board_ahci_low_power }, /* Sunrise M RAID */
+	{ PCI_VDEVICE(INTEL, 0xa107), board_ahci }, /* Sunrise M RAID */
 	{ PCI_VDEVICE(INTEL, 0xa10f), board_ahci }, /* Sunrise Point-H RAID */
 	{ PCI_VDEVICE(INTEL, 0xa182), board_ahci }, /* Lewisburg AHCI*/
 	{ PCI_VDEVICE(INTEL, 0xa186), board_ahci }, /* Lewisburg RAID*/
@@ -413,13 +405,13 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ PCI_VDEVICE(INTEL, 0xa356), board_ahci }, /* Cannon Lake PCH-H RAID */
 	{ PCI_VDEVICE(INTEL, 0x06d7), board_ahci }, /* Comet Lake-H RAID */
 	{ PCI_VDEVICE(INTEL, 0xa386), board_ahci }, /* Comet Lake PCH-V RAID */
-	{ PCI_VDEVICE(INTEL, 0x0f22), board_ahci_low_power }, /* Bay Trail AHCI */
-	{ PCI_VDEVICE(INTEL, 0x0f23), board_ahci_low_power }, /* Bay Trail AHCI */
-	{ PCI_VDEVICE(INTEL, 0x22a3), board_ahci_low_power }, /* Cherry Tr. AHCI */
-	{ PCI_VDEVICE(INTEL, 0x5ae3), board_ahci_low_power }, /* ApolloLake AHCI */
-	{ PCI_VDEVICE(INTEL, 0x34d3), board_ahci_low_power }, /* Ice Lake LP AHCI */
-	{ PCI_VDEVICE(INTEL, 0x02d3), board_ahci_low_power }, /* Comet Lake PCH-U AHCI */
-	{ PCI_VDEVICE(INTEL, 0x02d7), board_ahci_low_power }, /* Comet Lake PCH RAID */
+	{ PCI_VDEVICE(INTEL, 0x0f22), board_ahci }, /* Bay Trail AHCI */
+	{ PCI_VDEVICE(INTEL, 0x0f23), board_ahci }, /* Bay Trail AHCI */
+	{ PCI_VDEVICE(INTEL, 0x22a3), board_ahci }, /* Cherry Tr. AHCI */
+	{ PCI_VDEVICE(INTEL, 0x5ae3), board_ahci }, /* ApolloLake AHCI */
+	{ PCI_VDEVICE(INTEL, 0x34d3), board_ahci }, /* Ice Lake LP AHCI */
+	{ PCI_VDEVICE(INTEL, 0x02d3), board_ahci }, /* Comet Lake PCH-U AHCI */
+	{ PCI_VDEVICE(INTEL, 0x02d7), board_ahci }, /* Comet Lake PCH RAID */
 
 	/* JMicron 360/1/3/5/6, match class to avoid IDE function */
 	{ PCI_VENDOR_ID_JMICRON, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
@@ -447,7 +439,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */
 	{ PCI_VDEVICE(AMD, 0x7801), board_ahci_no_debounce_delay }, /* AMD Hudson-2 (AHCI mode) */
 	{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
-	{ PCI_VDEVICE(AMD, 0x7901), board_ahci_low_power }, /* AMD Green Sardine */
+	{ PCI_VDEVICE(AMD, 0x7901), board_ahci }, /* AMD Green Sardine */
 	/* AMD is using RAID class only for ahci controllers */
 	{ PCI_VENDOR_ID_AMD, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
 	  PCI_CLASS_STORAGE_RAID << 8, 0xffffff, board_ahci },
@@ -1594,11 +1586,6 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap,
 {
 	int policy = CONFIG_SATA_LPM_POLICY;
 
-
-	/* Ignore processing for chipsets that don't use policy */
-	if (!(hpriv->flags & AHCI_HFLAG_USE_LPM_POLICY))
-		return;
-
 	/* user modified policy via module param */
 	if (mobile_lpm_policy != -1) {
 		policy = mobile_lpm_policy;
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 5badbaca05a0..a2ca78e1df5c 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -235,14 +235,11 @@ enum {
 	AHCI_HFLAG_YES_ALPM		= (1 << 23), /* force ALPM cap on */
 	AHCI_HFLAG_NO_WRITE_TO_RO	= (1 << 24), /* don't write to read
 							only registers */
-	AHCI_HFLAG_USE_LPM_POLICY	= (1 << 25), /* chipset that should use
-							SATA_LPM_POLICY
-							as default lpm_policy */
-	AHCI_HFLAG_SUSPEND_PHYS		= (1 << 26), /* handle PHYs during
+	AHCI_HFLAG_SUSPEND_PHYS		= (1 << 25), /* handle PHYs during
 							suspend/resume */
-	AHCI_HFLAG_IGN_NOTSUPP_POWER_ON	= (1 << 27), /* ignore -EOPNOTSUPP
+	AHCI_HFLAG_IGN_NOTSUPP_POWER_ON	= (1 << 26), /* ignore -EOPNOTSUPP
 							from phy_power_on() */
-	AHCI_HFLAG_NO_SXS		= (1 << 28), /* SXS not supported */
+	AHCI_HFLAG_NO_SXS		= (1 << 27), /* SXS not supported */
 
 	/* ap->flags bits */
 
-- 
2.34.1

[PATCH v2 2/2] ata: ahci: Protect users from setting policies their drives don't support

[Mario Limonciello]

As the default low power policy applies to more chipsets and drives, it's
important to make sure that drives actually support the policy that a user
selected in their kernel configuration.

If the drive doesn't support slumber, don't let the default policies
dependent upon slumber (`min_power` or `min_power_with_partial`) affect the
disk.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
Changes from v1->v2:
* Move deeper into codepaths
* Reset to MED_POWER rather than ignore
 drivers/ata/libata-sata.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 071158c0c44c..0dc03888c62b 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -13,6 +13,7 @@
 #include <scsi/scsi_device.h>
 #include <linux/libata.h>
 
+#include "ahci.h"
 #include "libata.h"
 #include "libata-transport.h"
 
@@ -368,10 +369,20 @@ int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 		      bool spm_wakeup)
 {
 	struct ata_eh_context *ehc = &link->eh_context;
+	struct ata_port *ap = link->ap;
+	struct ahci_host_priv *hpriv;
 	bool woken_up = false;
 	u32 scontrol;
 	int rc;
 
+	hpriv = ap->host->private_data;
+	if (policy >= ATA_LPM_MIN_POWER_WITH_PARTIAL &&
+	  !(hpriv->cap & HOST_CAP_SSC)) {
+		dev_warn(ap->host->dev,
+			"This drive doesn't support slumber; restting policy to MED_POWER\n");
+		policy = ATA_LPM_MED_POWER;
+	}
+
 	rc = sata_scr_read(link, SCR_CONTROL, &scontrol);
 	if (rc)
 		return rc;
-- 
2.34.1

Re: [PATCH v2 2/2] ata: ahci: Protect users from setting policies their drives don't support

[Damien Le Moal]

On 2022/03/03 5:49, Mario Limonciello wrote:
> As the default low power policy applies to more chipsets and drives, it's
> important to make sure that drives actually support the policy that a user
> selected in their kernel configuration.
> 
> If the drive doesn't support slumber, don't let the default policies
> dependent upon slumber (`min_power` or `min_power_with_partial`) affect the
> disk.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> Changes from v1->v2:
> * Move deeper into codepaths
> * Reset to MED_POWER rather than ignore
>  drivers/ata/libata-sata.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 071158c0c44c..0dc03888c62b 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -13,6 +13,7 @@
>  #include <scsi/scsi_device.h>
>  #include <linux/libata.h>
>  
> +#include "ahci.h"
>  #include "libata.h"
>  #include "libata-transport.h"
>  
> @@ -368,10 +369,20 @@ int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy,
>  		      bool spm_wakeup)
>  {
>  	struct ata_eh_context *ehc = &link->eh_context;
> +	struct ata_port *ap = link->ap;
> +	struct ahci_host_priv *hpriv;
>  	bool woken_up = false;
>  	u32 scontrol;
>  	int rc;
>  
> +	hpriv = ap->host->private_data;
> +	if (policy >= ATA_LPM_MIN_POWER_WITH_PARTIAL &&
> +	  !(hpriv->cap & HOST_CAP_SSC)) {

Weird indentation.

> +		dev_warn(ap->host->dev,
> +			"This drive doesn't support slumber; restting policy to MED_POWER\n");

Typo. I would change this to:

"This device does not support slumber; defaulting to MED_POWER policy\n"

> +		policy = ATA_LPM_MED_POWER;
> +	}
> +
>  	rc = sata_scr_read(link, SCR_CONTROL, &scontrol);
>  	if (rc)
>  		return rc;


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 2/2] ata: ahci: Protect users from setting policies their drives don't support

[Damien Le Moal]

On 3/3/22 12:49, Mario Limonciello wrote:
> As the default low power policy applies to more chipsets and drives, it's
> important to make sure that drives actually support the policy that a user
> selected in their kernel configuration.
> 
> If the drive doesn't support slumber, don't let the default policies
> dependent upon slumber (`min_power` or `min_power_with_partial`) affect the
> disk.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Mario,

Can you resend a rebased version of this, on top of libata for-5.19 branch ?

> ---
> Changes from v1->v2:
> * Move deeper into codepaths
> * Reset to MED_POWER rather than ignore
>   drivers/ata/libata-sata.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 071158c0c44c..0dc03888c62b 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -13,6 +13,7 @@
>   #include <scsi/scsi_device.h>
>   #include <linux/libata.h>
>   
> +#include "ahci.h"
>   #include "libata.h"
>   #include "libata-transport.h"
>   
> @@ -368,10 +369,20 @@ int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy,
>   		      bool spm_wakeup)
>   {
>   	struct ata_eh_context *ehc = &link->eh_context;
> +	struct ata_port *ap = link->ap;
> +	struct ahci_host_priv *hpriv;
>   	bool woken_up = false;
>   	u32 scontrol;
>   	int rc;
>   
> +	hpriv = ap->host->private_data;
> +	if (policy >= ATA_LPM_MIN_POWER_WITH_PARTIAL &&
> +	  !(hpriv->cap & HOST_CAP_SSC)) {
> +		dev_warn(ap->host->dev,
> +			"This drive doesn't support slumber; restting policy to MED_POWER\n");

Typo here: s/restting/resetting. Also, s/doesn't/does not.

> +		policy = ATA_LPM_MED_POWER;

Here, shouldn't we use the default policy defined by 
CONFIG_SATA_LPM_POLICY ?

> +	}
> +
>   	rc = sata_scr_read(link, SCR_CONTROL, &scontrol);
>   	if (rc)
>   		return rc;


-- 
Damien Le Moal
Western Digital Research

RE: [PATCH v2 2/2] ata: ahci: Protect users from setting policies their drives don't support

[Limonciello, Mario]

[AMD Official Use Only]



> -----Original Message-----
> From: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Sent: Sunday, April 3, 2022 20:11
> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: open list:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers) <linux-
> ide@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>;
> hdegoede@redhat.com
> Subject: Re: [PATCH v2 2/2] ata: ahci: Protect users from setting policies their
> drives don't support
> 
> On 3/3/22 12:49, Mario Limonciello wrote:
> > As the default low power policy applies to more chipsets and drives, it's
> > important to make sure that drives actually support the policy that a user
> > selected in their kernel configuration.
> >
> > If the drive doesn't support slumber, don't let the default policies
> > dependent upon slumber (`min_power` or `min_power_with_partial`) affect
> the
> > disk.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> 
> Mario,
> 
> Can you resend a rebased version of this, on top of libata for-5.19 branch ?


OK.

> 
> > ---
> > Changes from v1->v2:
> > * Move deeper into codepaths
> > * Reset to MED_POWER rather than ignore
> >   drivers/ata/libata-sata.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> > index 071158c0c44c..0dc03888c62b 100644
> > --- a/drivers/ata/libata-sata.c
> > +++ b/drivers/ata/libata-sata.c
> > @@ -13,6 +13,7 @@
> >   #include <scsi/scsi_device.h>
> >   #include <linux/libata.h>
> >
> > +#include "ahci.h"
> >   #include "libata.h"
> >   #include "libata-transport.h"
> >
> > @@ -368,10 +369,20 @@ int sata_link_scr_lpm(struct ata_link *link, enum
> ata_lpm_policy policy,
> >   		      bool spm_wakeup)
> >   {
> >   	struct ata_eh_context *ehc = &link->eh_context;
> > +	struct ata_port *ap = link->ap;
> > +	struct ahci_host_priv *hpriv;
> >   	bool woken_up = false;
> >   	u32 scontrol;
> >   	int rc;
> >
> > +	hpriv = ap->host->private_data;
> > +	if (policy >= ATA_LPM_MIN_POWER_WITH_PARTIAL &&
> > +	  !(hpriv->cap & HOST_CAP_SSC)) {
> > +		dev_warn(ap->host->dev,
> > +			"This drive doesn't support slumber; restting policy to
> MED_POWER\n");
> 
> Typo here: s/restting/resetting. Also, s/doesn't/does not.
> 
> > +		policy = ATA_LPM_MED_POWER;
> 
> Here, shouldn't we use the default policy defined by
> CONFIG_SATA_LPM_POLICY ?

If they set it too aggressively we still don't want to honor it if the drive
can't do slumber I would expect.

> 
> > +	}
> > +
> >   	rc = sata_scr_read(link, SCR_CONTROL, &scontrol);
> >   	if (rc)
> >   		return rc;
> 
> 
> --
> Damien Le Moal
> Western Digital Research

Re: [PATCH v2 2/2] ata: ahci: Protect users from setting policies their drives don't support

[Damien Le Moal]

On 4/5/22 04:39, Limonciello, Mario wrote:
> [AMD Official Use Only]
> 
> 
> 
>> -----Original Message-----
>> From: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> Sent: Sunday, April 3, 2022 20:11
>> To: Limonciello, Mario <Mario.Limonciello@amd.com>
>> Cc: open list:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers) <linux-
>> ide@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>;
>> hdegoede@redhat.com
>> Subject: Re: [PATCH v2 2/2] ata: ahci: Protect users from setting policies their
>> drives don't support
>>
>> On 3/3/22 12:49, Mario Limonciello wrote:
>>> As the default low power policy applies to more chipsets and drives, it's
>>> important to make sure that drives actually support the policy that a user
>>> selected in their kernel configuration.
>>>
>>> If the drive doesn't support slumber, don't let the default policies
>>> dependent upon slumber (`min_power` or `min_power_with_partial`) affect
>> the
>>> disk.
>>>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>
>> Mario,
>>
>> Can you resend a rebased version of this, on top of libata for-5.19 branch ?
> 
> 
> OK.
> 
>>
>>> ---
>>> Changes from v1->v2:
>>> * Move deeper into codepaths
>>> * Reset to MED_POWER rather than ignore
>>>   drivers/ata/libata-sata.c | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
>>> index 071158c0c44c..0dc03888c62b 100644
>>> --- a/drivers/ata/libata-sata.c
>>> +++ b/drivers/ata/libata-sata.c
>>> @@ -13,6 +13,7 @@
>>>   #include <scsi/scsi_device.h>
>>>   #include <linux/libata.h>
>>>
>>> +#include "ahci.h"
>>>   #include "libata.h"
>>>   #include "libata-transport.h"
>>>
>>> @@ -368,10 +369,20 @@ int sata_link_scr_lpm(struct ata_link *link, enum
>> ata_lpm_policy policy,
>>>   		      bool spm_wakeup)
>>>   {
>>>   	struct ata_eh_context *ehc = &link->eh_context;
>>> +	struct ata_port *ap = link->ap;
>>> +	struct ahci_host_priv *hpriv;
>>>   	bool woken_up = false;
>>>   	u32 scontrol;
>>>   	int rc;
>>>
>>> +	hpriv = ap->host->private_data;
>>> +	if (policy >= ATA_LPM_MIN_POWER_WITH_PARTIAL &&
>>> +	  !(hpriv->cap & HOST_CAP_SSC)) {
>>> +		dev_warn(ap->host->dev,
>>> +			"This drive doesn't support slumber; restting policy to
>> MED_POWER\n");
>>
>> Typo here: s/restting/resetting. Also, s/doesn't/does not.
>>
>>> +		policy = ATA_LPM_MED_POWER;
>>
>> Here, shouldn't we use the default policy defined by
>> CONFIG_SATA_LPM_POLICY ?
> 
> If they set it too aggressively we still don't want to honor it if the drive
> can't do slumber I would expect.

True. But if the default is set to a higher performance mode, we should
not fall back to the med-power mode.

We should either (1) fallback to the closest higher performance policy
supported, or (2) not change the current policy at all. no ?

See what ahci_update_initial_lpm_policy() does to check the possible
"initial" (the default ?) policy.



> 
>>
>>> +	}
>>> +
>>>   	rc = sata_scr_read(link, SCR_CONTROL, &scontrol);
>>>   	if (rc)
>>>   		return rc;
>>
>>
>> --
>> Damien Le Moal
>> Western Digital Research


-- 
Damien Le Moal
Western Digital Research

RE: [PATCH v2 2/2] ata: ahci: Protect users from setting policies their drives don't support

[Limonciello, Mario]

[AMD Official Use Only]



> -----Original Message-----
> From: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Sent: Monday, April 4, 2022 18:31
> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: open list:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers) <linux-
> ide@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>;
> hdegoede@redhat.com
> Subject: Re: [PATCH v2 2/2] ata: ahci: Protect users from setting policies their
> drives don't support
> 
> On 4/5/22 04:39, Limonciello, Mario wrote:
> > [AMD Official Use Only]
> >
> >
> >
> >> -----Original Message-----
> >> From: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> >> Sent: Sunday, April 3, 2022 20:11
> >> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> >> Cc: open list:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers) <linux-
> >> ide@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>;
> >> hdegoede@redhat.com
> >> Subject: Re: [PATCH v2 2/2] ata: ahci: Protect users from setting policies
> their
> >> drives don't support
> >>
> >> On 3/3/22 12:49, Mario Limonciello wrote:
> >>> As the default low power policy applies to more chipsets and drives, it's
> >>> important to make sure that drives actually support the policy that a user
> >>> selected in their kernel configuration.
> >>>
> >>> If the drive doesn't support slumber, don't let the default policies
> >>> dependent upon slumber (`min_power` or `min_power_with_partial`)
> affect
> >> the
> >>> disk.
> >>>
> >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >>
> >> Mario,
> >>
> >> Can you resend a rebased version of this, on top of libata for-5.19 branch
> ?
> >
> >
> > OK.
> >
> >>
> >>> ---
> >>> Changes from v1->v2:
> >>> * Move deeper into codepaths
> >>> * Reset to MED_POWER rather than ignore
> >>>   drivers/ata/libata-sata.c | 11 +++++++++++
> >>>   1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> >>> index 071158c0c44c..0dc03888c62b 100644
> >>> --- a/drivers/ata/libata-sata.c
> >>> +++ b/drivers/ata/libata-sata.c
> >>> @@ -13,6 +13,7 @@
> >>>   #include <scsi/scsi_device.h>
> >>>   #include <linux/libata.h>
> >>>
> >>> +#include "ahci.h"
> >>>   #include "libata.h"
> >>>   #include "libata-transport.h"
> >>>
> >>> @@ -368,10 +369,20 @@ int sata_link_scr_lpm(struct ata_link *link,
> enum
> >> ata_lpm_policy policy,
> >>>   		      bool spm_wakeup)
> >>>   {
> >>>   	struct ata_eh_context *ehc = &link->eh_context;
> >>> +	struct ata_port *ap = link->ap;
> >>> +	struct ahci_host_priv *hpriv;
> >>>   	bool woken_up = false;
> >>>   	u32 scontrol;
> >>>   	int rc;
> >>>
> >>> +	hpriv = ap->host->private_data;
> >>> +	if (policy >= ATA_LPM_MIN_POWER_WITH_PARTIAL &&
> >>> +	  !(hpriv->cap & HOST_CAP_SSC)) {
> >>> +		dev_warn(ap->host->dev,
> >>> +			"This drive doesn't support slumber; restting policy to
> >> MED_POWER\n");
> >>
> >> Typo here: s/restting/resetting. Also, s/doesn't/does not.
> >>
> >>> +		policy = ATA_LPM_MED_POWER;
> >>
> >> Here, shouldn't we use the default policy defined by
> >> CONFIG_SATA_LPM_POLICY ?
> >
> > If they set it too aggressively we still don't want to honor it if the drive
> > can't do slumber I would expect.
> 
> True. But if the default is set to a higher performance mode, we should
> not fall back to the med-power mode.
> 
> We should either (1) fallback to the closest higher performance policy
> supported, or (2) not change the current policy at all. no ?
> 
> See what ahci_update_initial_lpm_policy() does to check the possible
> "initial" (the default ?) policy.

OK - take a look what I did in the resubmission:
https://lore.kernel.org/all/20220404194510.9206-2-mario.limonciello@amd.com/

> 
> 
> 
> >
> >>
> >>> +	}
> >>> +
> >>>   	rc = sata_scr_read(link, SCR_CONTROL, &scontrol);
> >>>   	if (rc)
> >>>   		return rc;
> >>
> >>
> >> --
> >> Damien Le Moal
> >> Western Digital Research
> 
> 
> --
> Damien Le Moal
> Western Digital Research