libata: sata_down_spd_limit should return if driver has not recorded sstatus speed

Message ID 1510697845-58071-1-git-send-email-dmilburn@redhat.com
State Not Applicable
Delegated to: David Miller
Headers show
Series
  • libata: sata_down_spd_limit should return if driver has not recorded sstatus speed
Related show

Commit Message

David Milburn Nov. 14, 2017, 10:17 p.m.
During hotplug, it is possible for 6Gbps link speed to
be limited all the way down to 1.5 Gbps which may lead
to a slower link speed when drive is re-connected.

This behavior has been seen on a Intel Lewisburg SATA
controller (8086:a1d2) with HGST HUH728080ALE600 drive
where SATA link speed was limited to 1.5 Gbps and
when re-connected the link came up 3.0 Gbps.

This patch was retested on above configuration and
showed the hotplugged link to come back online at max
speed (6Gbps). I did not see the downgrade when testing
on Intel C600/X79, but retested patched linux-4.14-rc5
kernel and didn't see any side effects from this
change. Also, successfully retested hotplug on port
multiplier 3Gbps link.

Signed-off-by: David Milburn <dmilburn@redhat.com>
---
 drivers/ata/libata-core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Tejun Heo Nov. 27, 2017, 7:19 p.m. | #1
Hello, David.

On Tue, Nov 14, 2017 at 04:17:25PM -0600, David Milburn wrote:
> During hotplug, it is possible for 6Gbps link speed to
> be limited all the way down to 1.5 Gbps which may lead
> to a slower link speed when drive is re-connected.

Can you please explain the scenario with more details?

>  	/* Mask off all speeds higher than or equal to the current
> -	 * one.  Force 1.5Gbps if current SPD is not available.
> +	 * one. At this point if current SPD is not available and we
> +	 * previously recorded the link speed from the Status register,
> +	 * the driver has already masked off the highest bit so mask
> +	 * should already be set to 1 or 0. Otherwise, we should
> +	 * not force 1.5Gbps on a link where we have not previously 
> +	 * recorded speed from Status register, just return in this case.
>  	 */
>  	if (spd > 1)
>  		mask &= (1 << (spd - 1)) - 1;
>  	else
> -		mask &= 1;
> +		return -EINVAL;

I get that the current behavior might not be ideal in certain
scenarios but the above seems weird, especially given the we have
link->sata_spd for cases where we can't access the spd value.

Thanks.
David Milburn Nov. 27, 2017, 8:54 p.m. | #2
Hi Tejun,

On 11/27/2017 01:19 PM, Tejun Heo wrote:
> Hello, David.
> 
> On Tue, Nov 14, 2017 at 04:17:25PM -0600, David Milburn wrote:
>> During hotplug, it is possible for 6Gbps link speed to
>> be limited all the way down to 1.5 Gbps which may lead
>> to a slower link speed when drive is re-connected.
> 
> Can you please explain the scenario with more details?

Yes, in this particular case (I had a second reporter, but,
they didn't respond to my questions), when disconnecting
the drive, the error handler limits the speed to 1.5 Gpps
by setting bit 4 of the control register. And when the drive
is reconnected the status register show a link speed of 3Gbps,
even though before hot unplug it was running a 6Gbps.

> 
>>   	/* Mask off all speeds higher than or equal to the current
>> -	 * one.  Force 1.5Gbps if current SPD is not available.
>> +	 * one. At this point if current SPD is not available and we
>> +	 * previously recorded the link speed from the Status register,
>> +	 * the driver has already masked off the highest bit so mask
>> +	 * should already be set to 1 or 0. Otherwise, we should
>> +	 * not force 1.5Gbps on a link where we have not previously
>> +	 * recorded speed from Status register, just return in this case.
>>   	 */
>>   	if (spd > 1)
>>   		mask &= (1 << (spd - 1)) - 1;
>>   	else
>> -		mask &= 1;
>> +		return -EINVAL;
> 
> I get that the current behavior might not be ideal in certain
> scenarios but the above seems weird, especially given the we have
> link->sata_spd for cases where we can't access the spd value.
> 

Yes, but link->sata_spd is initialized to 0, and the only time the
driver records "link->sata_spd" from the status register is in 
ata_eh_reset(). And on hot unplug, the driver sets the mask to 1
dropping the link from 6 to 1.5Gbps. And, if the driver doesn't
downgrade the link speed by writing to the control register, the link
comes back at 6Gbps on hot plug. It maybe particular controller/configs
behave differently since it is not always reproducible.

Thanks,
David


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo Dec. 4, 2017, 9:57 p.m. | #3
On Tue, Nov 14, 2017 at 04:17:25PM -0600, David Milburn wrote:
> During hotplug, it is possible for 6Gbps link speed to
> be limited all the way down to 1.5 Gbps which may lead
> to a slower link speed when drive is re-connected.
> 
> This behavior has been seen on a Intel Lewisburg SATA
> controller (8086:a1d2) with HGST HUH728080ALE600 drive
> where SATA link speed was limited to 1.5 Gbps and
> when re-connected the link came up 3.0 Gbps.
> 
> This patch was retested on above configuration and
> showed the hotplugged link to come back online at max
> speed (6Gbps). I did not see the downgrade when testing
> on Intel C600/X79, but retested patched linux-4.14-rc5
> kernel and didn't see any side effects from this
> change. Also, successfully retested hotplug on port
> multiplier 3Gbps link.
> 
> Signed-off-by: David Milburn <dmilburn@redhat.com>

Applied to libata/for-4.15-fixes.

Thanks.

Patch

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index ee4c1ec..b72b242 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3081,12 +3081,17 @@  int sata_down_spd_limit(struct ata_link *link, u32 spd_limit)
 	mask &= ~(1 << bit);
 
 	/* Mask off all speeds higher than or equal to the current
-	 * one.  Force 1.5Gbps if current SPD is not available.
+	 * one. At this point if current SPD is not available and we
+	 * previously recorded the link speed from the Status register,
+	 * the driver has already masked off the highest bit so mask
+	 * should already be set to 1 or 0. Otherwise, we should
+	 * not force 1.5Gbps on a link where we have not previously 
+	 * recorded speed from Status register, just return in this case.
 	 */
 	if (spd > 1)
 		mask &= (1 << (spd - 1)) - 1;
 	else
-		mask &= 1;
+		return -EINVAL;
 
 	/* were we already at the bottom? */
 	if (!mask)