* [PATCH 1/2] libata: initialize link speed from sstatus if online.
2017-10-19 18:30 [PATCH 0/2] libata: hotplug link speed David Milburn
@ 2017-10-19 18:30 ` David Milburn
2017-10-21 16:48 ` Tejun Heo
2017-10-19 18:30 ` [PATCH 2/2] Allow error handler to change speed only if link is online David Milburn
2017-10-21 16:53 ` [PATCH 0/2] libata: hotplug link speed Tejun Heo
2 siblings, 1 reply; 6+ messages in thread
From: David Milburn @ 2017-10-19 18:30 UTC (permalink / raw)
To: linux-ide; +Cc: tj
If not, a 6Gbps link may be limited to 1.5 Gbps instead of 3.0 Gbps, since
ata_dev_init initializes link->sata_spd to 0.
[ 938.387795] ata4: exception Emask 0x50 SAct 0x0 SErr 0x4090800 action 0xe frozen
[ 938.387803] ata4: irq_stat 0x00400040, connection status changed
[ 938.387807] ata4: SError: { HostInt PHYRdyChg 10B8B DevExch }
[ 938.387814] ata4: hard resetting link
[ 939.108967] ata4: SATA link down (SStatus 0 SControl 300)
[ 944.100701] ata4: hard resetting link
[ 944.404187] ata4: SATA link down (SStatus 0 SControl 300)
[ 944.404198] ata4: limiting SATA link speed to 1.5 Gbps
[ 949.394013] ata4: hard resetting link
[ 949.697465] ata4: SATA link down (SStatus 0 SControl 310)
[ 949.697476] ata4.00: disabled
[ 949.697490] ata4: EH complete
[ 949.697503] ata4.00: detaching (SCSI 3:0:0:0)
Signed-off-by: David Milburn <dmilburn@redhat.com>
---
drivers/ata/libata-core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index ee4c1ec..38c40cf 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3002,16 +3002,16 @@ int ata_bus_probe(struct ata_port *ap)
*/
static void sata_print_link_status(struct ata_link *link)
{
- u32 sstatus, scontrol, tmp;
+ u32 sstatus, scontrol;
if (sata_scr_read(link, SCR_STATUS, &sstatus))
return;
sata_scr_read(link, SCR_CONTROL, &scontrol);
if (ata_phys_link_online(link)) {
- tmp = (sstatus >> 4) & 0xf;
+ link->sata_spd = (sstatus >> 4) & 0xf;
ata_link_info(link, "SATA link up %s (SStatus %X SControl %X)\n",
- sata_spd_string(tmp), sstatus, scontrol);
+ sata_spd_string(link->sata_spd), sstatus, scontrol);
} else {
ata_link_info(link, "SATA link down (SStatus %X SControl %X)\n",
sstatus, scontrol);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] libata: initialize link speed from sstatus if online.
2017-10-19 18:30 ` [PATCH 1/2] libata: initialize link speed from sstatus if online David Milburn
@ 2017-10-21 16:48 ` Tejun Heo
0 siblings, 0 replies; 6+ messages in thread
From: Tejun Heo @ 2017-10-21 16:48 UTC (permalink / raw)
To: David Milburn; +Cc: linux-ide
Hello, David.
On Thu, Oct 19, 2017 at 01:30:35PM -0500, David Milburn wrote:
> @@ -3002,16 +3002,16 @@ int ata_bus_probe(struct ata_port *ap)
> */
> static void sata_print_link_status(struct ata_link *link)
> {
> - u32 sstatus, scontrol, tmp;
> + u32 sstatus, scontrol;
>
> if (sata_scr_read(link, SCR_STATUS, &sstatus))
> return;
> sata_scr_read(link, SCR_CONTROL, &scontrol);
>
> if (ata_phys_link_online(link)) {
> - tmp = (sstatus >> 4) & 0xf;
> + link->sata_spd = (sstatus >> 4) & 0xf;
> ata_link_info(link, "SATA link up %s (SStatus %X SControl %X)\n",
> - sata_spd_string(tmp), sstatus, scontrol);
> + sata_spd_string(link->sata_spd), sstatus, scontrol);
> } else {
> ata_link_info(link, "SATA link down (SStatus %X SControl %X)\n",
> sstatus, scontrol);
I don't think it makes sense to update a link field from a
print_status function.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] Allow error handler to change speed only if link is online.
2017-10-19 18:30 [PATCH 0/2] libata: hotplug link speed David Milburn
2017-10-19 18:30 ` [PATCH 1/2] libata: initialize link speed from sstatus if online David Milburn
@ 2017-10-19 18:30 ` David Milburn
2017-10-21 16:53 ` [PATCH 0/2] libata: hotplug link speed Tejun Heo
2 siblings, 0 replies; 6+ messages in thread
From: David Milburn @ 2017-10-19 18:30 UTC (permalink / raw)
To: linux-ide; +Cc: tj
This patch prevents the error handler from changing link->sata_spd unless the
link is online.
(debug output)
[ 79.658098] ATA_EH_RESET: sstatus 0x0 link->sata_spd 0x0
[ 79.658104] SATA_PRINT_LINK_STATUS: sstatus 0x0 scontrol 0x300
[ 79.658109] ata4: SATA link down (SStatus 0 SControl 300)
[ 79.658113] ATA_PHYS_LINK_OFFLINE: sata_scr_read sstatus 0x0
[ 79.658117] ATA_PHYS_LINK_OFFLINE: sata_scr_read sstatus 0x0
[ 79.658128] SATA_DOWN_SPD_LIMIT: sstatus 0x0
[ 79.658130] SATA_DOWN_SPD_LIMIT: spd 0x0 mask 0xffffffff
[ 79.658132] SATA_DOWN_SPD_LIMIT: bit 31 mask 0x7fffffff
[ 79.658134] SATA_DOWN_SPD_LIMIT: spd 0x0 mask 0x1
[ 79.658137] ata4: limiting SATA link speed to 1.5 Gbps
Signed-off-by: David Milburn <dmilburn@redhat.com>
---
drivers/ata/libata-core.c | 2 +-
drivers/ata/libata-eh.c | 6 ++++--
drivers/ata/libata.h | 1 +
3 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 38c40cf..57c7e56 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -177,7 +177,7 @@ struct ata_force_ent {
MODULE_VERSION(DRV_VERSION);
-static bool ata_sstatus_online(u32 sstatus)
+bool ata_sstatus_online(u32 sstatus)
{
return (sstatus & 0xf) == 0x3;
}
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index e4effef..d469951 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -2911,9 +2911,11 @@ int ata_eh_reset(struct ata_link *link, int classify,
}
/* record current link speed */
- if (sata_scr_read(link, SCR_STATUS, &sstatus) == 0)
+ if (!sata_scr_read(link, SCR_STATUS, &sstatus) &&
+ ata_sstatus_online(sstatus))
link->sata_spd = (sstatus >> 4) & 0xf;
- if (slave && sata_scr_read(slave, SCR_STATUS, &sstatus) == 0)
+ if (slave && !sata_scr_read(slave, SCR_STATUS, &sstatus) &&
+ ata_sstatus_online(sstatus))
slave->sata_spd = (sstatus >> 4) & 0xf;
/* thaw the port */
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 839d487..7787fcc 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -52,6 +52,7 @@ enum {
extern int libata_noacpi;
extern int libata_allow_tpm;
extern struct device_type ata_port_type;
+extern bool ata_sstatus_online(u32 sstatus);
extern struct ata_link *ata_dev_phys_link(struct ata_device *dev);
extern void ata_force_cbl(struct ata_port *ap);
extern u64 ata_tf_to_lba(const struct ata_taskfile *tf);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 0/2] libata: hotplug link speed
2017-10-19 18:30 [PATCH 0/2] libata: hotplug link speed David Milburn
2017-10-19 18:30 ` [PATCH 1/2] libata: initialize link speed from sstatus if online David Milburn
2017-10-19 18:30 ` [PATCH 2/2] Allow error handler to change speed only if link is online David Milburn
@ 2017-10-21 16:53 ` Tejun Heo
2017-10-23 19:38 ` David Milburn
2 siblings, 1 reply; 6+ messages in thread
From: Tejun Heo @ 2017-10-21 16:53 UTC (permalink / raw)
To: David Milburn; +Cc: linux-ide
Hello, David.
On Thu, Oct 19, 2017 at 01:30:34PM -0500, 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. ata_dev_init
> initializes link->spd to 0, but, the 0001 patch will
> reset it once the link comes online. The 0002 patch will
> allow the error handler to reset link->spd only if the
> link is online; otherwise, it will always get set to 0.
>
> This patch set 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 these
> changes. Also, successfully retested hotplug on port
> multiplier 3Gbps link.
While the problem seems valid, I'm not sure the patches are in the
right direction. Isn't the root cause the decision we're making in
sata_down_spd_limit() where the code is explicitly choosing 1.5Gbps if
spd information isn't available? It'd make more sense to make better
decisions there rather than faking the information going into them.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] libata: hotplug link speed
2017-10-21 16:53 ` [PATCH 0/2] libata: hotplug link speed Tejun Heo
@ 2017-10-23 19:38 ` David Milburn
0 siblings, 0 replies; 6+ messages in thread
From: David Milburn @ 2017-10-23 19:38 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide
Hi Tejun,
On 10/21/2017 11:53 AM, Tejun Heo wrote:
> Hello, David.
>
> On Thu, Oct 19, 2017 at 01:30:34PM -0500, 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. ata_dev_init
>> initializes link->spd to 0, but, the 0001 patch will
>> reset it once the link comes online. The 0002 patch will
>> allow the error handler to reset link->spd only if the
>> link is online; otherwise, it will always get set to 0.
>>
>> This patch set 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 these
>> changes. Also, successfully retested hotplug on port
>> multiplier 3Gbps link.
>
> While the problem seems valid, I'm not sure the patches are in the
> right direction. Isn't the root cause the decision we're making in
> sata_down_spd_limit() where the code is explicitly choosing 1.5Gbps if
> spd information isn't available? It'd make more sense to make better
> decisions there rather than faking the information going into them.
>
Ok, I will take a closer look at sata_down_spd_limit().
Thanks,
David
^ permalink raw reply [flat|nested] 6+ messages in thread