[PATCHSET] libata: improve media error handling

[PATCHSET] libata: improve media error handling

[Tejun Heo]

Hello, Jeff.

This patchset improves media error handling.  The problem is that ATA
devices transfer "indeterminate" amount of data on device error making
handling media errors difficult for highlevel driver to handle.  The
current high level driver, sd, assumes that data upto the first failed
block has been transferred successfully, so data corruption is
possible.

This patchset makes libata-eh perform IO upto the first failed block
and report the first failed block to highlevel driver iff the partial
transfer was successful.  Note that after this patchset LLDs using old
EH don't report the first failed block to high level.  It wasn't safe
to begin with and no libata driver properly reported the first failed
sector before improve-sense-data-generation patchset anyway.

With this patchset applied, libata media error handling is well
integrated with sd.

This patchset is against

  upstream(cfd15b0011498986ef14b6c53f5eaba89d2171f3)
  + improve-sense-data-generation patchset[1]

Thanks.

--
tejun

[1] http://thread.gmane.org/gmane.linux.ide/14035/focus=14035

[PATCH 1/4] libata: implement ata_exec_internal_sg()

[Tejun Heo]

Sg'ify ata_exec_internal() and call it ata_exec_internal_sg().
Wrapper function around ata_exec_internal_sg() is implemented to
provide ata_exec_internal() interface.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-core.c |   49 ++++++++++++++++++++++++++++++++++++++------
 drivers/ata/libata.h      |    4 +++
 2 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index dab577c..9ddc512 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1042,13 +1042,13 @@ void ata_qc_complete_internal(struct ata
 }
 
 /**
- *	ata_exec_internal - execute libata internal command
+ *	ata_exec_internal_sg - execute libata internal command
  *	@dev: Device to which the command is sent
  *	@tf: Taskfile registers for the command and the result
  *	@cdb: CDB for packet command
  *	@dma_dir: Data tranfer direction of the command
- *	@buf: Data buffer of the command
- *	@buflen: Length of data buffer
+ *	@sg: sg list for the data buffer of the command
+ *	@n_elem: Number of sg entries
  *
  *	Executes libata internal command with timeout.  @tf contains
  *	command on entry and result on return.  Timeout and error
@@ -1062,9 +1062,10 @@ void ata_qc_complete_internal(struct ata
  *	RETURNS:
  *	Zero on success, AC_ERR_* mask on failure
  */
