[PATCHSET] libata: update EH speed down logic, take #2

[PATCHSET] libata: update EH speed down logic, take #2

[Tejun Heo]

Hello,

This is the second take of update-EH-speed-down-logic patchset.
Changes from the last take[1] are...

* Updated to apply & build against the current linus#master.  Recently
  committed error-passthhrough-for-non-IO-command change broke build
  with this patchset applied.

This patchset is against the current linux#master (b55d1b18).

Thanks.

--
tejun

[1] http://thread.gmane.org/gmane.linux.ide/24357

[PATCH 1/8] libata: rearrange ATA_DFLAG_*

[Tejun Heo]

Area for DFLAGs which are cleared on INIT is full.  Extend it by 8
bits.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 include/linux/libata.h |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/libata.h b/include/linux/libata.h
index 1e27785..65274d5 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -141,10 +141,10 @@ enum {
 	ATA_DFLAG_NCQ_OFF	= (1 << 13), /* device limited to non-NCQ mode */
 	ATA_DFLAG_SPUNDOWN	= (1 << 14), /* XXX: for spindown_compat */
 	ATA_DFLAG_SLEEPING	= (1 << 15), /* device is sleeping */
-	ATA_DFLAG_INIT_MASK	= (1 << 16) - 1,
+	ATA_DFLAG_INIT_MASK	= (1 << 24) - 1,
 
-	ATA_DFLAG_DETACH	= (1 << 16),
-	ATA_DFLAG_DETACHED	= (1 << 17),
+	ATA_DFLAG_DETACH	= (1 << 24),
+	ATA_DFLAG_DETACHED	= (1 << 25),
 
 	ATA_DEV_UNKNOWN		= 0,	/* unknown device */
 	ATA_DEV_ATA		= 1,	/* ATA device */
-- 
1.5.2.4

[PATCH 2/8] libata: add protocol data transfer tests

[Tejun Heo]

Add protocol data transfer tests.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 include/linux/ata.h |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/include/linux/ata.h b/include/linux/ata.h
index 61535e7..2318531 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -323,6 +323,27 @@ enum ata_tf_protocols {
 	ATA_PROT_ATAPI_DMA,	/* packet command with special DMA sauce */
 };
 
+static inline int ata_is_nodata(u8 prot)
+{
+	return prot == ATA_PROT_NODATA || prot == ATA_PROT_ATAPI_NODATA;
+}
+
+static inline int ata_is_pio_data(u8 prot)
+{
+	return prot == ATA_PROT_PIO || prot == ATA_PROT_ATAPI;
+}
+
+static inline int ata_is_dma_data(u8 prot)
+{
+	return prot == ATA_PROT_DMA || prot == ATA_PROT_NCQ ||
+		prot == ATA_PROT_ATAPI_DMA;
+}
+
+static inline int ata_is_data(u8 prot)
+{
+	return ata_is_pio_data(prot) || ata_is_dma_data(prot);
+}
+
 enum ata_ioctls {
 	ATA_IOC_GET_IO32	= 0x309,
 	ATA_IOC_SET_IO32	= 0x324,
-- 
1.5.2.4

[PATCH 3/8] libata: factor out ata_eh_schedule_probe()

[Tejun Heo]

Factor out ata_eh_schedule_probe() from ata_eh_handle_dev_fail() and
ata_eh_recover().  This is to improve maintainability and make future
changes easier.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-eh.c |   37 ++++++++++++++++++++-----------------
 1 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index ed8813b..a37be1e 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -2438,6 +2438,22 @@ static int ata_eh_skip_recovery(struct ata_link *link)
 	return 1;
 }
 
+static int ata_eh_schedule_probe(struct ata_device *dev)
+{
+	struct ata_eh_context *ehc = &dev->link->eh_context;
+
+	if (!(ehc->i.probe_mask & (1 << dev->devno)) ||
+	    (ehc->did_probe_mask & (1 << dev->devno)))
+		return 0;
+
+	ata_eh_detach_dev(dev);
+	ata_dev_init(dev);
+	ehc->did_probe_mask |= (1 << dev->devno);
+	ehc->i.action |= ATA_EH_SOFTRESET;
+
+	return 1;
+}
+
 static int ata_eh_handle_dev_fail(struct ata_device *dev, int err)
 {
 	struct ata_eh_context *ehc = &dev->link->eh_context;
@@ -2469,16 +2485,9 @@ static int ata_eh_handle_dev_fail(struct ata_device *dev, int err)
 		if (ata_link_offline(dev->link))
 			ata_eh_detach_dev(dev);
 
-		/* probe if requested */
-		if ((ehc->i.probe_mask & (1 << dev->devno)) &&
-		    !(ehc->did_probe_mask & (1 << dev->devno))) {
-			ata_eh_detach_dev(dev);
-			ata_dev_init(dev);
-
+		/* schedule probe if necessary */
+		if (ata_eh_schedule_probe(dev))
 			ehc->tries[dev->devno] = ATA_EH_DEV_TRIES;
-			ehc->did_probe_mask |= (1 << dev->devno);
-			ehc->i.action |= ATA_EH_SOFTRESET;
-		}
 
 		return 1;
 	} else {
@@ -2555,14 +2564,8 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
 			if (dev->flags & ATA_DFLAG_DETACH)
 				ata_eh_detach_dev(dev);
 
-			if (!ata_dev_enabled(dev) &&
-			    ((ehc->i.probe_mask & (1 << dev->devno)) &&
-			     !(ehc->did_probe_mask & (1 << dev->devno)))) {
-				ata_eh_detach_dev(dev);
-				ata_dev_init(dev);
-				ehc->did_probe_mask |= (1 << dev->devno);
-				ehc->i.action |= ATA_EH_SOFTRESET;
-			}
+			/* schedule probe if necessary */
+			ata_eh_schedule_probe(dev);
 		}
 	}
 
-- 
1.5.2.4

[PATCH 4/8] libata: move ata_set_mode() to libata-eh.c

[Tejun Heo]

