libata: error processing + rw 6 byte fix

This is a revised patch following this post:
http://marc.theaimsgroup.com/?l=linux-scsi&m=112461881419898&w=2

The plan is to add MODE SELECT SCSI command support to libata so that parameters such as WCE and DRA can be changed by a user (i.e. Write(back) Cache Enable and Disable Read Ahead respectively). There are still some issues to be addressed with MODE SELECT so the attached is mainly a subset of the earlier one.
It improves error processing and fixes a READ_6/WRITE_6 SCSI command special case ** as requested by Jeff G.

The attached is against lk 2.6.13-rc6 and includes the START STOP UNIT patch.

ChangeLog:
- generalize SCSI error processing
- add block descriptor to MODE SENSE command
- make various changes to sync with sat-r05 (as noted in source)
- fix READ(6) and WRITE(6) SCSI command special

     case: transfer_length=0 -> transfer 256 blocks

** The special case can be tested with sg_dd:
sg_dd if=/dev/sda blk_sgio=1 cdbsz=6 of=. bs=512 bpt=256 count=256
This tests READ(6) with a transfer length of 0
(i.e. 256 blocks) in its cdb.
BTW the scsi_debug driver has the same bug.

Signed-off-by Douglas Gilbert <[email protected]>

drivers/scsi/libata-scsi.c2613rc6ua renamed from drivers/scsi/libata-scsi.c +6 -1

