| 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 | expand |
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.
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
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.
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)
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(-)