Skip to contentSkip to conversation

ahci minor quirk cleanups

PATCH Applied Niklas Cassel 3 participants 11 messages Feb 14, 2024 – Feb 19, 2024 libata

lore
Share & tools
lore

Niklas Cassel

author maintainer
#0 lore raw

v2 cover ahci minor quirk cleanups

Hello all,

Here comes some minor AHCI quirk cleanups.

Kind regards,
Niklas

Changes since V1:
-Renamed boards to start with board_ahci_pcs_quirk_* as suggested by
Damien.
-Moved the entries futher down to keep them alphabetically with the new
names.
-Changed the order of the HFLAGS for the boards to match the new names. -Added underscore in _no_sntf to make the name more clear and to match
board_ahci_no_debounce_delay.
-Added new patches 3/4 and patch 4/4.
-Dropped Reviewed-by tags because of too many changes.

Niklas Cassel (4):
ahci: rename board_ahci_nosntf
ahci: clean up ahci_broken_devslp quirk
ahci: drop unused board_ahci_noncq
ahci: rename board_ahci_nomsi

 drivers/ata/ahci.c | 46 ++++++++++++++++------------------------------
 1 file changed, 16 insertions(+), 30 deletions(-)
Signature · 1 line
2.43.0
author maintainer
#1 lore raw

PATCH v2 1/4 ahci: rename board_ahci_nosntf

Commit 7edbb6059274 ("ahci: clean up intel_pcs_quirk") added a new board type (board_ahci_pcs_quirk) which applies the Intel PCS quirk for legacy platforms. However, it also modified board_ahci_avn and board_ahci_nosntf to apply the same quirk.

board_ahci_avn is defined under the label:
/* board IDs for specific chipsets in alphabetical order */ This is a board for a specific chipset, so the naming is perfectly fine. (The name does not need to be suffixed with _pcs_quirk, since all controllers for this chipset require the quirk to be applied).

board_ahci_nosntf is defined under the label:
/* board IDs by feature in alphabetical order */
This is a board for a specific feature/quirk. However, it is used to apply two different quirks.

Rename board_ahci_nosntf to more clearly highlight that this board ID applies two different quirks.

