libata: improve EH speed down logic, take #3

PATCH New Tejun Heo 3 participants 20 messages Nov 27, 2007 – Dec 5, 2007

Subsystems (3)

SCSI libata SCSI libsas

lore

Share & tools

lore

Tejun Heo

author

2007-11-27 10:29 UTC #0 lore raw

PATCH 1/1 libata: improve EH speed down logic, take #3

Hello,

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

* 0002-add-protocol-data-transfer-tests is replaced with 0002-implement-protocol-tests which is more efficient and generic. The patch also replaces is_atapi_taskfile() and other open coded protocol tests with new test functions.

This patchset is against the current libata-dev#upstream (51a7ee37) and available from the following git tree.

master.kernel.org:/pub/scm/linux/kernel/git/tj/libata-dev.git improve-speed-down

Thanks.

--
tejun

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

Tejun Heo

author

2007-11-27 10:29 UTC #5 lore raw

PATCH 3/8 libata: factor out ata_eh_schedule_probe()

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 <[email protected]>

Cc Bartlomiej Zolnierkiewicz <[email protected]>

---
 drivers/ata/libata-eh.c |   38 +++++++++++++++++++++-----------------
 1 files changed, 21 insertions(+), 17 deletions(-)

drivers/ata/libata-eh.c +21 ~~-17~~

index c36c837..44c137e 100644
@@ -2343,6 +2343,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;

@@ -2374,16 +2390,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 {

@@ -2460,14 +2469,9 @@ 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 */
+			if (!ata_dev_enabled(dev))
+				ata_eh_schedule_probe(dev);
 		}
 	}

Signature · 1 line

1.5.2.4

Tejun Heo

author

2007-11-27 10:29 UTC #1 lore raw

PATCH 1/8 libata: rearrange ATA_DFLAG_*

In reply to Tejun Heo

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

Signed-off-by Tejun Heo <[email protected]>

---
 include/linux/libata.h |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

include/linux/libata.h +3 -3

index ef52a07..9cd06dd 100644
@@ -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 */

Signature · 1 line

1.5.2.4

Tejun Heo

author

2007-11-27 10:29 UTC #3 lore raw

PATCH 2/8 libata: implement protocol tests

In reply to Tejun Heo

Implement protocol tests - ata_is_atapi(), ata_is_nodata(), ata_is_pio(), ata_is_dma(), ata_is_ncq() and ata_is_data() and use them to replace is_atapi_taskfile() and hard coded protocol tests.

Signed-off-by Tejun Heo <[email protected]>

---
 drivers/ata/ahci.c            |    2 +-
 drivers/ata/libata-core.c     |   36 ++++-----------------
 drivers/ata/sata_fsl.c        |    2 +-
 drivers/ata/sata_sil.c        |   10 ++---
 drivers/ata/sata_sil24.c      |   26 ++-------------
 drivers/scsi/libsas/sas_ata.c |    2 +-
 include/linux/ata.h           |   71 +++++++++++++++++++++++++++++++++++++----
 7 files changed, 82 insertions(+), 67 deletions(-)

drivers/ata/ahci.c +1 -1

