Re: [PATCH] ata: libata: add workaround to flip LPM during suspend/resume

[Koba Ko]

Nack, due to some errors
1. unused declarations
2. missing judgement for ATA_LFLAG_NO_LPM_RECOVER

On Fri, Sep 1, 2023 at 3:08 AM Koba Ko <koba.ko@xxxxxxxxxxxxx> wrote:
>
> Due to TigerLake/Adler Lake AHCI controller's LPM regression,
> can't apply LPM on TigerLake/AdlerLake AHCI controller.
>
> Add a workaround to flip LPM during suspend/resume.
> When suspneding, apply LPM on TigerLake/AdlerLake AHCI.
> Restore it to target_lpm_policy after resuming.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217775
> Signed-off-by: Koba Ko <koba.ko@xxxxxxxxxxxxx>
> ---
>  drivers/ata/ahci.c      | 29 +++++++++++++++++++++++++++++
>  drivers/ata/libata-eh.c |  8 ++++++++
>  include/linux/libata.h  |  1 +
>  3 files changed, 38 insertions(+)
>
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 02503e903e4a8..5c9196cd2c738 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -95,6 +95,8 @@ static int ahci_p5wdh_hardreset(struct ata_link *link, unsigned int *class,
>  #ifdef CONFIG_PM
>  static int ahci_pci_device_runtime_suspend(struct device *dev);
>  static int ahci_pci_device_runtime_resume(struct device *dev);
> +static int ahci_intel_port_suspend(struct ata_port *ap, pm_message_t mesg);
> +static int ahci_intel_port_resume(struct ata_port *ap);
>  #ifdef CONFIG_PM_SLEEP
>  static int ahci_pci_device_suspend(struct device *dev);
>  static int ahci_pci_device_resume(struct device *dev);
> @@ -1025,6 +1027,30 @@ static void ahci_p5wdh_workaround(struct ata_host *host)
>                 ap->link.flags |= ATA_LFLAG_NO_SRST | ATA_LFLAG_ASSUME_ATA;
>         }
>  }
> +/*
> + * Intel TGL/ADL workaround, when suspending, put port into LPM,
> + * recover to max power after resuming.
> + */
> +static void ahci_intel_ahci_workaround(struct ata_host *host)
> +{
> +       struct pci_dev *pdev = to_pci_dev(host->dev);
> +       int i;
> +       static const struct pci_device_id ids[] = {
> +               { PCI_VDEVICE(INTEL, 0xa0d3)}, /* Tiger Lake UP{3,4} AHCI */
> +               { PCI_VDEVICE(INTEL, 0x7ae2)}, /* Alder Lake AHCI*/
> +               {}
> +       };
> +
> +       dev_info(&pdev->dev, "enabling Intel AHCI workaround\n");
> +
> +       if (pci_match_id(ids, pdev)) {
> +               for (i = 0; i < host->n_ports; i++) {
> +                       struct ata_port *ap = host->ports[i];
> +
> +                       ap->flags |= ATA_LFLAG_NO_LPM_RECOVER;
> +               }
> +       }
> +}
>
>  /*
>   * Macbook7,1 firmware forcibly disables MCP89 AHCI and changes PCI ID when
> @@ -1905,6 +1931,9 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>         /* apply workaround for ASUS P5W DH Deluxe mainboard */
>         ahci_p5wdh_workaround(host);
>
> +       /* apply workaround for Intel AHCI */
> +       ahci_intel_ahci_workaround(host);
> +
>         /* apply gtf filter quirk */
>         ahci_gtf_filter_workaround(host);
>
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 159ba6ba19ebb..de29e50843f99 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -4000,6 +4000,11 @@ static void ata_eh_handle_port_suspend(struct ata_port *ap)
>         int rc = 0;
>         struct ata_device *dev;
>
> +       ata_for_each_dev(dev, &ap->link, ENABLED) {
> +               ata_eh_set_lpm(&ap->link, ATA_LPM_MED_POWER_WITH_DIPM, &dev);
> +       }
> +
> +
>         /* are we suspending? */
>         spin_lock_irqsave(ap->lock, flags);
>         if (!(ap->pflags & ATA_PFLAG_PM_PENDING) ||
> @@ -4087,6 +4092,9 @@ static void ata_eh_handle_port_resume(struct ata_port *ap)
>         if (ap->ops->port_resume)
>                 ap->ops->port_resume(ap);
>
> +       ata_for_each_dev(dev, &ap->link, ENABLED) {
> +               ata_eh_set_lpm(&ap->link, ap->target_lpm_policy, &dev);
> +       }
>         /* tell ACPI that we're resuming */
>         ata_acpi_on_resume(ap);
>
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 52d58b13e5eee..e720fed6dbd7f 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -147,6 +147,7 @@ enum {
>         ATA_LFLAG_RST_ONCE      = (1 << 9), /* limit recovery to one reset */
>         ATA_LFLAG_CHANGED       = (1 << 10), /* LPM state changed on this link */
>         ATA_LFLAG_NO_DEBOUNCE_DELAY = (1 << 11), /* no debounce delay on link resume */
> +       ATA_LFLAG_NO_LPM_RECOVER = (1 << 12), /* disable LPM on this link */
>
>         /* struct ata_port flags */
>         ATA_FLAG_SLAVE_POSS     = (1 << 0), /* host supports slave dev */
> --
> 2.34.1
>