Fixes 7edbb6059274 ("ahci: clean up intel_pcs_quirk")
Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/ata/ahci.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
drivers/ata/ahci.c +7 -7
index 41b4c9777c85..a2efe916c6e2 100644
@@ -53,7 +53,6 @@ enum board_ids {
 	board_ahci_no_debounce_delay,
 	board_ahci_nomsi,
 	board_ahci_noncq,
-	board_ahci_nosntf,
 	/*
 	 * board_ahci_pcs_quirk is for legacy Intel platforms.
 	 * Modern Intel platforms should use board_ahci instead.

@@ -62,6 +61,7 @@ enum board_ids {
 	 * without testing that the platform actually works without the quirk.)
 	 */
 	board_ahci_pcs_quirk,
+	board_ahci_pcs_quirk_no_sntf,
 	board_ahci_yes_fbs,
 
 	/* board IDs for specific chipsets in alphabetical order */

@@ -165,16 +165,16 @@ static const struct ata_port_info ahci_port_info[] = {
 		.udma_mask	= ATA_UDMA6,
 		.port_ops	= &ahci_ops,
 	},
-	[board_ahci_nosntf] = {
-		AHCI_HFLAGS	(AHCI_HFLAG_NO_SNTF |
-				 AHCI_HFLAG_INTEL_PCS_QUIRK),
+	[board_ahci_pcs_quirk] = {
+		AHCI_HFLAGS	(AHCI_HFLAG_INTEL_PCS_QUIRK),
 		.flags		= AHCI_FLAG_COMMON,
 		.pio_mask	= ATA_PIO4,
 		.udma_mask	= ATA_UDMA6,
 		.port_ops	= &ahci_ops,
 	},
-	[board_ahci_pcs_quirk] = {
-		AHCI_HFLAGS	(AHCI_HFLAG_INTEL_PCS_QUIRK),
+	[board_ahci_pcs_quirk_no_sntf] = {
+		AHCI_HFLAGS	(AHCI_HFLAG_INTEL_PCS_QUIRK |
+				 AHCI_HFLAG_NO_SNTF),
 		.flags		= AHCI_FLAG_COMMON,
 		.pio_mask	= ATA_PIO4,
 		.udma_mask	= ATA_UDMA6,

@@ -271,7 +271,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ PCI_VDEVICE(INTEL, 0x2683), board_ahci_pcs_quirk }, /* ESB2 */
 	{ PCI_VDEVICE(INTEL, 0x27c6), board_ahci_pcs_quirk }, /* ICH7-M DH */
 	{ PCI_VDEVICE(INTEL, 0x2821), board_ahci_pcs_quirk }, /* ICH8 */
-	{ PCI_VDEVICE(INTEL, 0x2822), board_ahci_nosntf }, /* ICH8/Lewisburg RAID*/
+	{ PCI_VDEVICE(INTEL, 0x2822), board_ahci_pcs_quirk_no_sntf }, /* ICH8/Lewisburg RAID*/
 	{ PCI_VDEVICE(INTEL, 0x2824), board_ahci_pcs_quirk }, /* ICH8 */
 	{ PCI_VDEVICE(INTEL, 0x2829), board_ahci_pcs_quirk }, /* ICH8M */
 	{ PCI_VDEVICE(INTEL, 0x282a), board_ahci_pcs_quirk }, /* ICH8M */
Signature · 1 line
2.43.0
maintainer Reviewed-by on v2 1/4
#2 lore raw

Re: [PATCH v2 1/4] ahci: rename board_ahci_nosntf

Quoted 16 lines · Niklas Cassel

Commit 7edbb6059274 ("ahci: clean up intel_pcs_quirk") added a new board type (board_ahci_pcs_quirk) which applies the Intel PCS quirk for legacy platforms. However, it also modified board_ahci_avn and board_ahci_nosntf to apply the same quirk.

board_ahci_avn is defined under the label:
/* board IDs for specific chipsets in alphabetical order */ This is a board for a specific chipset, so the naming is perfectly fine. (The name does not need to be suffixed with _pcs_quirk, since all controllers for this chipset require the quirk to be applied).

board_ahci_nosntf is defined under the label:
/* board IDs by feature in alphabetical order */
This is a board for a specific feature/quirk. However, it is used to apply two different quirks.

Quoted 5 lines

Rename board_ahci_nosntf to more clearly highlight that this board ID applies two different quirks.

Fixes 7edbb6059274 ("ahci: clean up intel_pcs_quirk")
Signed-off-by Niklas Cassel <[email protected]>

Looks good.

Reviewed-by Damien Le Moal <[email protected]>
Signature · 2 lines
Damien Le Moal
Western Digital Research
author maintainer
#3 lore raw

PATCH v2 2/4 ahci: clean up ahci_broken_devslp quirk

In reply to Niklas Cassel

Most quirks are applied using a specific board type board_ahci_no* (e.g. board_ahci_nomsi, board_ahci_noncq), which then sets a flag representing the specific quirk.

ahci_pci_tbl (which is the table of all supported PCI devices), then uses that board type for the PCI vendor and device IDs which need to be quirked.

The ahci_broken_devslp quirk is not implemented in this standard way.

Modify the ahci_broken_devslp quirk to be implemented like the other quirks. This way, we will not have the same PCI device and vendor ID scattered over ahci.c. It will simply be defined in a single location.

Suggested-by Dan Williams <[email protected]>
Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/ata/ahci.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)
drivers/ata/ahci.c +10 -16
index a2efe916c6e2..d8e5b26524f6 100644
@@ -61,6 +61,7 @@ enum board_ids {
 	 * without testing that the platform actually works without the quirk.)
 	 */
 	board_ahci_pcs_quirk,
+	board_ahci_pcs_quirk_no_devslp,
 	board_ahci_pcs_quirk_no_sntf,
 	board_ahci_yes_fbs,
 

@@ -172,6 +173,14 @@ static const struct ata_port_info ahci_port_info[] = {
 		.udma_mask	= ATA_UDMA6,
 		.port_ops	= &ahci_ops,
 	},
+	[board_ahci_pcs_quirk_no_devslp] = {
+		AHCI_HFLAGS	(AHCI_HFLAG_INTEL_PCS_QUIRK |
+				 AHCI_HFLAG_NO_DEVSLP),
+		.flags		= AHCI_FLAG_COMMON,
+		.pio_mask	= ATA_PIO4,
+		.udma_mask	= ATA_UDMA6,
+		.port_ops	= &ahci_ops,
+	},
 	[board_ahci_pcs_quirk_no_sntf] = {
 		AHCI_HFLAGS	(AHCI_HFLAG_INTEL_PCS_QUIRK |
 				 AHCI_HFLAG_NO_SNTF),

@@ -420,7 +429,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ PCI_VDEVICE(INTEL, 0x06d7), board_ahci_pcs_quirk }, /* Comet Lake-H RAID */
 	{ PCI_VDEVICE(INTEL, 0xa386), board_ahci_pcs_quirk }, /* Comet Lake PCH-V RAID */
 	{ PCI_VDEVICE(INTEL, 0x0f22), board_ahci_pcs_quirk }, /* Bay Trail AHCI */
-	{ PCI_VDEVICE(INTEL, 0x0f23), board_ahci_pcs_quirk }, /* Bay Trail AHCI */
+	{ PCI_VDEVICE(INTEL, 0x0f23), board_ahci_pcs_quirk_no_devslp }, /* Bay Trail AHCI */
 	{ PCI_VDEVICE(INTEL, 0x22a3), board_ahci_pcs_quirk }, /* Cherry Tr. AHCI */
 	{ PCI_VDEVICE(INTEL, 0x5ae3), board_ahci_pcs_quirk }, /* ApolloLake AHCI */
 	{ PCI_VDEVICE(INTEL, 0x34d3), board_ahci_pcs_quirk }, /* Ice Lake LP AHCI */

@@ -1420,17 +1429,6 @@ static bool ahci_broken_online(struct pci_dev *pdev)
 	return pdev->bus->number == (val >> 8) && pdev->devfn == (val & 0xff);
 }
 
-static bool ahci_broken_devslp(struct pci_dev *pdev)
-{
-	/* device with broken DEVSLP but still showing SDS capability */
-	static const struct pci_device_id ids[] = {
-		{ PCI_VDEVICE(INTEL, 0x0f23)}, /* Valleyview SoC */
-		{}
-	};
-
-	return pci_match_id(ids, pdev);
-}
-
 #ifdef CONFIG_ATA_ACPI
 static void ahci_gtf_filter_workaround(struct ata_host *host)
 {

@@ -1823,10 +1821,6 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 				&dev_attr_remapped_nvme.attr,
 				NULL);
 
-	/* must set flag prior to save config in order to take effect */
-	if (ahci_broken_devslp(pdev))
-		hpriv->flags |= AHCI_HFLAG_NO_DEVSLP;
-
 #ifdef CONFIG_ARM64
 	if (pdev->vendor == PCI_VENDOR_ID_HUAWEI &&
 	    pdev->device == 0xa235 &&
Signature · 1 line
2.43.0
maintainer Reviewed-by on v2 2/4
#4 lore raw

Re: [PATCH v2 2/4] ahci: clean up ahci_broken_devslp quirk

Quoted 8 lines · Niklas Cassel

Most quirks are applied using a specific board type board_ahci_no* (e.g. board_ahci_nomsi, board_ahci_noncq), which then sets a flag representing the specific quirk.

ahci_pci_tbl (which is the table of all supported PCI devices), then uses that board type for the PCI vendor and device IDs which need to be quirked.

Quoted 8 lines

The ahci_broken_devslp quirk is not implemented in this standard way.

Modify the ahci_broken_devslp quirk to be implemented like the other quirks. This way, we will not have the same PCI device and vendor ID scattered over ahci.c. It will simply be defined in a single location.

Suggested-by Dan Williams <[email protected]>
Signed-off-by Niklas Cassel <[email protected]>

Looks good.

Reviewed-by Damien Le Moal <[email protected]>
Signature · 2 lines
Damien Le Moal
Western Digital Research
author maintainer
#5 lore raw

PATCH v2 3/4 ahci: drop unused board_ahci_noncq

In reply to Niklas Cassel

Since commit 66a7cbc303f4 ("ahci: disable MSI instead of NCQ on Samsung pci-e SSDs on macbooks") there is not a single entry in ahci_pci_tbl which uses board_ahci_noncq.

Since this is dead code, let's remove it.
We cannot remove AHCI_HFLAG_NO_NCQ, as this flag is still used by other boards.

Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/ata/ahci.c | 8 --------
 1 file changed, 8 deletions(-)
drivers/ata/ahci.c +0 -8
index d8e5b26524f6..9bff90fb0591 100644
@@ -52,7 +52,6 @@ enum board_ids {
 	board_ahci_ign_iferr,
 	board_ahci_no_debounce_delay,
 	board_ahci_nomsi,
-	board_ahci_noncq,
 	/*
 	 * board_ahci_pcs_quirk is for legacy Intel platforms.
 	 * Modern Intel platforms should use board_ahci instead.

@@ -159,13 +158,6 @@ static const struct ata_port_info ahci_port_info[] = {
 		.udma_mask	= ATA_UDMA6,
 		.port_ops	= &ahci_ops,
 	},
-	[board_ahci_noncq] = {
-		AHCI_HFLAGS	(AHCI_HFLAG_NO_NCQ),
-		.flags		= AHCI_FLAG_COMMON,
-		.pio_mask	= ATA_PIO4,
-		.udma_mask	= ATA_UDMA6,
-		.port_ops	= &ahci_ops,
-	},
 	[board_ahci_pcs_quirk] = {
 		AHCI_HFLAGS	(AHCI_HFLAG_INTEL_PCS_QUIRK),
 		.flags		= AHCI_FLAG_COMMON,
Signature · 1 line
2.43.0
maintainer Reviewed-by on v2 3/4
#6 lore raw

Re: [PATCH v2 3/4] ahci: drop unused board_ahci_noncq

Quoted 9 lines · Niklas Cassel

Since commit 66a7cbc303f4 ("ahci: disable MSI instead of NCQ on Samsung pci-e SSDs on macbooks") there is not a single entry in ahci_pci_tbl which uses board_ahci_noncq.

Since this is dead code, let's remove it.
We cannot remove AHCI_HFLAG_NO_NCQ, as this flag is still used by other boards.

Signed-off-by Niklas Cassel <[email protected]>

Looks good.

Reviewed-by Damien Le Moal <[email protected]>
Signature · 2 lines
Damien Le Moal
Western Digital Research
author maintainer
#7 lore raw

PATCH v2 4/4 ahci: rename board_ahci_nomsi

In reply to Niklas Cassel

The naming format of the board_ahci_no* boards are:
board_ahci_no_debounce_delay board_ahci_pcs_quirk_no_devslp board_ahci_pcs_quirk_no_sntf

Rename board_ahci_nomsi to board_ahci_no_msi to match the other boards.

Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/ata/ahci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/ata/ahci.c +4 -4
index 9bff90fb0591..006312f5d73c 100644
@@ -51,7 +51,7 @@ enum board_ids {
 	board_ahci_43bit_dma,
 	board_ahci_ign_iferr,
 	board_ahci_no_debounce_delay,
-	board_ahci_nomsi,
+	board_ahci_no_msi,
 	/*
 	 * board_ahci_pcs_quirk is for legacy Intel platforms.
 	 * Modern Intel platforms should use board_ahci instead.

@@ -151,7 +151,7 @@ static const struct ata_port_info ahci_port_info[] = {
 		.udma_mask	= ATA_UDMA6,
 		.port_ops	= &ahci_ops,
 	},
-	[board_ahci_nomsi] = {
+	[board_ahci_no_msi] = {
 		AHCI_HFLAGS	(AHCI_HFLAG_NO_MSI),
 		.flags		= AHCI_FLAG_COMMON,
 		.pio_mask	= ATA_PIO4,

@@ -621,8 +621,8 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	 * Samsung SSDs found on some macbooks.  NCQ times out if MSI is
 	 * enabled.  https://bugzilla.kernel.org/show_bug.cgi?id=60731
 	 */
-	{ PCI_VDEVICE(SAMSUNG, 0x1600), board_ahci_nomsi },
-	{ PCI_VDEVICE(SAMSUNG, 0xa800), board_ahci_nomsi },
+	{ PCI_VDEVICE(SAMSUNG, 0x1600), board_ahci_no_msi },
+	{ PCI_VDEVICE(SAMSUNG, 0xa800), board_ahci_no_msi },
 
 	/* Enmotus */
 	{ PCI_DEVICE(0x1c44, 0x8000), board_ahci },
Signature · 1 line
2.43.0
maintainer Reviewed-by on v2 4/4
#8 lore raw

Re: [PATCH v2 4/4] ahci: rename board_ahci_nomsi

Quoted 8 lines · Niklas Cassel

The naming format of the board_ahci_no* boards are:
board_ahci_no_debounce_delay board_ahci_pcs_quirk_no_devslp board_ahci_pcs_quirk_no_sntf

Rename board_ahci_nomsi to board_ahci_no_msi to match the other boards.

Signed-off-by Niklas Cassel <[email protected]>

Looks good.

Reviewed-by Damien Le Moal <[email protected]>
Signature · 2 lines
Damien Le Moal
Western Digital Research
Reviewed-by on the v2 series
#9 lore raw
In reply to Niklas Cassel
Quoted 20 lines · Niklas Cassel

Hello all,

Here comes some minor AHCI quirk cleanups.

Kind regards,
Niklas

Changes since V1:

(quoted diff) +0 -6
-Renamed boards to start with board_ahci_pcs_quirk_* as suggested by
 Damien.
-Moved the entries futher down to keep them alphabetically with the new
 names.
-Changed the order of the HFLAGS for the boards to match the new names.
-Added underscore in _no_sntf to make the name more clear and to match
 board_ahci_no_debounce_delay.
-Added new patches 3/4 and patch 4/4.
-Dropped Reviewed-by tags because of too many changes.
Quoted 5 lines

Niklas Cassel (4):
ahci: rename board_ahci_nosntf
ahci: clean up ahci_broken_devslp quirk
ahci: drop unused board_ahci_noncq
ahci: rename board_ahci_nomsi

All look good to me,

Reviewed-by Mika Westerberg <[email protected]>
author maintainer Applied
#10 lore raw
In reply to Niklas Cassel
Quoted 20 lines · Niklas Cassel

Hello all,

Here comes some minor AHCI quirk cleanups.

Kind regards,
Niklas

Changes since V1:

(quoted diff) +0 -6
-Renamed boards to start with board_ahci_pcs_quirk_* as suggested by
 Damien.
-Moved the entries futher down to keep them alphabetically with the new
 names.
-Changed the order of the HFLAGS for the boards to match the new names.
-Added underscore in _no_sntf to make the name more clear and to match
 board_ahci_no_debounce_delay.
-Added new patches 3/4 and patch 4/4.
-Dropped Reviewed-by tags because of too many changes.
Quoted 12 lines

Niklas Cassel (4):
ahci: rename board_ahci_nosntf
ahci: clean up ahci_broken_devslp quirk
ahci: drop unused board_ahci_noncq
ahci: rename board_ahci_nomsi

 drivers/ata/ahci.c | 46 ++++++++++++++++------------------------------
 1 file changed, 16 insertions(+), 30 deletions(-)
Signature · 1 line
2.43.0

Applied:
https://git.kernel.org/pub/scm/linux/kernel/git/libata/linux.git/log/?h=for-6.9