Re: [PATCH 08/10] ata: ahci: Disallow LPM policy control if not supported

[Niklas Cassel <cassel@kernel.org>]

On Mon, Jun 30, 2025 at 03:26:35PM +0900, Damien Le Moal wrote:
> Commit fa997b0576c9 ("ata: ahci: Do not enable LPM if no LPM states are
> supported by the HBA") introduced an early return in
> ahci_update_initial_lpm_policy() to ensure that the target_lpm_policy
> of ports belonging to a host that does not support the Partial, Slumber
> and DevSleep power states is unchanged and remains set to
> ATA_LPM_UNKNOWN and thus prevents the execution of
> ata_eh_link_set_lpm().
> 
> However, a user or a system daemon (e.g. systemd-udevd) may still
> attempt changing the LPM policy through the sysfs
> link_power_management_policy of the host.
> 
> Improve this to prevent sysfs LPM policy changes by setting the flag
> ATA_FLOAG_NO_LPM for the port of such host, and initialize the port
> target_lpm_policy to ATA_LPM_MAX_POWER to guarantee that no unsupported
> low power state is being used on the port and its link.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/ata/ahci.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 34698ae39f55..737f5d1bde11 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1793,7 +1793,10 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap)
>  	if ((ap->host->flags & ATA_HOST_NO_PART) &&
>  	    (ap->host->flags & ATA_HOST_NO_SSC) &&
>  	    (ap->host->flags & ATA_HOST_NO_DEVSLP)) {
> -		ata_port_dbg(ap, "no LPM states supported, not enabling LPM\n");
> +		ata_port_warn(ap,
> +			"No LPM states supported, forcing LPM max_power\n");

Do we really want this to be a warning?

I don't think there is anything wrong with an HBA that does not support any
LPM states, so it seems a bit excessive to give a warning about it.

e.g. [PATCH 06/10] ata: ahci: Disable DIPM if host lacks support
was only a at_port_dbg().


Or, if you really want to keep this warning, then perhaps we should move this
if-statement
(and the ap->pflags & ATA_PFLAG_EXTERNAL if-statement)
below the

update_policy:

label

And change it to:

if (policy != ATA_LPM_MAX_POWER &&
   (ap->host->flags & ATA_HOST_NO_PART) &&
   (ap->host->flags & ATA_HOST_NO_SSC) &&
   (ap->host->flags & ATA_HOST_NO_DEVSLP)) {


(and add the same policy != ATA_LPM_MAX_POWER &&
guard to the ap->pflags & ATA_PFLAG_EXTERNAL if-statement)

But I think that I prefer to just keep it as ata_port_dbg().


Kind regards,
Niklas