Re: Bug report for ahci-mvebu driver

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 1/27/23 14:13, Damien Le Moal wrote:
> On 1/25/23 02:02, marius@psihoexpert.ro wrote:
>> January 24, 2023 11:54 AM, "Damien Le Moal" <damien.lemoal@opensource.wdc.com> wrote:
>>
>>> OK. Good. But from the trace below, it looks like you have the previous
>>> patch still applied. Can you try only the above patch ?
>>
>> Done:
>> [ 15.135173][ T113] hardreset, Online=>Offline
>> [ 15.135182][ T113] sata_set_spd_needed, scontrol=0x300
>> [ 15.139656][ T113] __sata_set_spd_needed, initial limit=0xFFFFFFFF
>> [ 15.144934][ T113] __sata_set_spd_needed, corrected limit=0xFFFFFFFF
>> [ 15.151235][ T113] __sata_set_spd_needed, target=0x0
>> [ 15.157725][ T113] __sata_set_spd_needed, spd=0x0
>> [ 15.162805][ T113] __sata_set_spd_needed, final *scontrol=0x300
>> [ 15.168708][ T113] resume, do loop
>> [ 15.403902][ T113] resume, after do loop
>> [ 15.463901][ T113] debounce, SCR=0x100
> 
> [...]
> 
>> [ 16.413901][ T113] debounce, SCR=0x0
>> [ 16.473901][ T113] debounce, SCR=0x100
>> [ 16.477586][ T113] resume, return at end of function
>> [ 16.481446][ T113] hardreset, ata_phys_link_offline check failed
>> [ 16.486550][ T113] ata2: SATA link down (SStatus 100 SControl 300)
>> [ 16.499019][ T113] sata_down_spd_limit: limit 0x1, cur spd 0x0, saved limit 0xffffffff, hw limit
>> 0xffffffff
>> [ 16.508914][ T113] ata2: limiting SATA link speed to 1.5 Gbps
> 
> Marius,
> 
> Thanks again for testing.
> Could you try a new variation of the fix shown below ? This newer fix
> should avoid uselessly going down to 1.5 Gbps as shown above.
> Thanks !
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 884ae73b11ea..04938db566e5 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -3085,10 +3085,31 @@ int sata_down_spd_limit(struct ata_link *link, u32
> spd_limit)
>          * If not, use cached value in link->sata_spd.
>          */
>         rc = sata_scr_read(link, SCR_STATUS, &sstatus);
> -       if (rc == 0 && ata_sstatus_online(sstatus))
> +       if (rc == 0 && ata_sstatus_online(sstatus)) {
>                 spd = (sstatus >> 4) & 0xf;
> -       else
> +       } else {
> +               /*
> +                * If we reach here, the device is still offline or not
> +                * reporting a current speed yet. If we do not have a recorded
> +                * speed either, it means that probing the drive is failing,
> +                * most likely because problem with link speed autonegotiation
> +                * (e.g. reported case with mvebu adapter with a port multiplier
> +                * device). In this case, assume that probing was done at Gen3
> +                * speed and set the recorded speed to lower Gen2. With this,
> +                * the next probe retry will be done after sata_set_spd()
> +                * explicitly lowers the link speed. If the device then
> +                * responds, the actual maximum speed will be set once
> +                * libata-eh finishes the device revalidation.
> +                */
> +               if (!link->sata_spd) {
> +                       link->sata_spd = 2;
> +                       ata_link_warn(link,
> +                                     "No reported link speed. Assuming %s\n",
> +                                     sata_spd_string(link->sata_spd));
> +                       return;

Oops. My bad. This should be "return 0;" of course.

> +               }
>                 spd = link->sata_spd;
> +       }
> 
>         mask = link->sata_spd_limit;
>         if (mask <= 1)
> 

-- 
Damien Le Moal
Western Digital Research