[PATCH libata-dev-2.6 00/05] libata: scsi error handling improvements

[PATCH libata-dev-2.6 00/05] libata: scsi error handling improvements

[Brett Russ]

This patch series attempts to clean up the SCSI error handling a bit.
See comments in below TOC or patch emails.  All of the below have been
tested in success and error paths through the VERIFY_10 and ATA_16
commands using the AHCI driver.

IMPORTANT: the patchset below against libata-dev-2.6 relies on the
recent AHCI driver fixes recently patched into libata-2.6.  I am
including the two specific patches as 1 and 2 of this series for
completeness, although of course they should be merged from libata-2.6
instead.  Therefore, you may ignore these two unless you want to test
this series now on libata-dev.

[ Start of patch descriptions ]

01_libata_garzik-ahci-tf-read.patch
	: AHCI tf_read() support

	(included in libata-2.6) This is Jeff's tf_read() support
	patch for AHCI.

	Signed-off-by: Jeff Garzik <jgarzik@pobox.com>

02_libata_ahci-err-int.patch
	: AHCI error handling fix

	(included in libata-2.6) Fixes AHCI bits during handling of
	fatal error int.

03_libata_update_desc_code.patch
	: update ATA PT sense desc code

	Change the ATA pass through sense block descriptor code to
	0x09 per SAT

04_libata_control_pg_desc_bit.patch
	: support descriptor sense in ctrl page

	libata must support the descriptor format sense blocks as they
	are required to properly report results of ATA pass through
	commands as well as other SCSI commands reporting 48b LBAs.
	This patch adjusts the control mode page to properly report
	this.

05_libata_split_ata_to_sense_error.patch
	: rework how CCs generated

	This patch fixes several bugs as well as reorganizes the way
	check conditions are generated.  Bugs fixed: 1) in
	ata_scsi_qc_complete(), ATA_12/16 commands wouldn't call
	ata_pass_thru_cc() on error status; 2) ata_pass_thru_cc()
	wouldn't put the SK, ASC, and ASCQ from ata_to_sense_error()
	in the correct place in the sense block because
	ata_to_sense_error() was writing a fixed sense block.

	Per the recommendations in the comments, ata_to_sense_error()
	is now split into 3 parts.  The existing fcn is only used for
	outputting a sense key/ASC/ASCQ triplicate.  A new function
	ata_dump_status() was created to print the error info, similar
	to the ide variety.  A third function ata_gen_fixed_sense()
	was created to generate a fixed length sense block.  I added
	the use of the info field for 28b LBAs only.
	ata_pass_thru_cc() renamed to ata_gen_ata_desc_sense() to
	match naming convention, presumably to include another
	descriptor format function in the future (see question 2
	below).

	Questions:

	1) I made the ata_gen_..._sense() routines read the status
           register themselves rather than use the drv_stat values
           that used to be passed in?  These values seemed
           unreliable/useless since they were often hard coded (see
           calls to ata_qc_complete() for origins of most drv_stat
           variables).  Sound ok?

	2) the SAT spec has little about error handling and sense
           information, sepcifically what descriptor format is valid
           for use by SAT commands.  I want to use descriptor type 00
           (information) in my next patch until a spec says
           differently.  Sound ok?

[ End of patch descriptions ]

BR

Re: [PATCH libata-dev-2.6 01/05] libata: AHCI tf_read() support

[Brett Russ]

01_libata_garzik-ahci-tf-read.patch

	(included in libata-2.6) This is Jeff's tf_read() support
	patch for AHCI.

Signed-off-by: Jeff Garzik <jgarzik@pobox.com>

 ahci.c |   11 +++++++++++
 1 files changed, 11 insertions(+)