Move ata_set_mode() to libata-eh.c.  ata_set_mode() is surely an EH
action and will be more tightly coupled with the rest of error
handling.  Move it to libata-eh.c.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-core.c |   25 -------------------------
 drivers/ata/libata-eh.c   |   25 +++++++++++++++++++++++++
 drivers/ata/libata.h      |    2 +-
 3 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 164c7d9..0f02a07 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3234,31 +3234,6 @@ int ata_do_set_mode(struct ata_link *link, struct ata_device **r_failed_dev)
 }
 
 /**
- *	ata_set_mode - Program timings and issue SET FEATURES - XFER
- *	@link: link on which timings will be programmed
- *	@r_failed_dev: out paramter for failed device
- *
- *	Set ATA device disk transfer mode (PIO3, UDMA6, etc.).  If
- *	ata_set_mode() fails, pointer to the failing device is
- *	returned in @r_failed_dev.
- *
- *	LOCKING:
- *	PCI/etc. bus probe sem.
- *
- *	RETURNS:
- *	0 on success, negative errno otherwise
- */
-int ata_set_mode(struct ata_link *link, struct ata_device **r_failed_dev)
-{
-	struct ata_port *ap = link->ap;
-
-	/* has private set_mode? */
-	if (ap->ops->set_mode)
-		return ap->ops->set_mode(link, r_failed_dev);
-	return ata_do_set_mode(link, r_failed_dev);
-}
-
-/**
  *	ata_tf_to_host - issue ATA taskfile to host controller
  *	@ap: port to which command is being issued
  *	@tf: ATA taskfile register set
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index a37be1e..ae03201 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -2392,6 +2392,31 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
 	return rc;
 }
 
+/**
+ *	ata_set_mode - Program timings and issue SET FEATURES - XFER
+ *	@link: link on which timings will be programmed
+ *	@r_failed_dev: out paramter for failed device
+ *
+ *	Set ATA device disk transfer mode (PIO3, UDMA6, etc.).  If
+ *	ata_set_mode() fails, pointer to the failing device is
+ *	returned in @r_failed_dev.
+ *
+ *	LOCKING:
+ *	PCI/etc. bus probe sem.
+ *
+ *	RETURNS:
+ *	0 on success, negative errno otherwise
+ */
+int ata_set_mode(struct ata_link *link, struct ata_device **r_failed_dev)
+{
+	struct ata_port *ap = link->ap;
+
+	/* has private set_mode? */
+	if (ap->ops->set_mode)
+		return ap->ops->set_mode(link, r_failed_dev);
+	return ata_do_set_mode(link, r_failed_dev);
+}
+
 static int ata_link_nr_enabled(struct ata_link *link)
 {
 	struct ata_device *dev;
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 0e6cf3a..9e9a2a9 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -85,7 +85,6 @@ extern int ata_dev_configure(struct ata_device *dev);
 extern int sata_down_spd_limit(struct ata_link *link);
 extern int sata_set_spd_needed(struct ata_link *link);
 extern int ata_down_xfermask_limit(struct ata_device *dev, unsigned int sel);
-extern int ata_set_mode(struct ata_link *link, struct ata_device **r_failed_dev);
 extern void ata_sg_clean(struct ata_queued_cmd *qc);
 extern void ata_qc_free(struct ata_queued_cmd *qc);
 extern void ata_qc_issue(struct ata_queued_cmd *qc);
@@ -179,6 +178,7 @@ extern void ata_eh_report(struct ata_port *ap);
 extern int ata_eh_reset(struct ata_link *link, int classify,
 			ata_prereset_fn_t prereset, ata_reset_fn_t softreset,
 			ata_reset_fn_t hardreset, ata_postreset_fn_t postreset);
+extern int ata_set_mode(struct ata_link *link, struct ata_device **r_failed_dev);
 extern int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
 			  ata_reset_fn_t softreset, ata_reset_fn_t hardreset,
 			  ata_postreset_fn_t postreset,
-- 
1.5.2.4

[PATCH 5/8] libata: clean up EH speed down implementation

[Tejun Heo]

Clean up EH speed down implementation.

* is_io boolean variable is replaced eflags.  is_io is ATA_EFLAG_IS_IO.

* Error categories now have names.

* Better comments.

* Reorder 5min and 10min rules in ata_eh_speed_down_verdict()

* Use local variable @link to cache @dev->link in ata_eh_speed_down()

These changes are to improve readability and ease further changes.
This patch doesn't introduce any behavior change.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-eh.c |  117 +++++++++++++++++++++++++++++------------------
 include/linux/libata.h  |    2 +-
 2 files changed, 74 insertions(+), 45 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index ae03201..7f505f1 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -46,9 +46,20 @@
 #include "libata.h"
 
 enum {
+	/* speed down verdicts */
 	ATA_EH_SPDN_NCQ_OFF		= (1 << 0),
 	ATA_EH_SPDN_SPEED_DOWN		= (1 << 1),
 	ATA_EH_SPDN_FALLBACK_TO_PIO	= (1 << 2),
+
+	/* error flags */
+	ATA_EFLAG_IS_IO			= (1 << 0),
+
+	/* error categories */
+	ATA_ECAT_NONE			= 0,
+	ATA_ECAT_ATA_BUS		= 1,
+	ATA_ECAT_TOUT_HSM		= 2,
+	ATA_ECAT_UNK_DEV		= 3,
+	ATA_ECAT_NR			= 4,
 };
 
 /* Waiting in ->prereset can never be reliable.  It's sometimes nice
@@ -218,7 +229,7 @@ void ata_port_pbar_desc(struct ata_port *ap, int bar, ssize_t offset,
 
 #endif /* CONFIG_PCI */
 
-static void ata_ering_record(struct ata_ering *ering, int is_io,
+static void ata_ering_record(struct ata_ering *ering, unsigned int eflags,
 			     unsigned int err_mask)
 {
 	struct ata_ering_entry *ent;
@@ -229,7 +240,7 @@ static void ata_ering_record(struct ata_ering *ering, int is_io,
 	ering->cursor %= ATA_ERING_SIZE;
 
 	ent = &ering->ring[ering->cursor];
-	ent->is_io = is_io;
+	ent->eflags = eflags;
 	ent->err_mask = err_mask;
 	ent->timestamp = get_jiffies_64();
 }
@@ -1546,20 +1557,20 @@ static unsigned int ata_eh_analyze_tf(struct ata_queued_cmd *qc,
 	return action;
 }
 
-static int ata_eh_categorize_error(int is_io, unsigned int err_mask)
+static int ata_eh_categorize_error(unsigned int eflags, unsigned int err_mask)
 {
 	if (err_mask & AC_ERR_ATA_BUS)
-		return 1;
+		return ATA_ECAT_ATA_BUS;
 
 	if (err_mask & AC_ERR_TIMEOUT)
-		return 2;
+		return ATA_ECAT_TOUT_HSM;
 
-	if (is_io) {
+	if (eflags & ATA_EFLAG_IS_IO) {
 		if (err_mask & AC_ERR_HSM)
-			return 2;
+			return ATA_ECAT_TOUT_HSM;
 		if ((err_mask &
 		     (AC_ERR_DEV|AC_ERR_MEDIA|AC_ERR_INVALID)) == AC_ERR_DEV)
-			return 3;
+			return ATA_ECAT_UNK_DEV;
 	}
 
 	return 0;
@@ -1567,13 +1578,13 @@ static int ata_eh_categorize_error(int is_io, unsigned int err_mask)
 
 struct speed_down_verdict_arg {
 	u64 since;
-	int nr_errors[4];
+	int nr_errors[ATA_ECAT_NR];
 };
 
 static int speed_down_verdict_cb(struct ata_ering_entry *ent, void *void_arg)
 {
 	struct speed_down_verdict_arg *arg = void_arg;
-	int cat = ata_eh_categorize_error(ent->is_io, ent->err_mask);
+	int cat = ata_eh_categorize_error(ent->eflags, ent->err_mask);
 
 	if (ent->timestamp < arg->since)
 		return -1;
@@ -1590,22 +1601,33 @@ static int speed_down_verdict_cb(struct ata_ering_entry *ent, void *void_arg)
  *	whether NCQ needs to be turned off, transfer speed should be
  *	stepped down, or falling back to PIO is necessary.
  *
- *	Cat-1 is ATA_BUS error for any command.
+ *	ECAT_ATA_BUS	: ATA_BUS error for any command
+ *
+ *	ECAT_TOUT_HSM	: TIMEOUT for any command or HSM violation for
+ *			  IO commands
+ *
+ *	ECAT_UNK_DEV	: Unknown DEV error for IO commands
+ *
+ *	Verdicts are
  *
- *	Cat-2 is TIMEOUT for any command or HSM violation for known
- *	supported commands.
+ *	NCQ_OFF		: Turn off NCQ.
  *
- *	Cat-3 is is unclassified DEV error for known supported
- *	command.
+ *	SPEED_DOWN	: Speed down transfer speed but don't fall back
+ *			  to PIO.
  *
- *	NCQ needs to be turned off if there have been more than 3
- *	Cat-2 + Cat-3 errors during last 10 minutes.
+ *	FALLBACK_TO_PIO	: Fall back to PIO.
  *
- *	Speed down is necessary if there have been more than 3 Cat-1 +
- *	Cat-2 errors or 10 Cat-3 errors during last 10 minutes.
+ *	Even if multiple verdicts are returned, only one action is
+ *	taken per error.  ering is cleared after an action is taken.
  *
- *	Falling back to PIO mode is necessary if there have been more
- *	than 10 Cat-1 + Cat-2 + Cat-3 errors during last 5 minutes.
+ *	1. If more than 10 ATA_BUS, TOUT_HSM or UNK_DEV errors
+ *	   ocurred during last 5 mins, FALLBACK_TO_PIO
+ *
+ *	2. If more than 3 TOUT_HSM or UNK_DEV errors occurred
+ *	   during last 10 mins, NCQ_OFF.
+ *
+ *	3. If more than 3 ATA_BUS or TOUT_HSM errors, or more than 10
+ *	   UNK_DEV errors occurred during last 10 mins, SPEED_DOWN.
  *
  *	LOCKING:
  *	Inherited from caller.
@@ -1620,23 +1642,29 @@ static unsigned int ata_eh_speed_down_verdict(struct ata_device *dev)
 	struct speed_down_verdict_arg arg;
 	unsigned int verdict = 0;
 
-	/* scan past 10 mins of error history */
+	/* scan past 5 mins of error history */
 	memset(&arg, 0, sizeof(arg));
-	arg.since = j64 - min(j64, j10mins);
+	arg.since = j64 - min(j64, j5mins);
 	ata_ering_map(&dev->ering, speed_down_verdict_cb, &arg);
 
-	if (arg.nr_errors[2] + arg.nr_errors[3] > 3)
-		verdict |= ATA_EH_SPDN_NCQ_OFF;
-	if (arg.nr_errors[1] + arg.nr_errors[2] > 3 || arg.nr_errors[3] > 10)
-		verdict |= ATA_EH_SPDN_SPEED_DOWN;
+	if (arg.nr_errors[ATA_ECAT_ATA_BUS] +
+	    arg.nr_errors[ATA_ECAT_TOUT_HSM] +
+	    arg.nr_errors[ATA_ECAT_UNK_DEV] > 10)
+		verdict |= ATA_EH_SPDN_FALLBACK_TO_PIO;
 
-	/* scan past 3 mins of error history */
+	/* scan past 10 mins of error history */
 	memset(&arg, 0, sizeof(arg));
-	arg.since = j64 - min(j64, j5mins);
+	arg.since = j64 - min(j64, j10mins);
 	ata_ering_map(&dev->ering, speed_down_verdict_cb, &arg);
 
-	if (arg.nr_errors[1] + arg.nr_errors[2] + arg.nr_errors[3] > 10)
-		verdict |= ATA_EH_SPDN_FALLBACK_TO_PIO;
+	if (arg.nr_errors[ATA_ECAT_TOUT_HSM] +
+	    arg.nr_errors[ATA_ECAT_UNK_DEV] > 3)
+		verdict |= ATA_EH_SPDN_NCQ_OFF;
+
+	if (arg.nr_errors[ATA_ECAT_ATA_BUS] +
+	    arg.nr_errors[ATA_ECAT_TOUT_HSM] > 3 ||
+	    arg.nr_errors[ATA_ECAT_UNK_DEV] > 10)
+		verdict |= ATA_EH_SPDN_SPEED_DOWN;
 
 	return verdict;
 }
@@ -1644,7 +1672,7 @@ static unsigned int ata_eh_speed_down_verdict(struct ata_device *dev)
 /**
  *	ata_eh_speed_down - record error and speed down if necessary
  *	@dev: Failed device
- *	@is_io: Did the device fail during normal IO?
+ *	@eflags: mask of ATA_EFLAG_* flags
  *	@err_mask: err_mask of the error
  *
  *	Record error and examine error history to determine whether
@@ -1658,18 +1686,19 @@ static unsigned int ata_eh_speed_down_verdict(struct ata_device *dev)
  *	RETURNS:
  *	Determined recovery action.
  */
-static unsigned int ata_eh_speed_down(struct ata_device *dev, int is_io,
-				      unsigned int err_mask)
+static unsigned int ata_eh_speed_down(struct ata_device *dev,
+				unsigned int eflags, unsigned int err_mask)
 {
+	struct ata_link *link = dev->link;
 	unsigned int verdict;
 	unsigned int action = 0;
 
 	/* don't bother if Cat-0 error */
-	if (ata_eh_categorize_error(is_io, err_mask) == 0)
+	if (ata_eh_categorize_error(eflags, err_mask) == 0)
 		return 0;
 
 	/* record error and determine whether speed down is necessary */
-	ata_ering_record(&dev->ering, is_io, err_mask);
+	ata_ering_record(&dev->ering, eflags, err_mask);
 	verdict = ata_eh_speed_down_verdict(dev);
 
 	/* turn off NCQ? */
@@ -1685,7 +1714,7 @@ static unsigned int ata_eh_speed_down(struct ata_device *dev, int is_io,
 	/* speed down? */
 	if (verdict & ATA_EH_SPDN_SPEED_DOWN) {
 		/* speed down SATA link speed if possible */
-		if (sata_down_spd_limit(dev->link) == 0) {
+		if (sata_down_spd_limit(link) == 0) {
 			action |= ATA_EH_HARDRESET;
 			goto done;
 		}
@@ -1716,7 +1745,7 @@ static unsigned int ata_eh_speed_down(struct ata_device *dev, int is_io,
 	 * SATA.  Consider it only for PATA.
 	 */
 	if ((verdict & ATA_EH_SPDN_FALLBACK_TO_PIO) && (dev->spdn_cnt >= 2) &&
-	    (dev->link->ap->cbl != ATA_CBL_SATA) &&
+	    (link->ap->cbl != ATA_CBL_SATA) &&
 	    (dev->xfer_shift != ATA_SHIFT_PIO)) {
 		if (ata_down_xfermask_limit(dev, ATA_DNXFER_FORCE_PIO) == 0) {
 			dev->spdn_cnt = 0;
@@ -1748,8 +1777,8 @@ static void ata_eh_link_autopsy(struct ata_link *link)
 	struct ata_port *ap = link->ap;
 	struct ata_eh_context *ehc = &link->eh_context;
 	struct ata_device *dev;
-	unsigned int all_err_mask = 0;
-	int tag, is_io = 0;
+	unsigned int all_err_mask = 0, eflags = 0;
+	int tag;
 	u32 serror;
 	int rc;
 
@@ -1808,15 +1837,15 @@ static void ata_eh_link_autopsy(struct ata_link *link)
 		ehc->i.dev = qc->dev;
 		all_err_mask |= qc->err_mask;
 		if (qc->flags & ATA_QCFLAG_IO)
-			is_io = 1;
+			eflags |= ATA_EFLAG_IS_IO;
 	}
 
 	/* enforce default EH actions */
 	if (ap->pflags & ATA_PFLAG_FROZEN ||
 	    all_err_mask & (AC_ERR_HSM | AC_ERR_TIMEOUT))
 		ehc->i.action |= ATA_EH_SOFTRESET;
-	else if ((is_io && all_err_mask) ||
-		 (!is_io && (all_err_mask & ~AC_ERR_DEV)))
+	else if (((eflags & ATA_EFLAG_IS_IO) && all_err_mask) ||
+		 (!(eflags & ATA_EFLAG_IS_IO) && (all_err_mask & ~AC_ERR_DEV)))
 		ehc->i.action |= ATA_EH_REVALIDATE;
 
 	/* If we have offending qcs and the associated failed device,
@@ -1835,7 +1864,7 @@ static void ata_eh_link_autopsy(struct ata_link *link)
 		dev = link->device;
 
 	if (dev)
-		ehc->i.action |= ata_eh_speed_down(dev, is_io, all_err_mask);
+		ehc->i.action |= ata_eh_speed_down(dev, eflags, all_err_mask);
 
 	DPRINTK("EXIT\n");
 }
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 65274d5..4abfb3c 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -478,7 +478,7 @@ struct ata_port_stats {
 };
 
 struct ata_ering_entry {
-	int			is_io;
+	unsigned int		eflags;
 	unsigned int		err_mask;
 	u64			timestamp;
 };
-- 
1.5.2.4

[PATCH 6/8] libata: adjust speed down rules

[Tejun Heo]

Speed down rules were too conservative.  Adjust them a bit.

* More than 10 timeouts can't happen in 5 minutes as command timeout
  is 30secs.  Lower the limit for rule #1 to 6.

* 10 timeouts is too high for rule #3 too.  Lower it to 6.

* SATAPI can benefit from falling back to PIO too.  Allow SATAPI
  devices to fall back to PIO.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-eh.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 7f505f1..cf2afb0 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -1620,13 +1620,13 @@ static int speed_down_verdict_cb(struct ata_ering_entry *ent, void *void_arg)
  *	Even if multiple verdicts are returned, only one action is
  *	taken per error.  ering is cleared after an action is taken.
  *
- *	1. If more than 10 ATA_BUS, TOUT_HSM or UNK_DEV errors
+ *	1. If more than 6 ATA_BUS, TOUT_HSM or UNK_DEV errors
  *	   ocurred during last 5 mins, FALLBACK_TO_PIO
  *
  *	2. If more than 3 TOUT_HSM or UNK_DEV errors occurred
  *	   during last 10 mins, NCQ_OFF.
  *
- *	3. If more than 3 ATA_BUS or TOUT_HSM errors, or more than 10
+ *	3. If more than 3 ATA_BUS or TOUT_HSM errors, or more than 6
  *	   UNK_DEV errors occurred during last 10 mins, SPEED_DOWN.
  *
  *	LOCKING:
@@ -1649,7 +1649,7 @@ static unsigned int ata_eh_speed_down_verdict(struct ata_device *dev)
 
 	if (arg.nr_errors[ATA_ECAT_ATA_BUS] +
 	    arg.nr_errors[ATA_ECAT_TOUT_HSM] +
-	    arg.nr_errors[ATA_ECAT_UNK_DEV] > 10)
+	    arg.nr_errors[ATA_ECAT_UNK_DEV] > 6)
 		verdict |= ATA_EH_SPDN_FALLBACK_TO_PIO;
 
 	/* scan past 10 mins of error history */
@@ -1663,7 +1663,7 @@ static unsigned int ata_eh_speed_down_verdict(struct ata_device *dev)
 
 	if (arg.nr_errors[ATA_ECAT_ATA_BUS] +
 	    arg.nr_errors[ATA_ECAT_TOUT_HSM] > 3 ||
-	    arg.nr_errors[ATA_ECAT_UNK_DEV] > 10)
+	    arg.nr_errors[ATA_ECAT_UNK_DEV] > 6)
 		verdict |= ATA_EH_SPDN_SPEED_DOWN;
 
 	return verdict;
@@ -1742,10 +1742,10 @@ static unsigned int ata_eh_speed_down(struct ata_device *dev,
 	}
 
 	/* Fall back to PIO?  Slowing down to PIO is meaningless for
-	 * SATA.  Consider it only for PATA.
+	 * SATA ATA devices.  Consider it only for PATA and SATAPI.
 	 */
 	if ((verdict & ATA_EH_SPDN_FALLBACK_TO_PIO) && (dev->spdn_cnt >= 2) &&
-	    (link->ap->cbl != ATA_CBL_SATA) &&
+	    (link->ap->cbl != ATA_CBL_SATA || dev->class == ATA_DEV_ATAPI) &&
 	    (dev->xfer_shift != ATA_SHIFT_PIO)) {
 		if (ata_down_xfermask_limit(dev, ATA_DNXFER_FORCE_PIO) == 0) {
 			dev->spdn_cnt = 0;
-- 
1.5.2.4

[PATCH 7/8] libata: implement ATA_DFLAG_DUBIOUS_XFER

[Tejun Heo]

ATA_DFLAG_DUBIOUS_XFER is set whenever data transfer speed or method
changes and gets cleared when data transfer command succeeds in the
newly configured transfer mode.

This will be used to improve speed down logic.

Signed-off-by: Tejun Heo <htejun@gmail.com<
---
 drivers/ata/libata-core.c |   20 ++++++++++++++++++++
 drivers/ata/libata-eh.c   |   33 +++++++++++++++++++++++++++++++--
 include/linux/libata.h    |    3 +++
 3 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 0f02a07..47f0a30 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5777,6 +5777,23 @@ static void fill_result_tf(struct ata_queued_cmd *qc)
 	ap->ops->tf_read(ap, &qc->result_tf);
 }
 
+static void ata_verify_xfer(struct ata_queued_cmd *qc)
+{
+	struct ata_device *dev = qc->dev;
+
+	if (ata_tag_internal(qc->tag))
+		return;
+
+	if (ata_is_nodata(qc->tf.protocol))
+		return;
+
+	if ((dev->mwdma_mask || dev->udma_mask) &&
+	    ata_is_pio_data(qc->tf.protocol))
+		return;
+
+	dev->flags &= ~ATA_DFLAG_DUBIOUS_XFER;
+}
+
 /**
  *	ata_qc_complete - Complete an active ATA command
  *	@qc: Command to complete
@@ -5848,6 +5865,9 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
 			break;
 		}
 
+		if (unlikely(dev->flags & ATA_DFLAG_DUBIOUS_XFER))
+			ata_verify_xfer(qc);
+
 		__ata_qc_complete(qc);
 	} else {
 		if (qc->flags & ATA_QCFLAG_EH_SCHEDULED)
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index cf2afb0..99231ec 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -456,9 +456,20 @@ void ata_scsi_error(struct Scsi_Host *host)
 		spin_lock_irqsave(ap->lock, flags);
 
 		__ata_port_for_each_link(link, ap) {
+			struct ata_eh_context *ehc = &link->eh_context;
+			struct ata_device *dev;
+
 			memset(&link->eh_context, 0, sizeof(link->eh_context));
 			link->eh_context.i = link->eh_info;
 			memset(&link->eh_info, 0, sizeof(link->eh_info));
+
+			ata_link_for_each_dev(dev, link) {
+				int devno = dev->devno;
+
+				ehc->saved_xfer_mode[devno] = dev->xfer_mode;
+				if (ata_ncq_enabled(dev))
+					ehc->saved_ncq_enabled |= 1 << devno;
+			}
 		}
 
 		ap->pflags |= ATA_PFLAG_EH_IN_PROGRESS;
@@ -2439,11 +2450,27 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
 int ata_set_mode(struct ata_link *link, struct ata_device **r_failed_dev)
 {
 	struct ata_port *ap = link->ap;
+	struct ata_device *dev;
+	int rc;
 
 	/* has private set_mode? */
 	if (ap->ops->set_mode)
-		return ap->ops->set_mode(link, r_failed_dev);
-	return ata_do_set_mode(link, r_failed_dev);
+		rc = ap->ops->set_mode(link, r_failed_dev);
+	else
+		rc = ata_do_set_mode(link, r_failed_dev);
+
+	/* if transfer mode has changed, set DUBIOUS_XFER on device */
+	ata_link_for_each_dev(dev, link) {
+		struct ata_eh_context *ehc = &link->eh_context;
+		u8 saved_xfer_mode = ehc->saved_xfer_mode[dev->devno];
+		u8 saved_ncq = !!(ehc->saved_ncq_enabled & (1 << dev->devno));
+
+		if (dev->xfer_mode != saved_xfer_mode ||
+		    ata_ncq_enabled(dev) != saved_ncq)
+			dev->flags |= ATA_DFLAG_DUBIOUS_XFER;
+	}
+
+	return rc;
 }
 
 static int ata_link_nr_enabled(struct ata_link *link)
@@ -2504,6 +2531,8 @@ static int ata_eh_schedule_probe(struct ata_device *dev)
 	ata_dev_init(dev);
 	ehc->did_probe_mask |= (1 << dev->devno);
 	ehc->i.action |= ATA_EH_SOFTRESET;
+	ehc->saved_xfer_mode[dev->devno] = 0;
+	ehc->saved_ncq_enabled &= ~(1 << dev->devno);
 
 	return 1;
 }
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 4abfb3c..7c34ff5 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -141,6 +141,7 @@ enum {
 	ATA_DFLAG_NCQ_OFF	= (1 << 13), /* device limited to non-NCQ mode */
 	ATA_DFLAG_SPUNDOWN	= (1 << 14), /* XXX: for spindown_compat */
 	ATA_DFLAG_SLEEPING	= (1 << 15), /* device is sleeping */
+	ATA_DFLAG_DUBIOUS_XFER	= (1 << 16), /* data transfer not verified */
 	ATA_DFLAG_INIT_MASK	= (1 << 24) - 1,
 
 	ATA_DFLAG_DETACH	= (1 << 24),
@@ -555,6 +556,8 @@ struct ata_eh_context {
 	int			tries[ATA_MAX_DEVICES];
 	unsigned int		classes[ATA_MAX_DEVICES];
 	unsigned int		did_probe_mask;
+	unsigned int		saved_ncq_enabled;
+	u8			saved_xfer_mode[ATA_MAX_DEVICES];
 };
 
 struct ata_acpi_drive
-- 
1.5.2.4

[PATCH 8/8] libata: implement fast speed down for unverified data transfer mode

[Tejun Heo]

It's very likely that the configured data transfer mode is the wrong
one if device fails data transfers right after initial data transfer
mode configuration (including NCQ on/off and xfermode).  libata EH
needs to speed down fast before upper layers give up on probing.

This patch implement fast speed down rules to handle such cases
better.  Error occured while data transfer hasn't been verified
trigger fast back-to-back speed down actions until data transfer
works.

This change will make cable mis-detection and other initial
configuration problems corrected before partition scanning code gives
up.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-eh.c |   97 ++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 83 insertions(+), 14 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 99231ec..7835e1a 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -50,16 +50,23 @@ enum {
 	ATA_EH_SPDN_NCQ_OFF		= (1 << 0),
 	ATA_EH_SPDN_SPEED_DOWN		= (1 << 1),
 	ATA_EH_SPDN_FALLBACK_TO_PIO	= (1 << 2),
+	ATA_EH_SPDN_KEEP_ERRORS		= (1 << 3),
 
 	/* error flags */
 	ATA_EFLAG_IS_IO			= (1 << 0),
+	ATA_EFLAG_DUBIOUS_XFER		= (1 << 1),
 
 	/* error categories */
 	ATA_ECAT_NONE			= 0,
 	ATA_ECAT_ATA_BUS		= 1,
 	ATA_ECAT_TOUT_HSM		= 2,
 	ATA_ECAT_UNK_DEV		= 3,
-	ATA_ECAT_NR			= 4,
+	ATA_ECAT_DUBIOUS_ATA_BUS	= 4,
+	ATA_ECAT_DUBIOUS_TOUT_HSM	= 5,
+	ATA_ECAT_DUBIOUS_UNK_DEV	= 6,
+	ATA_ECAT_NR			= 7,
+
+	ATA_ECAT_DUBIOUS_BASE		= ATA_ECAT_DUBIOUS_ATA_BUS,
 };
 
 /* Waiting in ->prereset can never be reliable.  It's sometimes nice
@@ -245,6 +252,15 @@ static void ata_ering_record(struct ata_ering *ering, unsigned int eflags,
 	ent->timestamp = get_jiffies_64();
 }
 
+static struct ata_ering_entry *ata_ering_top(struct ata_ering *ering)
+{
+	struct ata_ering_entry *ent = &ering->ring[ering->cursor];
+
+	if (ent->err_mask)
+		return ent;
+	return NULL;
+}
+
 static void ata_ering_clear(struct ata_ering *ering)
 {
 	memset(ering, 0, sizeof(*ering));
@@ -1568,20 +1584,29 @@ static unsigned int ata_eh_analyze_tf(struct ata_queued_cmd *qc,
 	return action;
 }
 
-static int ata_eh_categorize_error(unsigned int eflags, unsigned int err_mask)
+static int ata_eh_categorize_error(unsigned int eflags, unsigned int err_mask,
+				   int *xfer_ok)
 {
+	int base = 0;
+
+	if (!(eflags & ATA_EFLAG_DUBIOUS_XFER))
+		*xfer_ok = 1;
+
+	if (!*xfer_ok)
+		base = ATA_ECAT_DUBIOUS_BASE;
+
 	if (err_mask & AC_ERR_ATA_BUS)
-		return ATA_ECAT_ATA_BUS;
+		return base + ATA_ECAT_ATA_BUS;
 
 	if (err_mask & AC_ERR_TIMEOUT)
-		return ATA_ECAT_TOUT_HSM;
+		return base + ATA_ECAT_TOUT_HSM;
 
 	if (eflags & ATA_EFLAG_IS_IO) {
 		if (err_mask & AC_ERR_HSM)
-			return ATA_ECAT_TOUT_HSM;
+			return base + ATA_ECAT_TOUT_HSM;
 		if ((err_mask &
 		     (AC_ERR_DEV|AC_ERR_MEDIA|AC_ERR_INVALID)) == AC_ERR_DEV)
-			return ATA_ECAT_UNK_DEV;
+			return base + ATA_ECAT_UNK_DEV;
 	}
 
 	return 0;
@@ -1589,18 +1614,22 @@ static int ata_eh_categorize_error(unsigned int eflags, unsigned int err_mask)
 
 struct speed_down_verdict_arg {
 	u64 since;
+	int xfer_ok;
 	int nr_errors[ATA_ECAT_NR];
 };
 
 static int speed_down_verdict_cb(struct ata_ering_entry *ent, void *void_arg)
 {
 	struct speed_down_verdict_arg *arg = void_arg;
-	int cat = ata_eh_categorize_error(ent->eflags, ent->err_mask);
+	int cat;
 
 	if (ent->timestamp < arg->since)
 		return -1;
 
+	cat = ata_eh_categorize_error(ent->eflags, ent->err_mask,
+				      &arg->xfer_ok);
 	arg->nr_errors[cat]++;
+
 	return 0;
 }
 
@@ -1619,6 +1648,9 @@ static int speed_down_verdict_cb(struct ata_ering_entry *ent, void *void_arg)
  *
  *	ECAT_UNK_DEV	: Unknown DEV error for IO commands
  *
+ *	ECAT_DUBIOUS_*	: Identical to above three but occurred while
+ *			  data transfer hasn't been verified.
+ *
  *	Verdicts are
  *
  *	NCQ_OFF		: Turn off NCQ.
@@ -1629,15 +1661,27 @@ static int speed_down_verdict_cb(struct ata_ering_entry *ent, void *void_arg)
  *	FALLBACK_TO_PIO	: Fall back to PIO.
  *
  *	Even if multiple verdicts are returned, only one action is
- *	taken per error.  ering is cleared after an action is taken.
+ *	taken per error.  An action triggered by non-DUBIOUS errors
+ *	clears ering, while one triggered by DUBIOUS_* errors doesn't.
+ *	This is to expedite speed down decisions right after device is
+ *	initially configured.
+ *
+ *	The followings are speed down rules.  #1 and #2 deal with
+ *	DUBIOUS errors.
  *
- *	1. If more than 6 ATA_BUS, TOUT_HSM or UNK_DEV errors
+ *	1. If more than one DUBIOUS_ATA_BUS or DUBIOUS_TOUT_HSM errors
+ *	   occurred during last 5 mins, SPEED_DOWN and FALLBACK_TO_PIO.
+ *
+ *	2. If more than one DUBIOUS_TOUT_HSM or DUBIOUS_UNK_DEV errors
+ *	   occurred during last 5 mins, NCQ_OFF.
+ *
+ *	3. If more than 8 ATA_BUS, TOUT_HSM or UNK_DEV errors
  *	   ocurred during last 5 mins, FALLBACK_TO_PIO
  *
- *	2. If more than 3 TOUT_HSM or UNK_DEV errors occurred
+ *	4. If more than 3 TOUT_HSM or UNK_DEV errors occurred
  *	   during last 10 mins, NCQ_OFF.
  *
- *	3. If more than 3 ATA_BUS or TOUT_HSM errors, or more than 6
+ *	5. If more than 3 ATA_BUS or TOUT_HSM errors, or more than 6
  *	   UNK_DEV errors occurred during last 10 mins, SPEED_DOWN.
  *
  *	LOCKING:
@@ -1658,6 +1702,15 @@ static unsigned int ata_eh_speed_down_verdict(struct ata_device *dev)
 	arg.since = j64 - min(j64, j5mins);
 	ata_ering_map(&dev->ering, speed_down_verdict_cb, &arg);
 
+	if (arg.nr_errors[ATA_ECAT_DUBIOUS_ATA_BUS] +
+	    arg.nr_errors[ATA_ECAT_DUBIOUS_TOUT_HSM] > 1)
+		verdict |= ATA_EH_SPDN_SPEED_DOWN |
+			ATA_EH_SPDN_FALLBACK_TO_PIO | ATA_EH_SPDN_KEEP_ERRORS;
+
+	if (arg.nr_errors[ATA_ECAT_DUBIOUS_TOUT_HSM] +
+	    arg.nr_errors[ATA_ECAT_DUBIOUS_UNK_DEV] > 1)
+		verdict |= ATA_EH_SPDN_NCQ_OFF | ATA_EH_SPDN_KEEP_ERRORS;
+
 	if (arg.nr_errors[ATA_ECAT_ATA_BUS] +
 	    arg.nr_errors[ATA_ECAT_TOUT_HSM] +
 	    arg.nr_errors[ATA_ECAT_UNK_DEV] > 6)
@@ -1701,11 +1754,12 @@ static unsigned int ata_eh_speed_down(struct ata_device *dev,
 				unsigned int eflags, unsigned int err_mask)
 {
 	struct ata_link *link = dev->link;
+	int xfer_ok = 0;
 	unsigned int verdict;
 	unsigned int action = 0;
 
 	/* don't bother if Cat-0 error */
-	if (ata_eh_categorize_error(eflags, err_mask) == 0)
+	if (ata_eh_categorize_error(eflags, err_mask, &xfer_ok) == 0)
 		return 0;
 
 	/* record error and determine whether speed down is necessary */
@@ -1768,7 +1822,8 @@ static unsigned int ata_eh_speed_down(struct ata_device *dev,
 	return 0;
  done:
 	/* device has been slowed down, blow error history */
-	ata_ering_clear(&dev->ering);
+	if (!(verdict & ATA_EH_SPDN_KEEP_ERRORS))
+		ata_ering_clear(&dev->ering);
 	return action;
 }
 
@@ -1874,8 +1929,11 @@ static void ata_eh_link_autopsy(struct ata_link *link)
 	    ata_dev_enabled(link->device))
 		dev = link->device;
 
-	if (dev)
+	if (dev) {
+		if (dev->flags & ATA_DFLAG_DUBIOUS_XFER)
+			eflags |= ATA_EFLAG_DUBIOUS_XFER;
 		ehc->i.action |= ata_eh_speed_down(dev, eflags, all_err_mask);
+	}
 
 	DPRINTK("EXIT\n");
 }
@@ -2453,6 +2511,17 @@ int ata_set_mode(struct ata_link *link, struct ata_device **r_failed_dev)
 	struct ata_device *dev;
 	int rc;
 
+	/* if data transfer is verified, clear DUBIOUS_XFER on ering top */
+	ata_link_for_each_dev(dev, link) {
+		if (!(dev->flags & ATA_DFLAG_DUBIOUS_XFER)) {
+			struct ata_ering_entry *ent;
+
+			ent = ata_ering_top(&dev->ering);
+			if (ent)
+				ent->eflags &= ~ATA_EFLAG_DUBIOUS_XFER;
+		}
+	}
+
 	/* has private set_mode? */
 	if (ap->ops->set_mode)
 		rc = ap->ops->set_mode(link, r_failed_dev);
-- 
1.5.2.4

Re: [PATCH 3/8] libata: factor out ata_eh_schedule_probe()

[Bartlomiej Zolnierkiewicz]

Hi,

On Monday 05 November 2007, Tejun Heo wrote:
> Factor out ata_eh_schedule_probe() from ata_eh_handle_dev_fail() and
> ata_eh_recover().  This is to improve maintainability and make future
> changes easier.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
>  drivers/ata/libata-eh.c |   37 ++++++++++++++++++++-----------------
>  1 files changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index ed8813b..a37be1e 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -2438,6 +2438,22 @@ static int ata_eh_skip_recovery(struct ata_link *link)
>  	return 1;
>  }
>  
> +static int ata_eh_schedule_probe(struct ata_device *dev)
> +{
> +	struct ata_eh_context *ehc = &dev->link->eh_context;
> +
> +	if (!(ehc->i.probe_mask & (1 << dev->devno)) ||
> +	    (ehc->did_probe_mask & (1 << dev->devno)))
> +		return 0;
> +
> +	ata_eh_detach_dev(dev);
> +	ata_dev_init(dev);
> +	ehc->did_probe_mask |= (1 << dev->devno);
> +	ehc->i.action |= ATA_EH_SOFTRESET;
> +
> +	return 1;
> +}
> +
>  static int ata_eh_handle_dev_fail(struct ata_device *dev, int err)
>  {
>  	struct ata_eh_context *ehc = &dev->link->eh_context;
> @@ -2469,16 +2485,9 @@ static int ata_eh_handle_dev_fail(struct ata_device *dev, int err)
>  		if (ata_link_offline(dev->link))
>  			ata_eh_detach_dev(dev);
>  
> -		/* probe if requested */
> -		if ((ehc->i.probe_mask & (1 << dev->devno)) &&
> -		    !(ehc->did_probe_mask & (1 << dev->devno))) {
> -			ata_eh_detach_dev(dev);
> -			ata_dev_init(dev);
> -
> +		/* schedule probe if necessary */
> +		if (ata_eh_schedule_probe(dev))
>  			ehc->tries[dev->devno] = ATA_EH_DEV_TRIES;
> -			ehc->did_probe_mask |= (1 << dev->devno);
> -			ehc->i.action |= ATA_EH_SOFTRESET;
> -		}
>  
>  		return 1;
>  	} else {
> @@ -2555,14 +2564,8 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
>  			if (dev->flags & ATA_DFLAG_DETACH)
>  				ata_eh_detach_dev(dev);
>  
> -			if (!ata_dev_enabled(dev) &&
> -			    ((ehc->i.probe_mask & (1 << dev->devno)) &&
> -			     !(ehc->did_probe_mask & (1 << dev->devno)))) {
> -				ata_eh_detach_dev(dev);
> -				ata_dev_init(dev);
> -				ehc->did_probe_mask |= (1 << dev->devno);
> -				ehc->i.action |= ATA_EH_SOFTRESET;
> -			}
> +			/* schedule probe if necessary */
> +			ata_eh_schedule_probe(dev);

!ata_dev_enabled() got lost, is this intended?

Re: [PATCH 3/8] libata: factor out ata_eh_schedule_probe()

[Tejun Heo]

Bartlomiej Zolnierkiewicz wrote:
>> -			if (!ata_dev_enabled(dev) &&
>> -			    ((ehc->i.probe_mask & (1 << dev->devno)) &&
>> -			     !(ehc->did_probe_mask & (1 << dev->devno)))) {
>> -				ata_eh_detach_dev(dev);
>> -				ata_dev_init(dev);
>> -				ehc->did_probe_mask |= (1 << dev->devno);
>> -				ehc->i.action |= ATA_EH_SOFTRESET;
>> -			}
>> +			/* schedule probe if necessary */
>> +			ata_eh_schedule_probe(dev);
> 
> !ata_dev_enabled() got lost, is this intended?

Definitely not.  Thanks a lot for catching this.  Jeff, I'll post an
updated version.  Thanks.

-- 
tejun

[PATCH 3/8 UPDATED] libata: factor out ata_eh_schedule_probe()

[Tejun Heo]

Factor out ata_eh_schedule_probe() from ata_eh_handle_dev_fail() and
ata_eh_recover().  This is to improve maintainability and make future
changes easier.

In the previous revision, ata_dev_enabled() test was accidentally
dropped while factoring out.  This problem was spotted by Bartlomiej.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ata/libata-eh.c |   38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

Index: work/drivers/ata/libata-eh.c
===================================================================
--- work.orig/drivers/ata/libata-eh.c
+++ work/drivers/ata/libata-eh.c
@@ -2438,6 +2438,22 @@ static int ata_eh_skip_recovery(struct a
 	return 1;
 }
 
+static int ata_eh_schedule_probe(struct ata_device *dev)
+{
+	struct ata_eh_context *ehc = &dev->link->eh_context;
+
+	if (!(ehc->i.probe_mask & (1 << dev->devno)) ||
+	    (ehc->did_probe_mask & (1 << dev->devno)))
+		return 0;
+
+	ata_eh_detach_dev(dev);
+	ata_dev_init(dev);
+	ehc->did_probe_mask |= (1 << dev->devno);
+	ehc->i.action |= ATA_EH_SOFTRESET;
+
+	return 1;
+}
+
 static int ata_eh_handle_dev_fail(struct ata_device *dev, int err)
 {
 	struct ata_eh_context *ehc = &dev->link->eh_context;
@@ -2469,16 +2485,9 @@ static int ata_eh_handle_dev_fail(struct
 		if (ata_link_offline(dev->link))
 			ata_eh_detach_dev(dev);
 
-		/* probe if requested */
-		if ((ehc->i.probe_mask & (1 << dev->devno)) &&
-		    !(ehc->did_probe_mask & (1 << dev->devno))) {
-			ata_eh_detach_dev(dev);
-			ata_dev_init(dev);
-
+		/* schedule probe if necessary */
+		if (ata_eh_schedule_probe(dev))
 			ehc->tries[dev->devno] = ATA_EH_DEV_TRIES;
-			ehc->did_probe_mask |= (1 << dev->devno);
-			ehc->i.action |= ATA_EH_SOFTRESET;
-		}
 
 		return 1;
 	} else {
@@ -2555,14 +2564,9 @@ int ata_eh_recover(struct ata_port *ap, 
 			if (dev->flags & ATA_DFLAG_DETACH)
 				ata_eh_detach_dev(dev);
 
-			if (!ata_dev_enabled(dev) &&
-			    ((ehc->i.probe_mask & (1 << dev->devno)) &&
-			     !(ehc->did_probe_mask & (1 << dev->devno)))) {
-				ata_eh_detach_dev(dev);
-				ata_dev_init(dev);
-				ehc->did_probe_mask |= (1 << dev->devno);
-				ehc->i.action |= ATA_EH_SOFTRESET;
-			}
+			/* schedule probe if necessary */
+			if (!ata_dev_enabled(dev))
+				ata_eh_schedule_probe(dev);
 		}
 	}
 

Re: [PATCHSET] libata: update EH speed down logic, take #2

[Tejun Heo]

Tejun Heo wrote:
> Hello,
> 
> This is the second take of update-EH-speed-down-logic patchset.
> Changes from the last take[1] are...
> 
> * Updated to apply & build against the current linus#master.  Recently
>   committed error-passthhrough-for-non-IO-command change broke build
>   with this patchset applied.
> 
> This patchset is against the current linux#master (b55d1b18).

Ping.

-- 
tejun

Re: [PATCHSET] libata: update EH speed down logic, take #2

[Jeff Garzik]

Tejun Heo wrote:
> Hello,
> 
> This is the second take of update-EH-speed-down-logic patchset.
> Changes from the last take[1] are...
> 
> * Updated to apply & build against the current linus#master.  Recently
>   committed error-passthhrough-for-non-IO-command change broke build
>   with this patchset applied.
> 
> This patchset is against the current linux#master (b55d1b18).

ACK patchset (I think I already did this, but just to reinforce)

comments follow with next patchset response...