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

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

On 7/1/25 3:13 PM, Hannes Reinecke wrote:
> 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?

"less confusing" with all LPM things is I think not possible :)

The idea is to keep things together as much as possible to facilitate
tweaking/maintenance. There is more like this coming to get port capabilities
out of ahci.c and into generic libata so that platform AHCI and libsas adapters
can be supported too. Right now, it is pretty much LPM == AHCI...

> 
> Cheers,
> 
> Hannes


-- 
Damien Le Moal
Western Digital Research