[PATCH] libata: write cache and read ahead

[PATCH] libata: write cache and read ahead

[Douglas Gilbert]

[-- Attachment #1: Type: text/plain, Size: 1509 bytes --]

The attachment is for discussion. It adds MODE SELECT
support to libata allowing the write(back) cache and
read ahead to be manipulated by users [i.e. the WCE and
DRA **  bits in the SCSI Caching mode page].

The patch is against lk 2.6.13-rc6 and includes the
SSU patch (but not the TUR patch).

It highlights several issues with libata:
   - may need hook so SCSI command specific logic can be
     executed _after_ a ATA command has completed successfully
   - one SCSI command can map to two or more
     ATA commands (e.g. a MODE SELECT caching
     page that changes the state of both WCE and DRA)
     I cannot see a graceful way to do this with libata.
   - patch generalizes error processing but probably
     not enough
   - may need lighter weight version of
     ata_dev_identify() when the "dev->id" array needs
     to be resync-ed (e.g. as required by the ATA information
     VPD page in sat-r05)

ChangeLog:
   - generalize SCSI error processing
   - add MODE SELECT (6 and 10) handling; active for
     WCE and DRA in Caching mode page
   - add block descriptor to MODE SENSE command
   - make various changes to sync with sat-r05 (as
     noted in source)
   - answer some "FIXME" questions in code and flag
     points for extra work


** Testing this patch highlighted a bug in sdparm effecting
   DRA (but not WCE). A beta version of sdparm-0.95 can be
   found at http://www.torque.net/sg/sdparm.html that fixes
   the problem.


Not to be applied to mainline kernel yet.

Doug Gilbert

[-- Attachment #2: libata_xd.diff --]
[-- Type: text/x-patch, Size: 21658 bytes --]

--- linux/include/linux/ata.h	2005-07-30 10:22:09.000000000 +1000
+++ linux/include/linux/ata.h2613rc4standby	2005-07-31 12:21:55.000000000 +1000
@@ -108,6 +108,8 @@
 
 	/* ATA device commands */
 	ATA_CMD_CHK_POWER	= 0xE5, /* check power mode */
+	ATA_CMD_STANDBY		= 0xE2, /* place in standby power mode */
+	ATA_CMD_IDLE		= 0xE3, /* place in idle power mode */
 	ATA_CMD_EDD		= 0x90,	/* execute device diagnostic */
 	ATA_CMD_FLUSH		= 0xE7,
 	ATA_CMD_FLUSH_EXT	= 0xEA,
--- linux/drivers/scsi/libata.h	2005-08-16 12:45:37.000000000 +1000
+++ linux/drivers/scsi/libata.h2613rc6xa	2005-08-17 23:01:08.000000000 +1000
@@ -69,21 +69,9 @@
 			        unsigned int buflen);
 extern unsigned int ata_scsiop_report_luns(struct ata_scsi_args *args, u8 *rbuf,
 				   unsigned int buflen);
-extern void ata_scsi_badcmd(struct scsi_cmnd *cmd,
-			    void (*done)(struct scsi_cmnd *),
-			    u8 asc, u8 ascq);
+extern void ata_scsi_set_sense(struct scsi_cmnd *cmd, u8 sk, u8 asc, u8 ascq);
 extern void ata_scsi_rbuf_fill(struct ata_scsi_args *args, 
                         unsigned int (*actor) (struct ata_scsi_args *args,
                                            u8 *rbuf, unsigned int buflen));
 
-static inline void ata_bad_scsiop(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *))
-{
-	ata_scsi_badcmd(cmd, done, 0x20, 0x00);
-}
-
-static inline void ata_bad_cdb(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *))
-{
-	ata_scsi_badcmd(cmd, done, 0x24, 0x00);
-}
-
 #endif /* __LIBATA_H__ */
--- linux/drivers/scsi/libata-scsi.c	2005-08-21 14:20:30.000000000 +1000
+++ linux/drivers/scsi/libata-scsi.c2613rc6xd	2005-08-21 14:16:52.000000000 +1000
@@ -37,6 +37,14 @@
 static struct ata_device *
 ata_scsi_find_dev(struct ata_port *ap, struct scsi_device *scsidev);
 
+static void ata_scsi_invalid_field(struct scsi_cmnd *cmd,
+				   void (*done)(struct scsi_cmnd *))
+{
+	ata_scsi_set_sense(cmd, ILLEGAL_REQUEST, 0x24, 0x0);
+	/* "Invalid field in cbd" */
+	done(cmd);
+}
+
 
 /**
  *	ata_std_bios_param - generic bios head/sector/cylinder calculator used by sd.
@@ -171,7 +179,6 @@
 {
 	struct scsi_cmnd *cmd = qc->scsicmd;
 	u8 err = 0;
-	unsigned char *sb = cmd->sense_buffer;
 	/* Based on the 3ware driver translation table */
 	static unsigned char sense_table[][4] = {
 		/* BBD|ECC|ID|MAR */
@@ -214,8 +221,6 @@
 	};
 	int i = 0;
 
-	cmd->result = SAM_STAT_CHECK_CONDITION;
-
 	/*
 	 *	Is this an error we can process/parse
 	 */
@@ -270,11 +275,9 @@
 		/* 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];
+			ata_scsi_set_sense(cmd, sense_table[i][1] /* sk */,
+					   sense_table[i][2] /* asc */,
+					   sense_table[i][3] /* ascq */ );
 			return;
 		}
 		i++;
@@ -289,11 +292,9 @@
 	{
 		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];
+			ata_scsi_set_sense(cmd, sense_table[i][1] /* sk */,
+					   sense_table[i][2] /* asc */,
+					   sense_table[i][3] /* ascq */ );
 			return;
 		}
 		i++;
@@ -302,15 +303,12 @@
 	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 == DMA_FROM_DEVICE) {
-		sb[12] = 0x11; /* "unrecovered read error" */
-		sb[13] = 0x04;
+		ata_scsi_set_sense(cmd, MEDIUM_ERROR, 0x11, 0x4);
+		/* "unrecovered read error" */
 	} else {
-		sb[12] = 0x0C; /* "write error -             */
-		sb[13] = 0x02; /*  auto-reallocation failed" */
+		ata_scsi_set_sense(cmd, MEDIUM_ERROR, 0xc, 0x2);
+		/* "write error - auto-reallocation failed" */
 	}
 }
 
@@ -391,6 +389,60 @@
 }
 
 /**
+ *	ata_scsi_start_stop_xlat - Translate SCSI START STOP UNIT command
+ *	@qc: Storage for translated ATA taskfile
+ *	@scsicmd: SCSI command to translate
+ *
+ *	Sets up an ATA taskfile to issue STANDBY (to stop) or READ VERIFY
+ *	(to start). Perhaps these commands should be preceded by
+ *	CHECK POWER MODE to see what power mode the device is already in.
+ *	[See SAT revision 5 at www.t10.org]
+ *
+ *	LOCKING:
+ *	spin_lock_irqsave(host_set lock)
+ *
+ *	RETURNS:
+ *	Zero on success, non-zero on error.
+ */
+
+static unsigned int ata_scsi_start_stop_xlat(struct ata_queued_cmd *qc,
+					     u8 *scsicmd)
+{
+	struct ata_taskfile *tf = &qc->tf;
+
+	tf->flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
+	tf->protocol = ATA_PROT_NODATA;
+	if (scsicmd[1] & 0x1) {
+		;	/* ignore IMMED bit, violates sat-r05 */
+	}
+	if (scsicmd[4] & 0x2)
+		return 1;	/* LOEJ bit set not supported */
+	if (((scsicmd[4] >> 4) & 0xf) != 0)
+		return 1;	/* power conditions not supported */
+	if (scsicmd[4] & 0x1) {
+		tf->nsect = 1;	/* 1 sector, lba=0 */
+		tf->lbah = 0x0;
+		tf->lbam = 0x0;
+		tf->lbal = 0x0;
+		tf->device |= ATA_LBA;
+		tf->command = ATA_CMD_VERIFY;	/* READ VERIFY */
+	} else {
+		tf->nsect = 0;	/* time period value (0 implies now) */
+		tf->command = ATA_CMD_STANDBY;
+		/* Consider: ATA STANDBY IMMEDIATE command */
+	}
+	/*
+	 * Standby and Idle condition timers could be implemented but that
+	 * would require libata to implement the Power condition mode page
+	 * and allow the user to change it. Changing mode pages requires
+	 * MODE SELECT to be implemented.
+	 */
+
+	return 0;
+}
+
+
+/**
  *	ata_scsi_flush_xlat - Translate SCSI SYNCHRONIZE CACHE command
  *	@qc: Storage for translated ATA taskfile
  *	@scsicmd: SCSI command to translate (ignored)
@@ -579,6 +631,10 @@
 	}
 
 	if (scsicmd[0] == READ_6 || scsicmd[0] == WRITE_6) {
+		/*
+		 * FIXME: for READ_6 and WRITE_6 (only)
+		 * transfer_len==0 -> 256 blocks !!
+		 */
 		qc->nsect = tf->nsect = scsicmd[4];
 		tf->lbal = scsicmd[3];
 		tf->lbam = scsicmd[2];