@@ -579,7 +631,19 @@
 	}
 
 	if (scsicmd[0] == READ_6 || scsicmd[0] == WRITE_6) {
-		qc->nsect = tf->nsect = scsicmd[4];
+		if (scsicmd[4] == 0) {
+			/*
+			 * For READ_6 and WRITE_6 (only)
+			 * transfer_len==0 -> 256 blocks !!
+			 */
+			if (lba48) {

drivers/scsi/libata-scsi.c2613rc6ua renamed from drivers/scsi/libata-scsi.c +7 -4

@@ -1027,6 +1121,13 @@
 	return sizeof(page);
 }
 
+/* As of sat-r05: considering defining this page (for QErr).
+ * D_SENSE is now 0 (fixed sense data format);
+ * guess extended self test completion time: 30 seconds (why?).
+ */
+static const u8 def_control_mpage[] = {
+		0xa, 0xa, 0, 0, 0, 0, 0, 0, 0xff, 0xff, 0, 30};
+
 /**
  *	ata_msense_ctl_mode - Simulate MODE SENSE control mode page
  *	@dev: Device associated with this MODE SENSE command

@@ -1041,16 +1142,17 @@
 
 static unsigned int ata_msense_ctl_mode(u8 **ptr_io, const u8 *last)
 {
-	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

drivers/scsi/libata-scsi.c2613rc6ua renamed from drivers/scsi/libata-scsi.c +6 -5

+static const u8 def_rw_recovery_mpage[] = {
+		0x1,			  /* page code */
+		0xa,			  /* page length */
+		(1 << 6),		  /* note: ARRE=1 */
+		0, 0, 0, 0, 0, 0, 0, 0, 0 /* 9 zeroes */
+	};	/* sat-r05 says AWRE will be 0 */
 
 /**
  *	ata_msense_rw_recovery - Simulate MODE SENSE r/w error recovery page

@@ -1066,15 +1168,9 @@
 
 static unsigned int ata_msense_rw_recovery(u8 **ptr_io, const u8 *last)
 {
-	const u8 page[] = {
-		0x1,			  /* page code */
-		0xa,			  /* page length */
-		(1 << 7) | (1 << 6),	  /* note auto r/w reallocation */
-		0, 0, 0, 0, 0, 0, 0, 0, 0 /* 9 zeroes */

Quoted 30 lines · Douglas Gilbert

This is a revised patch following this post:
http://marc.theaimsgroup.com/?l=linux-scsi&m=112461881419898&w=2

The plan is to add MODE SELECT SCSI command support to libata so that parameters such as WCE and DRA can be changed by a user (i.e. Write(back) Cache Enable and Disable Read Ahead respectively). There are still some issues to be addressed with MODE SELECT so the attached is mainly a subset of the earlier one.
It improves error processing and fixes a READ_6/WRITE_6 SCSI command special case ** as requested by Jeff G.

The attached is against lk 2.6.13-rc6 and includes the START STOP UNIT patch.

ChangeLog:
- generalize SCSI error processing
- add block descriptor to MODE SENSE command
- make various changes to sync with sat-r05 (as noted in source)
- fix READ(6) and WRITE(6) SCSI command special

     case: transfer_length=0 -> transfer 256 blocks

** The special case can be tested with sg_dd:
sg_dd if=/dev/sda blk_sgio=1 cdbsz=6 of=. bs=512 bpt=256 count=256
This tests READ(6) with a transfer length of 0
(i.e. 256 blocks) in its cdb.
BTW the scsi_debug driver has the same bug.

Quoted 1 line

Signed-off-by Douglas Gilbert <[email protected]>

First change needed: this patch must be incremental to your previous START STOP UNIT patch, since I have already applied that patch.

Quoted 11 lines

drivers/scsi/libata-scsi.c2613rc6ua renamed from drivers/scsi/libata-scsi.c +6 -1

@@ -579,7 +631,19 @@
     }
 
     if (scsicmd[0] == READ_6 || scsicmd[0] == WRITE_6) {
-        qc->nsect = tf->nsect = scsicmd[4];
+        if (scsicmd[4] == 0) {
+            /*
+             * For READ_6 and WRITE_6 (only)
+             * transfer_len==0 -> 256 blocks !!
+             */
+            if (lba48) {

drivers/scsi/libata-scsi.c2613rc6ua line 649

Quoted 10 lines

drivers/scsi/libata-scsi.c2613rc6ua renamed from drivers/scsi/libata-scsi.c +7 -0

+                tf->hob_nsect = 1;
+                qc->nsect = 256;
+            } else
+                return 1;
+        } else
+            qc->nsect = scsicmd[4];
+        tf->nsect = scsicmd[4];
         tf->lbal = scsicmd[3];
         tf->lbam = scsicmd[2];
         tf->lbah = scsicmd[1] & 0x1f; /* mask out reserved bits */

lba28 ATA commands behave similarly: for READ DMA, which only has an 8-bit sector count, 00h == 256 sectors.

So, you should not error out for that case.

Quoted 24 lines

drivers/scsi/libata-scsi.c2613rc6ua renamed from drivers/scsi/libata-scsi.c +7 -4

@@ -1027,6 +1121,13 @@
     return sizeof(page);
 }
 
+/* As of sat-r05: considering defining this page (for QErr).
+ * D_SENSE is now 0 (fixed sense data format);
+ * guess extended self test completion time: 30 seconds (why?).
+ */
+static const u8 def_control_mpage[] = {
+        0xa, 0xa, 0, 0, 0, 0, 0, 0, 0xff, 0xff, 0, 30};
+
 /**
  *    ata_msense_ctl_mode - Simulate MODE SENSE control mode page
  *    @dev: Device associated with this MODE SENSE command

@@ -1041,16 +1142,17 @@
 
 static unsigned int ata_msense_ctl_mode(u8 **ptr_io, const u8 *last)
 {
-    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

drivers/scsi/libata-scsi.c2613rc6ua line 1149

Quoted 10 lines

drivers/scsi/libata-scsi.c2613rc6ua renamed from drivers/scsi/libata-scsi.c +4 -5

-     * commands and any SCSI commands against a 48b LBA device.
-     */
-
-    ata_msense_push(ptr_io, last, page, sizeof(page));
-    return sizeof(page);
+    ata_msense_push(ptr_io, last, def_control_mpage,
+            sizeof(def_control_mpage));
+    return sizeof(def_control_mpage);
 }
