Re: [PATCH 02/10] ata: libata-core: Move device LPM quirk settings to ata_dev_config_lpm()

[Hannes Reinecke <hare@suse.de>]

On 6/30/25 08:26, Damien Le Moal wrote:
> Move the various cases of setting the ATA_QUIRK_NOLPM quirk flag for a
> device in ata_dev_configure() to the function ata_dev_config_lpm().
> This allows having all LPM related settings in one place to facilitate
> maintenance.
> 
> No functional changes.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/libata-core.c | 43 +++++++++++++++++++++++----------------
>   1 file changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 0d85474f6640..fdce96fd3ffa 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2798,6 +2798,32 @@ static void ata_dev_config_lpm(struct ata_device *dev)
>   	struct ata_port *ap = dev->link->ap;
>   	unsigned int err_mask;
>   
> +	if (ap->flags & ATA_FLAG_NO_LPM) {
> +		/*
> +		 * When the port does not support LPM, we cannot support it on
> +		 * the device either.
> +		 */
> +		dev->quirks |= ATA_QUIRK_NOLPM;
> +	} else {
> +		/*
> +		 * Some WD SATA-1 drives have issues with LPM, turn on NOLPM for
> +		 * them.
> +		 */
> +		if ((dev->quirks & ATA_QUIRK_WD_BROKEN_LPM) &&
> +		    (dev->id[ATA_ID_SATA_CAPABILITY] & 0xe) == 0x2)
> +			dev->quirks |= ATA_QUIRK_NOLPM;
> +
> +		/* ATI specific quirk */
> +		if ((dev->quirks & ATA_QUIRK_NO_LPM_ON_ATI) &&
> +		    ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI))
> +			dev->quirks |= ATA_QUIRK_NOLPM;
> +	}
> +
> +	if (dev->quirks & ATA_QUIRK_NOLPM) {
> +		ata_dev_warn(dev, "LPM support broken, forcing max_power\n");
> +		ap->target_lpm_policy = ATA_LPM_MAX_POWER;
> +	}
> +
>   	/*
>   	 * If the device port does not support Device Initiated Power Management
>   	 * (DIPM), and the device supports this feature, disable it.
> @@ -2881,23 +2907,6 @@ int ata_dev_configure(struct ata_device *dev)
>   	if (rc)
>   		return rc;
>   
> -	/* some WD SATA-1 drives have issues with LPM, turn on NOLPM for them */
> -	if ((dev->quirks & ATA_QUIRK_WD_BROKEN_LPM) &&
> -	    (id[ATA_ID_SATA_CAPABILITY] & 0xe) == 0x2)
> -		dev->quirks |= ATA_QUIRK_NOLPM;
> -
> -	if (dev->quirks & ATA_QUIRK_NO_LPM_ON_ATI &&
> -	    ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI))
> -		dev->quirks |= ATA_QUIRK_NOLPM;
> -
> -	if (ap->flags & ATA_FLAG_NO_LPM)
> -		dev->quirks |= ATA_QUIRK_NOLPM;
> -
> -	if (dev->quirks & ATA_QUIRK_NOLPM) {
> -		ata_dev_warn(dev, "LPM support broken, forcing max_power\n");
> -		dev->link->ap->target_lpm_policy = ATA_LPM_MAX_POWER;
> -	}
> -
>   	/* let ACPI work its magic */
>   	rc = ata_acpi_on_devcfg(dev);
>   	if (rc)

And this now is only dealing with modifying LPM setting, independent on
any DIPM setting. Why not make two functions (one for DIPM and one for
LPM) so make matters less confusing?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich