| Message ID | OM8HSmyIGyT2XF-f70AR7JA5kLpuIYEu5mYEIpyUT-4CC-u7ezc6po32ZIGOSN_7PlsF0RhOvUs8HpVJiAHGrh2ytgiBEltBpz0MuKiy-vg=@protonmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] libata/ahci: Fix PCS quirk application for suspend | expand |
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@protonmail.com> Looks good now. I will queue this up for 6.2 and add cc: stable. Thanks.
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@protonmail.com> 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; >
> Looks good now. I will queue this up for 6.2 and add cc: stable. > Thanks. Thanks for your reviews. Mby it is important to remind: branch 4.19.y is affected as well. Older lts branches are OK.
On 12/9/22 18:35, Adam Vodopjan wrote: >> Looks good now. I will queue this up for 6.2 and add cc: stable. >> Thanks. > > Thanks for your reviews. Mby it is important to remind: branch 4.19.y > is affected as well. Older lts branches are OK. All LTS kernels containing the patch you identified with the "Fixes" tag will get this patch. c312ef176399 is only in 5.4 and up if I am not mistaken. So the patch you sent probably does not apply as-is (I have not tried though). It will need backporting to 4.19. You will eventually get a notification about this. The patch needs Cc: stable@vger.kernel.org # v4.19+ after the Fixes tag. I will add that.
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;
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(-)