@@ -655,6 +711,10 @@
  *	This function sets up an ata_queued_cmd structure for the
  *	SCSI command, and sends that ata_queued_cmd to the hardware.
  *
+ *	xlat_func argument (actor) returns 0 if ok, 1 for invalid field
+ *	or 2 for a detected problem (or early termination) for which
+ *	the actor has already setup the result, sense key and sense data.
+ *
  *	LOCKING:
  *	spin_lock_irqsave(host_set lock)
  */
@@ -666,12 +726,13 @@
 {
 	struct ata_queued_cmd *qc;
 	u8 *scsicmd = cmd->cmnd;
+	unsigned int res;
 
 	VPRINTK("ENTER\n");
 
 	qc = ata_scsi_qc_new(ap, dev, cmd, done);
 	if (!qc)
-		return;
+		goto err_mem;
 
 	/* data is present; dma-map it */
 	if (cmd->sc_data_direction == DMA_FROM_DEVICE ||
@@ -679,7 +740,7 @@
 		if (unlikely(cmd->request_bufflen < 1)) {
 			printk(KERN_WARNING "ata%u(%u): WARNING: zero len r/w req\n",
 			       ap->id, dev->devno);
-			goto err_out;
+			goto err_did;
 		}
 
 		if (cmd->use_sg)
@@ -693,20 +754,38 @@
 
 	qc->complete_fn = ata_scsi_qc_complete;
 
-	if (xlat_func(qc, scsicmd))
-		goto err_out;
+	res = xlat_func(qc, scsicmd);
+	if (res == 1)
+		goto err_invalid;
+	else if (res > 1)
+		goto err_sense;
 
 	/* select device, send command to hardware */
 	if (ata_qc_issue(qc))
-		goto err_out;
+		goto err_did;
 
 	VPRINTK("EXIT\n");
 	return;
 
-err_out:
+err_invalid:
 	ata_qc_free(qc);
-	ata_bad_cdb(cmd, done);
-	DPRINTK("EXIT - badcmd\n");
+	ata_scsi_invalid_field(cmd, done);
+	DPRINTK("EXIT - invalid field\n");
+	return;
+
+err_sense:
+	ata_qc_free(qc);
+	done(cmd);
+	DPRINTK("EXIT - check condition (or do nothing)\n");
+	return;
+
+err_did:
+	ata_qc_free(qc);
+err_mem:
+	cmd->result = (DID_ERROR << 16);
+	done(cmd);
+	DPRINTK("EXIT - internal\n");
+	return;
 }
 
 /**
@@ -772,8 +851,9 @@
  *	Takes care of the hard work of simulating a SCSI command...
  *	Mapping the response buffer, calling the command's handler,
  *	and handling the handler's return value.  This return value
- *	indicates whether the handler wishes the SCSI command to be
- *	completed successfully, or not.
+ *	(i.e. @actor) is 0 for success, 1 for "Invalid field in cdb"
+ *	needs to be set, 2 for sense data and status already set
+ *	by actor.
  *
  *	LOCKING:
  *	spin_lock_irqsave(host_set lock)
@@ -792,11 +872,14 @@
 	rc = actor(args, rbuf, buflen);
 	ata_scsi_rbuf_put(cmd, rbuf);
 
-	if (rc)
-		ata_bad_cdb(cmd, args->done);
-	else {
+	if (rc == 0) {
 		cmd->result = SAM_STAT_GOOD;
 		args->done(cmd);
+	} else if (rc == 1)
+		ata_scsi_invalid_field(cmd, args->done);
+	else {
+		/* assume sense data and status set by actor */
+		args->done(cmd);
 	}
 }
 
@@ -875,7 +958,8 @@
 	const u8 pages[] = {
 		0x00,	/* page 0x00, this page */
 		0x80,	/* page 0x80, unit serial no page */
-		0x83	/* page 0x83, device ident page */
+		0x83,	/* page 0x83, device ident page */
+		/* page 0x89, ATA information page */
 	};
 	rbuf[3] = sizeof(pages);	/* number of supported EVPD pages */
 
@@ -994,6 +1078,13 @@
 	*ptr_io = ptr;
 }
 
+static const u8 def_caching_mpage[] = {
+		0x8,				/* page code */
+		0x12,				/* page length */
+		0, 0, 0, 0, 0, 0, 0, 0, 0, 0,	/* 10 zeroes */
+		0, 0, 0, 0, 0, 0, 0, 0		/* 8 zeroes */
+	};
+
 /**
  *	ata_msense_caching - Simulate MODE SENSE caching info page
  *	@id: device IDENTIFY data
@@ -1011,13 +1102,9 @@
 static unsigned int ata_msense_caching(u16 *id, u8 **ptr_io,
 				       const u8 *last)
 {
-	u8 page[] = {
-		0x8,				/* page code */
-		0x12,				/* page length */
-		0, 0, 0, 0, 0, 0, 0, 0, 0, 0,	/* 10 zeroes */
-		0, 0, 0, 0, 0, 0, 0, 0		/* 8 zeroes */
-	};
+	u8 page[0x12 + 2];
 
+	memcpy(page, def_caching_mpage, sizeof(page));
 	if (ata_id_wcache_enabled(id))
 		page[2] |= (1 << 2);	/* write cache enable */
 	if (!ata_id_rahead_enabled(id))
@@ -1027,6 +1114,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 +1135,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);
 }
+	
+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 +1161,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);
 }
 
 /**
@@ -1093,28 +1182,52 @@
 				  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) */
+	u8 pg, spg;
+	unsigned int ebd, page_control, six_byte, output_len, alloc_len, minlen;
 
 	VPRINTK("ENTER\n");
 
 	six_byte = (scsicmd[0] == MODE_SENSE);
+	ebd = !(scsicmd[1] & 0x8);	/* dbd bit inverted == edb */
 
-	/* we only support saved and current values (which we treat
-	 * in the same manner)
+	/*
+	 * LLBA bit in MS(10) ignored (permitted)
+	 * Only support current values (sat-r05)
 	 */
 	page_control = scsicmd[2] >> 6;
-	if ((page_control != 0) && (page_control != 3))
-		return 1;
+	if (page_control != 0) {
+		if (page_control == 3) {
+			ata_scsi_set_sense(args->cmd,
+					   ILLEGAL_REQUEST, 0x39, 0x0);
+			/* "Saving parameters not supported" */
+			return 2;
+		} else
+			return 1;
+	}
 
-	if (six_byte)
-		output_len = 4;
-	else
-		output_len = 8;
+	if (six_byte) {
+		output_len = 4 + (ebd ? 8 : 0);
+		alloc_len = scsicmd[4];
+	} else {
+		output_len = 8 + (ebd ? 8 : 0);
+		alloc_len = (scsicmd[7] << 8) + scsicmd[8];
+	}
+	minlen = (alloc_len < buflen) ? alloc_len : buflen;
 
 	p = rbuf + output_len;
-	last = rbuf + buflen - 1;
+	last = rbuf + minlen - 1;
 
-	switch(scsicmd[2] & 0x3f) {
+	pg = scsicmd[2] & 0x3f;
+	spg = scsicmd[3];
+	/* No mode subpages supported (yet) but asking for _all_
+	 * subpages may be valid
+	 */
+	if (spg && (spg != 0xff))
+		return 1;
+
+	switch(pg) {
 	case 0x01:		/* r/w error recovery */
 		output_len += ata_msense_rw_recovery(&p, last);
 		break;
@@ -1128,6 +1241,8 @@
 		break;
 		}
 
+	/* case 0x1c: Informational Exception Control mode page */
+
 	case 0x3f:		/* all pages */
 		output_len += ata_msense_rw_recovery(&p, last);
 		output_len += ata_msense_caching(args->id, &p, last);
@@ -1138,13 +1253,30 @@
 		return 1;
 	}
 
+	if (minlen < 1)
+		return 0;
 	if (six_byte) {
 		output_len--;
 		rbuf[0] = output_len;
+		if (ebd) {
+			if (minlen > 3)
+				rbuf[3] = sizeof(sat_blk_desc);
+			if (minlen > 11)
+				memcpy(rbuf + 4, sat_blk_desc,
+				       sizeof(sat_blk_desc));
+		}
 	} else {
 		output_len -= 2;
 		rbuf[0] = output_len >> 8;
-		rbuf[1] = output_len;
+		if (minlen > 1)
+			rbuf[1] = output_len;
+		if (ebd) {
+			if (minlen > 7)
+				rbuf[7] = sizeof(sat_blk_desc);
+			if (minlen > 15)
+				memcpy(rbuf + 8, sat_blk_desc,
+				       sizeof(sat_blk_desc));
+		}
 	}
 
 	return 0;
