Skip to contentSkip to conversation

ata: Extend Samsung LPM quirk to AMD and fix force documentation

PATCH Applied Niklas Cassel 3 participants 10 messages Sep 18 – Sep 21

Subsystems (2)
lore
Share & tools
lore

Niklas Cassel

author maintainer
#0 lore raw

cover ata: Extend Samsung LPM quirk to AMD and fix force documentation

Hello,

A user reports that a Samsung SSD 870 QVO 8TB connected to an AMD 600 Series chipset SATA controller times out on STANDBY IMMEDIATE during system suspend with med_power_with_dipm enabled. The command completes with max_performance instead.

Patch 1 extends the existing Samsung LPM quirk from ATI to AMD controllers, renaming it to ATA_QUIRK_NO_LPM_ON_ATI_AND_AMD. It keeps the same drive model matches and leaves the NCQ quirk restricted to ATI. LPM behavior on other controller vendors, including Intel, is unchanged. The underlying controller/device interaction has not been root-caused.

Patch 2 separately corrects the libata.force parameter documentation:
the supported PIO range, the scope of ncqati, and the set-only behavior of max_sec_1024 and max_sec_lba48. It introduces no new force options.

Note that the ATI (and now AMD) LPM quirk never had a libata.force option, and neither do we want to add one.
If a user wants to force enable/disable lpm, they can use the existing [no]lpm libata.force parameter. We therefore do not have and documentation in kernel-parameters.txt to change.

Bug report:
https://bugzilla.kernel.org/show_bug.cgi?id=221986

Kind regards,
Niklas

Niklas Cassel (2):
ata: libata-core: Extend Samsung LPM quirk to AMD controllers
ata: libata: Correct libata.force parameter documentation

 Documentation/admin-guide/kernel-parameters.txt | 10 ++++------
 drivers/ata/libata-core.c                       | 15 ++++++++-------
 include/linux/libata.h                          |  4 ++--
 3 files changed, 14 insertions(+), 15 deletions(-)
Signature · 1 line
2.55.0
author maintainer
#1 lore raw

PATCH 1/2 ata: libata-core: Extend Samsung LPM quirk to AMD controllers

A Samsung SSD 870 QVO 8TB connected to an AMD 600 Series chipset SATA controller is reported to time out on STANDBY IMMEDIATE during system suspend with med_power_with_dipm enabled. The command completes when using max_performance instead.

The existing Samsung LPM quirk only matches ATI controllers, leaving AMD controllers unaffected. Rename it to ATA_QUIRK_NO_LPM_ON_ATI_AND_AMD and extend the vendor check to AMD for the same Samsung SSD model patterns. Keep LPM behavior unchanged for other controller vendors, including Intel.

Leave ATA_QUIRK_NO_NCQ_ON_ATI restricted to ATI, since the reported AMD issue concerns LPM rather than NCQ.

---
 drivers/ata/libata-core.c | 15 ++++++++-------
 include/linux/libata.h    |  4 ++--
 2 files changed, 10 insertions(+), 9 deletions(-)
drivers/ata/libata-core.c +8 -7
index f482c0a6d7e9..92233fb25051 100644
@@ -2972,9 +2972,10 @@ static void ata_dev_config_lpm(struct ata_device *dev)
 		    (dev->id[ATA_ID_SATA_CAPABILITY] & 0xe) == 0x2)
 			dev->quirks |= ATA_QUIRK_NOLPM;
 
-		/* ATI specific quirk */
-		if ((dev->quirks & ATA_QUIRK_NO_LPM_ON_ATI) &&
-		    ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI))
+		/* ATI and AMD specific quirk */
+		if ((dev->quirks & ATA_QUIRK_NO_LPM_ON_ATI_AND_AMD) &&
+		    (ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI) ||
+		     ata_dev_check_adapter(dev, PCI_VENDOR_ID_AMD)))
 			dev->quirks |= ATA_QUIRK_NOLPM;
 	}
 
