[PATCH v4] ata: ahci: Fix PCS quirk application for suspend

[PATCH v4] ata: ahci: Fix PCS quirk application for suspend

[Adam Vodopjan]

Since kernel 5.3.4 my laptop (ICH8M controller) does not see Kingston
SV300S37A60G SSD disk connected into a SATA connector on wake from suspend.
The problem was introduced in c312ef176399 "libata/ahci: Drop PCS quirk for
Denverton and beyond": the quirk is not applied on wake from suspend as it
originally was.

It is worth to mention the commit contained another bug: the quirk is not
applied at all to controllers which require it. The fix 09d6ac8dc51a
"libata/ahci: Fix PCS quirk application" landed in 5.3.8. So testing my
patch anywhere between c312ef176399 and 09d6ac8dc51a is pointless.

Not all disks trigger the problem. For example nothing bad happens with
Western Digital WD5000LPCX HDD.

Test hardware:
- Acer 5920G with ICH8M SATA controller
- sda: some SATA HDD connnected into the DVD drive IDE port with a SATA-IDE
caddy. It is a boot disk
- sdb: Kingston SV300S37A60G SSD connected into the only SATA port

Sample "dmesg --notime | grep -E '^(sd |ata)'" output on wake:

sd 0:0:0:0: [sda] Starting disk
sd 2:0:0:0: [sdb] Starting disk
ata4: SATA link down (SStatus 4 SControl 300)
ata3: SATA link down (SStatus 4 SControl 300)
ata1.00: ACPI cmd ef/03:0c:00:00:00:a0 (SET FEATURES) filtered out
ata1.00: ACPI cmd ef/03:42:00:00:00:a0 (SET FEATURES) filtered out
ata1: FORCE: cable set to 80c
ata5: SATA link down (SStatus 0 SControl 300)
ata3: SATA link down (SStatus 4 SControl 300)
ata3: SATA link down (SStatus 4 SControl 300)
ata3.00: disabled
sd 2:0:0:0: rejecting I/O to offline device
ata3.00: detaching (SCSI 2:0:0:0)
sd 2:0:0:0: [sdb] Start/Stop Unit failed: Result: hostbyte=DID_NO_CONNECT
	driverbyte=DRIVER_OK
sd 2:0:0:0: [sdb] Synchronizing SCSI cache
sd 2:0:0:0: [sdb] Synchronize Cache(10) failed: Result:
	hostbyte=DID_BAD_TARGET driverbyte=DRIVER_OK
sd 2:0:0:0: [sdb] Stopping disk
sd 2:0:0:0: [sdb] Start/Stop Unit failed: Result: hostbyte=DID_BAD_TARGET
	driverbyte=DRIVER_OK

c312ef176399 dropped ahci_pci_reset_controller() which internally calls
ahci_reset_controller() and applies the PCS quirk if needed after that. It
was called each time a reset was required instead of just
ahci_reset_controller(). This patch puts the function back in place.

Fixes: c312ef176399 ("libata/ahci: Drop PCS quirk for Denverton and beyond")
Signed-off-by: Adam Vodopjan <grozzly@protonmail.com>
---
 drivers/ata/ahci.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 639de2d75d63..53ab2306da00 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -84,6 +84,7 @@ enum board_ids {
 static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent);
 static void ahci_remove_one(struct pci_dev *dev);
 static void ahci_shutdown_one(struct pci_dev *dev);