@@ -1237,6 +1369,151 @@
 }
 
 /**
+ *	ata_scsi_mode_select_xlat - Translate SCSI MODE SELECT commands
+ *	@qc: Storage for translated ATA taskfile
+ *	@scsicmd: SCSI command to translate
+ *
+ *	Sets up an ATA taskfile to issue SET FEATURES, if required.
+ *
+ *	LOCKING:
+ *	spin_lock_irqsave(host_set lock)
+ *
+ *	RETURNS:
+ *	Zero on success, non-zero on error.
+ */
+
+static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc,
+					      u8 *scsicmd)
+{
+	struct ata_taskfile *tf = &qc->tf;
+	struct scsi_cmnd *cmd = qc->scsicmd;
+	unsigned int param_len, ten, buflen, minlen, bdlen, offset;
+	unsigned int pglen, spf, mpg, wce, dra;
+	u8 *rbuf;
+	u8 apage[48];
+
+	tf->flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
+	tf->protocol = ATA_PROT_NODATA;
+	cmd->result = SAM_STAT_GOOD;
+	if (! (scsicmd[1] & 0x10))
+		return 1;	/* require PF bit set */
+	ten = (scsicmd[0] == MODE_SELECT_10) ? 1 : 0;
+	param_len =  ten ? ((scsicmd[7] << 8) + scsicmd[8]) : scsicmd[4];
+	if (param_len < (ten ? 8 : 4))
+		return 2;
+        buflen = ata_scsi_rbuf_get(cmd, &rbuf);
+	minlen = (param_len < buflen) ? param_len : buflen;
+	bdlen = ten ? ((rbuf[6] << 8) + rbuf[7]) : rbuf[3];
+	offset = (ten ? 8 : 4) + bdlen;
+	if (minlen > offset) {
+		memset(apage, 0, sizeof(apage));
+		pglen = minlen - offset;
+		pglen = (pglen < sizeof(apage)) ? pglen : sizeof(apage);
+		memcpy(apage, rbuf + offset, pglen);
+	}
+	ata_scsi_rbuf_put(cmd, rbuf);
+	if (minlen == offset)
+		return 2;
+	if (minlen < offset)
+		goto bad_param;
+	minlen -= offset;
+	spf = !!(apage[0] & 0x40);
+	mpg = apage[0] & 0x3f;
+	/* mspg = spf ? apage[1] : 0; */
+	if (spf)
+		goto bad_param;
+	else
+		pglen = apage[1];
+		if (pglen + 2 < minlen)
+			goto bad_param;
+	switch (mpg) {
+	case 0x1:
+		if (pglen != def_rw_recovery_mpage[1])
+			goto bad_param;
+		break;
+	case 0x8:
+		if (pglen != def_caching_mpage[1])
+			goto bad_param;
+		wce = !!(apage[2] & 0x4);
+		dra = !!(apage[12] & 0x20);
+		if (wce != !!(ata_id_wcache_enabled(qc->dev->id))) {
+			/* write(back) cache */
+			if (wce)
+				tf->feature = 0x2;	/* WCE -> enable */
+			else
+				tf->feature = 0x82;	/* disable */
+			tf->command = ATA_CMD_SET_FEATURES;
+			/* <<< If success, should dev->id be updated? >>> */
+			/* vvvvvvv   start of dubious code   vvvvvvvvvv */
+			if (wce)
+				qc->dev->id[85] |= 0x20;
+			else
+				qc->dev->id[85] &= ~0x20;
+			/* vvvvvvv   end of dubious code   vvvvvvvvvv */
+			return 0;
+		}
+/* FIXME: we may want to issue two SET FEATURES commands here */
+		if (dra != !(ata_id_rahead_enabled(qc->dev->id))) {
+			/* read look ahead */
+			if (dra)
+				tf->feature = 0x55;	/* DRA -> disable */
+			else
+				tf->feature = 0xaa;	/* enable */
+			tf->command = ATA_CMD_SET_FEATURES;
+			/* <<< If success, should dev->id be updated? >>> */
+			/* vvvvvvv   start of dubious code   vvvvvvvvvv */
+			if (dra)
+				qc->dev->id[85] &= ~0x40;
+			else
+				qc->dev->id[85] |= 0x40;
+			/* vvvvvvv   end of dubious code   vvvvvvvvvv */
+			return 0;
+		} 
+		break;
+	case 0xa:
+		if (pglen != def_control_mpage[1])
+			goto bad_param;
+		break;
+	default:
+		goto bad_param;
+	}
+	/* GOOD response, no ATA command issued */
+	return 2;
+
+bad_param:
+	ata_scsi_set_sense(cmd, ILLEGAL_REQUEST, 0x26, 0x0);
+	/* "Invalid field in parameter list" */
+	return 2;
+}
+
+/**
+ *	ata_scsi_set_sense - Set SCSI sense data and status
+ *	@cmd: SCSI request to be handled
+ *	@sk: SCSI-defined sense key
+ *	@asc: SCSI-defined additional sense code
+ *	@ascq: SCSI-defined additional sense code qualifier
+ *
+ *	Helper function that builds a valid fixed format, current
+ *	response code and the given sense key (sk), additional sense
+ *	code (asc) and additional sense code qualifier (ascq) with
+ *	a SCSI command status of %SAM_STAT_CHECK_CONDITION .
+ *
+ *	LOCKING:
+ *	Not required
+ */
+
+void ata_scsi_set_sense(struct scsi_cmnd *cmd, u8 sk, u8 asc, u8 ascq)
+{
+	cmd->result = SAM_STAT_CHECK_CONDITION;
+
+	cmd->sense_buffer[0] = 0x70;	/* fixed format, current */
+	cmd->sense_buffer[2] = sk;
+	cmd->sense_buffer[7] = 18 - 8;	/* additional sense length */
+	cmd->sense_buffer[12] = asc;
+	cmd->sense_buffer[13] = ascq;
+}
+
+/**
  *	ata_scsi_badcmd - End a SCSI request with an error
  *	@cmd: SCSI request to be handled
  *	@done: SCSI command completion function
@@ -1254,13 +1531,7 @@
 void ata_scsi_badcmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *), u8 asc, u8 ascq)
 {
 	DPRINTK("ENTER\n");
-	cmd->result = SAM_STAT_CHECK_CONDITION;
-
-	cmd->sense_buffer[0] = 0x70;
-	cmd->sense_buffer[2] = ILLEGAL_REQUEST;
-	cmd->sense_buffer[7] = 14 - 8;	/* addnl. sense len. FIXME: correct? */
-	cmd->sense_buffer[12] = asc;
-	cmd->sense_buffer[13] = ascq;
+	ata_scsi_set_sense(cmd, ILLEGAL_REQUEST, asc, ascq);
 
 	done(cmd);
 }
@@ -1414,9 +1685,10 @@
  *	Pointer to translation function if possible, %NULL if not.
  */
 
-static inline ata_xlat_func_t ata_get_xlat_func(struct ata_device *dev, u8 cmd)
+static inline ata_xlat_func_t ata_get_xlat_func(struct ata_device *dev,
+						u8 *cmd)
 {
-	switch (cmd) {
+	switch (cmd[0]) {
 	case READ_6:
 	case READ_10:
 	case READ_16:
@@ -1426,6 +1698,16 @@
 	case WRITE_16:
 		return ata_scsi_rw_xlat;
 
+	case INQUIRY:
+		if (((cmd[1] & 0x3) == 0x1) && (cmd[2] == 0x89)) {
+			/* needs to re-issue an ATA IDENTIFY cmd
+			 * and refill dev->id array. Then put this
+			 * data and the signature in the vpd response.
+			 */
+			/* return ata_scsi_ata_info_vpd_xlat; ?? */
+		}
+		break;
+
 	case SYNCHRONIZE_CACHE:
 		if (ata_try_flush_cache(dev))
 			return ata_scsi_flush_xlat;
@@ -1434,6 +1716,13 @@
 	case VERIFY:
 	case VERIFY_16:
 		return ata_scsi_verify_xlat;
+
+	case START_STOP:
+		return ata_scsi_start_stop_xlat;
+
+	case MODE_SELECT:
+	case MODE_SELECT_10:
+		return ata_scsi_mode_select_xlat;
 	}
 
 	return NULL;
@@ -1501,7 +1790,7 @@
 
 	if (dev->class == ATA_DEV_ATA) {
 		ata_xlat_func_t xlat_func = ata_get_xlat_func(dev,
-							      cmd->cmnd[0]);
+							      cmd->cmnd);
 
 		if (xlat_func)
 			ata_scsi_translate(ap, dev, cmd, done, xlat_func);
@@ -1547,12 +1836,13 @@
 		case TEST_UNIT_READY:
 		case FORMAT_UNIT:		/* FIXME: correct? */
 		case SEND_DIAGNOSTIC:		/* FIXME: correct? */
+			/* sat-r05: both should be translated */
 			ata_scsi_rbuf_fill(&args, ata_scsiop_noop);
 			break;
 
 		case INQUIRY:
 			if (scsicmd[1] & 2)	           /* is CmdDt set?  */
-				ata_bad_cdb(cmd, done);
+				ata_scsi_invalid_field(cmd, done);
 			else if ((scsicmd[1] & 1) == 0)    /* is EVPD clear? */
 				ata_scsi_rbuf_fill(&args, ata_scsiop_inq_std);
 			else if (scsicmd[2] == 0x00)
@@ -1562,7 +1852,7 @@
 			else if (scsicmd[2] == 0x83)
 				ata_scsi_rbuf_fill(&args, ata_scsiop_inq_83);
 			else
-				ata_bad_cdb(cmd, done);
+				ata_scsi_invalid_field(cmd, done);
 			break;
 
 		case MODE_SENSE:
@@ -1570,11 +1860,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;
@@ -1583,7 +1868,7 @@
 			if ((scsicmd[1] & 0x1f) == SAI_READ_CAPACITY_16)
 				ata_scsi_rbuf_fill(&args, ata_scsiop_read_cap);
 			else
-				ata_bad_cdb(cmd, done);
+				ata_scsi_invalid_field(cmd, done);
 			break;
 
 		case REPORT_LUNS:
@@ -1595,7 +1880,9 @@
 
 		/* all other commands */
 		default:
-			ata_bad_scsiop(cmd, done);
+			ata_scsi_set_sense(cmd, ILLEGAL_REQUEST, 0x20, 0x0);
+			/* "Invalid command operation code" */
+			done(cmd);
 			break;
 	}
 }