@@ -4136,7 +4137,7 @@ static const char * const ata_quirk_names[] = {
 	[__ATA_QUIRK_MAX_SEC]		= "maxsec",
 	[__ATA_QUIRK_MAX_TRIM_128M]	= "maxtrim128m",
 	[__ATA_QUIRK_NO_NCQ_ON_ATI]	= "noncqonati",
-	[__ATA_QUIRK_NO_LPM_ON_ATI]	= "nolpmonati",
+	[__ATA_QUIRK_NO_LPM_ON_ATI_AND_AMD] = "nolpmonatiandamd",
 	[__ATA_QUIRK_NO_ID_DEV_LOG]	= "noiddevlog",
 	[__ATA_QUIRK_NO_LOG_DIR]	= "nologdir",
 	[__ATA_QUIRK_NO_FUA]		= "nofua",
@@ -4420,15 +4421,15 @@ static const struct ata_dev_quirks_entry __ata_dev_quirks[] = {
 	{ "Samsung SSD 860*",		NULL,	ATA_QUIRK_NO_NCQ_TRIM |
 						ATA_QUIRK_ZERO_AFTER_TRIM |
 						ATA_QUIRK_NO_NCQ_ON_ATI |
-						ATA_QUIRK_NO_LPM_ON_ATI },
+						ATA_QUIRK_NO_LPM_ON_ATI_AND_AMD },
 	{ "Samsung SSD 870*",		NULL,	ATA_QUIRK_NO_NCQ_TRIM |
 						ATA_QUIRK_ZERO_AFTER_TRIM |
 						ATA_QUIRK_NO_NCQ_ON_ATI |
-						ATA_QUIRK_NO_LPM_ON_ATI },
+						ATA_QUIRK_NO_LPM_ON_ATI_AND_AMD },
 	{ "SAMSUNG*MZ7LH*",		NULL,	ATA_QUIRK_NO_NCQ_TRIM |
 						ATA_QUIRK_ZERO_AFTER_TRIM |
 						ATA_QUIRK_NO_NCQ_ON_ATI |
-						ATA_QUIRK_NO_LPM_ON_ATI },
+						ATA_QUIRK_NO_LPM_ON_ATI_AND_AMD },
 	{ "FCCT*M500*",			NULL,	ATA_QUIRK_NO_NCQ_TRIM |
 						ATA_QUIRK_ZERO_AFTER_TRIM },
 