index 2c686b4..a8fc9c0 100644
@@ -1505,7 +1505,7 @@ static void ahci_qc_prep(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
 	struct ahci_port_priv *pp = ap->private_data;
-	int is_atapi = is_atapi_taskfile(&qc->tf);
+	int is_atapi = ata_is_atapi(qc->tf.protocol);
 	void *cmd_tbl;
 	u32 opts;
 	const u32 cmd_fis_len = 5; /* five dwords */

drivers/ata/libata-core.c +7 ~~-29~~

index 5eb5983..8d5cd16 100644
@@ -5325,7 +5325,7 @@ static inline int ata_hsm_ok_in_wq(struct ata_port *ap, struct ata_queued_cmd *q
 		    (qc->tf.flags & ATA_TFLAG_WRITE))
 		    return 1;
 
-		if (is_atapi_taskfile(&qc->tf) &&
+		if (ata_is_atapi(qc->tf.protocol) &&
 		    !(qc->dev->flags & ATA_DFLAG_CDB_INTR))
 			return 1;
 	}

@@ -5922,30 +5922,6 @@ int ata_qc_complete_multiple(struct ata_port *ap, u32 qc_active,
 	return nr_done;
 }
 
-static inline int ata_should_dma_map(struct ata_queued_cmd *qc)
-{
-	struct ata_port *ap = qc->ap;
-
-	switch (qc->tf.protocol) {
-	case ATA_PROT_NCQ:
-	case ATA_PROT_DMA:
-	case ATA_PROT_ATAPI_DMA:
-		return 1;
-
-	case ATA_PROT_ATAPI:
-	case ATA_PROT_PIO:
-		if (ap->flags & ATA_FLAG_PIO_DMA)
-			return 1;
-
-		/* fall through */
-
-	default:
-		return 0;
-	}
-
-	/* never reached */
-}
-
 /**
  *	ata_qc_issue - issue taskfile to device
  *	@qc: command to issue to device

@@ -5962,6 +5938,7 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
 	struct ata_link *link = qc->dev->link;
+	u8 prot = qc->tf.protocol;
 
 	/* Make sure only one non-NCQ command is outstanding.  The
 	 * check is skipped for old EH because it reuses active qc to

@@ -5969,7 +5946,7 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
 	 */
 	WARN_ON(ap->ops->error_handler && ata_tag_valid(link->active_tag));
 
-	if (qc->tf.protocol == ATA_PROT_NCQ) {
+	if (prot == ATA_PROT_NCQ) {
 		WARN_ON(link->sactive & (1 << qc->tag));
 
 		if (!link->sactive)

@@ -5985,7 +5962,8 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
 	qc->flags |= ATA_QCFLAG_ACTIVE;
 	ap->qc_active |= 1 << qc->tag;
 
-	if (ata_should_dma_map(qc)) {
+	if (ata_is_dma(prot) || (ata_is_pio(prot) &&
+				 (ap->flags & ATA_FLAG_PIO_DMA))) {
 		if (qc->flags & ATA_QCFLAG_SG) {
 			if (ata_sg_setup(qc))
 				goto sg_err;

@@ -6184,8 +6162,8 @@ inline unsigned int ata_host_intr(struct ata_port *ap,
 		 */
 
 		/* Check the ATA_DFLAG_CDB_INTR flag is enough here.
-		 * The flag was turned on only for atapi devices.
-		 * No need to check is_atapi_taskfile(&qc->tf) again.
+		 * The flag was turned on only for atapi devices.  No
+		 * need to check ata_is_atapi(qc->tf.protocol) again.
 		 */
 		if (!(qc->dev->flags & ATA_DFLAG_CDB_INTR))
 			goto idle_irq;

drivers/ata/sata_fsl.c +1 -1

index d015b4a..a3c33f1 100644
@@ -417,7 +417,7 @@ static void sata_fsl_qc_prep(struct ata_queued_cmd *qc)
 	}
 
 	/* setup "ACMD - atapi command" in cmd. desc. if this is ATAPI cmd */
-	if (is_atapi_taskfile(&qc->tf)) {
+	if (ata_is_atapi(qc->tf.protocol)) {
 		desc_info |= ATAPI_CMD;
 		memset((void *)&cd->acmd, 0, 32);
 		memcpy((void *)&cd->acmd, qc->cdb, qc->dev->cdb_len);

drivers/ata/sata_sil.c +4 -6

index 4e6e381..4f31481 100644
@@ -420,15 +420,14 @@ static void sil_host_intr(struct ata_port *ap, u32 bmdma2)
 		 */
 
 		/* Check the ATA_DFLAG_CDB_INTR flag is enough here.
-		 * The flag was turned on only for atapi devices.
-		 * No need to check is_atapi_taskfile(&qc->tf) again.
+		 * The flag was turned on only for atapi devices.  No
+		 * need to check ata_is_atapi(qc->tf.protocol) again.
 		 */
 		if (!(qc->dev->flags & ATA_DFLAG_CDB_INTR))
 			goto err_hsm;
 		break;
 	case HSM_ST_LAST:
-		if (qc->tf.protocol == ATA_PROT_DMA ||
-		    qc->tf.protocol == ATA_PROT_ATAPI_DMA) {
+		if (ata_is_dma(qc->tf.protocol)) {
 			/* clear DMA-Start bit */
 			ap->ops->bmdma_stop(qc);

@@ -455,8 +454,7 @@ static void sil_host_intr(struct ata_port *ap, u32 bmdma2)
 	/* kick HSM in the ass */
 	ata_hsm_move(ap, qc, status, 0);
 
-	if (unlikely(qc->err_mask) && (qc->tf.protocol == ATA_PROT_DMA ||
-				       qc->tf.protocol == ATA_PROT_ATAPI_DMA))
+	if (unlikely(qc->err_mask) && ata_is_dma(qc->tf.protocol))
 		ata_ehi_push_desc(ehi, "BMDMA2 stat 0x%x", bmdma2);
 
 	return;

drivers/ata/sata_sil24.c +4 ~~-22~~

index 96fd526..4f25209 100644
@@ -831,10 +831,6 @@ static int sil24_qc_defer(struct ata_queued_cmd *qc)
 {
 	struct ata_link *link = qc->dev->link;
 	struct ata_port *ap = link->ap;
-	u8 prot = qc->tf.protocol;
-	int is_atapi = (prot == ATA_PROT_ATAPI ||
-			prot == ATA_PROT_ATAPI_NODATA ||
-			prot == ATA_PROT_ATAPI_DMA);
 
 	/* ATAPI commands completing with CHECK_SENSE cause various
 	 * weird problems if other commands are active.  PMP DMA CS

@@ -849,7 +845,7 @@ static int sil24_qc_defer(struct ata_queued_cmd *qc)
 			qc->flags |= ATA_QCFLAG_CLEAR_EXCL;
 		} else
 			return ATA_DEFER_PORT;
-	} else if (unlikely(is_atapi)) {
+	} else if (unlikely(ata_is_atapi(qc->tf.protocol))) {
 		ap->excl_link = link;
 		if (ap->nr_active_links)
 			return ATA_DEFER_PORT;

@@ -870,35 +866,21 @@ static void sil24_qc_prep(struct ata_queued_cmd *qc)
 
 	cb = &pp->cmd_block[sil24_tag(qc->tag)];
 
-	switch (qc->tf.protocol) {
-	case ATA_PROT_PIO:
-	case ATA_PROT_DMA:
-	case ATA_PROT_NCQ:
-	case ATA_PROT_NODATA:
+	if (!ata_is_atapi(qc->tf.protocol)) {
 		prb = &cb->ata.prb;
 		sge = cb->ata.sge;
-		break;
-
-	case ATA_PROT_ATAPI:
-	case ATA_PROT_ATAPI_DMA:
-	case ATA_PROT_ATAPI_NODATA:
+	} else {
 		prb = &cb->atapi.prb;
 		sge = cb->atapi.sge;
 		memset(cb->atapi.cdb, 0, 32);
 		memcpy(cb->atapi.cdb, qc->cdb, qc->dev->cdb_len);
 
-		if (qc->tf.protocol != ATA_PROT_ATAPI_NODATA) {
+		if (ata_is_data(qc->tf.protocol)) {
 			if (qc->tf.flags & ATA_TFLAG_WRITE)
 				ctrl = PRB_CTRL_PACKET_WRITE;
 			else
 				ctrl = PRB_CTRL_PACKET_READ;
 		}
-		break;
-
-	default:
-		prb = NULL;	/* shut up, gcc */
-		sge = NULL;
-		BUG();
 	}
 
 	prb->ctrl = cpu_to_le16(ctrl);

drivers/scsi/libsas/sas_ata.c +1 -1

index 0829b55..831294d 100644
@@ -176,7 +176,7 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc)
 
 	ata_tf_to_fis(&qc->tf, 1, 0, (u8*)&task->ata_task.fis);
 	task->uldd_task = qc;
-	if (is_atapi_taskfile(&qc->tf)) {
+	if (ata_is_atapi(qc->tf.protocol)) {
 		memcpy(task->ata_task.atapi_packet, qc->cdb, qc->dev->cdb_len);
 		task->total_xfer_len = qc->nbytes + qc->pad_len;
 		task->num_scatter = qc->pad_len ? qc->n_elem + 1 : qc->n_elem;

include/linux/ata.h +64 -7

89 lines not on this page · raw message

Signature · 1 line

1.5.2.4

Tejun Heo

author

2007-11-27 10:29 UTC #6 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/ata/libata-core.c |   25 -------------------------
 drivers/ata/libata-eh.c   |   25 +++++++++++++++++++++++++
 drivers/ata/libata.h      |    2 +-
 3 files changed, 26 insertions(+), 26 deletions(-)

drivers/ata/libata-core.c +0 ~~-25~~

index 8d5cd16..85053bf 100644
@@ -3181,31 +3181,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

drivers/ata/libata-eh.c +25 -0

index 44c137e..b1dc6b9 100644
@@ -2297,6 +2297,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;

drivers/ata/libata.h +1 -1

index c860705..f8ab11e 100644
@@ -86,7 +86,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);

@@ -183,6 +182,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,

Signature · 1 line

1.5.2.4

Tejun Heo

author

2007-11-27 10:29 UTC #9 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/ata/libata-eh.c |  117 +++++++++++++++++++++++++++++------------------
 include/linux/libata.h  |    2 +-
 2 files changed, 74 insertions(+), 45 deletions(-)

drivers/ata/libata-eh.c +73 ~~-44~~

index b1dc6b9..1245b96 100644
@@ -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();
 }

@@ -1451,20 +1462,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;

@@ -1472,13 +1483,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;

@@ -1495,22 +1506,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.

@@ -1525,23 +1547,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;
 }

@@ -1549,7 +1577,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

@@ -1563,18 +1591,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? */

@@ -1590,7 +1619,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;
 		}

@@ -1621,7 +1650,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;

@@ -1653,8 +1682,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;

@@ -1713,15 +1742,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,

@@ -1740,7 +1769,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");
 }

include/linux/libata.h +1 -1

index 9cd06dd..e9407f1 100644
@@ -480,7 +480,7 @@ struct ata_port_stats {
 };
 
 struct ata_ering_entry {
-	int			is_io;
+	unsigned int		eflags;
 	unsigned int		err_mask;
 	u64			timestamp;
 };

Signature · 1 line

1.5.2.4

Tejun Heo

author

2007-11-27 10:29 UTC #10 lore raw

PATCH 6/8 libata: adjust speed down rules

In reply to 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 <[email protected]>

---
 drivers/ata/libata-eh.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

drivers/ata/libata-eh.c +6 -6

index 1245b96..e479df2 100644
@@ -1525,13 +1525,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:

@@ -1554,7 +1554,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 */

@@ -1568,7 +1568,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;

@@ -1647,10 +1647,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;

Signature · 1 line

1.5.2.4

Tejun Heo

author

2007-11-27 10:29 UTC #18 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/ata/libata-eh.c |   97 ++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 83 insertions(+), 14 deletions(-)

drivers/ata/libata-eh.c +83 ~~-14~~

index 2ba6f59..77083b5 100644
@@ -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));

@@ -1473,20 +1489,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;

@@ -1494,18 +1519,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;
 }

@@ -1524,6 +1553,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.

@@ -1534,15 +1566,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:

@@ -1563,6 +1607,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)

@@ -1606,11 +1659,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 */

@@ -1673,7 +1727,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;
 }

@@ -1779,8 +1834,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");
 }