+static void ahci_intel_pcs_quirk(struct pci_dev *pdev, struct ahci_host_priv *hpriv);
 static int ahci_vt8251_hardreset(struct ata_link *link, unsigned int *class,
 				 unsigned long deadline);
 static int ahci_avn_hardreset(struct ata_link *link, unsigned int *class,
@@ -677,6 +678,25 @@ static void ahci_pci_save_initial_config(struct pci_dev *pdev,
 	ahci_save_initial_config(&pdev->dev, hpriv);
 }

+static int ahci_pci_reset_controller(struct ata_host *host)
+{
+	struct pci_dev *pdev = to_pci_dev(host->dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	int rc;
+
+	rc = ahci_reset_controller(host);
+	if (rc)
+		return rc;
+
+	/*
+	 * If platform firmware failed to enable ports, try to enable
+	 * them here.
+	 */
+	ahci_intel_pcs_quirk(pdev, hpriv);
+
+	return 0;
+}
+
 static void ahci_pci_init_controller(struct ata_host *host)
 {
 	struct ahci_host_priv *hpriv = host->private_data;
@@ -871,7 +891,7 @@ static int ahci_pci_device_runtime_resume(struct device *dev)
 	struct ata_host *host = pci_get_drvdata(pdev);
 	int rc;

-	rc = ahci_reset_controller(host);
+	rc = ahci_pci_reset_controller(host);
 	if (rc)
 		return rc;
 	ahci_pci_init_controller(host);
@@ -907,7 +927,7 @@ static int ahci_pci_device_resume(struct device *dev)
 		ahci_mcp89_apple_enable(pdev);

 	if (pdev->dev.power.power_state.event == PM_EVENT_SUSPEND) {
-		rc = ahci_reset_controller(host);
+		rc = ahci_pci_reset_controller(host);
 		if (rc)
 			return rc;

@@ -1785,12 +1805,6 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	/* save initial config */
 	ahci_pci_save_initial_config(pdev, hpriv);

-	/*
-	 * If platform firmware failed to enable ports, try to enable
-	 * them here.
-	 */
-	ahci_intel_pcs_quirk(pdev, hpriv);
-
 	/* prepare host */
 	if (hpriv->cap & HOST_CAP_NCQ) {
 		pi.flags |= ATA_FLAG_NCQ;
@@ -1900,7 +1914,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (rc)
 		return rc;

-	rc = ahci_reset_controller(host);
+	rc = ahci_pci_reset_controller(host);
 	if (rc)
 		return rc;

--
2.34.1

Re: [PATCH v4] ata: ahci: Fix PCS quirk application for suspend

[Adam Vodopjan]

Damien, have you seen this version of the patch?

Re: [PATCH v4] ata: ahci: Fix PCS quirk application for suspend

[Damien Le Moal]

On 12/13/22 19:37, Adam Vodopjan wrote:
> Damien, have you seen this version of the patch?

Yes. It looks OK. Missing the Cc stable tag, but I will add it when
applying. I will send this as a fix once 6.2-rc1 is out in a couple of
weeks (merge window open yesterday).

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4] ata: ahci: Fix PCS quirk application for suspend

[Adam Vodopjan]

On Tuesday, December 13th, 2022 at 1:38 PM, Damien Le Moal <damien.lemoal@opensource.wdc.com> wrote:

> On 12/13/22 19:37, Adam Vodopjan wrote:
> 
> > Damien, have you seen this version of the patch?
> 
> 
> Yes. It looks OK. Missing the Cc stable tag, but I will add it when
> applying. I will send this as a fix once 6.2-rc1 is out in a couple of
> weeks (merge window open yesterday).
> 

Thanks

Re: [PATCH v4] ata: ahci: Fix PCS quirk application for suspend

[Damien Le Moal]

On 12/9/22 18:26, Adam Vodopjan wrote:
> Since kernel 5.3.4 my laptop (ICH8M controller) does not see Kingston
> SV300S37A60G SSD disk connected into a SATA connector on wake from suspend.
> The problem was introduced in c312ef176399 "libata/ahci: Drop PCS quirk for
> Denverton and beyond": the quirk is not applied on wake from suspend as it
> originally was.
> 
> It is worth to mention the commit contained another bug: the quirk is not
> applied at all to controllers which require it. The fix 09d6ac8dc51a
> "libata/ahci: Fix PCS quirk application" landed in 5.3.8. So testing my
> patch anywhere between c312ef176399 and 09d6ac8dc51a is pointless.
> 
> Not all disks trigger the problem. For example nothing bad happens with
> Western Digital WD5000LPCX HDD.
> 
> Test hardware:
> - Acer 5920G with ICH8M SATA controller
> - sda: some SATA HDD connnected into the DVD drive IDE port with a SATA-IDE
> caddy. It is a boot disk
> - sdb: Kingston SV300S37A60G SSD connected into the only SATA port
> 
> Sample "dmesg --notime | grep -E '^(sd |ata)'" output on wake:
> 
> sd 0:0:0:0: [sda] Starting disk
> sd 2:0:0:0: [sdb] Starting disk
> ata4: SATA link down (SStatus 4 SControl 300)
> ata3: SATA link down (SStatus 4 SControl 300)
> ata1.00: ACPI cmd ef/03:0c:00:00:00:a0 (SET FEATURES) filtered out
> ata1.00: ACPI cmd ef/03:42:00:00:00:a0 (SET FEATURES) filtered out
> ata1: FORCE: cable set to 80c
> ata5: SATA link down (SStatus 0 SControl 300)
> ata3: SATA link down (SStatus 4 SControl 300)
> ata3: SATA link down (SStatus 4 SControl 300)
> ata3.00: disabled
> sd 2:0:0:0: rejecting I/O to offline device
> ata3.00: detaching (SCSI 2:0:0:0)
> sd 2:0:0:0: [sdb] Start/Stop Unit failed: Result: hostbyte=DID_NO_CONNECT
> 	driverbyte=DRIVER_OK
> sd 2:0:0:0: [sdb] Synchronizing SCSI cache
> sd 2:0:0:0: [sdb] Synchronize Cache(10) failed: Result:
> 	hostbyte=DID_BAD_TARGET driverbyte=DRIVER_OK
> sd 2:0:0:0: [sdb] Stopping disk
> sd 2:0:0:0: [sdb] Start/Stop Unit failed: Result: hostbyte=DID_BAD_TARGET
> 	driverbyte=DRIVER_OK
> 
> c312ef176399 dropped ahci_pci_reset_controller() which internally calls
> ahci_reset_controller() and applies the PCS quirk if needed after that. It
> was called each time a reset was required instead of just
> ahci_reset_controller(). This patch puts the function back in place.
> 
> Fixes: c312ef176399 ("libata/ahci: Drop PCS quirk for Denverton and beyond")
> Signed-off-by: Adam Vodopjan <grozzly@protonmail.com>

Applied to for-6.2-fixes with some tweaks to the commit message. Thanks !

-- 
Damien Le Moal
Western Digital Research