[PATCH v2] libata/ahci: Fix PCS quirk application for suspend

[PATCH v2] libata/ahci: Fix PCS quirk application for suspend

[grozzly]

There is a bug 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. Because of that my laptop (ICH8M controller) doesnt see 
KINGSTON SV300S37A60G SSD disk connected into a SATA connector on wake 
since kernel 5.3.4 or better to say 5.3.8 because there was another error 
in c312ef176399 until a fix arrived in 09d6ac8dc51a "libata/ahci: Fix PCS 
quirk application". Btw 4.19.y lts branch is affected as well.

The problem somewhy doesnt trigger on another disk though (WD5000LPCX HDD). 
I discovered it upgrading the laptop with the SSD in place of a HDD with 
some 5.4 kernel.

Here is my 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 to test kernels
- sdb: KINGSTON SV300S37A60G SSD connected into the only SATA port

Booting into vanilla 5.3.8 and beyond (built from upstream sources with 
configs extracted from https://kernel.ubuntu.com/~kernel-ppa/mainline/) I 
see both disks in lsblk. After wake from suspend the SSD is gone from lsblk 
output.

Here is 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

The patch is tested on 6.0.10, it solves the problem for my hardware.

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index c1eca72b4575..28d8c56cb4dd 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;
 
@@ -1788,12 +1808,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;
@@ -1903,7 +1917,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;

Re: [PATCH v2] libata/ahci: Fix PCS quirk application for suspend

[Damien Le Moal]

On 12/6/22 07:11, grozzly wrote:
> There is a bug 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. Because of that my laptop (ICH8M controller) doesnt see 
> KINGSTON SV300S37A60G SSD disk connected into a SATA connector on wake 
> since kernel 5.3.4 or better to say 5.3.8 because there was another error 
> in c312ef176399 until a fix arrived in 09d6ac8dc51a "libata/ahci: Fix PCS 
> quirk application". Btw 4.19.y lts branch is affected as well.
> 
> The problem somewhy doesnt trigger on another disk though (WD5000LPCX HDD). 

s/somewhy/somehow ?
s/doesnt/does not

> I discovered it upgrading the laptop with the SSD in place of a HDD with 
> some 5.4 kernel.
> 
> Here is my 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 to test kernels
> - sdb: KINGSTON SV300S37A60G SSD connected into the only SATA port
> 
> Booting into vanilla 5.3.8 and beyond (built from upstream sources with 
> configs extracted from https://kernel.ubuntu.com/~kernel-ppa/mainline/) I 
> see both disks in lsblk. After wake from suspend the SSD is gone from lsblk 
> output.
> 
> Here is 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
> 
> The patch is tested on 6.0.10, it solves the problem for my hardware.

Please address the patches to me directly in addition to the linux-ide
list (see scripts/check_maintainer.pl).

This needs to be tested with the latest 6.1-rc7 kernel. If you do so, you
can remove this last phrase in the commit message as it will be implied.

And this patch I think needs a "Fixes" tag:

Fixes: c312ef176399 ("libata/ahci: Drop PCS quirk for Denverton and beyond")

But you are also referencing 09d6ac8dc51a "libata/ahci: Fix PCS
quirk application" above, but the text is not super clear regarding the
patch that triggers the issue. Please clarify the text and add the Fixes
tag referencing the patch that introduces the issue.

Overall, the commit message describes the problem, but does not say HOW
you adrees and fix it. Please add a few lines describing the solution.

The patch applies but its format is weird: It is missing the "---"
separator here. How did you generate it ? Did you use "git format-patch" ?

> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index c1eca72b4575..28d8c56cb4dd 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;
>  
> @@ -1788,12 +1808,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;
> @@ -1903,7 +1917,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;
> 

-- 
Damien Le Moal
Western Digital Research