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
