[PATCH #upstream-fixes 1/4] libata: fix EH action overwriting in ata_eh_reset()

[PATCH #upstream-fixes 1/4] libata: fix EH action overwriting in ata_eh_reset()

[Tejun Heo]

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.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/ata/libata-eh.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 58bdc53..a3c0139 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -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) {
-- 
1.5.4.5

[PATCH 2/4 #upstream-fixes] libata: always do follow-up SRST if hardreset returned -EAGAIN

[Tejun Heo]

As 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.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/ata/libata-eh.c |   20 ++++++--------------
 1 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index a3c0139..b82e8ae 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -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 "
-- 
1.5.4.5

[PATCH #upstream-fixes 3/4] libata: use ata_link_printk() when printing SError

[Tejun Heo]

SError belongs to link not port.  Use ata_link_printk() to print it.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/ata/libata-eh.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index b82e8ae..5de0bf3 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -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 " : "",
-- 
1.5.4.5

[PATCH 4/4] libata: restore SControl on detach

[Tejun Heo]

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.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/ata/libata-core.c |   10 +++++-----
 include/linux/libata.h    |    1 +
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 9bef1a8..9481937 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -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);
 	}
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 5b247b8..3437c11 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -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 */
-- 
1.5.4.5

Re: [PATCH #upstream-fixes 1/4] libata: fix EH action overwriting in ata_eh_reset()

[Jeff Garzik]

Tejun Heo wrote:
> 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.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>

applied 1-4