-unsigned ata_exec_internal(struct ata_device *dev,
-			   struct ata_taskfile *tf, const u8 *cdb,
-			   int dma_dir, void *buf, unsigned int buflen)
+unsigned ata_exec_internal_sg(struct ata_device *dev,
+			      struct ata_taskfile *tf, const u8 *cdb,
+			      int dma_dir, struct scatterlist *sg,
+			      unsigned int n_elem)
 {
 	struct ata_port *ap = dev->ap;
 	u8 command = tf->command;
@@ -1120,7 +1121,12 @@ unsigned ata_exec_internal(struct ata_de
 	qc->flags |= ATA_QCFLAG_RESULT_TF;
 	qc->dma_dir = dma_dir;
 	if (dma_dir != DMA_NONE) {
-		ata_sg_init_one(qc, buf, buflen);
+		unsigned int i, buflen = 0;
+
+		for (i = 0; i < n_elem; i++)
+			buflen += sg[i].length;
+
+		ata_sg_init(qc, sg, n_elem);
 		qc->nsect = buflen / ATA_SECT_SIZE;
 	}
 
@@ -1204,6 +1210,35 @@ unsigned ata_exec_internal(struct ata_de
 }
 
 /**
+ *	ata_exec_internal_sg - execute libata internal command
+ *	@dev: Device to which the command is sent
+ *	@tf: Taskfile registers for the command and the result
+ *	@cdb: CDB for packet command
+ *	@dma_dir: Data tranfer direction of the command
+ *	@buf: Data buffer of the command
+ *	@buflen: Length of data buffer
+ *
+ *	Wrapper around ata_exec_internal_sg() which takes simple
+ *	buffer instead of sg list.
+ *
+ *	LOCKING:
+ *	None.  Should be called with kernel context, might sleep.
+ *
+ *	RETURNS:
+ *	Zero on success, AC_ERR_* mask on failure
+ */
+unsigned ata_exec_internal(struct ata_device *dev,
+			   struct ata_taskfile *tf, const u8 *cdb,
+			   int dma_dir, void *buf, unsigned int buflen)
+{
+	struct scatterlist sg;
+
+	sg_init_one(&sg, buf, buflen);
+
+	return ata_exec_internal_sg(dev, tf, cdb, dma_dir, &sg, 1);
+}
+
+/**
  *	ata_do_simple_cmd - execute simple internal command
  *	@dev: Device to which the command is sent
  *	@cmd: Opcode to execute
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 0549b19..c82318b 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -51,6 +51,10 @@ extern void ata_port_flush_task(struct a
 extern unsigned ata_exec_internal(struct ata_device *dev,
 				  struct ata_taskfile *tf, const u8 *cdb,
 				  int dma_dir, void *buf, unsigned int buflen);
+extern unsigned ata_exec_internal_sg(struct ata_device *dev,
+				     struct ata_taskfile *tf, const u8 *cdb,
+				     int dma_dir, struct scatterlist *sg,
+				     unsigned int n_elem);
 extern unsigned int ata_do_simple_cmd(struct ata_device *dev, u8 cmd);
 extern int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
 			   int post_reset, u16 *id);
-- 
1.4.3.3

Re: [PATCH 1/4] libata: implement ata_exec_internal_sg()

[Jeff Garzik]

Tejun Heo wrote:
> Sg'ify ata_exec_internal() and call it ata_exec_internal_sg().
> Wrapper function around ata_exec_internal_sg() is implemented to
> provide ata_exec_internal() interface.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK

[PATCH 2/4] libata: separate out rw ATA taskfile building into ata_build_tf()

[Tejun Heo]

Separate out rw ATA taskfile building from ata_scsi_rw_xlat() into
ata_build_tf().  This will be used to improve media error handling.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-core.c |  133 +++++++++++++++++++++++++++++++++++++++++++--
 drivers/ata/libata-scsi.c |  119 +++++------------------------------------
 drivers/ata/libata.h      |    4 +-
 3 files changed, 144 insertions(+), 112 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 9ddc512..1b33f05 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -199,7 +199,8 @@ static const u8 ata_rw_cmds[] = {
 
 /**
  *	ata_rwcmd_protocol - set taskfile r/w commands and protocol
- *	@qc: command to examine and configure
+ *	@tf: command to examine and configure
+ *	@dev: device tf belongs to
  *
  *	Examine the device configuration and tf->flags to calculate
  *	the proper read/write commands and protocol to use.
@@ -207,10 +208,8 @@ static const u8 ata_rw_cmds[] = {
  *	LOCKING:
  *	caller.
  */
-int ata_rwcmd_protocol(struct ata_queued_cmd *qc)
+static int ata_rwcmd_protocol(struct ata_taskfile *tf, struct ata_device *dev)
 {
-	struct ata_taskfile *tf = &qc->tf;
-	struct ata_device *dev = qc->dev;
 	u8 cmd;
 
 	int index, fua, lba48, write;
@@ -222,7 +221,7 @@ int ata_rwcmd_protocol(struct ata_queued
 	if (dev->flags & ATA_DFLAG_PIO) {
 		tf->protocol = ATA_PROT_PIO;
 		index = dev->multi_count ? 0 : 8;
-	} else if (lba48 && (qc->ap->flags & ATA_FLAG_PIO_LBA48)) {
+	} else if (lba48 && (dev->ap->flags & ATA_FLAG_PIO_LBA48)) {
 		/* Unable to use DMA due to host limitation */
 		tf->protocol = ATA_PROT_PIO;
 		index = dev->multi_count ? 0 : 8;
@@ -283,6 +282,130 @@ u64 ata_tf_read_block(struct ata_taskfil
 }
 
 /**
+ *	ata_build_tf - Build ATA taskfile for given request
+ *	@tf: Target ATA taskfile
+ *	@dev: ATA device @tf belongs to
+ *	@block: Block address
+ *	@n_block: Number of blocks
+ *	@tf_flags: RW/FUA etc...
+ *	@tag: tag
+ *
+ *	LOCKING:
+ *	None.
+ *
+ *	Build ATA taskfile @tf for request described by @block,
+ *	@n_block, @tf_flags and @tag on @dev.
+ *
+ *	RETURNS:
+ *
+ *	0 on success, -ERANGE if the request is too large for @dev,
+ *	-EINVAL if the request is invalid.
+ */
+int ata_build_tf(struct ata_taskfile *tf, struct ata_device *dev,
+		 u64 block, u32 n_block, unsigned int tf_flags,
+		 unsigned int tag)
+{
+	tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf->flags |= tf_flags;
+
+	if ((dev->flags & (ATA_DFLAG_PIO | ATA_DFLAG_NCQ_OFF |
+			   ATA_DFLAG_NCQ)) == ATA_DFLAG_NCQ) {
+		/* yay, NCQ */
+		if (!lba_48_ok(block, n_block))
+			return -ERANGE;
+
+		tf->protocol = ATA_PROT_NCQ;
+		tf->flags |= ATA_TFLAG_LBA | ATA_TFLAG_LBA48;
+
+		if (tf->flags & ATA_TFLAG_WRITE)
+			tf->command = ATA_CMD_FPDMA_WRITE;
+		else
+			tf->command = ATA_CMD_FPDMA_READ;
+
+		tf->nsect = tag << 3;
+		tf->hob_feature = (n_block >> 8) & 0xff;
+		tf->feature = n_block & 0xff;
+
+		tf->hob_lbah = (block >> 40) & 0xff;
+		tf->hob_lbam = (block >> 32) & 0xff;
+		tf->hob_lbal = (block >> 24) & 0xff;
+		tf->lbah = (block >> 16) & 0xff;
+		tf->lbam = (block >> 8) & 0xff;
+		tf->lbal = block & 0xff;
+
+		tf->device = 1 << 6;
+		if (tf->flags & ATA_TFLAG_FUA)
+			tf->device |= 1 << 7;
+	} else if (dev->flags & ATA_DFLAG_LBA) {
+		tf->flags |= ATA_TFLAG_LBA;
+
+		if (lba_28_ok(block, n_block)) {
+			/* use LBA28 */
+			tf->device |= (block >> 24) & 0xf;
+		} else if (lba_48_ok(block, n_block)) {
+			if (!(dev->flags & ATA_DFLAG_LBA48))
+				return -ERANGE;
+
+			/* use LBA48 */
+			tf->flags |= ATA_TFLAG_LBA48;
+
+			tf->hob_nsect = (n_block >> 8) & 0xff;
+
+			tf->hob_lbah = (block >> 40) & 0xff;
+			tf->hob_lbam = (block >> 32) & 0xff;
+			tf->hob_lbal = (block >> 24) & 0xff;
+		} else
+			/* request too large even for LBA48 */
+			return -ERANGE;
+
+		if (unlikely(ata_rwcmd_protocol(tf, dev) < 0))
+			return -EINVAL;
+
+		tf->nsect = n_block & 0xff;
+
+		tf->lbah = (block >> 16) & 0xff;
+		tf->lbam = (block >> 8) & 0xff;
+		tf->lbal = block & 0xff;
+
+		tf->device |= ATA_LBA;
+	} else {
+		/* CHS */
+		u32 sect, head, cyl, track;
+
+		/* The request -may- be too large for CHS addressing. */
+		if (!lba_28_ok(block, n_block))
+			return -ERANGE;
+
+		if (unlikely(ata_rwcmd_protocol(tf, dev) < 0))
+			return -EINVAL;
+
+		/* Convert LBA to CHS */
+		track = (u32)block / dev->sectors;
+		cyl   = track / dev->heads;
+		head  = track % dev->heads;
+		sect  = (u32)block % dev->sectors + 1;
+
+		DPRINTK("block %u track %u cyl %u head %u sect %u\n",
+			(u32)block, track, cyl, head, sect);
+
+		/* Check whether the converted CHS can fit.
+		   Cylinder: 0-65535
+		   Head: 0-15
+		   Sector: 1-255*/
+		if ((cyl >> 16) || (head >> 4) || (sect >> 8) || (!sect))
+			return -ERANGE;
+
+		tf->nsect = n_block & 0xff; /* Sector count 0 means 256 sectors */
+		tf->lbal = sect;
+		tf->lbam = cyl;
+		tf->lbah = cyl >> 8;
+		tf->device |= head;
+	}
+
+	return 0;
+}
+
+/**
  *	ata_pack_xfermask - Pack pio, mwdma and udma masks into xfer_mask
  *	@pio_mask: pio_mask
  *	@mwdma_mask: mwdma_mask
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 9b0d59d..0d1b96a 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1266,17 +1266,14 @@ nothing_to_do:
 
 static unsigned int ata_scsi_rw_xlat(struct ata_queued_cmd *qc, const u8 *scsicmd)
 {
-	struct ata_taskfile *tf = &qc->tf;
-	struct ata_device *dev = qc->dev;
+	unsigned int tf_flags = 0;
 	u64 block;
 	u32 n_block;
-
-	qc->flags |= ATA_QCFLAG_IO;
-	tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	int rc;
 
 	if (scsicmd[0] == WRITE_10 || scsicmd[0] == WRITE_6 ||
 	    scsicmd[0] == WRITE_16)
-		tf->flags |= ATA_TFLAG_WRITE;
+		tf_flags |= ATA_TFLAG_WRITE;
 
 	/* Calculate the SCSI LBA, transfer length and FUA. */
 	switch (scsicmd[0]) {
@@ -1284,7 +1281,7 @@ static unsigned int ata_scsi_rw_xlat(str
 	case WRITE_10:
 		scsi_10_lba_len(scsicmd, &block, &n_block);
 		if (unlikely(scsicmd[1] & (1 << 3)))
-			tf->flags |= ATA_TFLAG_FUA;
+			tf_flags |= ATA_TFLAG_FUA;
 		break;
 	case READ_6:
 	case WRITE_6:
@@ -1300,7 +1297,7 @@ static unsigned int ata_scsi_rw_xlat(str
 	case WRITE_16:
 		scsi_16_lba_len(scsicmd, &block, &n_block);
 		if (unlikely(scsicmd[1] & (1 << 3)))
-			tf->flags |= ATA_TFLAG_FUA;
+			tf_flags |= ATA_TFLAG_FUA;
 		break;
 	default:
 		DPRINTK("no-byte command\n");
@@ -1318,106 +1315,16 @@ static unsigned int ata_scsi_rw_xlat(str
 		 */
 		goto nothing_to_do;
 
-	if ((dev->flags & (ATA_DFLAG_PIO | ATA_DFLAG_NCQ_OFF |
-			   ATA_DFLAG_NCQ)) == ATA_DFLAG_NCQ) {
-		/* yay, NCQ */
-		if (!lba_48_ok(block, n_block))
-			goto out_of_range;
-
-		tf->protocol = ATA_PROT_NCQ;
-		tf->flags |= ATA_TFLAG_LBA | ATA_TFLAG_LBA48;
-
-		if (tf->flags & ATA_TFLAG_WRITE)
-			tf->command = ATA_CMD_FPDMA_WRITE;
-		else
-			tf->command = ATA_CMD_FPDMA_READ;
-
-		qc->nsect = n_block;
-
-		tf->nsect = qc->tag << 3;
-		tf->hob_feature = (n_block >> 8) & 0xff;
-		tf->feature = n_block & 0xff;
-
-		tf->hob_lbah = (block >> 40) & 0xff;
-		tf->hob_lbam = (block >> 32) & 0xff;
-		tf->hob_lbal = (block >> 24) & 0xff;
-		tf->lbah = (block >> 16) & 0xff;
-		tf->lbam = (block >> 8) & 0xff;
-		tf->lbal = block & 0xff;
-
-		tf->device = 1 << 6;
-		if (tf->flags & ATA_TFLAG_FUA)
-			tf->device |= 1 << 7;
-	} else if (dev->flags & ATA_DFLAG_LBA) {
-		tf->flags |= ATA_TFLAG_LBA;
-
-		if (lba_28_ok(block, n_block)) {
-			/* use LBA28 */
-			tf->device |= (block >> 24) & 0xf;
-		} else if (lba_48_ok(block, n_block)) {
-			if (!(dev->flags & ATA_DFLAG_LBA48))
-				goto out_of_range;
-
-			/* use LBA48 */
-			tf->flags |= ATA_TFLAG_LBA48;
-
-			tf->hob_nsect = (n_block >> 8) & 0xff;
-
-			tf->hob_lbah = (block >> 40) & 0xff;
-			tf->hob_lbam = (block >> 32) & 0xff;
-			tf->hob_lbal = (block >> 24) & 0xff;
-		} else
-			/* request too large even for LBA48 */
-			goto out_of_range;
-
-		if (unlikely(ata_rwcmd_protocol(qc) < 0))
-			goto invalid_fld;
-
-		qc->nsect = n_block;
-		tf->nsect = n_block & 0xff;
-
-		tf->lbah = (block >> 16) & 0xff;
-		tf->lbam = (block >> 8) & 0xff;
-		tf->lbal = block & 0xff;
-
-		tf->device |= ATA_LBA;
-	} else {
-		/* CHS */
-		u32 sect, head, cyl, track;
-
-		/* The request -may- be too large for CHS addressing. */
-		if (!lba_28_ok(block, n_block))
-			goto out_of_range;
-
-		if (unlikely(ata_rwcmd_protocol(qc) < 0))
-			goto invalid_fld;
-
-		/* Convert LBA to CHS */
-		track = (u32)block / dev->sectors;
-		cyl   = track / dev->heads;
-		head  = track % dev->heads;
-		sect  = (u32)block % dev->sectors + 1;
-
-		DPRINTK("block %u track %u cyl %u head %u sect %u\n",
-			(u32)block, track, cyl, head, sect);
-
-		/* Check whether the converted CHS can fit.
-		   Cylinder: 0-65535
-		   Head: 0-15
-		   Sector: 1-255*/
-		if ((cyl >> 16) || (head >> 4) || (sect >> 8) || (!sect))
-			goto out_of_range;
-
-		qc->nsect = n_block;
-		tf->nsect = n_block & 0xff; /* Sector count 0 means 256 sectors */
-		tf->lbal = sect;
-		tf->lbam = cyl;
-		tf->lbah = cyl >> 8;
-		tf->device |= head;
-	}
+	qc->flags |= ATA_QCFLAG_IO;
+	qc->nsect = n_block;
 
-	return 0;
+	rc = ata_build_tf(&qc->tf, qc->dev, block, n_block, tf_flags, qc->tag);
+	if (likely(rc == 0))
+		return 0;
 
+	if (rc == -ERANGE)
+		goto out_of_range;
+	/* treat all other errors as -EINVAL, fall through */
 invalid_fld:
 	ata_scsi_set_sense(qc->scsicmd, ILLEGAL_REQUEST, 0x24, 0x0);
 	/* "Invalid field in cbd" */
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index c82318b..d4791e7 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -44,7 +44,9 @@ extern int atapi_enabled;
 extern int atapi_dmadir;
 extern int libata_fua;
 extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev);
-extern int ata_rwcmd_protocol(struct ata_queued_cmd *qc);
+extern int ata_build_tf(struct ata_taskfile *tf, struct ata_device *dev,
+			u64 block, u32 n_block, unsigned int tf_flags,
+			unsigned int tag);
 extern u64 ata_tf_read_block(struct ata_taskfile *tf, struct ata_device *dev);
 extern void ata_dev_disable(struct ata_device *dev);
 extern void ata_port_flush_task(struct ata_port *ap);
-- 
1.4.3.3

Re: [PATCH 2/4] libata: separate out rw ATA taskfile building into ata_build_tf()

[Mark Lord]

This logic sequence bothers me slightly:

Tejun Heo wrote:
> Separate out rw ATA taskfile building from ata_scsi_rw_xlat() into
> ata_build_tf().  This will be used to improve media error handling.
...
> +	if ((dev->flags & (ATA_DFLAG_PIO | ATA_DFLAG_NCQ_OFF |
> +			   ATA_DFLAG_NCQ)) == ATA_DFLAG_NCQ) {
> +		/* yay, NCQ */
> +		if (!lba_48_ok(block, n_block))
> +			return -ERANGE;
...
> +	} else if (dev->flags & ATA_DFLAG_LBA) {
> +		tf->flags |= ATA_TFLAG_LBA;
> +
> +		if (lba_28_ok(block, n_block)) {
> +			/* use LBA28 */
> +			tf->device |= (block >> 24) & 0xf;
> +		} else if (lba_48_ok(block, n_block)) {
> +			if (!(dev->flags & ATA_DFLAG_LBA48))
> +				return -ERANGE;
...

In the first case, if !lba_48_ok(), then we should at least attempt
to see if the request can be issued via non-NCQ.

Something like this:

if (lba_48_ok() && dev->flags & (ATA_DFLAG_PIO|ATA_DFLAG_NCQ_OFF|ATA_DFLAG_NCQ)) == ATA_DFLAG_NCQ) {
   /* yay, NCQ */
   ...
} else if (dev->flags & ATA_DFLAG_LBA) {
   ...

..except prettier!  ;)

In theory, it looks like an impossible sequence anyway..
if we have the NCQ flag set, then we probably already *know*
that LBA48 is okay.  But since we're not making that assumption,
let's give it the benefit of a doubt and try non-NCQ if LBA48 is a no-go.

Cheers

Re: [PATCH 2/4] libata: separate out rw ATA taskfile building into ata_build_tf()

[Tejun Heo]

Mark Lord wrote:
> This logic sequence bothers me slightly:
> 
> Tejun Heo wrote:
>> Separate out rw ATA taskfile building from ata_scsi_rw_xlat() into
>> ata_build_tf().  This will be used to improve media error handling.
> ...
>> +    if ((dev->flags & (ATA_DFLAG_PIO | ATA_DFLAG_NCQ_OFF |
>> +               ATA_DFLAG_NCQ)) == ATA_DFLAG_NCQ) {
>> +        /* yay, NCQ */
>> +        if (!lba_48_ok(block, n_block))
>> +            return -ERANGE;
> ...
>> +    } else if (dev->flags & ATA_DFLAG_LBA) {
>> +        tf->flags |= ATA_TFLAG_LBA;
>> +
>> +        if (lba_28_ok(block, n_block)) {
>> +            /* use LBA28 */
>> +            tf->device |= (block >> 24) & 0xf;
>> +        } else if (lba_48_ok(block, n_block)) {
>> +            if (!(dev->flags & ATA_DFLAG_LBA48))
>> +                return -ERANGE;
> ...
> 
> In the first case, if !lba_48_ok(), then we should at least attempt
> to see if the request can be issued via non-NCQ.
> 
> Something like this:
> 
> if (lba_48_ok() && dev->flags & 
> (ATA_DFLAG_PIO|ATA_DFLAG_NCQ_OFF|ATA_DFLAG_NCQ)) == ATA_DFLAG_NCQ) {
>   /* yay, NCQ */
>   ...
> } else if (dev->flags & ATA_DFLAG_LBA) {
>   ...
> 
> ..except prettier!  ;)
> 
> In theory, it looks like an impossible sequence anyway..
> if we have the NCQ flag set, then we probably already *know*
> that LBA48 is okay.  But since we're not making that assumption,
> let's give it the benefit of a doubt and try non-NCQ if LBA48 is a no-go.

First of all, that function is moved verbatim from ata_scsi_rw_xlat(), 
and I don't really understand your point.  lba_48_ok() checks whether 
the given block and n_block are inside LBA48 limit.  NCQ commands use 
LBA48 (well, nsect is in the different registers but the limits are the 
same) which is the largest addressing mode currently available.  So, if 
a command cannot be issued using NCQ, it cannot be issued w/ any command 
mode.  ie. if !lba_48_ok(), that command just cannot be issued.

Am I misunderstanding you?

-- 
tejun

Re: [PATCH 2/4] libata: separate out rw ATA taskfile building into ata_build_tf()

[Mark Lord]

Tejun Heo wrote:
>
> First of all, that function is moved verbatim from ata_scsi_rw_xlat(), 

Yeah, I knew that.

Don't mind me, I'm just have a bit of a scatterbrain day,
switching back and forth between reviewing patches,
and deep thinking about another task.

For some odd reason I thought "lba_48_ok" checked whether
or not a drive could do LBA48.. probably because I have a routine
by that exact name in several drivers of my own coding.

But of course it's different here:  it's just checking the requested
block against the LBA48 range.   But is this even necessary here?
Presumably we already checked the block number against the device
max before getting to this point?

Or did we?  I don't actually see that code nearby,
but perhaps the SCSI mid-layer did it for us?
If not, *where* do we have that check?

Anyway, it will all work as is.
I'm just wondering if we can nuke a few lines or not.

Cheers

Re: [PATCH 2/4] libata: separate out rw ATA taskfile building into ata_build_tf()

[Jeff Garzik]

Tejun Heo wrote:
> Separate out rw ATA taskfile building from ata_scsi_rw_xlat() into
> ata_build_tf().  This will be used to improve media error handling.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK, provided you name it ata_build_rw_tf() or ata_build_addr_tf() or 
somesuch

[PATCH 3/4] libata: prepare ata_sg_clean() for invocation from EH

[Tejun Heo]

Make ata_sg_clean() global and don't allow NCQ for internal commands.

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

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 1b33f05..9688bbf 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -309,7 +309,8 @@ int ata_build_tf(struct ata_taskfile *tf
 	tf->flags |= tf_flags;
 
 	if ((dev->flags & (ATA_DFLAG_PIO | ATA_DFLAG_NCQ_OFF |
-			   ATA_DFLAG_NCQ)) == ATA_DFLAG_NCQ) {
+			   ATA_DFLAG_NCQ)) == ATA_DFLAG_NCQ &&
+	    likely(tag != ATA_TAG_INTERNAL)) {
 		/* yay, NCQ */
 		if (!lba_48_ok(block, n_block))
 			return -ERANGE;
@@ -3517,8 +3518,7 @@ static unsigned int ata_dev_init_params(
  *	LOCKING:
  *	spin_lock_irqsave(host lock)
  */
-
-static void ata_sg_clean(struct ata_queued_cmd *qc)
+void ata_sg_clean(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
 	struct scatterlist *sg = qc->__sg;
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index d4791e7..c0f27c3 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -66,6 +66,7 @@ extern int sata_down_spd_limit(struct at
 extern int sata_set_spd_needed(struct ata_port *ap);
 extern int ata_down_xfermask_limit(struct ata_device *dev, int force_pio0);
 extern int ata_set_mode(struct ata_port *ap, 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);
 extern void __ata_qc_complete(struct ata_queued_cmd *qc);
-- 
1.4.3.3

Re: [PATCH 3/4] libata: prepare ata_sg_clean() for invocation from EH

[Jeff Garzik]

Tejun Heo wrote:
> Make ata_sg_clean() global and don't allow NCQ for internal commands.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK

[PATCH 4/4] libata: improve media error handling

[Tejun Heo]

The ATA spec says "The amount of data transferred is indeterminate"
when READ/WRITE commands fail with error status.  TF regs contain the
address of the first sector which failed, but that's it.  libata
reports the reported sector to sd which assumes data upto the first
failed sector was transferred successfully.  This can result in data
corruption.

This patch implements highlevel command-aware recovery which currently
has only one recovery action - ata_eh_do_partial().  If the device
reports the first failed block, it tries to transfer upto that block.

SCSI sense generation is updated such that the first failed block is
reported to SCSI layer iff partial IO occurred.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-eh.c   |  143 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata-scsi.c |   36 +++++++-----
 include/linux/libata.h    |    2 +
 3 files changed, 166 insertions(+), 15 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 104836e..0917423 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -1873,6 +1873,144 @@ static int ata_eh_resume(struct ata_port
 	return 0;
 }
 
+/**
+ *	ata_eh_do_partial - perform partial IO after media error
+ *	@qc: target ATA command
+ *
+ *	The amount of succesfully transferred data is 'indeterminate'
+ *	after a device error.  This makes media error recovery
+ *	difficult for high level driver.  This function makes sure
+ *	data upto the first bad sector is successfully transferred.
+ *
+ *	LOCKING:
+ *	Kernel thread context (may sleep).
+ *
+ *	RETURNS:
+ *	0 on success, -errno otherwise
+ */
+static int ata_eh_do_partial(struct ata_queued_cmd *qc)
+{
+	struct ata_port *ap = qc->ap;
+	struct ata_device *dev = qc->dev;
+	struct ata_eh_context *ehc = &ap->eh_context;
+	struct ata_taskfile tf;
+	u64 begin, bad;
+	struct scatterlist *sg;
+	unsigned int good_bytes, n_elem, idx, len, stored_len, err_mask;
+	int dma_dir, rc;
+
+	/* determine initial good blocks and dma_dir */
+	begin = ata_tf_read_block(&qc->tf, qc->dev);
+	bad = ata_tf_read_block(&qc->result_tf, qc->dev);
+
+	if (bad < begin || begin + qc->nsect <= bad) {
+		ata_dev_printk(dev, KERN_WARNING, "bogus bad block reported "
+			       "begin=%llu nsect=%u bad=%llu, assuming %llu\n",
+			       (unsigned long long)begin, qc->nsect,
+			       (unsigned long long)bad,
+			       (unsigned long long)begin + qc->nsect / 2);
+		return 0;
+	}
+
+	good_bytes = (bad - begin) << 9;
+
+	if (!good_bytes)
+		return 0;
+
+	dma_dir = DMA_FROM_DEVICE;
+	if (qc->tf.flags & ATA_TFLAG_WRITE)
+		dma_dir = DMA_TO_DEVICE;
+
+	/* we're gonna reuse sglist, store & clear existing mapping */
+	sg = qc->__sg;
+	n_elem = qc->orig_n_elem;
+
+	if (qc->flags & ATA_QCFLAG_DMAMAP)
+		ata_sg_clean(qc);
+
+	/* find boundary sg */
+	len = 0;
+	for (idx = 0; idx < n_elem; idx++) {
+		if (len + sg[idx].length > good_bytes)
+			break;
+		len += sg[idx].length;
+	}
+	BUG_ON(idx >= n_elem);
+
+	/* build TF */
+	ata_tf_init(dev, &tf);
+	rc = ata_build_tf(&tf, dev, begin, good_bytes >> 9,
+			  qc->tf.flags & (ATA_TFLAG_WRITE | ATA_TFLAG_FUA),
+			  ATA_TAG_INTERNAL);
+	if (rc < 0) {
+		ata_dev_printk(dev, KERN_ERR, "failed to build TF for "
+			       "partial IO (rc=%d)\n", rc);
+		return rc;
+	}
+
+	/* temporarily trim sg and perform partial IO */
+	stored_len = sg[idx].length;
+	sg[idx].length = good_bytes - len;
+
+	err_mask = ata_exec_internal_sg(dev, &tf, NULL, dma_dir, sg, idx + 1);
+
+	sg[idx].length = stored_len;
+
+	if (err_mask) {
+		ata_dev_printk(dev, KERN_ERR,
+			       "partial IO failed (err_mask=0x%x)\n", err_mask);
+		return -EIO;
+	}
+
+	ehc->has_partial_mask |= 1 << dev->devno;
+	return 0;
+}
+
+/**
+ *	ata_eh_hl_recover - highlevel command-aware recovery
+ *	@ap: target host port
+ *	@r_failed_dev: result parameter to indicate failing device
+ *
+ *	Highlevel command-aware recovery.
+ *
+ *	LOCKING:
+ *	Kernel thread context (may sleep).
+ *
+ *	RETURNS:
+ *	0 on success, -errno otherwise
+ */
+static int ata_eh_hl_recover(struct ata_port *ap,
+			     struct ata_device **r_failed_dev)
+{
+	struct ata_eh_context *ehc = &ap->eh_context;
+	int tag;
+
+	for (tag = 0; tag < ATA_MAX_QUEUE; tag++) {
+		struct ata_queued_cmd *qc = __ata_qc_from_tag(ap, tag);
+		struct ata_device *dev = qc->dev;
+		int rc;
+
+		if (!(qc->flags & ATA_QCFLAG_FAILED))
+			continue;
+
+		if (dev->class == ATA_DEV_ATA) {
+			if (!(ehc->did_partial_mask & (1 << dev->devno)) &&
+			    (qc->flags & ATA_QCFLAG_IO) &&
+			    (qc->err_mask & AC_ERR_MEDIA) &&
+			    !(qc->err_mask & ~(AC_ERR_MEDIA | AC_ERR_DEV))) {
+				ehc->did_partial_mask |= 1 << dev->devno;
+				rc = ata_eh_do_partial(qc);
+				if (rc) {
+					*r_failed_dev = dev;
+					return rc;
+				}
+			}
+		}
+	}
+
+	return 0;
+}
+
 static int ata_port_nr_enabled(struct ata_port *ap)
 {
 	int i, cnt = 0;
@@ -2030,6 +2168,11 @@ static int ata_eh_recover(struct ata_por
 		ehc->i.flags &= ~ATA_EHI_SETMODE;
 	}
 
+	/* perform highlevel recovery */
+	rc = ata_eh_hl_recover(ap, &dev);
+	if (rc)
+		goto dev_fail;
+
 	/* suspend devices */
 	rc = ata_eh_suspend(ap, &dev);
 	if (rc)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 0d1b96a..b73bec7 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -754,13 +754,14 @@ static void ata_gen_passthru_sense(struc
  */
 static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
 {
+	struct ata_port *ap = qc->ap;
 	struct ata_device *dev = qc->dev;
+	struct ata_eh_context *ehc = &ap->eh_context;
 	struct scsi_cmnd *cmd = qc->scsicmd;
 	struct ata_taskfile *tf = &qc->result_tf;
 	unsigned char *sb = cmd->sense_buffer;
 	unsigned char *desc = sb + 8;
 	int verbose = qc->ap->ops->error_handler == NULL;
-	u64 block;
 
 	memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
 
@@ -779,20 +780,25 @@ static void ata_gen_ata_sense(struct ata
 		sb[1] &= 0x0f;
 	}
 
-	block = ata_tf_read_block(&qc->result_tf, dev);
-
-	/* information sense data descriptor */
-	sb[7] = 12;
-	desc[0] = 0x00;
-	desc[1] = 10;
-
-	desc[2] |= 0x80;	/* valid */
-	desc[6] = block >> 40;
-	desc[7] = block >> 32;
-	desc[8] = block >> 24;
-	desc[9] = block >> 16;
-	desc[10] = block >> 8;
-	desc[11] = block;
+	/* If partial transfer occurred, tell upper layer how many
+	 * bytes are completed.
+	 */
+	if (ehc->has_partial_mask & (1 << dev->devno)) {
+		u64 block = ata_tf_read_block(&qc->result_tf, dev);
+
+		/* information sense data descriptor */
+		sb[7] = 12;
+		desc[0] = 0x00;
+		desc[1] = 10;
+
+		desc[2] |= 0x80;	/* valid */
+		desc[6] = block >> 40;
+		desc[7] = block >> 32;
+		desc[8] = block >> 24;
+		desc[9] = block >> 16;
+		desc[10] = block >> 8;
+		desc[11] = block;
+	}
 }
 
 static void ata_scsi_sdev_config(struct scsi_device *sdev)
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 0d0ddea..b485d00 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -527,6 +527,8 @@ struct ata_eh_context {
 	int			tries[ATA_MAX_DEVICES];
 	unsigned int		classes[ATA_MAX_DEVICES];
 	unsigned int		did_probe_mask;
+	unsigned int		did_partial_mask;
+	unsigned int		has_partial_mask;
 };
 
 struct ata_port {
-- 
1.4.3.3

Re: [PATCH 4/4] libata: improve media error handling

[Jeff Garzik]

Tejun Heo wrote:
> The ATA spec says "The amount of data transferred is indeterminate"
> when READ/WRITE commands fail with error status.  TF regs contain the
> address of the first sector which failed, but that's it.  libata
> reports the reported sector to sd which assumes data upto the first
> failed sector was transferred successfully.  This can result in data
> corruption.
> 
> This patch implements highlevel command-aware recovery which currently
> has only one recovery action - ata_eh_do_partial().  If the device
> reports the first failed block, it tries to transfer upto that block.
> 
> SCSI sense generation is updated such that the first failed block is
> reported to SCSI layer iff partial IO occurred.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
>  drivers/ata/libata-eh.c   |  143 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/ata/libata-scsi.c |   36 +++++++-----
>  include/linux/libata.h    |    2 +
>  3 files changed, 166 insertions(+), 15 deletions(-)

(finally reviewing a patch from a 3 months ago)

ACK, please resend at your leisure

In general, I have worries about how solid partial completion really is. 
  I am mainly thinking of SCSI and VFS not libata, here.  Your patch 
looks sane enough, so let's see how things go...

	Jeff