Index: libata-dev-2.6/drivers/scsi/ahci.c
===================================================================
--- libata-dev-2.6.orig/drivers/scsi/ahci.c	2005-03-17 12:36:29.000000000 -0500
+++ libata-dev-2.6/drivers/scsi/ahci.c	2005-03-17 17:16:57.000000000 -0500
@@ -179,6 +179,7 @@ static void ahci_eng_timeout(struct ata_
 static int ahci_port_start(struct ata_port *ap);
 static void ahci_port_stop(struct ata_port *ap);
 static void ahci_host_stop(struct ata_host_set *host_set);
+static void ahci_tf_read(struct ata_port *ap, struct ata_taskfile *tf);
 static void ahci_qc_prep(struct ata_queued_cmd *qc);
 static u8 ahci_check_status(struct ata_port *ap);
 static u8 ahci_check_err(struct ata_port *ap);
@@ -213,6 +214,8 @@ static struct ata_port_operations ahci_o
 	.check_err		= ahci_check_err,
 	.dev_select		= ata_noop_dev_select,
 
+	.tf_read		= ahci_tf_read,
+
 	.phy_reset		= ahci_phy_reset,
 
 	.qc_prep		= ahci_qc_prep,
@@ -466,6 +469,14 @@ static u8 ahci_check_err(struct ata_port
 	return (readl(mmio + PORT_TFDATA) >> 8) & 0xFF;
 }
 
+static void ahci_tf_read(struct ata_port *ap, struct ata_taskfile *tf)
+{
+	struct ahci_port_priv *pp = ap->private_data;
+	u8 *d2h_fis = pp->rx_fis + RX_FIS_D2H_REG;
+
+	ata_tf_from_fis(d2h_fis, tf);
+}
+
 static void ahci_fill_sg(struct ata_queued_cmd *qc)
 {
 	struct ahci_port_priv *pp = qc->ap->private_data;

Re: [PATCH libata-dev-2.6 02/05] libata: AHCI error handling fix

[Brett Russ]

02_libata_ahci-err-int.patch

	(included in libata-2.6) Fixes AHCI bits during handling of
	fatal error int.

Signed-off-by: Brett Russ <russb@emc.com>

 ahci.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Index: libata-dev-2.6/drivers/scsi/ahci.c
===================================================================
--- libata-dev-2.6.orig/drivers/scsi/ahci.c	2005-03-17 17:16:57.000000000 -0500
+++ libata-dev-2.6/drivers/scsi/ahci.c	2005-03-17 17:16:57.000000000 -0500
@@ -548,7 +548,7 @@ static void ahci_intr_error(struct ata_p
 
 	/* stop DMA */
 	tmp = readl(port_mmio + PORT_CMD);
-	tmp &= PORT_CMD_START | PORT_CMD_FIS_RX;
+	tmp &= ~PORT_CMD_START;
 	writel(tmp, port_mmio + PORT_CMD);
 
 	/* wait for engine to stop.  TODO: this could be
@@ -580,7 +580,7 @@ static void ahci_intr_error(struct ata_p
 
 	/* re-start DMA */
 	tmp = readl(port_mmio + PORT_CMD);
-	tmp |= PORT_CMD_START | PORT_CMD_FIS_RX;
+	tmp |= PORT_CMD_START;
 	writel(tmp, port_mmio + PORT_CMD);
 	readl(port_mmio + PORT_CMD); /* flush */
 

Re: [PATCH libata-dev-2.6 03/05] libata: update ATA PT sense desc code

[Brett Russ]

03_libata_update_desc_code.patch

	Change the ATA pass through sense block descriptor code to
	0x09 per SAT

Signed-off-by: Brett Russ <russb@emc.com>

 libata-scsi.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Index: libata-dev-2.6/drivers/scsi/libata-scsi.c
===================================================================
--- libata-dev-2.6.orig/drivers/scsi/libata-scsi.c	2005-03-08 08:47:48.000000000 -0500
+++ libata-dev-2.6/drivers/scsi/libata-scsi.c	2005-03-17 17:16:58.000000000 -0500
@@ -531,7 +531,7 @@ void ata_pass_thru_cc(struct ata_queued_
 	 */
 	sb[0] = 0x72 ;
 
-	desc[0] = 0x8e ;	/* TODO: replace with official value. */
+	desc[0] = 0x09;
 
 	/*
 	 * Set length of additional sense data.
@@ -2059,7 +2059,7 @@ void ata_scsi_simulate(u16 *id,
 			ata_scsi_rbuf_fill(&args, ata_scsiop_report_luns);
 			break;
 
-		/* mandantory commands we haven't implemented yet */
+		/* mandatory commands we haven't implemented yet */
 		case REQUEST_SENSE:
 
 		/* all other commands */

Re: [PATCH libata-dev-2.6 03/05] libata: update ATA PT sense desc code

[Jeff Garzik]

Brett Russ wrote:
> 03_libata_update_desc_code.patch
> 
> 	Change the ATA pass through sense block descriptor code to
> 	0x09 per SAT
> 
> Signed-off-by: Brett Russ <russb@emc.com>

applied

Re: [PATCH libata-dev-2.6 04/05] libata: support descriptor sense in ctrl page

[Brett Russ]

04_libata_control_pg_desc_bit.patch

	libata must support the descriptor format sense blocks as they
	are required to properly report results of ATA pass through
	commands as well as other SCSI commands reporting 48b LBAs.
	This patch adjusts the control mode page to properly report
	this.

Signed-off-by: Brett Russ <russb@emc.com>

 libata-scsi.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletion(-)

Index: libata-dev-2.6/drivers/scsi/libata-scsi.c
===================================================================
--- libata-dev-2.6.orig/drivers/scsi/libata-scsi.c	2005-03-17 17:16:58.000000000 -0500
+++ libata-dev-2.6/drivers/scsi/libata-scsi.c	2005-03-17 17:16:58.000000000 -0500
@@ -1370,7 +1370,12 @@ static unsigned int ata_msense_caching(u
 
 static unsigned int ata_msense_ctl_mode(u8 **ptr_io, const u8 *last)
 {
-	const u8 page[] = {0xa, 0xa, 2, 0, 0, 0, 0, 0, 0xff, 0xff, 0, 30};
+	const u8 page[] = {0xa, 0xa, 6, 0, 0, 0, 0, 0, 0xff, 0xff, 0, 30};
+
+	/* byte 2: set the descriptor format sense data bit (bit 2)
+	 * since we need to support returning this format for SAT
+	 * commands and any SCSI commands against a 48b LBA device.
+	 */
 
 	ata_msense_push(ptr_io, last, page, sizeof(page));
 	return sizeof(page);

Re: [PATCH libata-dev-2.6 04/05] libata: support descriptor sense in ctrl page

[Jeff Garzik]

Brett Russ wrote:
> 04_libata_control_pg_desc_bit.patch
> 
> 	libata must support the descriptor format sense blocks as they
> 	are required to properly report results of ATA pass through
> 	commands as well as other SCSI commands reporting 48b LBAs.
> 	This patch adjusts the control mode page to properly report
> 	this.
> 
> Signed-off-by: Brett Russ <russb@emc.com>

applied

Re: [PATCH libata-dev-2.6 05/05] libata: rework how CCs generated

[Brett Russ]

05_libata_split_ata_to_sense_error.patch

	This patch fixes several bugs as well as reorganizes the way
	check conditions are generated.  Bugs fixed: 1) in
	ata_scsi_qc_complete(), ATA_12/16 commands wouldn't call
	ata_pass_thru_cc() on error status; 2) ata_pass_thru_cc()
	wouldn't put the SK, ASC, and ASCQ from ata_to_sense_error()
	in the correct place in the sense block because
	ata_to_sense_error() was writing a fixed sense block.

	Per the recommendations in the comments, ata_to_sense_error()
	is now split into 3 parts.  The existing fcn is only used for
	outputting a sense key/ASC/ASCQ triplicate.  A new function
	ata_dump_status() was created to print the error info, similar
	to the ide variety.  A third function ata_gen_fixed_sense()
	was created to generate a fixed length sense block.  I added
	the use of the info field for 28b LBAs only.
	ata_pass_thru_cc() renamed to ata_gen_ata_desc_sense() to
	match naming convention, presumably to include another
	descriptor format function in the future (see question 2
	below).

	Questions:

	1) I made the ata_gen_..._sense() routines read the status
           register themselves rather than use the drv_stat values
           that used to be passed in?  These values seemed
           unreliable/useless since they were often hard coded (see
           calls to ata_qc_complete() for origins of most drv_stat
           variables).  Sound ok?

	2) the SAT spec has little about error handling and sense
           information, sepcifically what descriptor format is valid
           for use by SAT commands.  I want to use descriptor type 00
           (information) in my next patch until a spec says
           differently.  Sound ok?

Signed-off-by: Brett Russ <russb@emc.com>

 libata-scsi.c |  342 +++++++++++++++++++++++++++++++++-------------------------
 libata.h      |    1 
 2 files changed, 197 insertions(+), 146 deletions(-)

Index: libata-dev-2.6/drivers/scsi/libata-scsi.c
===================================================================
--- libata-dev-2.6.orig/drivers/scsi/libata-scsi.c	2005-03-17 17:16:58.000000000 -0500
+++ libata-dev-2.6/drivers/scsi/libata-scsi.c	2005-03-17 17:16:59.000000000 -0500
@@ -331,24 +331,69 @@ struct ata_queued_cmd *ata_scsi_qc_new(s
 }
 
 /**
+ *	ata_dump_status - user friendly display of error info
+ *	@id: id of the port in question
+ *	@tf: ptr to filled out taskfile
+ *
+ *	Decode and dump the ATA error/status registers for the user so
+ *	that they have some idea what really happened at the non
+ *	make-believe layer.
+ *
+ *	LOCKING:
+ *	inherited from caller
+ */
+void ata_dump_status(unsigned id, struct ata_taskfile *tf)
+{
+	u8 stat = tf->command, err = tf->feature;
+
+	printk(KERN_WARNING "ata%u: status=0x%02x { ", id, stat);
+	if (stat & ATA_BUSY) {
+		printk("Busy }\n");	/* Data is not valid in this case */
+	} else {
+		if (stat & 0x40)	printk("DriveReady ");
+		if (stat & 0x20)	printk("DeviceFault ");
+		if (stat & 0x10)	printk("SeekComplete ");
+		if (stat & 0x08)	printk("DataRequest ");
+		if (stat & 0x04)	printk("CorrectedError ");
+		if (stat & 0x02)	printk("Index ");
+		if (stat & 0x01)	printk("Error ");
+		printk("}\n");
+
+		if (err) {
+			printk(KERN_WARNING "ata%u: error=0x%02x { ", id, err);
+			if (err & 0x04)		printk("DriveStatusError ");
+			if (err & 0x80) {
+				if (err & 0x04) printk("BadCRC ");
+				else printk("Sector ");
+			}
+			if (err & 0x40)		printk("UncorrectableError ");
+			if (err & 0x10)		printk("SectorIdNotFound ");
+			if (err & 0x02)		printk("TrackZeroNotFound ");
+			if (err & 0x01)		printk("AddrMarkNotFound ");
+			printk("}\n");
+		}
+	}
+}
+
+/**
  *	ata_to_sense_error - convert ATA error to SCSI error
- *	@qc: Command that we are erroring out
  *	@drv_stat: value contained in ATA status register
+ *	@drv_err: value contained in ATA error register
+ *	@sk: the sense key we'll fill out
+ *	@asc: the additional sense code we'll fill out
+ *	@ascq: the additional sense code qualifier we'll fill out
  *
- *	Converts an ATA error into a SCSI error. While we are at it
- *	we decode and dump the ATA error for the user so that they
- *	have some idea what really happened at the non make-believe
- *	layer.
+ *	Converts an ATA error into a SCSI error.  Fill out pointers to
+ *	SK, ASC, and ASCQ bytes for later use in fixed or descriptor
+ *	format sense blocks.
  *
  *	LOCKING:
  *	spin_lock_irqsave(host_set lock)
  */
-
-void ata_to_sense_error(struct ata_queued_cmd *qc, u8 drv_stat)
+void ata_to_sense_error(unsigned id, u8 drv_stat, u8 drv_err, u8 *sk, u8 *asc, 
+			u8 *ascq)
 {
-	struct scsi_cmnd *cmd = qc->scsicmd;
-	u8 err = 0;
-	unsigned char *sb = cmd->sense_buffer;
+	int i;
 	/* Based on the 3ware driver translation table */
 	static unsigned char sense_table[][4] = {
 		/* BBD|ECC|ID|MAR */
@@ -389,147 +434,99 @@ void ata_to_sense_error(struct ata_queue
 		{0x04, 		RECOVERED_ERROR, 0x11, 0x00},	// Recovered ECC error	  Medium error, recovered
 		{0xFF, 0xFF, 0xFF, 0xFF}, // END mark
 	};
-	int i = 0;
-
-	cmd->result = SAM_STAT_CHECK_CONDITION;
 
 	/*
 	 *	Is this an error we can process/parse
 	 */
-
-	if(drv_stat & ATA_ERR)
-		/* Read the err bits */
-		err = ata_chk_err(qc->ap);
-
-	/* Display the ATA level error info */
-
-	printk(KERN_WARNING "ata%u: status=0x%02x { ", qc->ap->id, drv_stat);
-	if(drv_stat & 0x80)
-	{
-		printk("Busy ");
-		err = 0;	/* Data is not valid in this case */
-	}
-	else {
-		if(drv_stat & 0x40)	printk("DriveReady ");
-		if(drv_stat & 0x20)	printk("DeviceFault ");
-		if(drv_stat & 0x10)	printk("SeekComplete ");
-		if(drv_stat & 0x08)	printk("DataRequest ");
-		if(drv_stat & 0x04)	printk("CorrectedError ");
-		if(drv_stat & 0x02)	printk("Index ");
-		if(drv_stat & 0x01)	printk("Error ");
-	}
-	printk("}\n");
-
-	if(err)
-	{
-		printk(KERN_WARNING "ata%u: error=0x%02x { ", qc->ap->id, err);
-		if(err & 0x04)		printk("DriveStatusError ");
-		if(err & 0x80)
-		{
-			if(err & 0x04)
-				printk("BadCRC ");
-			else
-				printk("Sector ");
-		}
-		if(err & 0x40)		printk("UncorrectableError ");
-		if(err & 0x10)		printk("SectorIdNotFound ");
-		if(err & 0x02)		printk("TrackZeroNotFound ");
-		if(err & 0x01)		printk("AddrMarkNotFound ");
-		printk("}\n");
-
-		/* Should we dump sector info here too ?? */
+	if (drv_stat & ATA_BUSY) {
+		drv_err = 0;	/* Ignore the err bits, they're invalid */
 	}
 
+	printk(KERN_ERR "ata%u: translating stat 0x%02x err 0x%02x to sense\n",
+	       id,drv_stat,drv_err);
 
-	/* Look for err */
-	while(sense_table[i][0] != 0xFF)
-	{
-		/* Look for best matches first */
-		if((sense_table[i][0] & err) == sense_table[i][0])
-		{
-			sb[0] = 0x70;
-			sb[2] = sense_table[i][1];
-			sb[7] = 0x0a;
-			sb[12] = sense_table[i][2];
-			sb[13] = sense_table[i][3];
-			return;
+	if (drv_err) {
+		/* Look for drv_err */
+		for (i = 0; sense_table[i][0] != 0xFF; i++) {
+			/* Look for best matches first */
+			if ((sense_table[i][0] & drv_err) == sense_table[i][0]) {
+				*sk = sense_table[i][1];
+				*asc = sense_table[i][2];
+				*ascq = sense_table[i][3];
+				return;
+			}
 		}
-		i++;
+		/* No immediate match */
+		printk(KERN_DEBUG "ata%u: no sense translation for "
+		       "error 0x%02x\n", id, drv_err);
 	}
-	/* No immediate match */
-	if(err)
-		printk(KERN_DEBUG "ata%u: no sense translation for 0x%02x\n", qc->ap->id, err);
 
-	i = 0;
 	/* Fall back to interpreting status bits */
-	while(stat_table[i][0] != 0xFF)
-	{
-		if(stat_table[i][0] & drv_stat)
-		{
-			sb[0] = 0x70;
-			sb[2] = stat_table[i][1];
-			sb[7] = 0x0a;
-			sb[12] = stat_table[i][2];
-			sb[13] = stat_table[i][3];
+	for (i = 0; stat_table[i][0] != 0xFF; i++) {
+		if (stat_table[i][0] & drv_stat) {
+			*sk = stat_table[i][1];
+			*asc = stat_table[i][2];
+			*ascq = stat_table[i][3];
 			return;
 		}
-		i++;
-	}
-	/* No error ?? */
-	printk(KERN_ERR "ata%u: called with no error (%02X)!\n", qc->ap->id, drv_stat);
-	/* additional-sense-code[-qualifier] */
-
-	sb[0] = 0x70;
-	sb[2] = MEDIUM_ERROR;
-	sb[7] = 0x0A;
-	if (cmd->sc_data_direction == SCSI_DATA_READ) {
-		sb[12] = 0x11; /* "unrecovered read error" */
-		sb[13] = 0x04;
-	} else {
-		sb[12] = 0x0C; /* "write error -             */
-		sb[13] = 0x02; /*  auto-reallocation failed" */
 	}
+	/* No error?  Undecoded? */
+	printk(KERN_ERR "ata%u: no sense translation for status: 0x%02x\n", 
+	       id, drv_stat);
+
+	/* For our last chance pick, use medium read error because
+	 * it's much more common than an ATA drive telling you a write
+	 * has failed.
+	 */
+	*sk = MEDIUM_ERROR;
+	*asc = 0x11; /* "unrecovered read error" */
+	*ascq = 0x04; /*  "auto-reallocation failed" */
 }
 
 /*
- *	ata_pass_thru_cc - Generate check condition sense block.
+ *	ata_gen_ata_desc_sense - Generate check condition sense block.
  *	@qc: Command that completed.
  *
- *	Regardless of whether the command errored or not, return
- *	a sense block. Copy all controller registers into
- *	the sense block. Clear sense key, ASC & ASCQ if
- *	there is no error.
+ *	This function is specific to the ATA descriptor format sense
+ *	block specified for the ATA pass through commands.  Regardless
+ *	of whether the command errored or not, return a sense
+ *	block. Copy all controller registers into the sense
+ *	block. Clear sense key, ASC & ASCQ if there is no error.
  *
  *	LOCKING:
  *	spin_lock_irqsave(host_set lock)
  */
-void ata_pass_thru_cc(struct ata_queued_cmd *qc, u8 drv_stat)
+void ata_gen_ata_desc_sense(struct ata_queued_cmd *qc)
 {
 	struct scsi_cmnd *cmd = qc->scsicmd;
 	struct ata_taskfile *tf = &qc->tf;
 	unsigned char *sb = cmd->sense_buffer;
-	unsigned char *desc = sb + 8 ;
+	unsigned char *desc = sb + 8;
+
+	memset(sb, 0, 32);
 
 	cmd->result = SAM_STAT_CHECK_CONDITION;
 
 	/*
+	 * Read the controller registers.
+	 */
+	assert(NULL != qc->ap->ops->tf_read);
+	qc->ap->ops->tf_read(qc->ap, tf);
+
+	/*
 	 * Use ata_to_sense_error() to map status register bits
-	 * onto sense key, asc & ascq. We will overwrite some
-	 * (many) of the fields later.
-	 *
-	 * TODO: reorganise better, by splitting ata_to_sense_error()
+	 * onto sense key, asc & ascq.
 	 */
-	if (unlikely(drv_stat & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ))) {
-		ata_to_sense_error(qc, drv_stat) ;
-	} else {
-		sb[3] = sb[2] = sb[1] = 0x00 ;
+	if (unlikely(tf->command & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ))) {
+		ata_to_sense_error(qc->ap->id, tf->command, tf->feature,
+				   &sb[1], &sb[2], &sb[3]);
+		sb[1] &= 0x0f;
 	}
 
 	/*
-	 * Sense data is current and format is
-	 * descriptor.
+	 * Sense data is current and format is descriptor.
 	 */
-	sb[0] = 0x72 ;
+	sb[0] = 0x72;
 
 	desc[0] = 0x09;
 
@@ -538,35 +535,78 @@ void ata_pass_thru_cc(struct ata_queued_
 	 * Since we only populate descriptor 0, the total
 	 * length is the same (fixed) length as descriptor 0.
 	 */
-	desc[1] = sb[7] = 14 ;
-
-	/*
-	 * Read the controller registers.
-	 */
-	qc->ap->ops->tf_read(qc->ap, tf);
+	desc[1] = sb[7] = 14;
 
 	/*
 	 * Copy registers into sense buffer.
 	 */
-	desc[2] = 0x00 ;
-	desc[3] = tf->feature ;	/* Note: becomes error register when read. */
-	desc[5] = tf->nsect ;
-	desc[7] = tf->lbal ;
-	desc[9] = tf->lbam ;
-	desc[11] = tf->lbah ;
-	desc[12] = tf->device ;
-	desc[13] = drv_stat ;
+	desc[2] = 0x00;
+	desc[3] = tf->feature;	/* == error reg */
+	desc[5] = tf->nsect;
+	desc[7] = tf->lbal;
+	desc[9] = tf->lbam;
+	desc[11] = tf->lbah;
+	desc[12] = tf->device;
+	desc[13] = tf->command; /* == status reg */
 
 	/*
 	 * Fill in Extend bit, and the high order bytes
 	 * if applicable.
 	 */
 	if (tf->flags & ATA_TFLAG_LBA48) {
-		desc[2] |= 0x01 ;
-		desc[4] = tf->hob_nsect ;
-		desc[6] = tf->hob_lbal ;
-		desc[8] = tf->hob_lbam ;
-		desc[10] = tf->hob_lbah ;
+		desc[2] |= 0x01;
+		desc[4] = tf->hob_nsect;
+		desc[6] = tf->hob_lbal;
+		desc[8] = tf->hob_lbam;
+		desc[10] = tf->hob_lbah;
+	}
+}
+
+/**
+ *	ata_gen_fixed_sense - generate a SCSI fixed sense block
+ *	@qc: Command that we are erroring out
+ *
+ *	Leverage ata_to_sense_error() to give us the codes.  Fit our
+ *	LBA in here if there's room.
+ *
+ *	LOCKING:
+ *	inherited from caller
+ */
+void ata_gen_fixed_sense(struct ata_queued_cmd *qc)
+{
+	struct scsi_cmnd *cmd = qc->scsicmd;
+	struct ata_taskfile *tf = &qc->tf;
+	unsigned char *sb = cmd->sense_buffer;
+
+	memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
+
+	cmd->result = SAM_STAT_CHECK_CONDITION;
+
+	/*
+	 * Read the controller registers.
+	 */
+	assert(NULL != qc->ap->ops->tf_read);
+	qc->ap->ops->tf_read(qc->ap, tf);
+
+	/*
+	 * Use ata_to_sense_error() to map status register bits
+	 * onto sense key, asc & ascq.
+	 */
+	if (unlikely(tf->command & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ))) {
+		ata_to_sense_error(qc->ap->id, tf->command, tf->feature,
+				   &sb[2], &sb[12], &sb[13]);
+		sb[2] &= 0x0f;
+	}
+
+	sb[0] = 0x70;
+	sb[7] = 0x0a;
+	if (tf->flags & ATA_TFLAG_LBA && !(tf->flags & ATA_TFLAG_LBA48)) {
+		/* A small (28b) LBA will fit in the 32b info field */
+		sb[0] |= 0x80;		/* set valid bit */
+		sb[3] = tf->device & 0x0f;
+		sb[4] = tf->lbah;
+		sb[5] = tf->lbam;
+		sb[6] = tf->lbal;
 	}
 }
 
@@ -946,23 +986,35 @@ static unsigned int ata_scsi_rw_xlat(str
 static int ata_scsi_qc_complete(struct ata_queued_cmd *qc, u8 drv_stat)
 {
 	struct scsi_cmnd *cmd = qc->scsicmd;
+ 	int need_sense = drv_stat & (ATA_ERR | ATA_BUSY | ATA_DRQ);
 
-	/*
-	 * If this was a pass-thru command, and the user requested
-	 * a check condition return including register values.
-	 * Note that check condition is generated, and the ATA
-	 * register values are returned, whether the command completed
-	 * successfully or not. If there was no error, SK, ASC and
-	 * ASCQ will all be zero.
+	/* For ATA pass thru (SAT) commands, generate a sense block if
+	 * user mandated it or if there's an error.  Note that if we
+	 * generate because the user forced us to, a check condition
+	 * is generated and the ATA register values are returned
+	 * whether the command completed successfully or not. If there
+	 * was no error, SK, ASC and ASCQ will all be zero.
 	 */
 	if (((cmd->cmnd[0] == ATA_16) || (cmd->cmnd[0] == ATA_12)) &&
-						(cmd->cmnd[2] & 0x20)) {
-		ata_pass_thru_cc(qc, drv_stat) ;
+ 	    ((cmd->cmnd[2] & 0x20) || need_sense)) {
+ 		ata_gen_ata_desc_sense(qc);
 	} else {
-		if (unlikely(drv_stat & (ATA_ERR | ATA_BUSY | ATA_DRQ)))
-			ata_to_sense_error(qc, drv_stat);
-		else
+		if (!need_sense) {
 			cmd->result = SAM_STAT_GOOD;
+		} else {
+			/* TODO: decide which descriptor format to use
+			 * for 48b LBA devices and call that here
+			 * instead of the fixed desc, which is only
+			 * good for smaller LBA (and maybe CHS?)
+			 * devices.
+			 */
+			ata_gen_fixed_sense(qc);
+		}
+	}
+
+	if (need_sense) {
+		/* The ata_gen_..._sense routines fill in tf */
+		ata_dump_status(qc->ap->id, &qc->tf);
 	}
 
 	qc->scsidone(cmd);
Index: libata-dev-2.6/drivers/scsi/libata.h
===================================================================
--- libata-dev-2.6.orig/drivers/scsi/libata.h	2005-03-17 01:33:26.000000000 -0500
+++ libata-dev-2.6/drivers/scsi/libata.h	2005-03-17 17:16:59.000000000 -0500
@@ -45,7 +45,6 @@ extern int ata_cmd_ioctl(struct scsi_dev
 
 
 /* libata-scsi.c */
-extern void ata_to_sense_error(struct ata_queued_cmd *qc, u8 drv_stat);
 extern int ata_scsi_error(struct Scsi_Host *host);
 extern unsigned int ata_scsiop_inq_std(struct ata_scsi_args *args, u8 *rbuf,
 			       unsigned int buflen);

Re: [PATCH libata-dev-2.6 05/05] libata: rework how CCs generated

[Jeff Garzik]

Brett Russ wrote:
> 05_libata_split_ata_to_sense_error.patch
> 
> 	This patch fixes several bugs as well as reorganizes the way
> 	check conditions are generated.  Bugs fixed: 1) in
> 	ata_scsi_qc_complete(), ATA_12/16 commands wouldn't call
> 	ata_pass_thru_cc() on error status; 2) ata_pass_thru_cc()
> 	wouldn't put the SK, ASC, and ASCQ from ata_to_sense_error()
> 	in the correct place in the sense block because
> 	ata_to_sense_error() was writing a fixed sense block.
> 
> 	Per the recommendations in the comments, ata_to_sense_error()
> 	is now split into 3 parts.  The existing fcn is only used for
> 	outputting a sense key/ASC/ASCQ triplicate.  A new function
> 	ata_dump_status() was created to print the error info, similar
> 	to the ide variety.  A third function ata_gen_fixed_sense()
> 	was created to generate a fixed length sense block.  I added
> 	the use of the info field for 28b LBAs only.
> 	ata_pass_thru_cc() renamed to ata_gen_ata_desc_sense() to
> 	match naming convention, presumably to include another
> 	descriptor format function in the future (see question 2
> 	below).
> 
> 	Questions:
> 
> 	1) I made the ata_gen_..._sense() routines read the status
>            register themselves rather than use the drv_stat values
>            that used to be passed in?  These values seemed
>            unreliable/useless since they were often hard coded (see
>            calls to ata_qc_complete() for origins of most drv_stat
>            variables).  Sound ok?
> 
> 	2) the SAT spec has little about error handling and sense
>            information, sepcifically what descriptor format is valid
>            for use by SAT commands.  I want to use descriptor type 00
>            (information) in my next patch until a spec says
>            differently.  Sound ok?
> 
> Signed-off-by: Brett Russ <russb@emc.com>

Patch in general is OK, but I would prefer that it be split up a bit 
more.  Suggested split:

* whitespace changes (obscures reviewing the code)
* create and use ata_dump_status()
* the rest of the changes

Regards,

	Jeff

[PATCH libata-dev-2.6 00/03] libata: scsi error handling improvements

[Brett Russ]

These patches are a resubmit of patch 5/5 of the first series
submitted 2005-03-17.  Jeff requested that the single patch be split
into a suggested 3 smaller patches; the results are below.  I also
took this opportunity to further clean up the changes.

[ Start of patch descriptions ]

01_libata_libata-whitespace.patch
	: whitespace updates

	This patch adjusts some whitespace to bring the format of
	libata-scsi.c to a consistent state.

02_libata_ata_dump_status.patch
	: create/use ata_dump_status()

	This patch introduces the ata_dump_status() function, which
	for now is called from ata_to_sense_error() only.

03_libata_rework-cc-generation.patch
	: rework check condition handling

	This patch refactors the check condition creation within
	libata.  Changes include:

	- ata_to_sense_error() now *only* performs the translation
          from an ATA status/error register combination to a SCSI
          SK/ASC/ASCQ combination.  Additionally, the translation is
          logged at level KERNEL_ERR and any untranslatable combos are
          logged at level KERNEL_WARNING.

	- ata_dump_status() is modified to take a taskfile struct as
          argument in preparation for a future patch which will add
          proper display of the failing location (LBA or CHS)

	- created ata_gen_fixed_sense() to generate a fixed length CC
          sense block.

	- ata_pass_thru_cc() has been renamed to
          ata_gen_ata_desc_sense() to fit the naming convention
          mentioned above.  Its guts were changed a bit as well.

	- ata_scsi_qc_complete() has been modified to fix a bug where
          ATA_12/16 commands would not generate a sense block on
          error.  Other changes made here as well, including the call
          to ata_dump_status().

[ End of patch descriptions ]

Re: [PATCH libata-dev-2.6 01/03] libata: whitespace updates

[Brett Russ]

01_libata_libata-whitespace.patch

	This patch adjusts some whitespace to bring the format of
	libata-scsi.c to a consistent state.

Signed-off-by: Brett Russ <russb@emc.com>

 libata-scsi.c |  112 +++++++++++++++++++++++++---------------------------------
 1 files changed, 50 insertions(+), 62 deletions(-)

Index: libata-dev-2.6/drivers/scsi/libata-scsi.c
===================================================================
--- libata-dev-2.6.orig/drivers/scsi/libata-scsi.c	2005-03-23 15:35:08.000000000 -0500
+++ libata-dev-2.6/drivers/scsi/libata-scsi.c	2005-03-23 15:35:09.000000000 -0500
@@ -397,56 +397,46 @@ void ata_to_sense_error(struct ata_queue
 	 *	Is this an error we can process/parse
 	 */
 
-	if(drv_stat & ATA_ERR)
-		/* Read the err bits */
-		err = ata_chk_err(qc->ap);
+	if (drv_stat & ATA_ERR)
+		err = ata_chk_err(qc->ap);	/* Read the err bits */
 
 	/* Display the ATA level error info */
 
 	printk(KERN_WARNING "ata%u: status=0x%02x { ", qc->ap->id, drv_stat);
-	if(drv_stat & 0x80)
-	{
+	if (drv_stat & 0x80) {
 		printk("Busy ");
 		err = 0;	/* Data is not valid in this case */
-	}
-	else {
-		if(drv_stat & 0x40)	printk("DriveReady ");
-		if(drv_stat & 0x20)	printk("DeviceFault ");
-		if(drv_stat & 0x10)	printk("SeekComplete ");
-		if(drv_stat & 0x08)	printk("DataRequest ");
-		if(drv_stat & 0x04)	printk("CorrectedError ");
-		if(drv_stat & 0x02)	printk("Index ");
-		if(drv_stat & 0x01)	printk("Error ");
+	} else {
+		if (drv_stat & 0x40)	printk("DriveReady ");
+		if (drv_stat & 0x20)	printk("DeviceFault ");
+		if (drv_stat & 0x10)	printk("SeekComplete ");
+		if (drv_stat & 0x08)	printk("DataRequest ");
+		if (drv_stat & 0x04)	printk("CorrectedError ");
+		if (drv_stat & 0x02)	printk("Index ");
+		if (drv_stat & 0x01)	printk("Error ");
 	}
 	printk("}\n");
 
-	if(err)
-	{
+	if (err) {
 		printk(KERN_WARNING "ata%u: error=0x%02x { ", qc->ap->id, err);
-		if(err & 0x04)		printk("DriveStatusError ");
-		if(err & 0x80)
-		{
-			if(err & 0x04)
-				printk("BadCRC ");
-			else
-				printk("Sector ");
+		if (err & 0x04)		printk("DriveStatusError ");
+		if (err & 0x80) {
+			if (err & 0x04)	printk("BadCRC ");
+			else		printk("Sector ");
 		}
-		if(err & 0x40)		printk("UncorrectableError ");
-		if(err & 0x10)		printk("SectorIdNotFound ");
-		if(err & 0x02)		printk("TrackZeroNotFound ");
-		if(err & 0x01)		printk("AddrMarkNotFound ");
+		if (err & 0x40)		printk("UncorrectableError ");
+		if (err & 0x10)		printk("SectorIdNotFound ");
+		if (err & 0x02)		printk("TrackZeroNotFound ");
+		if (err & 0x01)		printk("AddrMarkNotFound ");
 		printk("}\n");
 
 		/* Should we dump sector info here too ?? */
 	}
 
-
 	/* Look for err */
-	while(sense_table[i][0] != 0xFF)
-	{
+	while (sense_table[i][0] != 0xFF) {
 		/* Look for best matches first */
-		if((sense_table[i][0] & err) == sense_table[i][0])
-		{
+		if ((sense_table[i][0] & err) == sense_table[i][0]) {
 			sb[0] = 0x70;
 			sb[2] = sense_table[i][1];
 			sb[7] = 0x0a;
@@ -457,15 +447,13 @@ void ata_to_sense_error(struct ata_queue
 		i++;
 	}
 	/* No immediate match */
-	if(err)
+	if (err)
 		printk(KERN_DEBUG "ata%u: no sense translation for 0x%02x\n", qc->ap->id, err);
 
 	i = 0;
 	/* Fall back to interpreting status bits */
-	while(stat_table[i][0] != 0xFF)
-	{
-		if(stat_table[i][0] & drv_stat)
-		{
+	while (stat_table[i][0] != 0xFF) {
+		if (stat_table[i][0] & drv_stat) {
 			sb[0] = 0x70;
 			sb[2] = stat_table[i][1];
 			sb[7] = 0x0a;
@@ -508,7 +496,7 @@ void ata_pass_thru_cc(struct ata_queued_
 	struct scsi_cmnd *cmd = qc->scsicmd;
 	struct ata_taskfile *tf = &qc->tf;
 	unsigned char *sb = cmd->sense_buffer;
-	unsigned char *desc = sb + 8 ;
+	unsigned char *desc = sb + 8;
 
 	cmd->result = SAM_STAT_CHECK_CONDITION;
 
@@ -520,16 +508,16 @@ void ata_pass_thru_cc(struct ata_queued_
 	 * TODO: reorganise better, by splitting ata_to_sense_error()
 	 */
 	if (unlikely(drv_stat & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ))) {
-		ata_to_sense_error(qc, drv_stat) ;
+		ata_to_sense_error(qc, drv_stat);
 	} else {
-		sb[3] = sb[2] = sb[1] = 0x00 ;
+		sb[3] = sb[2] = sb[1] = 0x00;
 	}
 
 	/*
 	 * Sense data is current and format is
 	 * descriptor.
 	 */
-	sb[0] = 0x72 ;
+	sb[0] = 0x72;
 
 	desc[0] = 0x09;
 
@@ -538,7 +526,7 @@ void ata_pass_thru_cc(struct ata_queued_
 	 * Since we only populate descriptor 0, the total
 	 * length is the same (fixed) length as descriptor 0.
 	 */
-	desc[1] = sb[7] = 14 ;
+	desc[1] = sb[7] = 14;
 
 	/*
 	 * Read the controller registers.
@@ -548,25 +536,25 @@ void ata_pass_thru_cc(struct ata_queued_
 	/*
 	 * Copy registers into sense buffer.
 	 */
-	desc[2] = 0x00 ;
-	desc[3] = tf->feature ;	/* Note: becomes error register when read. */
-	desc[5] = tf->nsect ;
-	desc[7] = tf->lbal ;
-	desc[9] = tf->lbam ;
-	desc[11] = tf->lbah ;
-	desc[12] = tf->device ;
-	desc[13] = drv_stat ;
+	desc[2] = 0x00;
+	desc[3] = tf->feature;	/* Note: becomes error register when read. */
+	desc[5] = tf->nsect;
+	desc[7] = tf->lbal;
+	desc[9] = tf->lbam;
+	desc[11] = tf->lbah;
+	desc[12] = tf->device;
+	desc[13] = drv_stat;
 
 	/*
 	 * Fill in Extend bit, and the high order bytes
 	 * if applicable.
 	 */
 	if (tf->flags & ATA_TFLAG_LBA48) {
-		desc[2] |= 0x01 ;
-		desc[4] = tf->hob_nsect ;
-		desc[6] = tf->hob_lbal ;
-		desc[8] = tf->hob_lbam ;
-		desc[10] = tf->hob_lbah ;
+		desc[2] |= 0x01;
+		desc[4] = tf->hob_nsect;
+		desc[6] = tf->hob_lbal;
+		desc[8] = tf->hob_lbam;
+		desc[10] = tf->hob_lbah;
 	}
 }
 
@@ -957,7 +945,7 @@ static int ata_scsi_qc_complete(struct a
 	 */
 	if (((cmd->cmnd[0] == ATA_16) || (cmd->cmnd[0] == ATA_12)) &&
 						(cmd->cmnd[2] & 0x20)) {
-		ata_pass_thru_cc(qc, drv_stat) ;
+		ata_pass_thru_cc(qc, drv_stat);
 	} else {
 		if (unlikely(drv_stat & (ATA_ERR | ATA_BUSY | ATA_DRQ)))
 			ata_to_sense_error(qc, drv_stat);
@@ -1811,9 +1799,9 @@ ata_scsi_pass_thru(struct ata_queued_cmd
 			tf->hob_lbal = scsicmd[7];
 			tf->hob_lbam = scsicmd[9];
 			tf->hob_lbah = scsicmd[11];
-			tf->flags |= ATA_TFLAG_LBA48 ;
+			tf->flags |= ATA_TFLAG_LBA48;
 		} else
-			tf->flags &= ~ATA_TFLAG_LBA48 ;
+			tf->flags &= ~ATA_TFLAG_LBA48;
 
 		/*
 		 * Always copy low byte, device and command registers.
@@ -1829,7 +1817,7 @@ ata_scsi_pass_thru(struct ata_queued_cmd
 		/*
 		 * 12-byte CDB - incapable of extended commands.
 		 */
-		tf->flags &= ~ATA_TFLAG_LBA48 ;
+		tf->flags &= ~ATA_TFLAG_LBA48;
 
 		tf->feature = scsicmd[3];
 		tf->nsect = scsicmd[4];
@@ -1856,7 +1844,7 @@ ata_scsi_pass_thru(struct ata_queued_cmd
 	 * and pass on write indication (used for PIO/DMA
 	 * setup.)
 	 */
-	tf->flags |= (ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE) ;
+	tf->flags |= (ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE);
 
 	if (cmd->sc_data_direction == SCSI_DATA_WRITE)
 		tf->flags |= ATA_TFLAG_WRITE;
@@ -1867,7 +1855,7 @@ ata_scsi_pass_thru(struct ata_queued_cmd
 	 * TODO: find out if we need to do more here to
 	 *       cover scatter/gather case.
 	 */
-	qc->nsect = cmd->bufflen / ATA_SECT_SIZE ;
+	qc->nsect = cmd->bufflen / ATA_SECT_SIZE;
 
 	return 0;
 }
@@ -1907,7 +1895,7 @@ static inline ata_xlat_func_t ata_get_xl
 
 	case ATA_12:
 	case ATA_16:
-		return ata_scsi_pass_thru ;
+		return ata_scsi_pass_thru;
 	}
 
 	return NULL;

Re: [PATCH libata-dev-2.6 01/03] libata: whitespace updates

[Jeff Garzik]

applied patches 1-3, thanks much.

Small administrivia request, though:  after using my patch merging 
script to merge your patches, I have to do the following things manually:

* delete diffstat output
* delete patch filename
* un-indent patch description text

It would be great if I didn't have to do those manual steps :)

On the other hand, I really like your "patch 0/N" email with a 
description of the entire patchset.  Keep sending those, thanks.

Everything else looks great.

	Jeff

Re: [PATCH libata-dev-2.6 02/03] libata: create/use ata_dump_status()

[Brett Russ]

02_libata_ata_dump_status.patch

	This patch introduces the ata_dump_status() function, which
	for now is called from ata_to_sense_error() only.

Signed-off-by: Brett Russ <russb@emc.com>

 libata-scsi.c |   76 +++++++++++++++++++++++++++++++++-------------------------
 1 files changed, 44 insertions(+), 32 deletions(-)

Index: libata-dev-2.6/drivers/scsi/libata-scsi.c
===================================================================
--- libata-dev-2.6.orig/drivers/scsi/libata-scsi.c	2005-03-23 15:35:09.000000000 -0500
+++ libata-dev-2.6/drivers/scsi/libata-scsi.c	2005-03-23 15:35:09.000000000 -0500
@@ -331,6 +331,49 @@ struct ata_queued_cmd *ata_scsi_qc_new(s
 }
 
 /**
+ *	ata_dump_status - user friendly display of error info
+ *	@id: id of the port in question
+ *	@tf: ptr to filled out taskfile
+ *
+ *	Decode and dump the ATA error/status registers for the user so
+ *	that they have some idea what really happened at the non
+ *	make-believe layer.
+ *
+ *	LOCKING:
+ *	inherited from caller
+ */
+void ata_dump_status(unsigned id, u8 stat, u8 err)
+{
+	printk(KERN_WARNING "ata%u: status=0x%02x { ", id, stat);
+	if (stat & ATA_BUSY) {
+		printk("Busy }\n");	/* Data is not valid in this case */
+	} else {
+		if (stat & 0x40)	printk("DriveReady ");
+		if (stat & 0x20)	printk("DeviceFault ");
+		if (stat & 0x10)	printk("SeekComplete ");
+		if (stat & 0x08)	printk("DataRequest ");
+		if (stat & 0x04)	printk("CorrectedError ");
+		if (stat & 0x02)	printk("Index ");
+		if (stat & 0x01)	printk("Error ");
+		printk("}\n");
+
+		if (err) {
+			printk(KERN_WARNING "ata%u: error=0x%02x { ", id, err);
+			if (err & 0x04)		printk("DriveStatusError ");
+			if (err & 0x80) {
+				if (err & 0x04)	printk("BadCRC ");
+				else		printk("Sector ");
+			}
+			if (err & 0x40)		printk("UncorrectableError ");
+			if (err & 0x10)		printk("SectorIdNotFound ");
+			if (err & 0x02)		printk("TrackZeroNotFound ");
+			if (err & 0x01)		printk("AddrMarkNotFound ");
+			printk("}\n");
+		}
+	}
+}
+
+/**
  *	ata_to_sense_error - convert ATA error to SCSI error
  *	@qc: Command that we are erroring out
  *	@drv_stat: value contained in ATA status register
@@ -400,38 +443,7 @@ void ata_to_sense_error(struct ata_queue
 	if (drv_stat & ATA_ERR)
 		err = ata_chk_err(qc->ap);	/* Read the err bits */
 
-	/* Display the ATA level error info */
-
-	printk(KERN_WARNING "ata%u: status=0x%02x { ", qc->ap->id, drv_stat);
-	if (drv_stat & 0x80) {
-		printk("Busy ");
-		err = 0;	/* Data is not valid in this case */
-	} else {
-		if (drv_stat & 0x40)	printk("DriveReady ");
-		if (drv_stat & 0x20)	printk("DeviceFault ");
-		if (drv_stat & 0x10)	printk("SeekComplete ");
-		if (drv_stat & 0x08)	printk("DataRequest ");
-		if (drv_stat & 0x04)	printk("CorrectedError ");
-		if (drv_stat & 0x02)	printk("Index ");
-		if (drv_stat & 0x01)	printk("Error ");
-	}
-	printk("}\n");
-
-	if (err) {
-		printk(KERN_WARNING "ata%u: error=0x%02x { ", qc->ap->id, err);
-		if (err & 0x04)		printk("DriveStatusError ");
-		if (err & 0x80) {
-			if (err & 0x04)	printk("BadCRC ");
-			else		printk("Sector ");
-		}
-		if (err & 0x40)		printk("UncorrectableError ");
-		if (err & 0x10)		printk("SectorIdNotFound ");
-		if (err & 0x02)		printk("TrackZeroNotFound ");
-		if (err & 0x01)		printk("AddrMarkNotFound ");
-		printk("}\n");
-
-		/* Should we dump sector info here too ?? */
-	}
+	ata_dump_status(qc->ap->id, drv_stat, err);
 
 	/* Look for err */
 	while (sense_table[i][0] != 0xFF) {

Re: [PATCH libata-dev-2.6 03/03] libata: rework check condition handling

[Brett Russ]

03_libata_rework-cc-generation.patch

	This patch refactors the check condition creation within
	libata.  Changes include:

	- ata_to_sense_error() now *only* performs the translation
          from an ATA status/error register combination to a SCSI
          SK/ASC/ASCQ combination.  Additionally, the translation is
          logged at level KERNEL_ERR and any untranslatable combos are
          logged at level KERNEL_WARNING.

	- ata_dump_status() is modified to take a taskfile struct as
          argument in preparation for a future patch which will add
          proper display of the failing location (LBA or CHS)

	- created ata_gen_fixed_sense() to generate a fixed length CC
          sense block.

	- ata_pass_thru_cc() has been renamed to
          ata_gen_ata_desc_sense() to fit the naming convention
          mentioned above.  Its guts were changed a bit as well.

	- ata_scsi_qc_complete() has been modified to fix a bug where
          ATA_12/16 commands would not generate a sense block on
          error.  Other changes made here as well, including the call
          to ata_dump_status().

Signed-off-by: Brett Russ <russb@emc.com>

 libata-scsi.c |  238 +++++++++++++++++++++++++++++++++++-----------------------
 libata.h      |    1 
 2 files changed, 147 insertions(+), 92 deletions(-)

Index: libata-dev-2.6/drivers/scsi/libata-scsi.c
===================================================================
--- libata-dev-2.6.orig/drivers/scsi/libata-scsi.c	2005-03-23 15:35:09.000000000 -0500
+++ libata-dev-2.6/drivers/scsi/libata-scsi.c	2005-03-23 15:35:10.000000000 -0500
@@ -342,8 +342,10 @@ struct ata_queued_cmd *ata_scsi_qc_new(s
  *	LOCKING:
  *	inherited from caller
  */
-void ata_dump_status(unsigned id, u8 stat, u8 err)
+void ata_dump_status(unsigned id, struct ata_taskfile *tf)
 {
+	u8 stat = tf->command, err = tf->feature;
+
 	printk(KERN_WARNING "ata%u: status=0x%02x { ", id, stat);
 	if (stat & ATA_BUSY) {
 		printk("Busy }\n");	/* Data is not valid in this case */
@@ -375,23 +377,23 @@ void ata_dump_status(unsigned id, u8 sta
 
 /**
  *	ata_to_sense_error - convert ATA error to SCSI error
- *	@qc: Command that we are erroring out
  *	@drv_stat: value contained in ATA status register
+ *	@drv_err: value contained in ATA error register
+ *	@sk: the sense key we'll fill out
+ *	@asc: the additional sense code we'll fill out
+ *	@ascq: the additional sense code qualifier we'll fill out
  *
- *	Converts an ATA error into a SCSI error. While we are at it
- *	we decode and dump the ATA error for the user so that they
- *	have some idea what really happened at the non make-believe
- *	layer.
+ *	Converts an ATA error into a SCSI error.  Fill out pointers to
+ *	SK, ASC, and ASCQ bytes for later use in fixed or descriptor
+ *	format sense blocks.
  *
  *	LOCKING:
  *	spin_lock_irqsave(host_set lock)
  */
-
-void ata_to_sense_error(struct ata_queued_cmd *qc, u8 drv_stat)
+void ata_to_sense_error(unsigned id, u8 drv_stat, u8 drv_err, u8 *sk, u8 *asc, 
+			u8 *ascq)
 {
-	struct scsi_cmnd *cmd = qc->scsicmd;
-	u8 err = 0;
-	unsigned char *sb = cmd->sense_buffer;
+	int i;
 	/* Based on the 3ware driver translation table */
 	static unsigned char sense_table[][4] = {
 		/* BBD|ECC|ID|MAR */
@@ -432,102 +434,101 @@ void ata_to_sense_error(struct ata_queue
 		{0x04, 		RECOVERED_ERROR, 0x11, 0x00},	// Recovered ECC error	  Medium error, recovered
 		{0xFF, 0xFF, 0xFF, 0xFF}, // END mark
 	};
-	int i = 0;
-
-	cmd->result = SAM_STAT_CHECK_CONDITION;
 
 	/*
 	 *	Is this an error we can process/parse
 	 */
+	if (drv_stat & ATA_BUSY) {
+		drv_err = 0;	/* Ignore the err bits, they're invalid */
+	}
 
-	if (drv_stat & ATA_ERR)
-		err = ata_chk_err(qc->ap);	/* Read the err bits */
-
-	ata_dump_status(qc->ap->id, drv_stat, err);
-
-	/* Look for err */
-	while (sense_table[i][0] != 0xFF) {
-		/* Look for best matches first */
-		if ((sense_table[i][0] & err) == sense_table[i][0]) {
-			sb[0] = 0x70;
-			sb[2] = sense_table[i][1];
-			sb[7] = 0x0a;
-			sb[12] = sense_table[i][2];
-			sb[13] = sense_table[i][3];
-			return;
+	if (drv_err) {
+		/* Look for drv_err */
+		for (i = 0; sense_table[i][0] != 0xFF; i++) {
+			/* Look for best matches first */
+			if ((sense_table[i][0] & drv_err) == 
+			    sense_table[i][0]) {
+				*sk = sense_table[i][1];
+				*asc = sense_table[i][2];
+				*ascq = sense_table[i][3];
+				goto translate_done;
+			}
 		}
-		i++;
+		/* No immediate match */
+		printk(KERN_WARNING "ata%u: no sense translation for "
+		       "error 0x%02x\n", id, drv_err);
 	}
-	/* No immediate match */
-	if (err)
-		printk(KERN_DEBUG "ata%u: no sense translation for 0x%02x\n", qc->ap->id, err);
 
-	i = 0;
 	/* Fall back to interpreting status bits */
-	while (stat_table[i][0] != 0xFF) {
+	for (i = 0; stat_table[i][0] != 0xFF; i++) {
 		if (stat_table[i][0] & drv_stat) {
-			sb[0] = 0x70;
-			sb[2] = stat_table[i][1];
-			sb[7] = 0x0a;
-			sb[12] = stat_table[i][2];
-			sb[13] = stat_table[i][3];
-			return;
+			*sk = stat_table[i][1];
+			*asc = stat_table[i][2];
+			*ascq = stat_table[i][3];
+			goto translate_done;
 		}
-		i++;
-	}
-	/* No error ?? */
-	printk(KERN_ERR "ata%u: called with no error (%02X)!\n", qc->ap->id, drv_stat);
-	/* additional-sense-code[-qualifier] */
-
-	sb[0] = 0x70;
-	sb[2] = MEDIUM_ERROR;
-	sb[7] = 0x0A;
-	if (cmd->sc_data_direction == SCSI_DATA_READ) {
-		sb[12] = 0x11; /* "unrecovered read error" */
-		sb[13] = 0x04;
-	} else {
-		sb[12] = 0x0C; /* "write error -             */
-		sb[13] = 0x02; /*  auto-reallocation failed" */
 	}
+	/* No error?  Undecoded? */
+	printk(KERN_WARNING "ata%u: no sense translation for status: 0x%02x\n", 
+	       id, drv_stat);
+
+	/* For our last chance pick, use medium read error because
+	 * it's much more common than an ATA drive telling you a write
+	 * has failed.
+	 */
+	*sk = MEDIUM_ERROR;
+	*asc = 0x11; /* "unrecovered read error" */
+	*ascq = 0x04; /*  "auto-reallocation failed" */
+
+ translate_done:
+	printk(KERN_ERR "ata%u: translated ATA stat/err 0x%02x/%02x to "
+	       "SCSI SK/ASC/ASCQ 0x%x/%02x/%02x\n", id, drv_stat, drv_err,
+	       *sk, *asc, *ascq);
+	return;
 }
 
 /*
- *	ata_pass_thru_cc - Generate check condition sense block.
+ *	ata_gen_ata_desc_sense - Generate check condition sense block.
  *	@qc: Command that completed.
  *
- *	Regardless of whether the command errored or not, return
- *	a sense block. Copy all controller registers into
- *	the sense block. Clear sense key, ASC & ASCQ if
- *	there is no error.
+ *	This function is specific to the ATA descriptor format sense
+ *	block specified for the ATA pass through commands.  Regardless
+ *	of whether the command errored or not, return a sense
+ *	block. Copy all controller registers into the sense
+ *	block. Clear sense key, ASC & ASCQ if there is no error.
  *
  *	LOCKING:
  *	spin_lock_irqsave(host_set lock)
  */
-void ata_pass_thru_cc(struct ata_queued_cmd *qc, u8 drv_stat)
+void ata_gen_ata_desc_sense(struct ata_queued_cmd *qc)
 {
 	struct scsi_cmnd *cmd = qc->scsicmd;
 	struct ata_taskfile *tf = &qc->tf;
 	unsigned char *sb = cmd->sense_buffer;
 	unsigned char *desc = sb + 8;
 
+	memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
+
 	cmd->result = SAM_STAT_CHECK_CONDITION;
 
 	/*
+	 * Read the controller registers.
+	 */
+	assert(NULL != qc->ap->ops->tf_read);
+	qc->ap->ops->tf_read(qc->ap, tf);
+
+	/*
 	 * Use ata_to_sense_error() to map status register bits
-	 * onto sense key, asc & ascq. We will overwrite some
-	 * (many) of the fields later.
-	 *
-	 * TODO: reorganise better, by splitting ata_to_sense_error()
+	 * onto sense key, asc & ascq.
 	 */
-	if (unlikely(drv_stat & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ))) {
-		ata_to_sense_error(qc, drv_stat);
-	} else {
-		sb[3] = sb[2] = sb[1] = 0x00;
+	if (unlikely(tf->command & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ))) {
+		ata_to_sense_error(qc->ap->id, tf->command, tf->feature,
+				   &sb[1], &sb[2], &sb[3]);
+		sb[1] &= 0x0f;
 	}
 
 	/*
-	 * Sense data is current and format is
-	 * descriptor.
+	 * Sense data is current and format is descriptor.
 	 */
 	sb[0] = 0x72;
 
@@ -541,21 +542,16 @@ void ata_pass_thru_cc(struct ata_queued_
 	desc[1] = sb[7] = 14;
 
 	/*
-	 * Read the controller registers.
-	 */
-	qc->ap->ops->tf_read(qc->ap, tf);
-
-	/*
 	 * Copy registers into sense buffer.
 	 */
 	desc[2] = 0x00;
-	desc[3] = tf->feature;	/* Note: becomes error register when read. */
+	desc[3] = tf->feature;	/* == error reg */
 	desc[5] = tf->nsect;
 	desc[7] = tf->lbal;
 	desc[9] = tf->lbam;
 	desc[11] = tf->lbah;
 	desc[12] = tf->device;
-	desc[13] = drv_stat;
+	desc[13] = tf->command; /* == status reg */
 
 	/*
 	 * Fill in Extend bit, and the high order bytes
@@ -571,6 +567,54 @@ void ata_pass_thru_cc(struct ata_queued_
 }
 
 /**
+ *	ata_gen_fixed_sense - generate a SCSI fixed sense block
+ *	@qc: Command that we are erroring out
+ *
+ *	Leverage ata_to_sense_error() to give us the codes.  Fit our
+ *	LBA in here if there's room.
+ *
+ *	LOCKING:
+ *	inherited from caller
+ */
+void ata_gen_fixed_sense(struct ata_queued_cmd *qc)
+{
+	struct scsi_cmnd *cmd = qc->scsicmd;
+	struct ata_taskfile *tf = &qc->tf;
+	unsigned char *sb = cmd->sense_buffer;
+
+	memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
+
+	cmd->result = SAM_STAT_CHECK_CONDITION;
+
+	/*
+	 * Read the controller registers.
+	 */
+	assert(NULL != qc->ap->ops->tf_read);
+	qc->ap->ops->tf_read(qc->ap, tf);
+
+	/*
+	 * Use ata_to_sense_error() to map status register bits
+	 * onto sense key, asc & ascq.
+	 */
+	if (unlikely(tf->command & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ))) {
+		ata_to_sense_error(qc->ap->id, tf->command, tf->feature,
+				   &sb[2], &sb[12], &sb[13]);
+		sb[2] &= 0x0f;
+	}
+
+	sb[0] = 0x70;
+	sb[7] = 0x0a;
+	if (tf->flags & ATA_TFLAG_LBA && !(tf->flags & ATA_TFLAG_LBA48)) {
+		/* A small (28b) LBA will fit in the 32b info field */
+		sb[0] |= 0x80;		/* set valid bit */
+		sb[3] = tf->device & 0x0f;
+		sb[4] = tf->lbah;
+		sb[5] = tf->lbam;
+		sb[6] = tf->lbal;
+	}
+}
+
+/**
  *	ata_scsi_slave_config - Set SCSI device attributes
  *	@sdev: SCSI device to examine
  *
@@ -946,23 +990,35 @@ static unsigned int ata_scsi_rw_xlat(str
 static int ata_scsi_qc_complete(struct ata_queued_cmd *qc, u8 drv_stat)
 {
 	struct scsi_cmnd *cmd = qc->scsicmd;
+ 	int need_sense = drv_stat & (ATA_ERR | ATA_BUSY | ATA_DRQ);
 
-	/*
-	 * If this was a pass-thru command, and the user requested
-	 * a check condition return including register values.
-	 * Note that check condition is generated, and the ATA
-	 * register values are returned, whether the command completed
-	 * successfully or not. If there was no error, SK, ASC and
-	 * ASCQ will all be zero.
+	/* For ATA pass thru (SAT) commands, generate a sense block if
+	 * user mandated it or if there's an error.  Note that if we
+	 * generate because the user forced us to, a check condition
+	 * is generated and the ATA register values are returned
+	 * whether the command completed successfully or not. If there
+	 * was no error, SK, ASC and ASCQ will all be zero.
 	 */
 	if (((cmd->cmnd[0] == ATA_16) || (cmd->cmnd[0] == ATA_12)) &&
-						(cmd->cmnd[2] & 0x20)) {
-		ata_pass_thru_cc(qc, drv_stat);
+ 	    ((cmd->cmnd[2] & 0x20) || need_sense)) {
+ 		ata_gen_ata_desc_sense(qc);
 	} else {
-		if (unlikely(drv_stat & (ATA_ERR | ATA_BUSY | ATA_DRQ)))
-			ata_to_sense_error(qc, drv_stat);
-		else
+		if (!need_sense) {
 			cmd->result = SAM_STAT_GOOD;
+		} else {
+			/* TODO: decide which descriptor format to use
+			 * for 48b LBA devices and call that here
+			 * instead of the fixed desc, which is only
+			 * good for smaller LBA (and maybe CHS?)
+			 * devices.
+			 */
+			ata_gen_fixed_sense(qc);
+		}
+	}
+
+	if (need_sense) {
+		/* The ata_gen_..._sense routines fill in tf */
+		ata_dump_status(qc->ap->id, &qc->tf);
 	}
 
 	qc->scsidone(cmd);
Index: libata-dev-2.6/drivers/scsi/libata.h
===================================================================
--- libata-dev-2.6.orig/drivers/scsi/libata.h	2005-03-17 01:33:26.000000000 -0500
+++ libata-dev-2.6/drivers/scsi/libata.h	2005-03-23 15:35:10.000000000 -0500
@@ -45,7 +45,6 @@ extern int ata_cmd_ioctl(struct scsi_dev
 
 
 /* libata-scsi.c */
-extern void ata_to_sense_error(struct ata_queued_cmd *qc, u8 drv_stat);
 extern int ata_scsi_error(struct Scsi_Host *host);
 extern unsigned int ata_scsiop_inq_std(struct ata_scsi_args *args, u8 *rbuf,
 			       unsigned int buflen);