+   

Please don't shuffle code around, AND change it at the same time. It has the effect of hiding small changes.

In this case, you have chosen to clear the D_SENSE bit (ok) and the GLTSD bit (not ok).

Quoted 19 lines

drivers/scsi/libata-scsi.c2613rc6ua renamed from drivers/scsi/libata-scsi.c +6 -0

+static const u8 def_rw_recovery_mpage[] = {
+        0x1,              /* page code */
+        0xa,              /* page length */
+        (1 << 6),          /* note: ARRE=1 */
+        0, 0, 0, 0, 0, 0, 0, 0, 0 /* 9 zeroes */
+    };    /* sat-r05 says AWRE will be 0 */
 
 /**
  *    ata_msense_rw_recovery - Simulate MODE SENSE r/w error recovery

page

drivers/scsi/libata-scsi.c2613rc6ua renamed from drivers/scsi/libata-scsi.c +0 -5

@@ -1066,15 +1168,9 @@
 
 static unsigned int ata_msense_rw_recovery(u8 **ptr_io, const u8 *last)
 {
-    const u8 page[] = {
-        0x1,              /* page code */
-        0xa,              /* page length */
-        (1 << 7) | (1 << 6),      /* note auto r/w reallocation */
-        0, 0, 0, 0, 0, 0, 0, 0, 0 /* 9 zeroes */

drivers/scsi/libata-scsi.c2613rc6ua line 1176

Quoted 10 lines

drivers/scsi/libata-scsi.c2613rc6ua renamed from drivers/scsi/libata-scsi.c +3 -4

-    };
-
-    ata_msense_push(ptr_io, last, page, sizeof(page));
-    return sizeof(page);
+    ata_msense_push(ptr_io, last, def_rw_recovery_mpage,
+            sizeof(def_rw_recovery_mpage));
+    return sizeof(def_rw_recovery_mpage);
 }
 
 /**

Similarly, do not clear bits without discussion.

In this case, I disagree with SAT. ATA devices will definitely reallocate around defective sectors in the background, trying its damndest to complete a write without failing.

Reads are obviously different. The device doesn't known, until the OS asks for a sector, whether its corrupt or not. The device is free to reallocate this sector as well, in the background, once it discovers that a sector is defective.

How to interpret this? Re-reading SBC-2, it is my feeling that ATA devices should either set both AWRE and ARRE since they do perform background reallocation, or, set NEITHER bit, since the libata SAT layer does not support the error reporting bits mentioned in SBC-2 for R/W Recovery Page.

As of the current implementation, I made a conscious decision to ignore SAT and set both bits. I'm willing to consider changing to the 'neither bit' setup, but not one where ARRE is the only bit set.

drivers/scsi/libata-scsi.c2613rc6ua line 1193

Quoted 7 lines

drivers/scsi/libata-scsi.c2613rc6ua renamed from drivers/scsi/libata-scsi.c +2 -1

@@ -1093,28 +1189,54 @@
                   unsigned int buflen)
 {
     u8 *scsicmd = args->cmd->cmnd, *p, *last;
-    unsigned int page_control, six_byte, output_len;
+    const u8 sat_blk_desc[] = {0, 0, 0, 0, 0, 0, 0x2, 0x0};
+    /* 512 byte blocks, number of blocks = 0, (sat-r05) */

NOTE: don't hardcode 512, since that will change very soon in SATA.

1K ATA devices are just around the corner.

Quoted 4 lines

drivers/scsi/libata-scsi.c2613rc6ua renamed from drivers/scsi/libata-scsi.c +0 -0

@@ -1570,11 +1737,6 @@
             ata_scsi_rbuf_fill(&args, ata_scsiop_mode_sense);
             break;
 

drivers/scsi/libata-scsi.c2613rc6ua line 1742