include/linux/libata.h +2 -2
index 313e96173b19..48bde275968d 100644
@@ -76,7 +76,7 @@ enum ata_quirks {
 	__ATA_QUIRK_MAX_SEC,		/* Limit max sectors */
 	__ATA_QUIRK_MAX_TRIM_128M,	/* Limit max trim size to 128M */
 	__ATA_QUIRK_NO_NCQ_ON_ATI,	/* Disable NCQ on ATI chipset */
-	__ATA_QUIRK_NO_LPM_ON_ATI,	/* Disable LPM on ATI chipset */
+	__ATA_QUIRK_NO_LPM_ON_ATI_AND_AMD, /* Disable LPM on ATI and AMD chipsets */
 	__ATA_QUIRK_NO_ID_DEV_LOG,	/* Identify device log missing */
 	__ATA_QUIRK_NO_LOG_DIR,		/* Do not read log directory */
 	__ATA_QUIRK_NO_FUA,		/* Do not use FUA */
@@ -115,7 +115,7 @@ enum {
 	ATA_QUIRK_MAX_SEC		= BIT_ULL(__ATA_QUIRK_MAX_SEC),
 	ATA_QUIRK_MAX_TRIM_128M		= BIT_ULL(__ATA_QUIRK_MAX_TRIM_128M),
 	ATA_QUIRK_NO_NCQ_ON_ATI		= BIT_ULL(__ATA_QUIRK_NO_NCQ_ON_ATI),
-	ATA_QUIRK_NO_LPM_ON_ATI		= BIT_ULL(__ATA_QUIRK_NO_LPM_ON_ATI),
+	ATA_QUIRK_NO_LPM_ON_ATI_AND_AMD	= BIT_ULL(__ATA_QUIRK_NO_LPM_ON_ATI_AND_AMD),
 	ATA_QUIRK_NO_ID_DEV_LOG		= BIT_ULL(__ATA_QUIRK_NO_ID_DEV_LOG),
 	ATA_QUIRK_NO_LOG_DIR		= BIT_ULL(__ATA_QUIRK_NO_LOG_DIR),
 	ATA_QUIRK_NO_FUA		= BIT_ULL(__ATA_QUIRK_NO_FUA),
Signature · 1 line
2.55.0
Reviewed-by on v1 1/2
#2 lore raw

Re: [PATCH 1/2] ata: libata-core: Extend Samsung LPM quirk to AMD controllers

Quoted 11 lines · Niklas Cassel

A Samsung SSD 870 QVO 8TB connected to an AMD 600 Series chipset SATA controller is reported to time out on STANDBY IMMEDIATE during system suspend with med_power_with_dipm enabled. The command completes when using max_performance instead.

The existing Samsung LPM quirk only matches ATI controllers, leaving AMD controllers unaffected. Rename it to ATA_QUIRK_NO_LPM_ON_ATI_AND_AMD and extend the vendor check to AMD for the same Samsung SSD model patterns. Keep LPM behavior unchanged for other controller vendors, including Intel.

Quoted 5 lines

Leave ATA_QUIRK_NO_NCQ_ON_ATI restricted to ATI, since the reported AMD issue concerns LPM rather than NCQ.

Quoted 72 lines
  drivers/ata/libata-core.c | 15 ++++++++-------
  include/linux/libata.h    |  4 ++--
  2 files changed, 10 insertions(+), 9 deletions(-)
drivers/ata/libata-core.c +8 -7
index f482c0a6d7e9..92233fb25051 100644
@@ -2972,9 +2972,10 @@ static void ata_dev_config_lpm(struct ata_device *dev)
  		    (dev->id[ATA_ID_SATA_CAPABILITY] & 0xe) == 0x2)
  			dev->quirks |= ATA_QUIRK_NOLPM;
 
-		/* ATI specific quirk */
-		if ((dev->quirks & ATA_QUIRK_NO_LPM_ON_ATI) &&
-		    ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI))
+		/* ATI and AMD specific quirk */
+		if ((dev->quirks & ATA_QUIRK_NO_LPM_ON_ATI_AND_AMD) &&
+		    (ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI) ||
+		     ata_dev_check_adapter(dev, PCI_VENDOR_ID_AMD)))
  			dev->quirks |= ATA_QUIRK_NOLPM;
  	}
 
@@ -4136,7 +4137,7 @@ static const char * const ata_quirk_names[] = {
  	[__ATA_QUIRK_MAX_SEC]		= "maxsec",
  	[__ATA_QUIRK_MAX_TRIM_128M]	= "maxtrim128m",
  	[__ATA_QUIRK_NO_NCQ_ON_ATI]	= "noncqonati",
-	[__ATA_QUIRK_NO_LPM_ON_ATI]	= "nolpmonati",
+	[__ATA_QUIRK_NO_LPM_ON_ATI_AND_AMD] = "nolpmonatiandamd",
  	[__ATA_QUIRK_NO_ID_DEV_LOG]	= "noiddevlog",
  	[__ATA_QUIRK_NO_LOG_DIR]	= "nologdir",
  	[__ATA_QUIRK_NO_FUA]		= "nofua",
@@ -4420,15 +4421,15 @@ static const struct ata_dev_quirks_entry __ata_dev_quirks[] = {
  	{ "Samsung SSD 860*",		NULL,	ATA_QUIRK_NO_NCQ_TRIM |
  						ATA_QUIRK_ZERO_AFTER_TRIM |
  						ATA_QUIRK_NO_NCQ_ON_ATI |
-						ATA_QUIRK_NO_LPM_ON_ATI },
+						ATA_QUIRK_NO_LPM_ON_ATI_AND_AMD },
  	{ "Samsung SSD 870*",		NULL,	ATA_QUIRK_NO_NCQ_TRIM |
  						ATA_QUIRK_ZERO_AFTER_TRIM |
  						ATA_QUIRK_NO_NCQ_ON_ATI |
-						ATA_QUIRK_NO_LPM_ON_ATI },
+						ATA_QUIRK_NO_LPM_ON_ATI_AND_AMD },
  	{ "SAMSUNG*MZ7LH*",		NULL,	ATA_QUIRK_NO_NCQ_TRIM |
  						ATA_QUIRK_ZERO_AFTER_TRIM |
  						ATA_QUIRK_NO_NCQ_ON_ATI |
-						ATA_QUIRK_NO_LPM_ON_ATI },
+						ATA_QUIRK_NO_LPM_ON_ATI_AND_AMD },
  	{ "FCCT*M500*",			NULL,	ATA_QUIRK_NO_NCQ_TRIM |
  						ATA_QUIRK_ZERO_AFTER_TRIM },
 
