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; > -- Damien Le Moal Western Digital Research
