Re: Bug report for ahci-mvebu driver

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

On 1/24/23 05:00, marius@psihoexpert.ro wrote:
> January 23, 2023 9:02 AM, "Damien Le Moal"
> <damien.lemoal@opensource.wdc.com> wrote:
> 
>> 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);
> 
> It doesn't work. I didn't really expect it to work, since manually
> forcing 3Gbps didn't work either - probably a hardware issue like you
> said.

Well, this one was not intended to limit the link speed to anything.
target == 0 written to scontrol means "no autonegociation speed limit",
while target == 3 means "limit autonegotiated speed to gen 3", which are
effectively equivalent since the max speed is gen 3 :) The point was to
force an scontrol write: in your case, spd is 0 and the default code with
target == 0 ends up with __sata_set_spd_needed returning false and so
sata_set_spd() doing nothing. Setting target to 3 forced trying to set the
speed to gen3. Wanted to see what that did.

Given that things are working by forcing 1.5gbps, and you endup with the
correct 3gbps link speed, it seems that this hardware does not like
negotiation starting from the highest speed. The SATA-IO specifications do
clearly mandate that adapters should allow for any initial speed, the
highest or the lowest and be ready for negotiation from any starting
combination of device and adapter speeds. But it seems that the designers
of this adapter and/or of the pmp box missed that chapter :)

> I think that the old behaviour (before commit
> 2dc0b46b5ea30f169b0b272253ea846a5a281731) of slowing down sata speed
> when a connection can't be established is in fact the right thing to
> do. Maybe a longer delay before reducing the link speed would satisfy
> the random cases that the commit author reported. Maybe he was just
> having a loose connector issue - especially since he only reported one
> case.

I do not think that a longer delay will change anything in your case since
the device presence cannot be established when starting speed negotiation
without a limit: see the "debounce, SCR=0x100" messages in your debug,
while the 1.5gbps link speed forcing leads to an immediate device presence
detected (debounce, SCR=0x113 message).

Can you please try this one:

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 884ae73b11ea..a4e2a93af0e5 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3085,10 +3085,17 @@ 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 {
+               /*
+                * Device is not reporting a speed yet. Use the last recorded
+                * speed. If we do not have that either, start with Gen3
speed.
+                */
+               if (!link->sata_spd)
+                       link->sata_spd = 3;
                spd = link->sata_spd;
+       }

        mask = link->sata_spd_limit;
        if (mask <= 1)

If that does not work, we'll have to handle your case with a HORKAGE flag
for that adapter.

-- 
Damien Le Moal
Western Digital Research