Re: [PATCH 2/7] ata: libata-eh: Add ata_eh_set_lpm() WARN_ON

[Damien Le Moal <dlemoal@kernel.org>]

On 5/14/25 19:22, Niklas Cassel wrote:
> link->lpm_policy is initialized to ATA_LPM_UNKNOWN in ata_eh_reset().
> 
> ata_eh_set_lpm() is then only called if
> link->lpm_policy != ap->target_lpm_policy (after reset)
> 
> and then only if link->lpm_policy > ATA_LPM_MAX_POWER (before
> revalidation).
> 
> This means that ata_eh_set_lpm() is currently never called with
> policy == ATA_LPM_UNKNOWN.
> 
> Add a WARN_ON so that it is more obvious from reading the code that this
> function is never called with policy == ATA_LPM_UNKNOWN.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/ata/libata-eh.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index edbc5d7572d1..d2ccdb9a2840 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -3442,6 +3442,13 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
>  	    (link->flags & ATA_LFLAG_NO_LPM) || (ap && !ap->ops->set_lpm))
>  		return 0;
>  
> +	/*
> +	 * This function currently assumes that it will never be supplied policy
> +	 * ATA_LPM_UNKNOWN.
> +	 */
> +	if (WARN_ON(policy == ATA_LPM_UNKNOWN))

WARN_ON_ONCE() would be better here.

> +		return 0;
> +
>  	/*
>  	 * DIPM is enabled only for ATA_LPM_MIN_POWER,
>  	 * ATA_LPM_MIN_POWER_WITH_PARTIAL, and ATA_LPM_MED_POWER_WITH_DIPM, as


-- 
Damien Le Moal
Western Digital Research