libata: fix EH action overwriting in ata_eh_reset()
PATCH 2/4 libata: always do follow-up SRST if hardreset returned -EAGAIN
In reply to Tejun HeoAs an optimization, follow-up SRST used to be skipped if classification wasn't requested even when hardreset requested it via -EAGAIN. However, some hardresets can't wait for device readiness and skipping SRST can cause timeout or other failures during revalidation. Always perform follow-up SRST if hardreset returns -EAGAIN. This makes reset paths more predictable and thus less error-prone.
While at it, move hardreset error checking such that it's done right after hardreset is finished. This simplifies followup SRST condition check a bit and makes the reset path easier to modify.
--- drivers/ata/libata-eh.c | 20 ++++++-------------- 1 files changed, 6 insertions(+), 14 deletions(-)
drivers/ata/libata-eh.c +6 -14
@@ -2171,18 +2171,12 @@ static int ata_do_reset(struct ata_link *link, ata_reset_fn_t reset, }static int ata_eh_followup_srst_needed (struct ata_link *link, -int rc,int classify, -const unsigned int *classes) +int rc,const unsigned int *classes) {if ((link->flags &ATA_LFLAG_NO_SRST ) ||ata_link_offline (link))return 0 ; -if (rc == -EAGAIN ) { -if (classify) -return 1 ; - rc =0 ; - } -if (rc !=0 ) -return 0 ; +if (rc == -EAGAIN ) +return 1 ;if (sata_pmp_supported (link->ap) &&ata_is_host_link (link))return 1 ;return 0 ;
@@ -2305,9 +2299,11 @@ int ata_eh_reset(struct ata_link *link, int classify, ehc->i.flags |=ATA_EHI_DID_SOFTRESET ; rc =ata_do_reset (link, reset, classes, deadline); +if (rc && rc != -EAGAIN ) + goto fail;if (reset == hardreset && -ata_eh_followup_srst_needed (link, rc, classify, classes)) { +ata_eh_followup_srst_needed (link, rc, classes)) {/* okay, let's do follow-up softreset */ reset = softreset;
@@ -2322,10 +2318,6 @@ int ata_eh_reset(struct ata_link *link, int classify,ata_eh_about_to_do (link,NULL ,ATA_EH_RESET ); rc =ata_do_reset (link, reset, classes, deadline); } - -/* -EAGAIN can happen if we skipped followup SRST */ -if (rc && rc != -EAGAIN ) - goto fail; }else {if (verbose) ata_link_printk(link, KERN_INFO, "no reset method "
Signature · 1 line
1.5.4.5
PATCH 1/4 libata: fix EH action overwriting in ata_eh_reset()
ehc->i.action got accidentally overwritten to ATA_EH_HARD/SOFTRESET in ata_eh_reset(). The original intention was to clear reset action which wasn't selected. This can cause unexpected behavior when other EH actions are scheduled together with reset. Fix it.
--- drivers/ata/libata-eh.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/ata/libata-eh.c +2 -2
@@ -2247,10 +2247,10 @@ int ata_eh_reset(struct ata_link *link, int classify, ehc->i.action &= ~ATA_EH_RESET ;if (hardreset) { reset = hardreset; - ehc->i.action =ATA_EH_HARDRESET ; + ehc->i.action |=ATA_EH_HARDRESET ; }else if (softreset) { reset = softreset; - ehc->i.action =ATA_EH_SOFTRESET ; + ehc->i.action |=ATA_EH_SOFTRESET ; }if (prereset) {
Signature · 1 line
1.5.4.5
PATCH 3/4 libata: use ata_link_printk() when printing SError
In reply to Tejun HeoSError belongs to link not port. Use ata_link_printk() to print it.
--- drivers/ata/libata-eh.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/ata/libata-eh.c +1 -1
@@ -2040,7 +2040,7 @@ static void ata_eh_link_report(struct ata_link *link) }if (ehc->i.serror) - ata_port_printk(ap, KERN_ERR, + ata_link_printk(link, KERN_ERR, "SError: { %s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s}\n", ehc->i.serror & SERR_DATA_RECOVERED ? "RecovData " : "", ehc->i.serror & SERR_COMM_RECOVERED ? "RecovComm " : "",
Signature · 1 line
1.5.4.5
PATCH 4/4 libata: restore SControl on detach
Save SControl during probing and restore it on detach. This prevents adjustments made by libata drivers to seep into the next driver which gets attached (be it a libata one or not).
It's not clear whether SControl also needs to be restored on suspend. The next system to have control (ACPI or kexec'd kernel) would probably like to see the original SControl value but there's no guarantee that a link is gonna keep working after SControl is adjusted without a reset and adding a reset and modified recovery cycle soley for this is an overkill. For now, do it only for detach.
--- drivers/ata/libata-core.c | 10 +++++----- include/linux/libata.h | 1 + 2 files changed, 6 insertions(+), 5 deletions(-)
drivers/ata/libata-core.c +5 -5
@@ -5158,15 +5158,14 @@ void ata_link_init(struct ata_port *ap, struct ata_link *link, int pmp)*/ int sata_link_init_spd (struct ata_link *link) { -u32 scontrol;u8 spd;int rc; - rc =sata_scr_read (link,SCR_CONTROL , &scontrol); + rc =sata_scr_read (link,SCR_CONTROL , &link->saved_scontrol);if (rc)return rc; - spd = (scontrol >>4 ) &0xf ; + spd = (link->saved_scontrol >>4 ) &0xf ;if (spd) link->hw_sata_spd_limit &= (1 << spd) -1 ;
@@ -5753,9 +5752,10 @@ static void ata_port_detach(struct ata_port *ap)ata_port_wait_eh (ap);/* EH is now guaranteed to see UNLOADING - EH context belongs -* to us. Disable all existing devices. +* to us. Restore SControl and disable all existing devices. */ -ata_port_for_each_link (link, ap) { +__ata_port_for_each_link (link, ap) { +sata_scr_write (link,SCR_CONTROL , link->saved_scontrol);ata_link_for_each_dev (dev, link)ata_dev_disable (dev); }
include/linux/libata.h +1 -0
@@ -646,6 +646,7 @@ struct ata_link {unsigned int flags;/* ATA_LFLAG_xxx */ +u32 saved_scontrol;/* SControl on probe */ unsigned int hw_sata_spd_limit;unsigned int sata_spd_limit;unsigned int sata_spd;/* current SATA PHY speed */
Signature · 1 line
1.5.4.5
Quoted 6 lines · Tejun Heo
ehc->i.actiongot accidentally overwritten to ATA_EH_HARD/SOFTRESET inata_eh_reset(). The original intention was to clear reset action which wasn't selected. This can cause unexpected behavior when other EH actions are scheduled together with reset. Fix it.Signed-off-by Tejun Heo <[email protected]>
applied 1-4