Re: [PATCH] libata: write cache and read ahead

[Jeff Garzik]

Douglas Gilbert wrote:
> The attachment is for discussion. It adds MODE SELECT
> support to libata allowing the write(back) cache and
> read ahead to be manipulated by users [i.e. the WCE and
> DRA **  bits in the SCSI Caching mode page].

In general I approve of this concept.  I've wanted libata to do MODE 
SELECT for a while, but have been too slack to worry about it myself.


> The patch is against lk 2.6.13-rc6 and includes the
> SSU patch (but not the TUR patch).
> 
> It highlights several issues with libata:
>    - may need hook so SCSI command specific logic can be
>      executed _after_ a ATA command has completed successfully

You should just be able to do this via ata_queued_cmd completion 
callback.  See Tejun Heo's "mod15write quirk fix" patch for an example.


>    - one SCSI command can map to two or more
>      ATA commands (e.g. a MODE SELECT caching
>      page that changes the state of both WCE and DRA)
>      I cannot see a graceful way to do this with libata.

ditto above comment


>    - patch generalizes error processing but probably
>      not enough

what, ata_scsi_invalid_field() and ata_scsi_set_sense() use?  Those look 
like a good start to me.


>    - may need lighter weight version of
>      ata_dev_identify() when the "dev->id" array needs
>      to be resync-ed (e.g. as required by the ATA information
>      VPD page in sat-r05)

well no implementation will be completely "light"...  ideally each time 
you attempt to change the configuration, you should IDENTIFY DEVICE 
again to make sure that you achieved the desired effect.  We should do 
this after SET FEATURES - XFER MODE in fact, but don't ATM.


> ChangeLog:
>    - add MODE SELECT (6 and 10) handling; active for
>      WCE and DRA in Caching mode page
>    - add block descriptor to MODE SENSE command
>    - make various changes to sync with sat-r05 (as
>      noted in source)
>    - answer some "FIXME" questions in code and flag
>      points for extra work
> Not to be applied to mainline kernel yet.

As bits become ready for mainline, please submit them as separate 
patches.  For example, you could separate out simple stuff like 
ata_scsi_set_sense() use, and go ahead and get that into the kernel.


> @@ -579,6 +631,10 @@
>  	}
>  
>  	if (scsicmd[0] == READ_6 || scsicmd[0] == WRITE_6) {
> +		/*
> +		 * FIXME: for READ_6 and WRITE_6 (only)
> +		 * transfer_len==0 -> 256 blocks !!
> +		 */
>  		qc->nsect = tf->nsect = scsicmd[4];
>  		tf->lbal = scsicmd[3];
>  		tf->lbam = scsicmd[2];

Good catch.  Interested in submitting a fix for this sooner rather than 
later?


> @@ -994,6 +1078,13 @@
>  	*ptr_io = ptr;
>  }
>  
> +static const u8 def_caching_mpage[] = {
> +		0x8,				/* page code */
> +		0x12,				/* page length */
> +		0, 0, 0, 0, 0, 0, 0, 0, 0, 0,	/* 10 zeroes */
> +		0, 0, 0, 0, 0, 0, 0, 0		/* 8 zeroes */
> +	};
> +
>  /**
>   *	ata_msense_caching - Simulate MODE SENSE caching info page
>   *	@id: device IDENTIFY data
> @@ -1011,13 +1102,9 @@
>  static unsigned int ata_msense_caching(u16 *id, u8 **ptr_io,
>  				       const u8 *last)
>  {
> -	u8 page[] = {
> -		0x8,				/* page code */
> -		0x12,				/* page length */
> -		0, 0, 0, 0, 0, 0, 0, 0, 0, 0,	/* 10 zeroes */
> -		0, 0, 0, 0, 0, 0, 0, 0		/* 8 zeroes */
> -	};
> +	u8 page[0x12 + 2];
>  
> +	memcpy(page, def_caching_mpage, sizeof(page));
>  	if (ata_id_wcache_enabled(id))
>  		page[2] |= (1 << 2);	/* write cache enable */
>  	if (!ata_id_rahead_enabled(id))

Don't define the size of automatic variable 'page' with numeric 
constants.  That sort of method is very error-prone.


> @@ -1093,28 +1182,52 @@
>  				  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) */
> +	u8 pg, spg;
> +	unsigned int ebd, page_control, six_byte, output_len, alloc_len, minlen;
>  
>  	VPRINTK("ENTER\n");
>  
>  	six_byte = (scsicmd[0] == MODE_SENSE);
> +	ebd = !(scsicmd[1] & 0x8);	/* dbd bit inverted == edb */
>  
> -	/* we only support saved and current values (which we treat
> -	 * in the same manner)
> +	/*
> +	 * LLBA bit in MS(10) ignored (permitted)
> +	 * Only support current values (sat-r05)
>  	 */
>  	page_control = scsicmd[2] >> 6;
> -	if ((page_control != 0) && (page_control != 3))
> -		return 1;
> +	if (page_control != 0) {
> +		if (page_control == 3) {
> +			ata_scsi_set_sense(args->cmd,
> +					   ILLEGAL_REQUEST, 0x39, 0x0);
> +			/* "Saving parameters not supported" */
> +			return 2;
> +		} else
> +			return 1;
> +	}

[style] I would probably use a 'switch' statement here


