Douglas Gilbert wrote:
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 <dougg@xxxxxxxxxx>
First change needed: this patch must be incremental to your previous
START STOP UNIT patch, since I have already applied that patch.
@@ -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) {
+ 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.
@@ -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
- * 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).
+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 */
- };
-
- 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.
@@ -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.
@@ -1570,11 +1737,6 @@
ata_scsi_rbuf_fill(&args, ata_scsiop_mode_sense);
break;
- 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.
Thanks, and regards,
Jeff
-
: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
