On 12/9/22 00:25, Adam Tukaj 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@xxxxxxxxxxxxxx>
Patch author name and signed-off-by name do not match. Please fix that.
(use scripts/checkpatch.pl to check your patch)
Also please change the patch title to:
ata: ahci: Fix PCS quirk application for suspend
> ---
> 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;
>
--
Damien Le Moal
Western Digital Research
