diff mbox series

libata/ahci: Fix PCS quirk application for suspend

Message ID GDLzJIZ_OPwuAzIjUg0sSe2tVHaJJvuAvlIv056rty6JTo-noLGRiVuY7YIEb0u-4w0kIZQvP2EwQuoKGwafcglUiXUQCkf6ufsZuv3IjgQ=@protonmail.com
State New
Headers show
Series libata/ahci: Fix PCS quirk application for suspend | expand

Commit Message

Adam Vodopjan Dec. 1, 2022, 10:49 p.m. UTC 
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. Compared to c312ef176399, I miraculously revived ahci_pci_reset_controller() and intergrated internals of ahci_intel_pcs_quirk() into it.

Comments

Damien Le Moal Dec. 5, 2022, 1:42 p.m. UTC | #1 
On 12/2/22 07:49, 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.

Please correctly format your emails (wrap lines at under 80 chars)

> 
> 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. Compared to c312ef176399, I miraculously revived ahci_pci_reset_controller() and intergrated internals of ahci_intel_pcs_quirk() into it.
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index c1eca72b4575..2f2c8176808c 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -677,6 +677,43 @@ 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);
> +	const struct pci_device_id *id = pci_match_id(ahci_pci_tbl, pdev);
> +	int rc;
> +
> +	rc = ahci_reset_controller(host);
> +	if (rc)
> +		return rc;
> +
> +	/*
> +	 * Only apply the 6-port PCS quirk for known legacy platforms.
> +	 * Skip applying the quirk on Denverton and beyond
> +	 */
> +	if (id && id->vendor == PCI_VENDOR_ID_INTEL &&
> +			((enum board_ids) id->driver_data) < board_ahci_pcs7) {
> +		struct ahci_host_priv *hpriv = host->private_data;
> +		u16 tmp16;
> +
> +		/*
> +		 * port_map is determined from PORTS_IMPL PCI register which is
> +		 * implemented as write or write-once register.  If the register
> +		 * isn't programmed, ahci automatically generates it from number
> +		 * of ports, which is good enough for PCS programming. It is
> +		 * otherwise expected that platform firmware enables the ports
> +		 * before the OS boots.
> +		 */
> +		pci_read_config_word(pdev, PCS_6, &tmp16);
> +		if ((tmp16 & hpriv->port_map) != hpriv->port_map) {
> +			tmp16 |= hpriv->port_map;
> +			pci_write_config_word(pdev, PCS_6, tmp16);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static void ahci_pci_init_controller(struct ata_host *host)
>  {
>  	struct ahci_host_priv *hpriv = host->private_data;
> @@ -871,7 +908,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 +944,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;
>  
> @@ -1624,36 +1661,6 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap,
>  		ap->target_lpm_policy = policy;
>  }
>  
> -static void ahci_intel_pcs_quirk(struct pci_dev *pdev, struct ahci_host_priv *hpriv)

Why remove this function ? Calling it after ahci_reset_controller() in
ahci_pci_reset_controller() would be a lot cleaner and less changes for
the same result. It is always better to try to keep "quirk" code separate
from the regular generic code.

Can you send a proper patch for this ?

> -{
> -	const struct pci_device_id *id = pci_match_id(ahci_pci_tbl, pdev);
> -	u16 tmp16;
> -
> -	/*
> -	 * Only apply the 6-port PCS quirk for known legacy platforms.
> -	 */
> -	if (!id || id->vendor != PCI_VENDOR_ID_INTEL)
> -		return;
> -
> -	/* Skip applying the quirk on Denverton and beyond */
> -	if (((enum board_ids) id->driver_data) >= board_ahci_pcs7)
> -		return;
> -
> -	/*
> -	 * port_map is determined from PORTS_IMPL PCI register which is
> -	 * implemented as write or write-once register.  If the register
> -	 * isn't programmed, ahci automatically generates it from number
> -	 * of ports, which is good enough for PCS programming. It is
> -	 * otherwise expected that platform firmware enables the ports
> -	 * before the OS boots.
> -	 */
> -	pci_read_config_word(pdev, PCS_6, &tmp16);
> -	if ((tmp16 & hpriv->port_map) != hpriv->port_map) {
> -		tmp16 |= hpriv->port_map;
> -		pci_write_config_word(pdev, PCS_6, tmp16);
> -	}
> -}
> -
>  static ssize_t remapped_nvme_show(struct device *dev,
>  				  struct device_attribute *attr,
>  				  char *buf)
> @@ -1788,12 +1795,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 +1904,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;
>
diff mbox series

Patch

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index c1eca72b4575..2f2c8176808c 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -677,6 +677,43 @@  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);
+	const struct pci_device_id *id = pci_match_id(ahci_pci_tbl, pdev);
+	int rc;
+
+	rc = ahci_reset_controller(host);
+	if (rc)
+		return rc;
+
+	/*
+	 * Only apply the 6-port PCS quirk for known legacy platforms.
+	 * Skip applying the quirk on Denverton and beyond
+	 */
+	if (id && id->vendor == PCI_VENDOR_ID_INTEL &&
+			((enum board_ids) id->driver_data) < board_ahci_pcs7) {
+		struct ahci_host_priv *hpriv = host->private_data;
+		u16 tmp16;
+
+		/*
+		 * port_map is determined from PORTS_IMPL PCI register which is
+		 * implemented as write or write-once register.  If the register
+		 * isn't programmed, ahci automatically generates it from number
+		 * of ports, which is good enough for PCS programming. It is
+		 * otherwise expected that platform firmware enables the ports
+		 * before the OS boots.
+		 */
+		pci_read_config_word(pdev, PCS_6, &tmp16);
+		if ((tmp16 & hpriv->port_map) != hpriv->port_map) {
+			tmp16 |= hpriv->port_map;
+			pci_write_config_word(pdev, PCS_6, tmp16);
+		}
+	}
+
+	return 0;
+}
+
 static void ahci_pci_init_controller(struct ata_host *host)
 {
 	struct ahci_host_priv *hpriv = host->private_data;
@@ -871,7 +908,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 +944,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;
 
@@ -1624,36 +1661,6 @@  static void ahci_update_initial_lpm_policy(struct ata_port *ap,
 		ap->target_lpm_policy = policy;
 }
 
-static void ahci_intel_pcs_quirk(struct pci_dev *pdev, struct ahci_host_priv *hpriv)
-{
-	const struct pci_device_id *id = pci_match_id(ahci_pci_tbl, pdev);
-	u16 tmp16;
-
-	/*
-	 * Only apply the 6-port PCS quirk for known legacy platforms.
-	 */
-	if (!id || id->vendor != PCI_VENDOR_ID_INTEL)
-		return;
-
-	/* Skip applying the quirk on Denverton and beyond */
-	if (((enum board_ids) id->driver_data) >= board_ahci_pcs7)
-		return;
-
-	/*
-	 * port_map is determined from PORTS_IMPL PCI register which is
-	 * implemented as write or write-once register.  If the register
-	 * isn't programmed, ahci automatically generates it from number
-	 * of ports, which is good enough for PCS programming. It is
-	 * otherwise expected that platform firmware enables the ports
-	 * before the OS boots.
-	 */
-	pci_read_config_word(pdev, PCS_6, &tmp16);
-	if ((tmp16 & hpriv->port_map) != hpriv->port_map) {
-		tmp16 |= hpriv->port_map;
-		pci_write_config_word(pdev, PCS_6, tmp16);
-	}
-}
-
 static ssize_t remapped_nvme_show(struct device *dev,
 				  struct device_attribute *attr,
 				  char *buf)
@@ -1788,12 +1795,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 +1904,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;