Re: Bug report for ahci-mvebu driver

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

On 1/20/23 03:46, marius@psihoexpert.ro wrote:
> January 19, 2023 2:29 AM, "Damien Le Moal" <damien.lemoal@opensource.wdc.com> wrote:
> 
>> On 2023/01/19 4:43, marius@psihoexpert.ro wrote:
>>
>>> [ 73.075849][ T112] hardreset, Online=>Offline
>>> [ 73.075856][ T112] sata_set_spd_needed, scontrol=0x300
>>> [ 73.080328][ T112] __sata_set_spd_needed, initial limit=0xFFFFFFFF
>>> [ 73.085601][ T112] __sata_set_spd_needed, corrected limit=0xFFFFFFFF
>>> [ 73.091900][ T112] __sata_set_spd_needed, target=0x0
>>> [ 73.098386][ T112] __sata_set_spd_needed, spd=0x0
>>> [ 73.103475][ T112] __sata_set_spd_needed, final *scontrol=0x300
>>
>> Can you share the patch/diff printing these ? Just to be sure I look at the
>> right place :)
> 
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -427,6 +430,7 @@ static int __sata_set_spd_needed(struct ata_link *link, u32 *scontrol)
>         u32 limit, target, spd;
> 
>         limit = link->sata_spd_limit;
> +       printk(KERN_DEBUG "__sata_set_spd_needed, initial limit=0x%X",limit);
> 
>         /* Don't configure downstream link faster than upstream link.
>          * It doesn't speed up anything and some PMPs choke on such
> @@ -435,14 +439,18 @@ static int __sata_set_spd_needed(struct ata_link *link, u32 *scontrol)
>         if (!ata_is_host_link(link) && host_link->sata_spd)
>                 limit &= (1 << host_link->sata_spd) - 1;
> 
> +       printk(KERN_DEBUG "__sata_set_spd_needed, corrected limit=0x%X",limit);
>         if (limit == UINT_MAX)
>                 target = 0;
>         else
>                 target = fls(limit);
> 
> +       printk(KERN_DEBUG "__sata_set_spd_needed, target=0x%X",target);
>         spd = (*scontrol >> 4) & 0xf;
>         *scontrol = (*scontrol & ~0xf0) | ((target & 0xf) << 4);
> 
> +       printk(KERN_DEBUG "__sata_set_spd_needed, spd=0x%X",spd);
> +       printk(KERN_DEBUG "__sata_set_spd_needed, final *scontrol=0x%X",*scontrol);
>         return spd != target;
>  }
> 
> @@ -467,6 +475,7 @@ static int sata_set_spd_needed(struct ata_link *link)
> 
>         if (sata_scr_read(link, SCR_CONTROL, &scontrol))
>                 return 1;
> +       printk(KERN_DEBUG "sata_set_spd_needed, scontrol=0x%X",scontrol);
> 
>         return __sata_set_spd_needed(link, &scontrol);
>  }

Thanks for that. Unfortunately, my PMP box is currently disconnected from
my test machine in the lab. Need to get there to reconnect it :)

But I think I got an idea of what is wrong here. Can you try this patch:

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 18ef14e749a0..cb12054c733f 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -436,7 +436,8 @@ static int __sata_set_spd_needed(struct ata_link
*link, u32 *scontrol)
                limit &= (1 << host_link->sata_spd) - 1;

        if (limit == UINT_MAX)
-               target = 0;
+               /* Try highest gen 3 limit */
+               target = 3;
        else
                target = fls(limit);

Note that with further digging, it seems that even for a working system
with all drives detected, we always end-up with something like:

cat
/sys/devices/pci0000:00/0000:00:17.0/ata6/link6/ata_link/link6/sata_spd_limit
<unknown>

And same for hw_sata_spd_limit.

So the UINT_MAX blind initialization of the speed limit before probing
seems to never be corrected and set to the actual limit of the
link/device. That needs further correction. But for your case, the above
should fix the issue I think.

-- 
Damien Le Moal
Western Digital Research