include/linux/libata.h +2 -2
index 313e96173b19..48bde275968d 100644
@@ -76,7 +76,7 @@ enum ata_quirks {
  	__ATA_QUIRK_MAX_SEC,		/* Limit max sectors */
  	__ATA_QUIRK_MAX_TRIM_128M,	/* Limit max trim size to 128M */
  	__ATA_QUIRK_NO_NCQ_ON_ATI,	/* Disable NCQ on ATI chipset */
-	__ATA_QUIRK_NO_LPM_ON_ATI,	/* Disable LPM on ATI chipset */
+	__ATA_QUIRK_NO_LPM_ON_ATI_AND_AMD, /* Disable LPM on ATI and AMD chipsets */
  	__ATA_QUIRK_NO_ID_DEV_LOG,	/* Identify device log missing */
  	__ATA_QUIRK_NO_LOG_DIR,		/* Do not read log directory */
  	__ATA_QUIRK_NO_FUA,		/* Do not use FUA */
@@ -115,7 +115,7 @@ enum {
  	ATA_QUIRK_MAX_SEC		= BIT_ULL(__ATA_QUIRK_MAX_SEC),
  	ATA_QUIRK_MAX_TRIM_128M		= BIT_ULL(__ATA_QUIRK_MAX_TRIM_128M),
  	ATA_QUIRK_NO_NCQ_ON_ATI		= BIT_ULL(__ATA_QUIRK_NO_NCQ_ON_ATI),
-	ATA_QUIRK_NO_LPM_ON_ATI		= BIT_ULL(__ATA_QUIRK_NO_LPM_ON_ATI),
+	ATA_QUIRK_NO_LPM_ON_ATI_AND_AMD	= BIT_ULL(__ATA_QUIRK_NO_LPM_ON_ATI_AND_AMD),
  	ATA_QUIRK_NO_ID_DEV_LOG		= BIT_ULL(__ATA_QUIRK_NO_ID_DEV_LOG),
  	ATA_QUIRK_NO_LOG_DIR		= BIT_ULL(__ATA_QUIRK_NO_LOG_DIR),
  	ATA_QUIRK_NO_FUA		= BIT_ULL(__ATA_QUIRK_NO_FUA),
maintainer Reviewed-by on v1 1/2
#3 lore raw

Re: [PATCH 1/2] ata: libata-core: Extend Samsung LPM quirk to AMD controllers

In reply to Niklas Cassel
Quoted 11 lines · Niklas Cassel

A Samsung SSD 870 QVO 8TB connected to an AMD 600 Series chipset SATA controller is reported to time out on STANDBY IMMEDIATE during system suspend with med_power_with_dipm enabled. The command completes when using max_performance instead.

The existing Samsung LPM quirk only matches ATI controllers, leaving AMD controllers unaffected. Rename it to ATA_QUIRK_NO_LPM_ON_ATI_AND_AMD and extend the vendor check to AMD for the same Samsung SSD model patterns. Keep LPM behavior unchanged for other controller vendors, including Intel.

Quoted 5 lines

Leave ATA_QUIRK_NO_NCQ_ON_ATI restricted to ATI, since the reported AMD issue concerns LPM rather than NCQ.

Looks OK.

Signature · 2 lines
Damien Le Moal
Western Digital Research
author maintainer
#4 lore raw

PATCH 2/2 ata: libata: Correct libata.force parameter documentation

In reply to Niklas Cassel

Align the documented libata.force options with their implementation. The force table accepts PIO modes 0 through 6, not mode 7, and ncqati controls NCQ generally rather than only queued TRIM.

The max_sec_1024 and max_sec_lba48 options only set transfer size limits. Remove the misleading claim that they can also clear them.

---
 Documentation/admin-guide/kernel-parameters.txt | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)
Documentation/admin-guide/kernel-parameters.txt +4 -6
index 33cd30996e47..24459f7ff810 100644
@@ -3491,7 +3491,7 @@ Kernel parameters
 
 			* SATA link speed limit: 1.5Gbps or 3.0Gbps.
 
-			* Transfer mode: pio[0-7], mwdma[0-4] and udma[0-7].
+			* Transfer mode: pio[0-6], mwdma[0-4] and udma[0-7].
 			  udma[/][16,25,33,44,66,100,133] notation is also
 			  allowed.
 
@@ -3509,7 +3509,7 @@ Kernel parameters
 
 			* [no]ncqtrim: Enable or disable queued DSM TRIM.
 
-			* [no]ncqati: Enable or disable NCQ trim on ATI chipset.
+			* [no]ncqati: Enable or disable NCQ on ATI chipsets.
 
 			* [no]trim: Enable or disable (unqueued) TRIM.
 
@@ -3540,11 +3540,9 @@ Kernel parameters
 
 			* max_sec_128: Set transfer size limit to 128 sectors.
 
-			* max_sec_1024: Set or clear transfer size limit to
-			  1024 sectors.
+			* max_sec_1024: Set transfer size limit to 1024 sectors.
 
-			* max_sec_lba48: Set or clear transfer size limit to
-			  65535 sectors.
+			* max_sec_lba48: Set transfer size limit to 65535 sectors.
 
 			* external: Mark port as external (hotplug-capable).
 
Signature · 1 line
2.55.0
author maintainer
#5 lore raw

Re: [PATCH 2/2] ata: libata: Correct libata.force parameter documentation

Quoted 26 lines · Niklas Cassel

Align the documented libata.force options with their implementation. The force table accepts PIO modes 0 through 6, not mode 7, and ncqati controls NCQ generally rather than only queued TRIM.

The max_sec_1024 and max_sec_lba48 options only set transfer size limits. Remove the misleading claim that they can also clear them.

---
 Documentation/admin-guide/kernel-parameters.txt | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)