Quoted 8 lines

drivers/scsi/libata-scsi.c2613rc6ua renamed from drivers/scsi/libata-scsi.c +0 -5

-        case MODE_SELECT:    /* unconditionally return */
-        case MODE_SELECT_10:    /* bad-field-in-cdb */
-            ata_bad_cdb(cmd, done);
-            break;
-
         case READ_CAPACITY:
             ata_scsi_rbuf_fill(&args, ata_scsiop_read_cap);
             break;

At this point in the patch series, you should not be removing this code AFAICS.

Finally, overall, it would really be nice if you would split your changes into a series of patches. i.e.

[PATCH 1/4] START STOP UNIT (already applied)
[PATCH 2/4] ata_scsi_set_sense() impl, and usage
[PATCH 3/4] twiddle mode parameter page values
[PATCH 4/4] MODE SENSE cleanup, blk descriptor, move mode page values etc.

I don't mean to carp, but separating out changes in this manner is strongly encouraged under Linux, and greatly aids in reviewing and applying each patch. You don't have to enumerate EVERY change into a separate patch, just be reasonable: group sets of associated changes into patches.

Quoted 10 lines · Jeff Garzik

drivers/scsi/libata-scsi.c2613rc6ua line 1193

Quoted 7 lines

drivers/scsi/libata-scsi.c2613rc6ua renamed from drivers/scsi/libata-scsi.c +2 -1

