Re: [PATCH 2/2] ata: ahci: Protect users from setting policies their drives don't support

[Hans de Goede]

Hi,

On 2/28/22 22:33, Mario Limonciello wrote:
> As the default low power policy applies to more chipsets and drives, it's
> important to make sure that drives actually support the policy that a user
> selected in their kernel configuration.
> 
> If the drive doesn't support slumber, don't let the default policies
> dependent upon slumber (`min_power` or `min_power_with_partial`) affect the
> disk.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> ---
> Changes from RFC v1 -> PATCH v1:
> * Move the warning and protection from drives that don't support slumber further
>   into the calling path.
>  drivers/ata/libahci.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 0ed484e04fd6..9dee721ed1fe 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -785,7 +785,15 @@ static int ahci_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
>  		pp->intr_mask &= ~PORT_IRQ_PHYRDY;
>  		writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
>  
> -		sata_link_scr_lpm(link, policy, false);
> +		if (policy >= ATA_LPM_MIN_POWER_WITH_PARTIAL &&
> +		   !(hpriv->cap & HOST_CAP_SSC)) {
> +			struct pci_dev *pdev = to_pci_dev(ap->host->dev);
> +
> +			dev_warn(&pdev->dev,
> +				 "This drive doesn't support slumber; ignoring SATA policy\n");

Not sure if completely ignoring the policy here is a good idea. IMHO it would
be better to fallback to MED_POWER_WITH_DIPM at least for the haswell - skylake(++++)
generations having MED_POWER_WITH_DIPM is important to allow the package to
go the higher PC states.

> +		} else {
> +			sata_link_scr_lpm(link, policy, false);
> +		}
>  	}
>  
>  	if (hpriv->cap & HOST_CAP_ALPM) {

Regards,

Hans