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

[Niklas Cassel]

On Fri, Sep 01, 2023 at 10:34:57AM +0800, Koba Ko 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>
> ---

Hello Koba,

I understand that it is very frustrating to not be able to go to the
deepest C-state.

If you want LPM, we should add the PCI device and vendor id as a
board_ahci_low_power entry.

I am awake that a lot of people reported regressions when that entry was
added, and that is was thus reverted.

But adding hooks to temporary enable/disable LPM during suspend/resume
does not seem like the right thing to do.

Since you are enabling/disabling LPM, even if it is only during
suspend/resume, this should once again cause problems for those who
reported regressions, no?

Perhaps you have tested this on one of the laptops that reported
regressions and know that his workaround is safe?

If you do own one of those systems, isn't it better if we instead:
1) re-introduce the TigerLake AHCI board_ahci_low_power entry
2) debug and fix the root cause of the regressions on TigerLake laptops


Enabling/disabling LPM only during suspend/resume seems like a huge hack
(and I don't see why this wouldn't once again break peoples systems).


Kind regards,
Niklas


> V2:
> * remove the unused declarations
> * add a condition for ATA_LFLAG_NO_LPM_RECOVER during suspned/resume
> ---
>  drivers/ata/ahci.c      | 27 +++++++++++++++++++++++++++
>  drivers/ata/libata-eh.c | 10 ++++++++++
>  include/linux/libata.h  |  1 +
>  3 files changed, 38 insertions(+)
>
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 02503e903e4a8..658fac695adf1 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1025,6 +1025,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 +1929,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..0743a9986a5ac 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -4000,6 +4000,12 @@ static void ata_eh_handle_port_suspend(struct ata_port *ap)
>	int rc = 0;
>	struct ata_device *dev;
>
> +	if (!!(ap->flags & ATA_LFLAG_NO_LPM_RECOVER))
> +		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 +4093,10 @@ static void ata_eh_handle_port_resume(struct ata_port *ap)
>	if (ap->ops->port_resume)
>		ap->ops->port_resume(ap);
>
> +	if (!!(ap->flags & ATA_LFLAG_NO_LPM_RECOVER))
> +		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.25.1
>