@@ -1093,28 +1189,54 @@
                  unsigned int buflen)
 {
    u8 *scsicmd = args->cmd->cmnd, *p, *last;
-    unsigned int page_control, six_byte, output_len;
+    const u8 sat_blk_desc[] = {0, 0, 0, 0, 0, 0, 0x2, 0x0};
+    /* 512 byte blocks, number of blocks = 0, (sat-r05) */

NOTE: don't hardcode 512, since that will change very soon in SATA.

So put ATA_SECT_SIZE in there for the time being?

Quoted 7 lines

1K ATA devices are just around the corner.

Quoted 4 lines

drivers/scsi/libata-scsi.c2613rc6ua renamed from drivers/scsi/libata-scsi.c +0 -0

@@ -1570,11 +1737,6 @@
            ata_scsi_rbuf_fill(&args, ata_scsiop_mode_sense);
            break;
 

Quoted 12 lines

drivers/scsi/libata-scsi.c2613rc6ua line 1742

Quoted 8 lines

drivers/scsi/libata-scsi.c2613rc6ua renamed from drivers/scsi/libata-scsi.c +0 -5

-        case MODE_SELECT:    /* unconditionally return */
-        case MODE_SELECT_10:    /* bad-field-in-cdb */
-            ata_bad_cdb(cmd, done);
-            break;
-
        case READ_CAPACITY:
            ata_scsi_rbuf_fill(&args, ata_scsiop_read_cap);
            break;

At this point in the patch series, you should not be removing this code AFAICS.

It is a trick and an error correction. In this version of libata MODE SELECT commands don't have "invalid field in cdb" as the ata_bad_cmd() sets, but "invalid command operation code" which is what the default case sets.

drivers/scsi/libata-scsi.c2613rc6ua line 643

Quoted 12 lines · Douglas Gilbert

	if (scsicmd[0] == READ_6 || scsicmd[0] == WRITE_6) {

drivers/scsi/libata-scsi.c2613rc6ua renamed from drivers/scsi/libata-scsi.c +10 -1

-		qc->nsect = tf->nsect = scsicmd[4];
+		if (scsicmd[4] == 0) {
+			/*
+			 * For READ_6 and WRITE_6 (only)
+			 * transfer_len==0 -> 256 blocks !!
+			 */
+			if (lba48) {
+				tf->hob_nsect = 1;
+				qc->nsect = 256;
+			} else
+				return 1;

This isn't quite right, for 28-bit lba a 0 sector value means 256 sectors to transfer as well. So just make that:

        if (lba48) {
                tf->hob_nsect = 1;
                qc->nsect = 256;
        }

        /* continue */

and it should work fine. Similarly for 48-bit lba, 0 means 16^2 sectors.

Quoted 19 lines · Jens Axboe

drivers/scsi/libata-scsi.c2613rc6ua line 643

Quoted 12 lines · Douglas Gilbert

	if (scsicmd[0] == READ_6 || scsicmd[0] == WRITE_6) {

drivers/scsi/libata-scsi.c2613rc6ua renamed from drivers/scsi/libata-scsi.c +10 -1

-		qc->nsect = tf->nsect = scsicmd[4];
+		if (scsicmd[4] == 0) {
+			/*
+			 * For READ_6 and WRITE_6 (only)
+			 * transfer_len==0 -> 256 blocks !!
+			 */
+			if (lba48) {
+				tf->hob_nsect = 1;
+				qc->nsect = 256;
+			} else
+				return 1;

This isn't quite right, for 28-bit lba a 0 sector value means 256 sectors to transfer as well. So just make that:

Quoted 8 lines

        if (lba48) {
                tf->hob_nsect = 1;
                qc->nsect = 256;
        }

        /* continue */

and it should work fine. Similarly for 48-bit lba, 0 means 16^2 sectors.

Jens,
Since for 28-bit lba a 0 sector value means 256 sectors do I need to check for the lba48 case at all? As proposed to Jeff is this ok (for READ_6 and WRITE_6):

       if (scsicmd[4] == 0) {
           /*
            * For READ_6 and WRITE_6 (only)
            * transfer_len==0 -> 256 blocks !!
            */
           qc->nsect = 256;
       } else
           qc->nsect = scsicmd[4];
       tf->nsect = scsicmd[4];

Also I noticed while testing the original code with READ_6 (sectors=0) that the device locked up (power cycle required). So given the point you make for 48-bit lba, 0 means 16^2 sectors, then the READ_10 (sectors=0) and READ_16 (sectors=0) which are valid nops according to SBC-2 may also lock up in libata.

Does the attached look OK to everybody?

------------------------------------------------------------------------

drivers/scsi/libata-scsi.c +6 -0

@@ -581,6 +581,12 @@ static unsigned int ata_scsi_rw_xlat(str
 
 	if (scsicmd[0] == READ_6 || scsicmd[0] == WRITE_6) {
 		qc->nsect = tf->nsect = scsicmd[4];
+		if (!qc->nsect) {
+			qc->nsect = 256;
+			if (lba48)
+				tf->hob_nsect = 1;
+		}
+
 		tf->lbal = scsicmd[3];
 		tf->lbam = scsicmd[2];
 		tf->lbah = scsicmd[1] & 0x1f; /* mask out reserved bits */

Quoted 2 lines · Jeff Garzik

Does the attached look OK to everybody?

Jeff,
Yes.

And after an exchange with Jens, it would probably be safer to map transfer_length=0 for READ_10 and READ_16 (as well as WRITE_10 and WRITE_16) to a nop on the ATA side. Otherwise an ATA disk may attempt to transfer 2**16 sectors. [Only READ_6 and WRITE_6 have the quirky "0 means 256" definition, in the larger commands "0 means 0".]

Quoted 11 lines

drivers/scsi/libata-scsi.c +6 -0

 	if (scsicmd[0] == READ_6 || scsicmd[0] == WRITE_6) {
 		qc->nsect = tf->nsect = scsicmd[4];
+		if (!qc->nsect) {
+			qc->nsect = 256;
+			if (lba48)
+				tf->hob_nsect = 1;
+		}
+
 		tf->lbal = scsicmd[3];
 		tf->lbam = scsicmd[2];
 		tf->lbah = scsicmd[1] & 0x1f; /* mask out reserved bits */

Here is the patch I just checked in.