Documentation/admin-guide/kernel-parameters.txt +1 -1
index 33cd30996e47..24459f7ff810 100644
@@ -3491,7 +3491,7 @@ Kernel parameters
 
 			* SATA link speed limit: 1.5Gbps or 3.0Gbps.
 
-			* Transfer mode: pio[0-7], mwdma[0-4] and udma[0-7].
+			* Transfer mode: pio[0-6], mwdma[0-4] and udma[0-7].
 			  udma[/][16,25,33,44,66,100,133] notation is also
 			  allowed.
 

Documentation/admin-guide/kernel-parameters.txt Kernel parameters · line 3512

Quoted 5 lines
Documentation/admin-guide/kernel-parameters.txt +1 -1
@@ -3509,7 +3509,7 @@ Kernel parameters
 
 			* [no]ncqtrim: Enable or disable queued DSM TRIM.
 
-			* [no]ncqati: Enable or disable NCQ trim on ATI chipset.
+			* [no]ncqati: Enable or disable NCQ on ATI chipsets.

Hmm.. I guess we could theoretically remove the [no]ncqati parameter from libata.force.
If a user wants to enable/disable ncq, they can use the [no]ncq libata.force parameter.

However, since this libata.force kernel parameter already exists, I guess it might actually be used by someone, so we can't really remove it without potentially breaking existing setups, so I guess the easiest thing is just to keep it, even if it is not strictly needed.

Kind regards,
Niklas

maintainer
#6 lore raw

Re: [PATCH 2/2] ata: libata: Correct libata.force parameter documentation

Quoted 38 lines · Niklas Cassel
Quoted 26 lines · Niklas Cassel

Align the documented libata.force options with their implementation. The force table accepts PIO modes 0 through 6, not mode 7, and ncqati controls NCQ generally rather than only queued TRIM.

The max_sec_1024 and max_sec_lba48 options only set transfer size limits. Remove the misleading claim that they can also clear them.

