[PATCH v2 0/4] ahci minor quirk cleanups

[PATCH v2 0/4] ahci minor quirk cleanups

[Niklas Cassel]

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(-)

-- 
2.43.0

[PATCH v2 1/4] ahci: rename board_ahci_nosntf

[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.

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 <cassel@kernel.org>
---
 drivers/ata/ahci.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 41b4c9777c85..a2efe916c6e2 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -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 */
-- 
2.43.0

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

[Damien Le Moal]

On 2/14/24 22:00, Niklas Cassel wrote:
> 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 <cassel@kernel.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

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

[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 <dan.j.williams@intel.com>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/ahci.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index a2efe916c6e2..d8e5b26524f6 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -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 &&
-- 
2.43.0

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

[Damien Le Moal]

On 2/14/24 22:00, Niklas Cassel wrote:
> 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 <dan.j.williams@intel.com>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

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

[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 <cassel@kernel.org>
---
 drivers/ata/ahci.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index d8e5b26524f6..9bff90fb0591 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -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,
-- 
2.43.0

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

[Damien Le Moal]

On 2/14/24 22:00, Niklas Cassel wrote:
> 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 <cassel@kernel.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research

[PATCH v2 4/4] ahci: rename board_ahci_nomsi

[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 <cassel@kernel.org>
---
 drivers/ata/ahci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 9bff90fb0591..006312f5d73c 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -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 },
-- 
2.43.0

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

[Damien Le Moal]

On 2/14/24 22:00, Niklas Cassel wrote:
> 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 <cassel@kernel.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 0/4] ahci minor quirk cleanups

[Mika Westerberg]

On Wed, Feb 14, 2024 at 02:00:08PM +0100, Niklas Cassel wrote:
> 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

All look good to me,

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v2 0/4] ahci minor quirk cleanups

[Niklas Cassel]

On Wed, Feb 14, 2024 at 02:00:08PM +0100, Niklas Cassel wrote:
> 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(-)
> 
> -- 
> 2.43.0
> 

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