> @@ -1237,6 +1369,151 @@
>  }
>  
>  /**
> + *	ata_scsi_mode_select_xlat - Translate SCSI MODE SELECT commands
> + *	@qc: Storage for translated ATA taskfile
> + *	@scsicmd: SCSI command to translate
> + *
> + *	Sets up an ATA taskfile to issue SET FEATURES, if required.
> + *
> + *	LOCKING:
> + *	spin_lock_irqsave(host_set lock)
> + *
> + *	RETURNS:
> + *	Zero on success, non-zero on error.
> + */
> +
> +static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc,
> +					      u8 *scsicmd)
> +{
> +	struct ata_taskfile *tf = &qc->tf;
> +	struct scsi_cmnd *cmd = qc->scsicmd;
> +	unsigned int param_len, ten, buflen, minlen, bdlen, offset;
> +	unsigned int pglen, spf, mpg, wce, dra;
> +	u8 *rbuf;
> +	u8 apage[48];

Overall, add some blank lines to the code below, to break it up a bit. 
Suggested places to break up code into "paragraphs" include 'return' 
statements or 'if' tests with long stretches of code inside.


> +	tf->flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
> +	tf->protocol = ATA_PROT_NODATA;
> +	cmd->result = SAM_STAT_GOOD;
> +	if (! (scsicmd[1] & 0x10))
> +		return 1;	/* require PF bit set */
> +	ten = (scsicmd[0] == MODE_SELECT_10) ? 1 : 0;
> +	param_len =  ten ? ((scsicmd[7] << 8) + scsicmd[8]) : scsicmd[4];
> +	if (param_len < (ten ? 8 : 4))
> +		return 2;
> +        buflen = ata_scsi_rbuf_get(cmd, &rbuf);

indentation problem

> +	minlen = (param_len < buflen) ? param_len : buflen;
> +	bdlen = ten ? ((rbuf[6] << 8) + rbuf[7]) : rbuf[3];
> +	offset = (ten ? 8 : 4) + bdlen;
> +	if (minlen > offset) {
> +		memset(apage, 0, sizeof(apage));
> +		pglen = minlen - offset;
> +		pglen = (pglen < sizeof(apage)) ? pglen : sizeof(apage);
> +		memcpy(apage, rbuf + offset, pglen);
> +	}
> +	ata_scsi_rbuf_put(cmd, rbuf);
> +	if (minlen == offset)
> +		return 2;
> +	if (minlen < offset)
> +		goto bad_param;
> +	minlen -= offset;
> +	spf = !!(apage[0] & 0x40);

'!!' operation not needed.


> +	mpg = apage[0] & 0x3f;
> +	/* mspg = spf ? apage[1] : 0; */
> +	if (spf)
> +		goto bad_param;
> +	else
> +		pglen = apage[1];
> +		if (pglen + 2 < minlen)
> +			goto bad_param;
> +	switch (mpg) {
> +	case 0x1:
> +		if (pglen != def_rw_recovery_mpage[1])
> +			goto bad_param;
> +		break;
> +	case 0x8:
> +		if (pglen != def_caching_mpage[1])
> +			goto bad_param;
> +		wce = !!(apage[2] & 0x4);
> +		dra = !!(apage[12] & 0x20);
> +		if (wce != !!(ata_id_wcache_enabled(qc->dev->id))) {
> +			/* write(back) cache */
> +			if (wce)
> +				tf->feature = 0x2;	/* WCE -> enable */
> +			else
> +				tf->feature = 0x82;	/* disable */

define named constants for these numeric constants, include/linux/ata.h


> +			tf->command = ATA_CMD_SET_FEATURES;
> +			/* <<< If success, should dev->id be updated? >>> */
> +			/* vvvvvvv   start of dubious code   vvvvvvvvvv */
> +			if (wce)
> +				qc->dev->id[85] |= 0x20;
> +			else
> +				qc->dev->id[85] &= ~0x20;
> +			/* vvvvvvv   end of dubious code   vvvvvvvvvv */
> +			return 0;

SET FEATURES should be followed by a second command, IDENTIFY [PACKET] 
DEVICE, to update dev->id and also to verify that the device set the 
feature as requested.


> +/* FIXME: we may want to issue two SET FEATURES commands here */
> +		if (dra != !(ata_id_rahead_enabled(qc->dev->id))) {

bug: need one more '!' AFAICS


> +			/* read look ahead */
> +			if (dra)
> +				tf->feature = 0x55;	/* DRA -> disable */
> +			else
> +				tf->feature = 0xaa;	/* enable */
> +			tf->command = ATA_CMD_SET_FEATURES;
> +			/* <<< If success, should dev->id be updated? >>> */
> +			/* vvvvvvv   start of dubious code   vvvvvvvvvv */
> +			if (dra)
> +				qc->dev->id[85] &= ~0x40;
> +			else
> +				qc->dev->id[85] |= 0x40;
> +			/* vvvvvvv   end of dubious code   vvvvvvvvvv */
> +			return 0;

same comment as with 'wce'.  if both dra and wce change, then four 
commands should follow: SET FEATURES, IDENTIFY DEVICE, SET FEATURES, 
IDENTIFY DEVICE.  If you wanted to be optimal, you could expend more 
[code] effort and only issue SF, SF, ID.


> +	case 0xa:
> +		if (pglen != def_control_mpage[1])
> +			goto bad_param;
> +		break;
> +	default:
> +		goto bad_param;
> +	}
> +	/* GOOD response, no ATA command issued */
> +	return 2;
> +
> +bad_param:
> +	ata_scsi_set_sense(cmd, ILLEGAL_REQUEST, 0x26, 0x0);
> +	/* "Invalid field in parameter list" */
> +	return 2;
> +}
> +
> +/**
> + *	ata_scsi_set_sense - Set SCSI sense data and status
> + *	@cmd: SCSI request to be handled
> + *	@sk: SCSI-defined sense key
> + *	@asc: SCSI-defined additional sense code
> + *	@ascq: SCSI-defined additional sense code qualifier
> + *
> + *	Helper function that builds a valid fixed format, current
> + *	response code and the given sense key (sk), additional sense
> + *	code (asc) and additional sense code qualifier (ascq) with
> + *	a SCSI command status of %SAM_STAT_CHECK_CONDITION .
> + *
> + *	LOCKING:
> + *	Not required
> + */
> +
> +void ata_scsi_set_sense(struct scsi_cmnd *cmd, u8 sk, u8 asc, u8 ascq)
> +{
> +	cmd->result = SAM_STAT_CHECK_CONDITION;
> +
> +	cmd->sense_buffer[0] = 0x70;	/* fixed format, current */
> +	cmd->sense_buffer[2] = sk;
> +	cmd->sense_buffer[7] = 18 - 8;	/* additional sense length */
> +	cmd->sense_buffer[12] = asc;
> +	cmd->sense_buffer[13] = ascq;
> +}
> +
> +/**
>   *	ata_scsi_badcmd - End a SCSI request with an error
>   *	@cmd: SCSI request to be handled
>   *	@done: SCSI command completion function
> @@ -1254,13 +1531,7 @@
>  void ata_scsi_badcmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *), u8 asc, u8 ascq)
>  {
>  	DPRINTK("ENTER\n");
> -	cmd->result = SAM_STAT_CHECK_CONDITION;
> -
> -	cmd->sense_buffer[0] = 0x70;
> -	cmd->sense_buffer[2] = ILLEGAL_REQUEST;
> -	cmd->sense_buffer[7] = 14 - 8;	/* addnl. sense len. FIXME: correct? */
> -	cmd->sense_buffer[12] = asc;
> -	cmd->sense_buffer[13] = ascq;
> +	ata_scsi_set_sense(cmd, ILLEGAL_REQUEST, asc, ascq);
>  
>  	done(cmd);
>  }
> @@ -1414,9 +1685,10 @@
>   *	Pointer to translation function if possible, %NULL if not.
>   */
>  
> -static inline ata_xlat_func_t ata_get_xlat_func(struct ata_device *dev, u8 cmd)
> +static inline ata_xlat_func_t ata_get_xlat_func(struct ata_device *dev,
> +						u8 *cmd)
>  {
> -	switch (cmd) {
> +	switch (cmd[0]) {
>  	case READ_6:
>  	case READ_10:
>  	case READ_16:
> @@ -1426,6 +1698,16 @@
>  	case WRITE_16:
>  		return ata_scsi_rw_xlat;
>  
> +	case INQUIRY:
> +		if (((cmd[1] & 0x3) == 0x1) && (cmd[2] == 0x89)) {

what do these magic numbers indicate?


> +			/* needs to re-issue an ATA IDENTIFY cmd
> +			 * and refill dev->id array. Then put this
> +			 * data and the signature in the vpd response.
> +			 */
> +			/* return ata_scsi_ata_info_vpd_xlat; ?? */

IFF your diagnosis is correct, then I agree with the proposed solution.

The rest looks OK.

	Jeff

Re: [PATCH] libata: write cache and read ahead

[Douglas Gilbert]

NOTIFYJeff Garzik wrote:
> Douglas Gilbert wrote:
> 
>> The attachment is for discussion. It adds MODE SELECT
>> support to libata allowing the write(back) cache and
>> read ahead to be manipulated by users [i.e. the WCE and
>> DRA **  bits in the SCSI Caching mode page].
> 
> 
> In general I approve of this concept.  I've wanted libata to do MODE
> SELECT for a while, but have been too slack to worry about it myself.

Jeff,
I was surprised how much code needed changing.
With MODE SELECT's issues with libata addressed
various other SAT "extras" should be much easier
to implement. That should make libata more attractive
as a SAT layer for SAS LLDDs (that don't do it already
in firmware).

With repect to my libata TEST UNIT READY patch there
is a good chance that can be dropped. The feedback
from the t10 reflector seems to indicate the current
definition may be changed, unfortunately the proposed
changes may require some SAT state information being
held for each libata device (e.g. pseudo "STOPPED"
state to mimic SBC-2). It also seems some power up
issues may need to involve the transport layer
for resolution. For example, SSP's additional sense code
of LOGICAL UNIT NOT READY, NOTIFY (ENABLE SPINUP)
REQUIRED may also need to be issued by libata.

>> The patch is against lk 2.6.13-rc6 and includes the
>> SSU patch (but not the TUR patch).
>>
>> It highlights several issues with libata:
>>    - may need hook so SCSI command specific logic can be
>>      executed _after_ a ATA command has completed successfully
> 
> 
> You should just be able to do this via ata_queued_cmd completion
> callback.  See Tejun Heo's "mod15write quirk fix" patch for an example.

Fine, I'll look at that patch.

>>    - one SCSI command can map to two or more
>>      ATA commands (e.g. a MODE SELECT caching
>>      page that changes the state of both WCE and DRA)
>>      I cannot see a graceful way to do this with libata.
> 
> 
> ditto above comment
> 
> 
>>    - patch generalizes error processing but probably
>>      not enough
> 
> 
> what, ata_scsi_invalid_field() and ata_scsi_set_sense() use?  Those look
> like a good start to me.
> 
> 
>>    - may need lighter weight version of
>>      ata_dev_identify() when the "dev->id" array needs
>>      to be resync-ed (e.g. as required by the ATA information
>>      VPD page in sat-r05)
> 
> 
> well no implementation will be completely "light"...  ideally each time
> you attempt to change the configuration, you should IDENTIFY DEVICE
> again to make sure that you achieved the desired effect.  We should do
> this after SET FEATURES - XFER MODE in fact, but don't ATM.

Another potential issue that I didn't mention was support
for the "IMMED" bit in commands like START STOP UNIT; i.e.
issue the corresponding ATA command but return to the caller
without waiting for the ATA command to finish.

>> ChangeLog:
>>    - add MODE SELECT (6 and 10) handling; active for
>>      WCE and DRA in Caching mode page
>>    - add block descriptor to MODE SENSE command
>>    - make various changes to sync with sat-r05 (as
>>      noted in source)
>>    - answer some "FIXME" questions in code and flag
>>      points for extra work
>> Not to be applied to mainline kernel yet.
> 
> 
> As bits become ready for mainline, please submit them as separate
> patches.  For example, you could separate out simple stuff like
> ata_scsi_set_sense() use, and go ahead and get that into the kernel.

Ok, I should have a stripped down patch in a few hours.

>> @@ -579,6 +631,10 @@
>>      }
>>  
>>      if (scsicmd[0] == READ_6 || scsicmd[0] == WRITE_6) {
>> +        /*
>> +         * FIXME: for READ_6 and WRITE_6 (only)
>> +         * transfer_len==0 -> 256 blocks !!
>> +         */
>>          qc->nsect = tf->nsect = scsicmd[4];
>>          tf->lbal = scsicmd[3];
>>          tf->lbam = scsicmd[2];
> 
> 
> Good catch.  Interested in submitting a fix for this sooner rather than
> later?

It will be in that patch.

>> @@ -994,6 +1078,13 @@
>>      *ptr_io = ptr;
>>  }
>>  
>> +static const u8 def_caching_mpage[] = {
>> +        0x8,                /* page code */
>> +        0x12,                /* page length */
>> +        0, 0, 0, 0, 0, 0, 0, 0, 0, 0,    /* 10 zeroes */
>> +        0, 0, 0, 0, 0, 0, 0, 0        /* 8 zeroes */
>> +    };
>> +
>>  /**
>>   *    ata_msense_caching - Simulate MODE SENSE caching info page
>>   *    @id: device IDENTIFY data
>> @@ -1011,13 +1102,9 @@
>>  static unsigned int ata_msense_caching(u16 *id, u8 **ptr_io,
>>                         const u8 *last)
>>  {
>> -    u8 page[] = {
>> -        0x8,                /* page code */
>> -        0x12,                /* page length */
>> -        0, 0, 0, 0, 0, 0, 0, 0, 0, 0,    /* 10 zeroes */
>> -        0, 0, 0, 0, 0, 0, 0, 0        /* 8 zeroes */
>> -    };
>> +    u8 page[0x12 + 2];
>>  
>> +    memcpy(page, def_caching_mpage, sizeof(page));
>>      if (ata_id_wcache_enabled(id))
>>          page[2] |= (1 << 2);    /* write cache enable */
>>      if (!ata_id_rahead_enabled(id))
> 
> 
> Don't define the size of automatic variable 'page' with numeric
> constants.  That sort of method is very error-prone.

Showing my aversion to writing something like:
   u8 page[sizeof(def_caching_mpage)];

Using sizeof in an array declaration seems to be a grey area in C,
but gcc accepts it so perhaps I shouldn't worry.

>> @@ -1093,28 +1182,52 @@
>>                    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) */
>> +    u8 pg, spg;
>> +    unsigned int ebd, page_control, six_byte, output_len, alloc_len,
>> minlen;
>>  
>>      VPRINTK("ENTER\n");
>>  
>>      six_byte = (scsicmd[0] == MODE_SENSE);
>> +    ebd = !(scsicmd[1] & 0x8);    /* dbd bit inverted == edb */
>>  
>> -    /* we only support saved and current values (which we treat
>> -     * in the same manner)
>> +    /*
>> +     * LLBA bit in MS(10) ignored (permitted)
>> +     * Only support current values (sat-r05)
>>       */
>>      page_control = scsicmd[2] >> 6;
>> -    if ((page_control != 0) && (page_control != 3))
>> -        return 1;
>> +    if (page_control != 0) {
>> +        if (page_control == 3) {
>> +            ata_scsi_set_sense(args->cmd,
>> +                       ILLEGAL_REQUEST, 0x39, 0x0);
>> +            /* "Saving parameters not supported" */
>> +            return 2;
>> +        } else
>> +            return 1;
>> +    }
> 
> 
> [style] I would probably use a 'switch' statement here

ok

>> @@ -1237,6 +1369,151 @@
>>  }
>>  
>>  /**
>> + *    ata_scsi_mode_select_xlat - Translate SCSI MODE SELECT commands
>> + *    @qc: Storage for translated ATA taskfile
>> + *    @scsicmd: SCSI command to translate
>> + *
>> + *    Sets up an ATA taskfile to issue SET FEATURES, if required.
>> + *
>> + *    LOCKING:
>> + *    spin_lock_irqsave(host_set lock)
>> + *
>> + *    RETURNS:
>> + *    Zero on success, non-zero on error.
>> + */
>> +
>> +static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc,
>> +                          u8 *scsicmd)
>> +{
>> +    struct ata_taskfile *tf = &qc->tf;
>> +    struct scsi_cmnd *cmd = qc->scsicmd;
>> +    unsigned int param_len, ten, buflen, minlen, bdlen, offset;
>> +    unsigned int pglen, spf, mpg, wce, dra;
>> +    u8 *rbuf;
>> +    u8 apage[48];
> 
> 
> Overall, add some blank lines to the code below, to break it up a bit.
> Suggested places to break up code into "paragraphs" include 'return'
> statements or 'if' tests with long stretches of code inside.
> 
> 
>> +    tf->flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
>> +    tf->protocol = ATA_PROT_NODATA;
>> +    cmd->result = SAM_STAT_GOOD;
>> +    if (! (scsicmd[1] & 0x10))
>> +        return 1;    /* require PF bit set */
>> +    ten = (scsicmd[0] == MODE_SELECT_10) ? 1 : 0;
>> +    param_len =  ten ? ((scsicmd[7] << 8) + scsicmd[8]) : scsicmd[4];
>> +    if (param_len < (ten ? 8 : 4))
>> +        return 2;
>> +        buflen = ata_scsi_rbuf_get(cmd, &rbuf);
> 
> 
> indentation problem
> 
>> +    minlen = (param_len < buflen) ? param_len : buflen;
>> +    bdlen = ten ? ((rbuf[6] << 8) + rbuf[7]) : rbuf[3];
>> +    offset = (ten ? 8 : 4) + bdlen;
>> +    if (minlen > offset) {
>> +        memset(apage, 0, sizeof(apage));
>> +        pglen = minlen - offset;
>> +        pglen = (pglen < sizeof(apage)) ? pglen : sizeof(apage);
>> +        memcpy(apage, rbuf + offset, pglen);
>> +    }
>> +    ata_scsi_rbuf_put(cmd, rbuf);
>> +    if (minlen == offset)
>> +        return 2;
>> +    if (minlen < offset)
>> +        goto bad_param;
>> +    minlen -= offset;
>> +    spf = !!(apage[0] & 0x40);
> 
> 
> '!!' operation not needed.

More coffee needed (by me).

>> +    mpg = apage[0] & 0x3f;
>> +    /* mspg = spf ? apage[1] : 0; */
>> +    if (spf)
>> +        goto bad_param;
>> +    else
>> +        pglen = apage[1];
>> +        if (pglen + 2 < minlen)
>> +            goto bad_param;
>> +    switch (mpg) {
>> +    case 0x1:
>> +        if (pglen != def_rw_recovery_mpage[1])
>> +            goto bad_param;
>> +        break;
>> +    case 0x8:
>> +        if (pglen != def_caching_mpage[1])
>> +            goto bad_param;
>> +        wce = !!(apage[2] & 0x4);
>> +        dra = !!(apage[12] & 0x20);
>> +        if (wce != !!(ata_id_wcache_enabled(qc->dev->id))) {
>> +            /* write(back) cache */
>> +            if (wce)
>> +                tf->feature = 0x2;    /* WCE -> enable */
>> +            else
>> +                tf->feature = 0x82;    /* disable */
> 
> 
> define named constants for these numeric constants, include/linux/ata.h

ok

>> +            tf->command = ATA_CMD_SET_FEATURES;
>> +            /* <<< If success, should dev->id be updated? >>> */
>> +            /* vvvvvvv   start of dubious code   vvvvvvvvvv */
>> +            if (wce)
>> +                qc->dev->id[85] |= 0x20;
>> +            else
>> +                qc->dev->id[85] &= ~0x20;
>> +            /* vvvvvvv   end of dubious code   vvvvvvvvvv */
>> +            return 0;
> 
> 
> SET FEATURES should be followed by a second command, IDENTIFY [PACKET]
> DEVICE, to update dev->id and also to verify that the device set the
> feature as requested.
> 
> 
>> +/* FIXME: we may want to issue two SET FEATURES commands here */
>> +        if (dra != !(ata_id_rahead_enabled(qc->dev->id))) {
> 
> 
> bug: need one more '!' AFAICS

More coffee needed (by you) IMO. The logical inversion is required
since the SCSI bit is negated ("Disable Read Ahead") while the ATA
helper function returns true (actually > 0) when it is enabled.
I tested it on real hardware and it seemed to do the correct
thing (after I fixed the sdparm bug).

>> +            /* read look ahead */
>> +            if (dra)
>> +                tf->feature = 0x55;    /* DRA -> disable */
>> +            else
>> +                tf->feature = 0xaa;    /* enable */
>> +            tf->command = ATA_CMD_SET_FEATURES;
>> +            /* <<< If success, should dev->id be updated? >>> */
>> +            /* vvvvvvv   start of dubious code   vvvvvvvvvv */
>> +            if (dra)
>> +                qc->dev->id[85] &= ~0x40;
>> +            else
>> +                qc->dev->id[85] |= 0x40;
>> +            /* vvvvvvv   end of dubious code   vvvvvvvvvv */
>> +            return 0;
> 
> 
> same comment as with 'wce'.  if both dra and wce change, then four
> commands should follow: SET FEATURES, IDENTIFY DEVICE, SET FEATURES,
> IDENTIFY DEVICE.  If you wanted to be optimal, you could expend more
> [code] effort and only issue SF, SF, ID.
> 
> 
>> +    case 0xa:
>> +        if (pglen != def_control_mpage[1])
>> +            goto bad_param;
>> +        break;
>> +    default:
>> +        goto bad_param;
>> +    }
>> +    /* GOOD response, no ATA command issued */
>> +    return 2;
>> +
>> +bad_param:
>> +    ata_scsi_set_sense(cmd, ILLEGAL_REQUEST, 0x26, 0x0);
>> +    /* "Invalid field in parameter list" */
>> +    return 2;
>> +}
>> +
>> +/**
>> + *    ata_scsi_set_sense - Set SCSI sense data and status
>> + *    @cmd: SCSI request to be handled
>> + *    @sk: SCSI-defined sense key
>> + *    @asc: SCSI-defined additional sense code
>> + *    @ascq: SCSI-defined additional sense code qualifier
>> + *
>> + *    Helper function that builds a valid fixed format, current
>> + *    response code and the given sense key (sk), additional sense
>> + *    code (asc) and additional sense code qualifier (ascq) with
>> + *    a SCSI command status of %SAM_STAT_CHECK_CONDITION .
>> + *
>> + *    LOCKING:
>> + *    Not required
>> + */
>> +
>> +void ata_scsi_set_sense(struct scsi_cmnd *cmd, u8 sk, u8 asc, u8 ascq)
>> +{
>> +    cmd->result = SAM_STAT_CHECK_CONDITION;
>> +
>> +    cmd->sense_buffer[0] = 0x70;    /* fixed format, current */
>> +    cmd->sense_buffer[2] = sk;
>> +    cmd->sense_buffer[7] = 18 - 8;    /* additional sense length */
>> +    cmd->sense_buffer[12] = asc;
>> +    cmd->sense_buffer[13] = ascq;
>> +}
>> +
>> +/**
>>   *    ata_scsi_badcmd - End a SCSI request with an error
>>   *    @cmd: SCSI request to be handled
>>   *    @done: SCSI command completion function
>> @@ -1254,13 +1531,7 @@
>>  void ata_scsi_badcmd(struct scsi_cmnd *cmd, void (*done)(struct
>> scsi_cmnd *), u8 asc, u8 ascq)
>>  {
>>      DPRINTK("ENTER\n");
>> -    cmd->result = SAM_STAT_CHECK_CONDITION;
>> -
>> -    cmd->sense_buffer[0] = 0x70;
>> -    cmd->sense_buffer[2] = ILLEGAL_REQUEST;
>> -    cmd->sense_buffer[7] = 14 - 8;    /* addnl. sense len. FIXME:
>> correct? */
>> -    cmd->sense_buffer[12] = asc;
>> -    cmd->sense_buffer[13] = ascq;
>> +    ata_scsi_set_sense(cmd, ILLEGAL_REQUEST, asc, ascq);
>>  
>>      done(cmd);
>>  }
>> @@ -1414,9 +1685,10 @@
>>   *    Pointer to translation function if possible, %NULL if not.
>>   */
>>  
>> -static inline ata_xlat_func_t ata_get_xlat_func(struct ata_device
>> *dev, u8 cmd)
>> +static inline ata_xlat_func_t ata_get_xlat_func(struct ata_device *dev,
>> +                        u8 *cmd)
>>  {
>> -    switch (cmd) {
>> +    switch (cmd[0]) {
>>      case READ_6:
>>      case READ_10:
>>      case READ_16:
>> @@ -1426,6 +1698,16 @@
>>      case WRITE_16:
>>          return ata_scsi_rw_xlat;
>>  
>> +    case INQUIRY:
>> +        if (((cmd[1] & 0x3) == 0x1) && (cmd[2] == 0x89)) {
> 
> 
> what do these magic numbers indicate?

Place holder for ATA Information VPD page [0x89]. Valid when
CmdDt=0 and EVPD=1. That page is defined in sat-r05. I'm about to
decode that page in sg_inq and sdparm. Its payload is the ATA
IDENTIFY command's response and the ATA device's "signature"
[whatever that is].

>> +            /* needs to re-issue an ATA IDENTIFY cmd
>> +             * and refill dev->id array. Then put this
>> +             * data and the signature in the vpd response.
>> +             */
>> +            /* return ata_scsi_ata_info_vpd_xlat; ?? */
> 
> 
> IFF your diagnosis is correct, then I agree with the proposed solution.
> 
> The rest looks OK.

Doug Gilbert

Re: [PATCH] libata: write cache and read ahead

[Jeff Garzik]

Douglas Gilbert wrote:
> With repect to my libata TEST UNIT READY patch there
> is a good chance that can be dropped. The feedback
> from the t10 reflector seems to indicate the current
> definition may be changed, unfortunately the proposed
> changes may require some SAT state information being
> held for each libata device (e.g. pseudo "STOPPED"

Keeping some state is not a big deal.  If it grows beyond a single 
struct member, though, I would probably be inclined to encapsulate the 
state in a separate struct ata_scsi_xlat_state or somesuch.


> state to mimic SBC-2). It also seems some power up
> issues may need to involve the transport layer
> for resolution. For example, SSP's additional sense code
> of LOGICAL UNIT NOT READY, NOTIFY (ENABLE SPINUP)
> REQUIRED may also need to be issued by libata.

libata, in general, is pretty ignorant of PM issues :)


> Another potential issue that I didn't mention was support
> for the "IMMED" bit in commands like START STOP UNIT; i.e.
> issue the corresponding ATA command but return to the caller
> without waiting for the ATA command to finish.

No need to avoid the issue, I know all about it :)

No plans to support stuff such as IMMED; too much pain for little gain.


> Showing my aversion to writing something like:
>    u8 page[sizeof(def_caching_mpage)];
> 
> Using sizeof in an array declaration seems to be a grey area in C,
> but gcc accepts it so perhaps I shouldn't worry.

Your example (quoted) should be fine on all compilers we care about :)


>>>+/* FIXME: we may want to issue two SET FEATURES commands here */
>>>+        if (dra != !(ata_id_rahead_enabled(qc->dev->id))) {
>>
>>
>>bug: need one more '!' AFAICS
> 
> 
> More coffee needed (by you) IMO. The logical inversion is required

Whoops, indeed, sorry about that.



>>>+    case INQUIRY:
>>>+        if (((cmd[1] & 0x3) == 0x1) && (cmd[2] == 0x89)) {
>>
>>
>>what do these magic numbers indicate?
> 
> 
> Place holder for ATA Information VPD page [0x89]. Valid when
> CmdDt=0 and EVPD=1. That page is defined in sat-r05. I'm about to
> decode that page in sg_inq and sdparm. Its payload is the ATA
> IDENTIFY command's response and the ATA device's "signature"
> [whatever that is].

Let's not add such a placeholder.  I'd rather just wait until the real 
thing appears :)

	Jeff

Re: [PATCH] libata: write cache and read ahead

[Luben Tuikov]

On 08/22/05 01:12, Douglas Gilbert wrote:
> I was surprised how much code needed changing.
> With MODE SELECT's issues with libata addressed
> various other SAT "extras" should be much easier
> to implement. That should make libata more attractive
> as a SAT layer for SAS LLDDs (that don't do it already
> in firmware).

Doug, how about never needing a SAT layer for SAS LLDDs
for ATA/ATAPI devices.

Would you object if I "give" you a "domain device" to which
you can send FISes (+ the packet if ATAPI) through
a SAM-4 intefrace: Execute Task + TMFs?

Jeff?

	Luben

Re: [PATCH] libata: write cache and read ahead

[Christoph Hellwig]

On Tue, Aug 23, 2005 at 07:22:59PM -0400, Luben Tuikov wrote:
> On 08/22/05 01:12, Douglas Gilbert wrote:
> > I was surprised how much code needed changing.
> > With MODE SELECT's issues with libata addressed
> > various other SAT "extras" should be much easier
> > to implement. That should make libata more attractive
> > as a SAT layer for SAS LLDDs (that don't do it already
> > in firmware).
> 
> Doug, how about never needing a SAT layer for SAS LLDDs
> for ATA/ATAPI devices.
> 
> Would you object if I "give" you a "domain device" to which
> you can send FISes (+ the packet if ATAPI) through
> a SAM-4 intefrace: Execute Task + TMFs?

Personally I think that would be nice, but when I mentioned the basic idea
(sending FISes through BLOCK_PC block later commands and let the scsi layer
handle then as vendor-specific commands) Jeff didn't like that because
then we'd need a non-sd upper driver although using the scsi midlayer.

Re: [PATCH] libata: write cache and read ahead

[Jeff Garzik]

Luben Tuikov wrote:
> On 08/22/05 01:12, Douglas Gilbert wrote:
> 
>>I was surprised how much code needed changing.
>>With MODE SELECT's issues with libata addressed
>>various other SAT "extras" should be much easier
>>to implement. That should make libata more attractive
>>as a SAT layer for SAS LLDDs (that don't do it already
>>in firmware).
> 
> 
> Doug, how about never needing a SAT layer for SAS LLDDs
> for ATA/ATAPI devices.
> 
> Would you object if I "give" you a "domain device" to which
> you can send FISes (+ the packet if ATAPI) through
> a SAM-4 intefrace: Execute Task + TMFs?
> 
> Jeff?

SAT is the useful piece of libata that makes everything work.  You can't 
rip out it and still expect to do anything useful ;-)

libata relies on SCSI's device class drivers -- sd, sr, st, sg -- to 
manage devices and generate commands to service.  And in turn, we rely 
on SAT (libata-scsi.c) to translate device class commands into ATA commands.

If the SAT layer went away, then nothing would exist to generate 
commands for libata to service.

	Jeff

Re: [PATCH] libata: write cache and read ahead

[Jeff Garzik]

Christoph Hellwig wrote:
> On Tue, Aug 23, 2005 at 07:22:59PM -0400, Luben Tuikov wrote:
> 
>>On 08/22/05 01:12, Douglas Gilbert wrote:
>>
>>>I was surprised how much code needed changing.
>>>With MODE SELECT's issues with libata addressed
>>>various other SAT "extras" should be much easier
>>>to implement. That should make libata more attractive
>>>as a SAT layer for SAS LLDDs (that don't do it already
>>>in firmware).
>>
>>Doug, how about never needing a SAT layer for SAS LLDDs
>>for ATA/ATAPI devices.
>>
>>Would you object if I "give" you a "domain device" to which
>>you can send FISes (+ the packet if ATAPI) through
>>a SAM-4 intefrace: Execute Task + TMFs?
> 
> 
> Personally I think that would be nice, but when I mentioned the basic idea
> (sending FISes through BLOCK_PC block later commands and let the scsi layer
> handle then as vendor-specific commands) Jeff didn't like that because
> then we'd need a non-sd upper driver although using the scsi midlayer.


That's something different than when Luben was talking about.

Addressing what you [Christoph] are talking about, the block layer needs 
to grow a generic method for submitting device commands, be they ATA, 
SCSI, SCSI w/ variable-length CDB, I2O, Promise SX8 message, whatever. 
Stuff that into the request_queue, and the transport driver handles it 
from there.

	Jeff

Re: [PATCH] libata: write cache and read ahead

[Douglas Gilbert]

Luben Tuikov wrote:
> On 08/22/05 01:12, Douglas Gilbert wrote:
> 
>>I was surprised how much code needed changing.
>>With MODE SELECT's issues with libata addressed
>>various other SAT "extras" should be much easier
>>to implement. That should make libata more attractive
>>as a SAT layer for SAS LLDDs (that don't do it already
>>in firmware).
> 
> 
> Doug, how about never needing a SAT layer for SAS LLDDs
> for ATA/ATAPI devices.

Well that might make things easier for the SAS LLDD
driver writer but may not change things a lot
upstream.

The primary target for SAT is a layer within SAS
enclosures/expanders sitting between a SATA
transport with one or more SATA disks at the other
end and SSP. Evidently SSP has advantages over STP
in this role. Further, SATL isolating SATA devices from
a multi-initiator SAS fabric, can manage SCSI tags
from multiple initiators and map them onto the SATA
disk's NCQ tags.
Basically SATL adds value at that level. [It is also
a useful document to improve all those horrible USB/1394
mass storage SCSI translation layers out there (but
I'm not holding my breath)].

So from the point of view of a SAS LLDD, SATA disks
(LUs) will appear both via STP and SSP.

> Would you object if I "give" you a "domain device" to which
> you can send FISes (+ the packet if ATAPI) through
> a SAM-4 intefrace: Execute Task + TMFs?

Well that is what CSMI (SDI) does and I think we will
be implementing its functionality one way or another ;-).
I just checked SDI and it allows the STP_PASSTHROUGH
to be replaced by a SAT (like) emulation. That is when
the SDI_STP_PASSTHROUGH ioctl can return SDI_SCSI_EMULATION
which tells the user to go away and use the SAT like
interface :-). That is why I said that it may not change
things a lot upstream.

As for ATAPI, 99% of the time it is conveying MMC set.
With BD and HD-DVD coming down the line, app writers
in that space probably won't want to worry about
transport issues any more than they absolutely need to.
SAT defines a "ATA information" VPD page (0x89) which
conveys back the response to IDENTIFY DEVICE (for
non-packet devices) or IDENTIFY PACKET DEVICE. SAT
also defines 12 byte and 16 bytes ATA PASSTHROUGH
SCSI commands which would be handy for smartmontools.

Looking at SAT draft, I would suggest that it is an immature
state, being work in progress (most recent teleconference:
yesterday). And libata is trailing SAT by a good bit.
And libata needs to slide over something like you are
suggesting to be useful in the SAS context.

I intend to take up the issue of SMP in another post.


Doug Gilbert

Re: [PATCH] libata: write cache and read ahead

[Luben Tuikov]

On 08/24/05 06:04, Jeff Garzik wrote:
> 
> SAT is the useful piece of libata that makes everything work.  You can't 
> rip out it and still expect to do anything useful ;-)
> 
> libata relies on SCSI's device class drivers -- sd, sr, st, sg -- to 
> manage devices and generate commands to service.  And in turn, we rely 
> on SAT (libata-scsi.c) to translate device class commands into ATA commands.
> 
> If the SAT layer went away, then nothing would exist to generate 
> commands for libata to service.

Ok, makes sense and requires minimal effort.  I like it.

	Luben

Re: [PATCH] libata: write cache and read ahead

[Luben Tuikov]

On 08/24/05 07:03, Douglas Gilbert wrote:
> Luben Tuikov wrote:
> 
>>On 08/22/05 01:12, Douglas Gilbert wrote:
>>
>>
>>>I was surprised how much code needed changing.
>>>With MODE SELECT's issues with libata addressed
>>>various other SAT "extras" should be much easier
>>>to implement. That should make libata more attractive
>>>as a SAT layer for SAS LLDDs (that don't do it already
>>>in firmware).
>>
>>
>>Doug, how about never needing a SAT layer for SAS LLDDs
>>for ATA/ATAPI devices.
> 
> 
> Well that might make things easier for the SAS LLDD
> driver writer but may not change things a lot
> upstream.

Ok.

Just a general note to make my stance clear:
I'm in favour of making lower layers as simple as possible,
in order to allow greater _versatility_ at a _higher_ level.
Of course none of this without following a well defined
spec.

> The primary target for SAT is a layer within SAS
> enclosures/expanders sitting between a SATA
> transport with one or more SATA disks at the other
> end and SSP. Evidently SSP has advantages over STP
> in this role. Further, SATL isolating SATA devices from
> a multi-initiator SAS fabric, can manage SCSI tags
> from multiple initiators and map them onto the SATA
> disk's NCQ tags.
> Basically SATL adds value at that level. [It is also

Yes, I agree.

> a useful document to improve all those horrible USB/1394
> mass storage SCSI translation layers out there (but
> I'm not holding my breath)].
> 
> So from the point of view of a SAS LLDD, SATA disks
> (LUs) will appear both via STP and SSP.

Hmm.

>>Would you object if I "give" you a "domain device" to which
>>you can send FISes (+ the packet if ATAPI) through
>>a SAM-4 intefrace: Execute Task + TMFs?
> 
> 
> Well that is what CSMI (SDI) does and I think we will
> be implementing its functionality one way or another ;-).
> I just checked SDI and it allows the STP_PASSTHROUGH
> to be replaced by a SAT (like) emulation. That is when
> the SDI_STP_PASSTHROUGH ioctl can return SDI_SCSI_EMULATION
> which tells the user to go away and use the SAT like
> interface :-). That is why I said that it may not change
> things a lot upstream.

What I meant here by "Execute Task" is that when you call
it, lldd->execute_task(), the task _immeditely_ goes out
on the transport.  That is, you're really touching the
transport at that point.

The concept of "passthrough" at this point is rather moot,
_unless_ you are "passing through" something inside the task
work to be done, which the transport wouldn't care.

I like this shielding/separation, since it clearly defines
what each layer needs to worry about.

> As for ATAPI, 99% of the time it is conveying MMC set.
> With BD and HD-DVD coming down the line, app writers
> in that space probably won't want to worry about
> transport issues any more than they absolutely need to.
> SAT defines a "ATA information" VPD page (0x89) which
> conveys back the response to IDENTIFY DEVICE (for
> non-packet devices) or IDENTIFY PACKET DEVICE. SAT
> also defines 12 byte and 16 bytes ATA PASSTHROUGH
> SCSI commands which would be handy for smartmontools.

When I give you the "domain_device" you'll have that info
in it, since of course obtaining it is transport specific,
_unless_ you use the tools you mention above, which means
you've already established a nexus to the device.

> Looking at SAT draft, I would suggest that it is an immature
> state, being work in progress (most recent teleconference:
> yesterday). And libata is trailing SAT by a good bit.
> And libata needs to slide over something like you are
> suggesting to be useful in the SAS context.

Yes, I agree.

	Luben