@@ -2358,6 +2416,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);

Signature · 1 line

1.5.2.4

Tejun Heo

author

2007-11-27 10:29 UTC #17 lore raw

PATCH 7/8 libata: implement ATA_DFLAG_DUBIOUS_XFER

In reply to 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 <[email protected]<

---
 drivers/ata/libata-core.c |   19 +++++++++++++++++++
 drivers/ata/libata-eh.c   |   33 +++++++++++++++++++++++++++++++--
 include/linux/libata.h    |    3 +++
 3 files changed, 53 insertions(+), 2 deletions(-)

drivers/ata/libata-core.c +19 -0

index 85053bf..fe453c7 100644
@@ -5764,6 +5764,22 @@ 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(qc->tf.protocol))
+		return;
+
+	dev->flags &= ~ATA_DFLAG_DUBIOUS_XFER;
+}
+
 /**
  *	ata_qc_complete - Complete an active ATA command
  *	@qc: Command to complete

@@ -5835,6 +5851,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)

drivers/ata/libata-eh.c +31 -2

index e479df2..2ba6f59 100644
@@ -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;

@@ -2344,11 +2355,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)

@@ -2409,6 +2436,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;
 }

include/linux/libata.h +3 -0

index e9407f1..a1b47d8 100644
@@ -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),

@@ -557,6 +558,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

Signature · 1 line

1.5.2.4

Mark Lord

2007-11-27 22:32 UTC #11 lore raw

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

In reply to Tejun Heo

Quoted 9 lines · 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.

Mmmm... I see two remaining problems with this:

-- bad sectors (media errors) will cause inappropriate speed-downs.
-- deliberate (or otherwise) errors via SG_IO will cause inappropriate speed-downs.

Right?

Tejun Heo

author

2007-11-27 23:07 UTC #12 lore raw

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

Hello, Mark.

Quoted 8 lines · Mark Lord

Quoted 7 lines · 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.

Quoted 7 lines

Quoted 2 lines
* SATAPI can benefit from falling back to PIO too. Allow SATAPI devices to fall back to PIO.
..
Mmmm... I see two remaining problems with this:
-- bad sectors (media errors) will cause inappropriate speed-downs.

No. Media error is category 0 error which doesn't affect speed down at all.

Quoted 2 lines

-- deliberate (or otherwise) errors via SG_IO will cause inappropriate speed-downs.

No, only device errors issued via block layer are considered for speed down (ATA_QCFLAG_IO). SG_IO commands will never trigger speed down unless the error is HSM or timeout.

Thanks.

Signature · 1 line

tejun

Mark Lord

2007-11-27 23:33 UTC #13 lore raw

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

Hi Tejun,

Quoted 1 line · Tejun Heo

Mark Lord wrote:

Quoted 6 lines

Quoted 2 lines
-- deliberate (or otherwise) errors via SG_IO will cause inappropriate speed-downs.
No, only device errors issued via block layer are considered for speed down (ATA_QCFLAG_IO). SG_IO commands will never trigger speed down unless the error is HSM or timeout.

Mmmm... what category is "device rejected the command" (ABRT) ? Is that HSM ?

Cheers!

Tejun Heo

author

2007-11-27 23:35 UTC #14 lore raw

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

Quoted 8 lines · Mark Lord

Hi Tejun,
Quoted 1 line · Tejun Heo
Mark Lord wrote:
..
Quoted 3 lines
Quoted 2 lines
-- deliberate (or otherwise) errors via SG_IO will cause inappropriate speed-downs.

Quoted 7 lines

Quoted 3 lines
No, only device errors issued via block layer are considered for speed down (ATA_QCFLAG_IO). SG_IO commands will never trigger speed down unless the error is HSM or timeout.
..
Mmmm... what category is "device rejected the command" (ABRT) ? Is that HSM ?

Nope, that's UNK_DEV if the command is IO (came from block layer) and cat-0 (don't care) if it has come from SG_IO.

Signature · 1 line

tejun

Mark Lord

2007-11-28 01:10 UTC #15 lore raw

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

Quoted 11 lines · Tejun Heo

Quoted 10 lines · Mark Lord
Hi Tejun,
Quoted 1 line · Tejun Heo
Mark Lord wrote:
..
Quoted 5 lines
Quoted 2 lines
-- deliberate (or otherwise) errors via SG_IO will cause inappropriate speed-downs.
No, only device errors issued via block layer are considered for speed down (ATA_QCFLAG_IO). SG_IO commands will never trigger speed down unless the error is HSM or timeout.

Quoted 7 lines

Quoted 4 lines
..
Mmmm... what category is "device rejected the command" (ABRT) ? Is that HSM ?
Nope, that's UNK_DEV if the command is IO (came from block layer) and cat-0 (don't care) if it has come from SG_IO.

Perfect, thanks!

Last question: what about CD/DVD writers?

Are the data-out commands for those done via block, or via SG_IO ? (my memory fails me here).

Do they ever get speed up/down based on WRITE errors ?

Cheers

Tejun Heo

author

2007-11-28 08:30 UTC #16 lore raw

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

Hello, Mark.

Quoted 6 lines · Mark Lord

Last question: what about CD/DVD writers?
Are the data-out commands for those done via block, or via SG_IO ? (my memory fails me here).
Do they ever get speed up/down based on WRITE errors ?

That depends on how the writing commands are issued. If the commands are issued via SG_IO (which is the case for wodim and probably for others too), it won't affect speed down. If the commands are issued by writing to /dev/sr0, it will.

Thanks.

Signature · 1 line

tejun

Jeff Garzik

2007-12-01 23:05 UTC #2 lore raw

Re: [PATCH 1/8] libata: rearrange ATA_DFLAG_*

In reply to Tejun Heo

Quoted 7 lines · Tejun Heo

Area for DFLAGs which are cleared on INIT is full. Extend it by 8 bits.
Signed-off-by Tejun Heo <[email protected]>
---
 include/linux/libata.h |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

applied 1-8 to #upstream

Jeff Garzik

2007-12-01 23:06 UTC #4 lore raw

Re: [PATCH 2/8] libata: implement protocol tests

In reply to Tejun Heo

drivers/ata/libata-core.c void ata_qc_issue(struct ata_queued_cmd *qc) · line 5952

Quoted 13 lines · Tejun Heo

drivers/ata/libata-core.c +1 -1

index 5eb5983..8d5cd16 100644
@@ -5969,7 +5946,7 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
 	 */
 	WARN_ON(ap->ops->error_handler && ata_tag_valid(link->active_tag));
 
-	if (qc->tf.protocol == ATA_PROT_NCQ) {
+	if (prot == ATA_PROT_NCQ) {
 		WARN_ON(link->sactive & (1 << qc->tag));
 
 		if (!link->sactive)

missed one?

Jeff Garzik

2007-12-01 23:07 UTC #7 lore raw

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

In reply to Tejun Heo

Quoted 5 lines · 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 <[email protected]>

Does this mean SET FEATURES - XFER MODE via SG_IO will be possible soon?
;-)

Jeff Garzik

2007-12-01 23:07 UTC #19 lore raw

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

In reply to Tejun Heo

Quoted 10 lines · 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.

Quoted 5 lines

This change will make cable mis-detection and other initial configuration problems corrected before partition scanning code gives up.
Signed-off-by Tejun Heo <[email protected]>

I agree strongly.

Good work!

Tejun Heo

author

2007-12-05 01:22 UTC #8 lore raw

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

In reply to Jeff Garzik

Quoted 9 lines · Jeff Garzik

Quoted 5 lines · 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 <[email protected]>
Does this mean SET FEATURES - XFER MODE via SG_IO will be possible soon?
;-)

Hopefully, yeah.

Signature · 1 line

tejun