---
 Documentation/admin-guide/kernel-parameters.txt | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)
Documentation/admin-guide/kernel-parameters.txt +1 -1
index 33cd30996e47..24459f7ff810 100644
@@ -3491,7 +3491,7 @@ Kernel parameters
 
 			* SATA link speed limit: 1.5Gbps or 3.0Gbps.
 
-			* Transfer mode: pio[0-7], mwdma[0-4] and udma[0-7].
+			* Transfer mode: pio[0-6], mwdma[0-4] and udma[0-7].
 			  udma[/][16,25,33,44,66,100,133] notation is also
 			  allowed.
 

Documentation/admin-guide/kernel-parameters.txt Kernel parameters · line 3512

Quoted 5 lines
Documentation/admin-guide/kernel-parameters.txt +1 -1
@@ -3509,7 +3509,7 @@ Kernel parameters
 
 			* [no]ncqtrim: Enable or disable queued DSM TRIM.
 
-			* [no]ncqati: Enable or disable NCQ trim on ATI chipset.
+			* [no]ncqati: Enable or disable NCQ on ATI chipsets.

Hmm.. I guess we could theoretically remove the [no]ncqati parameter from libata.force.
If a user wants to enable/disable ncq, they can use the [no]ncq libata.force parameter.

Quoted 4 lines

However, since this libata.force kernel parameter already exists, I guess it might actually be used by someone, so we can't really remove it without potentially breaking existing setups, so I guess the easiest thing is just to keep it, even if it is not strictly needed.

It can be needed on a machine that has multiple adapters from different vendors.

Signature · 2 lines
Damien Le Moal
Western Digital Research
author maintainer
#7 lore raw

Re: [PATCH 2/2] ata: libata: Correct libata.force parameter documentation

Quoted 6 lines · Damien Le Moal
Quoted 6 lines

Hmm.. I guess we could theoretically remove the [no]ncqati parameter from libata.force.
If a user wants to enable/disable ncq, they can use the [no]ncq libata.force parameter.

Quoted 6 lines
Quoted 4 lines

However, since this libata.force kernel parameter already exists, I guess it might actually be used by someone, so we can't really remove it without potentially breaking existing setups, so I guess the easiest thing is just to keep it, even if it is not strictly needed.

It can be needed on a machine that has multiple adapters from different vendors.

libata.force parameters can be applied per port, so even for machines with multiple adapters, it would have been sufficient IMO.

Kind regards,
Niklas

maintainer Reviewed-by on v1 2/2
#8 lore raw

Re: [PATCH 2/2] ata: libata: Correct libata.force parameter documentation

In reply to Niklas Cassel
Quoted 8 lines · Niklas Cassel

Align the documented libata.force options with their implementation. The force table accepts PIO modes 0 through 6, not mode 7, and ncqati controls NCQ generally rather than only queued TRIM.

The max_sec_1024 and max_sec_lba48 options only set transfer size limits. Remove the misleading claim that they can also clear them.

Looks good.

Signature · 2 lines
Damien Le Moal
Western Digital Research
author maintainer Applied
#9 lore raw
In reply to Niklas Cassel
Quoted 5 lines · Niklas Cassel

A user reports that a Samsung SSD 870 QVO 8TB connected to an AMD 600 Series chipset SATA controller times out on STANDBY IMMEDIATE during system suspend with med_power_with_dipm enabled. The command completes with max_performance instead.

Quoted 7 lines

Patch 1 extends the existing Samsung LPM quirk from ATI to AMD controllers, renaming it to ATA_QUIRK_NO_LPM_ON_ATI_AND_AMD. It keeps the same drive model matches and leaves the NCQ quirk restricted to ATI. LPM behavior on other controller vendors, including Intel, is unchanged. The underlying controller/device interaction has not been root-caused.

[...]

Applied to libata/linux.git (for-7.3-fixes), thanks!

[1/2] ata: libata-core: Extend Samsung LPM quirk to AMD controllers

      https://git.kernel.org/libata/linux/c/e6bae503
[2/2] ata: libata: Correct libata.force parameter documentation
      https://git.kernel.org/libata/linux/c/88a0474d

Kind regards,
Niklas