On 1/29/23 02:08, marius@xxxxxxxxxxxxxx wrote:
> January 27, 2023 8:28 AM, "Damien Le Moal" <damien.lemoal@xxxxxxxxxxxxxxxxxx> wrote:
>
>> On 1/27/23 14:13, Damien Le Moal wrote:
>>
>>> 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
>
>
> It doesn't work.
>
> [ 15.145150][ T113] hardreset, Online=>Offline
> [ 15.145159][ T113] sata_set_spd_needed, scontrol=0x300
> [ 15.149632][ T113] __sata_set_spd_needed, initial limit=0xFFFFFFFF
> [ 15.154910][ T113] __sata_set_spd_needed, corrected limit=0xFFFFFFFF
> [ 15.161211][ T113] __sata_set_spd_needed, target=0x0
> [ 15.167700][ T113] __sata_set_spd_needed, spd=0x0
> [ 15.172781][ T113] __sata_set_spd_needed, final *scontrol=0x300
> [ 15.178685][ T113] resume, do loop
> [ 15.413909][ T113] resume, after do loop
> [ 15.473909][ T113] debounce, SCR=0x100
> ..
> [ 16.553930][ T113] debounce, SCR=0x100
> [ 16.557791][ T113] resume, return at end of function
> [ 16.561650][ T113] hardreset, ata_phys_link_offline check failed
> [ 16.566754][ T113] ata2: SATA link down (SStatus 101 SControl 300)
> [ 16.579222][ T113] sata_down_spd_limit: limit 0x1, cur spd 0x0, saved limit 0xffffffff, hw limit 0xffffffff
> [ 16.589115][ T113] ata2: No reported link speed. Assuming 3.0 Gbps
> [ 16.595436][ T113] hardreset, Online=>Offline
> [ 16.595442][ T113] sata_set_spd_needed, scontrol=0x300
> [ 16.599911][ T113] __sata_set_spd_needed, initial limit=0xFFFFFFFF
> [ 16.605178][ T113] __sata_set_spd_needed, corrected limit=0xFFFFFFFF
> [ 16.611479][ T113] __sata_set_spd_needed, target=0x0
> [ 16.617975][ T113] __sata_set_spd_needed, spd=0x0
> [ 16.623055][ T113] __sata_set_spd_needed, final *scontrol=0x300
OK. That is because the limit is not being changed with the added "return 0".
What about this version:
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 884ae73b11ea..f404e2c2869e 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 speed limit to the lower Gen2 speed.
+ * 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_limit == UINT_MAX) {
+ link->sata_spd_limit = 2;
+ ata_link_warn(link,
+ "No reported SATA link speed. Limiting to
%s\n",
+ sata_spd_string(link->sata_spd_limit));
+ return 0;
+ }
spd = link->sata_spd;
+ }
mask = link->sata_spd_limit;
if (mask <= 1)
Does this work ?
--
Damien Le Moal
Western Digital Research
