Re: [PATCH 04/10] ata: libata-eh: Avoid unnecessary resets when revalidating devices

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

On 7/1/25 3:23 PM, Hannes Reinecke wrote:
> On 6/30/25 08:26, Damien Le Moal wrote:
>> In ata_eh_revalidate_and_attach(), a link LPM policy is always
>> set to ATA_LPM_MAX_POWER before calling ata_dev_revalidate() to ensure
>> that the call to ata_phys_link_offline() does not return true, thus
>> causing an unnecessary device reset. This change was introduced
>> with commit 71d7b6e51ad3 ("ata: libata-eh: avoid needless hard reset
>> when revalidating link").
>>
>> However, setting the link LPM policy to ATA_LPM_MAX_POWER may be
>> visible only after some time, depending on the power state the link was
>> in. E.g. transitioning out of the Partial state should take no longer
>> than a few microseconds, but transitioning out of the Slumber or
>> DevSleep state may take several milliseconds. So despite the changes
>> introduced with commit 71d7b6e51ad3 ("ata: libata-eh: avoid needless
>> hard reset when revalidating link"), we can still endup with
>> ata_phys_link_offline() seeing a link SCR_STATUS register signaling that
>> the device is present (DET is equal to 1h) but that the link PHY is
>> still in a low power mode (e.g. IPM is 2h, signaling "Interface in
>> Partial power management state"). In such cases, ata_phys_link_offline()
>> returns true, causing an EIO return for ata_eh_revalidate_and_attach()
>> and a device reset.
>>
>> Avoid such unnecessary device resets by introducing a relaxed version
>> of the link offline test implemented by ata_phys_link_offline() with
>> the new helper function ata_eh_link_established(). This functions
>> returns true if for the link SCR_STATUS register we see that:
>>   - A device is still present, that is, the DET field is 1h (Device
>>     presence detected but Phy communication not established) or 3h
>>     (Device presence detected and Phy communication established).
>>   - Communication is established, that is, the IPM field is not 0h,
>>     indicating that the PHY is online or in a low power state.
>>
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> ---
>>   drivers/ata/libata-eh.c | 29 ++++++++++++++++++++++++++++-
>>   1 file changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
>> index f98d5123e1e4..7f5d13f9ca73 100644
>> --- a/drivers/ata/libata-eh.c
>> +++ b/drivers/ata/libata-eh.c
>> @@ -2071,6 +2071,33 @@ static void ata_eh_get_success_sense(struct ata_link
>> *link)
>>       ata_eh_done(link, dev, ATA_EH_GET_SUCCESS_SENSE);
>>   }
>>   +/*
>> + * Check if a link is established. This is a relaxed version of
>> + * ata_phys_link_online() which accounts for the fact that this is potentially
>> + * called after changing the link power management policy, which may not be
>> + * reflected immediately in the SSTAUS register (e.g., we may still be seeing
>> + * the PHY in partial, slumber or devsleep Partial power management state.
>> + * So check that:
>> + * - A device is still present, that is, DET is 1h (Device presence detected
>> + *   but Phy communication not established) or 3h (Device presence detected and
>> + *   Phy communication established)
>> + * - Communication is established, that is, IPM is not 0h, indicating that PHY
>> + *   is online or in a low power state.
>> + */
>> +static bool ata_eh_link_established(struct ata_link *link)
>> +{
>> +    u32 sstatus;
>> +    u8 det, ipm;
>> +
>> +    if (sata_scr_read(link, SCR_STATUS, &sstatus))
>> +        return false;
>> +
>> +    det = sstatus & 0x0f;
>> +    ipm = (sstatus >> 8) & 0x0f;
>> +
>> +    return (det & 0x01) && ipm;
>> +}
>> +
>>   /**
>>    *    ata_eh_link_set_lpm - configure SATA interface power management
>>    *    @link: link to configure
>> @@ -3275,7 +3302,7 @@ static int ata_eh_revalidate_and_attach(struct ata_link
>> *link,
>>                       goto err;
>>               }
>>   -            if (ata_phys_link_offline(ata_dev_phys_link(dev))) {
>> +            if (!ata_eh_link_established(ata_dev_phys_link(dev))) {
>>                   rc = -EIO;
>>                   goto err;
>>               }
> 
> Makes me wonder: if the phy is taking some time, don't we need to wait
> at some point for the transition to complete?

There is a 10ms wait already in sata_link_scr_lpm() but it seems to not always
be enough. The specs say that transitions out of HIPM "shall not take more than
10ms", but hey, we all know how devices always follow the specs, right ? :)

> From a cursory glance we just continue, and (apparently) hope that
> everything will be well eventually.
> Hmm?

It is fine to continue because transitions out of DIPM/HIPM/DevSleep are
automatic if you send a command. So we actually do not need to wait at all and
probably can remove that 10ms sleep in sata_link_scr_lpm(). But I have not for now.


-- 
Damien Le Moal
Western Digital Research