Skip to contentSkip to conversation

libata: ATA Information VPD page, lk 2.6.14-rc1+

PATCH In review Douglas Gilbert 3 participants 5 messages Sep 19, 2005 – Oct 5, 2005

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 libata: ATA Information VPD page, lk 2.6.14-rc1+

Jeff,
This patch adds support for the ATA Information VPD page defined in sat-r05.pdf (at www.t10.org). This VPD page contains the response to the IDENTIFY (PACKET) DEVICE ATA command amongst of data.

It is against lk 2.6.14-rc1 plus my "libata: scsi error handling" patch that I just sent.

The patch can be exercised with "sg_inq -a /dev/sda" (from sg3_utils-1.17 beta at http://www.torque.net/sg ) or "sdparm --page=ai /dev/sda" (from the sdparm-0.95 beta at http://www.torque.net/sg/sdparm.html ). smartmontools could use this information if it was available from linux.

As required by the sat-r05 draft, the code executes an IDENTIFY (PACKET) DEVICE ATA command to get the most recent values in dev->id . I know Tejun has been working in this area and may have a better way of doing this (e.g. the inside out spinlocks are a worry). The "signature" part is a hack; registers need to be read after a device reset and held.

Changelog:
- add support for the ATA Information VPD page [0x89]
- add function to redo IDENTIFY (PACKET) DEVICE ATA command to update the array dev->id, assumes ata_dev_identify() has been called.

Doug Gilbert

drivers/scsi/libata-scsi.c2614rc1err_ai renamed from drivers/scsi/libata-scsi.c +153 -0
@@ -739,6 +739,84 @@
 	return 0;
 }
 
+static int ata_qc_complete_s_noop(struct ata_queued_cmd *qc, u8 drv_stat)
+{
+        return 0;
+}
+
+/**
+ *	ata_dev_redo_identify - obtain IDENTIFY x DEVICE page again
+ *	@ap: port on which device we wish to probe resides
+ *	@device: device bus address, starting at zero
+ *
+ *	Following bus reset, we issue the IDENTIFY [PACKET] DEVICE
+ *	command, and read back the 512-byte device information page.
+ *	The device information page is fed to us via the standard
+ *	PIO-IN protocol, but we hand-code it here. (TODO: investigate
+ *	using standard PIO-IN paths)
+ *
+ *	LOCKING:
+ *	Inherited from caller.  Some functions called by this function
+ *	obtain the host_set lock.
+ *
+ *	RETURNS:
+ *	Zero on success, -1 of failure
+ */
+
+static int ata_dev_redo_identify(struct ata_port *ap, unsigned int device)
+{
+	struct ata_device *dev = &ap->device[device];
+	u8 status;
+	DECLARE_COMPLETION(wait);
+	struct ata_queued_cmd *qc;
+	unsigned long flags;
+	int rc;
+
+	if (!ata_dev_present(dev)) {
+		return -1;
+	}
+
+	ata_dev_select(ap, device, 1, 1); /* select device 0/1 */
+
+	qc = ata_qc_new_init(ap, dev);
+	BUG_ON(qc == NULL);
+
+	ata_sg_init_one(qc, dev->id, sizeof(dev->id));
+	qc->dma_dir = DMA_FROM_DEVICE;
+	qc->tf.protocol = ATA_PROT_PIO;
+	qc->nsect = 1;
+
+	if (dev->class == ATA_DEV_ATA) {
+		qc->tf.command = ATA_CMD_ID_ATA;
+	} else {
+		qc->tf.command = ATA_CMD_ID_ATAPI;
+	}
+
+	qc->waiting = &wait;
+	qc->complete_fn = ata_qc_complete_s_noop;
+
+	spin_lock_irqsave(&ap->host_set->lock, flags);
+	rc = ata_qc_issue(qc);
+	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+	if (rc)
+		goto err_out;
+	else
+		wait_for_completion(&wait);
+
+	status = ata_chk_status(ap);
+	if (status & ATA_ERR) {
+		goto err_out;
+	}
+
+	swap_buf_le16(dev->id, ATA_ID_WORDS);
+
+	return 0;
+
+err_out:
+	return -1;
+}
+
 /**
  *	ata_scsi_translate - Translate then issue SCSI command to ATA device
  *	@ap: ATA port to which the command is addressed
@@ -1059,6 +1137,79 @@
 }
 
 /**
+ *	ata_scsiop_inq_89 - Simulate INQUIRY EVPD page 83, ATA information
+ *	@args: device IDENTIFY data / SCSI command of interest.
+ *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
+ *	@buflen: Response buffer length.
+ *
+ *	Yields ATA (and SAT layer) information. Defined per sat-r05
+ *	This function should also be called for S-ATAPI devices.
+ *
+ *	LOCKING:
+ *	spin_lock_irqsave(host_set lock)
+ */
+
+unsigned int ata_scsiop_inq_89(struct ata_scsi_args *args, u8 *rbuf,
+			       unsigned int buflen)
+{
+	struct ata_port *ap;
+	struct ata_device *dev;
+	struct scsi_cmnd *cmd = args->cmd;
+	struct scsi_device *scsidev = cmd->device;
+	u8 *scsicmd = cmd->cmnd;
+	unsigned int out_len;
+	int res;
+	const int spec_page_len = 568;
+	u8 b[60];
+	int is_atapi_dev = 0;
+
+	out_len = (scsicmd[3] << 8) + scsicmd[4];
+	out_len = (buflen < out_len) ? buflen : out_len;
+	memset(b, 0, sizeof(b));
+	ap = (struct ata_port *)&scsidev->host->hostdata[0];
+	if (ap) {
+		dev = ata_scsi_find_dev(ap, scsidev);
+		if (dev && (dev->class != ATA_DEV_ATA)) {
+			is_atapi_dev = 1;
+			b[0] = 0x5;	/* assume MMC device, dubious */
+		}
+	} else
+		dev = NULL;
+	b[1] = 0x89;			/* this page code */
+	b[2] = (spec_page_len >> 8) & 0xff;
+	b[3] = spec_page_len & 0xff;
+	strncpy(b + 8, "linux   ", 8);
+	strncpy(b + 16, "libata          ", 16);
+	strncpy(b + 32, "0001", 4);
+	/* signature stuff goes here, where to fetch it from? */
+	b[36] = 0x34;		/* FIS type */
+	b[36 + 1] = 0x0;	/* interrupt + PM port */
+	b[36 + 4] = 0x1;	/* lba low */
+	b[36 + 5] = is_atapi_dev ? 0x14 : 0x0;	/* lba mid */
+	b[36 + 6] = is_atapi_dev ? 0xeb : 0x0;	/* lba high */
+	b[36 + 12] = 0x1;	/* sector count */
+
+	b[56] = is_atapi_dev ? 0xa1 : 0xec;	/* command code */
+
+	memcpy(rbuf, b, ((sizeof(b) < out_len) ? sizeof(b) : out_len));
+	if ((out_len <= sizeof(b)) || (! ap) || (! dev))
+	    return 0;
+	
+	spin_unlock_irq(&ap->host_set->lock);
+	res = ata_dev_redo_identify(ap, scsidev->id);
+	spin_lock_irq(&ap->host_set->lock);
+	if (res) {
+		/* sat-r05 says ok, leave IDENTIFY response all zeroes */
+		DPRINTK("ata_dev_redo_identify failed\n");
+		return 0;
+	}
+	out_len -= 60;
+	memcpy(rbuf + 60, dev->id,
+	       ((out_len < sizeof(dev->id)) ? out_len : sizeof(dev->id)));
+	return 0;
+}
+
+/**
  *	ata_scsiop_noop - Command handler that simply returns success.
  *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
@@ -1715,6 +1866,8 @@
 				ata_scsi_rbuf_fill(&args, ata_scsiop_inq_80);
 			else if (scsicmd[2] == 0x83)
 				ata_scsi_rbuf_fill(&args, ata_scsiop_inq_83);
+			else if (scsicmd[2] == 0x89)
+				ata_scsi_rbuf_fill(&args, ata_scsiop_inq_89);
 			else
 				ata_scsi_invalid_field(cmd, done);
 			break;
#1 lore raw
Quoted 24 lines · Douglas Gilbert

Jeff,
This patch adds support for the ATA Information VPD page defined in sat-r05.pdf (at www.t10.org). This VPD page contains the response to the IDENTIFY (PACKET) DEVICE ATA command amongst of data.

It is against lk 2.6.14-rc1 plus my "libata: scsi error handling" patch that I just sent.

The patch can be exercised with "sg_inq -a /dev/sda" (from sg3_utils-1.17 beta at http://www.torque.net/sg ) or "sdparm --page=ai /dev/sda" (from the sdparm-0.95 beta at http://www.torque.net/sg/sdparm.html ). smartmontools could use this information if it was available from linux.

As required by the sat-r05 draft, the code executes an IDENTIFY (PACKET) DEVICE ATA command to get the most recent values in dev->id . I know Tejun has been working in this area and may have a better way of doing this (e.g. the inside out spinlocks are a worry). The "signature" part is a hack; registers need to be read after a device reset and held.

Quoted 5 lines

Changelog:
- add support for the ATA Information VPD page [0x89]
- add function to redo IDENTIFY (PACKET) DEVICE ATA command to update the array dev->id, assumes ata_dev_identify() has been called.

Basic idea OK, patch rejected due to the following concerns. Please resend an updated patch.

Quoted 27 lines
drivers/scsi/libata-scsi.c2614rc1err_ai renamed from drivers/scsi/libata-scsi.c +23 -0
@@ -1059,6 +1137,79 @@
 }
 
 /**
+ *	ata_scsiop_inq_89 - Simulate INQUIRY EVPD page 83, ATA information
+ *	@args: device IDENTIFY data / SCSI command of interest.
+ *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
+ *	@buflen: Response buffer length.
+ *
+ *	Yields ATA (and SAT layer) information. Defined per sat-r05
+ *	This function should also be called for S-ATAPI devices.
+ *
+ *	LOCKING:
+ *	spin_lock_irqsave(host_set lock)
+ */
+
+unsigned int ata_scsiop_inq_89(struct ata_scsi_args *args, u8 *rbuf,
+			       unsigned int buflen)
+{
+	struct ata_port *ap;
+	struct ata_device *dev;
+	struct scsi_cmnd *cmd = args->cmd;
+	struct scsi_device *scsidev = cmd->device;
+	u8 *scsicmd = cmd->cmnd;
+	unsigned int out_len;
+	int res;
+	const int spec_page_len = 568;

drivers/scsi/libata-scsi.c2614rc1err_ai line 1172

Quoted 10 lines
drivers/scsi/libata-scsi.c2614rc1err_ai renamed from drivers/scsi/libata-scsi.c +10 -0
+	u8 b[60];
+	int is_atapi_dev = 0;
+
+	out_len = (scsicmd[3] << 8) + scsicmd[4];
+	out_len = (buflen < out_len) ? buflen : out_len;
+	memset(b, 0, sizeof(b));
+	ap = (struct ata_port *)&scsidev->host->hostdata[0];
+	if (ap) {
+		dev = ata_scsi_find_dev(ap, scsidev);
+		if (dev && (dev->class != ATA_DEV_ATA)) {

test dev->class == ATA_DEV_ATAPI, as we may soon add port multiplier device classes, breaking the assumption you're making here.

drivers/scsi/libata-scsi.c2614rc1err_ai line 1182

Quoted 10 lines
drivers/scsi/libata-scsi.c2614rc1err_ai renamed from drivers/scsi/libata-scsi.c +10 -0
+			is_atapi_dev = 1;
+			b[0] = 0x5;	/* assume MMC device, dubious */
+		}
+	} else
+		dev = NULL;
+	b[1] = 0x89;			/* this page code */
+	b[2] = (spec_page_len >> 8) & 0xff;
+	b[3] = spec_page_len & 0xff;
+	strncpy(b + 8, "linux   ", 8);
+	strncpy(b + 16, "libata          ", 16);

can we stuff DRV_VERSION in there too?

drivers/scsi/libata-scsi.c2614rc1err_ai line 1190

Quoted 8 lines
drivers/scsi/libata-scsi.c2614rc1err_ai renamed from drivers/scsi/libata-scsi.c +8 -0
+	strncpy(b + 32, "0001", 4);
+	/* signature stuff goes here, where to fetch it from? */
+	b[36] = 0x34;		/* FIS type */
+	b[36 + 1] = 0x0;	/* interrupt + PM port */
+	b[36 + 4] = 0x1;	/* lba low */
+	b[36 + 5] = is_atapi_dev ? 0x14 : 0x0;	/* lba mid */
+	b[36 + 6] = is_atapi_dev ? 0xeb : 0x0;	/* lba high */
+	b[36 + 12] = 0x1;	/* sector count */

this is a sufficient simulation for now. for the future, when other devices such as enclosure, port multipliers, and such are supported, we'll probably want to cache the signature returned by the device.

Quoted 4 lines
drivers/scsi/libata-scsi.c2614rc1err_ai renamed from drivers/scsi/libata-scsi.c +4 -0
+	b[56] = is_atapi_dev ? 0xa1 : 0xec;	/* command code */
+
+	memcpy(rbuf, b, ((sizeof(b) < out_len) ? sizeof(b) : out_len));
+	if ((out_len <= sizeof(b)) || (! ap) || (! dev))

drivers/scsi/libata-scsi.c2614rc1err_ai line 1205

Quoted 10 lines
drivers/scsi/libata-scsi.c2614rc1err_ai renamed from drivers/scsi/libata-scsi.c +10 -0
+	    return 0;
+	
+	spin_unlock_irq(&ap->host_set->lock);
+	res = ata_dev_redo_identify(ap, scsidev->id);
+	spin_lock_irq(&ap->host_set->lock);
+	if (res) {
+		/* sat-r05 says ok, leave IDENTIFY response all zeroes */
+		DPRINTK("ata_dev_redo_identify failed\n");
+		return 0;
+	}

Just eliminate this. dev->id should be considered always current. If it is not, that should be fixed elsewhere in libata.

The rest of the patch is OK.

Jeff

#2 lore raw
Quoted 3 lines · Jeff Garzik
Quoted 1 line · Douglas Gilbert

Jeff,

<snip>

Quoted 10 lines
Quoted 6 lines

Changelog:
- add support for the ATA Information VPD page [0x89]
- add function to redo IDENTIFY (PACKET) DEVICE ATA command to update the array dev->id, assumes ata_dev_identify() has been called.

Basic idea OK, patch rejected due to the following concerns. Please resend an updated patch.

Jeff,
This patch was sent on 19 September 2005.
Why does a response take so long? [Has Luben's
"I request inclusion ..." thread been going that
long :-) ] With the passage of time, it takes me
longer to rework and retest. Please also consider
the submitter's time.

IMO you are quite capable of making the changes
that you are now requesting me to make ... but
I'll have another shot ...

In the last year or so I have suggested around twenty libata patches and bug fixes which have resulted in large signal_to_noise and ignore_or_reject ratios. Progress seems glacial in terms of SCSI support. Others have submitted more code than me. What happened to the ATA PASS THROUGH SCSI commands? [I would like to use them in smartmontools, hdparm is coded for them.] My suspicion is that SAT layer in libata is being maintained "half-baked" so it can be displaced more easily when the "emulation is bad" policy is implemented some time in the future. Makes it hard to get motivated to (re-)roll another SAT libata patch.

Quoted 34 lines
Quoted 34 lines
drivers/scsi/libata-scsi.c2614rc1err_ai renamed from drivers/scsi/libata-scsi.c +3 -0
@@ -1059,6 +1137,79 @@
 }
 
 /**
+ *    ata_scsiop_inq_89 - Simulate INQUIRY EVPD page 83, ATA information
+ *    @args: device IDENTIFY data / SCSI command of interest.
+ *    @rbuf: Response buffer, to which simulated SCSI cmd output is

sent.
+ * @buflen: Response buffer length.
+ *
+ * Yields ATA (and SAT layer) information. Defined per sat-r05
+ * This function should also be called for S-ATAPI devices.
+ *
+ * LOCKING:
+ * spin_lock_irqsave(host_set lock)
+ */

drivers/scsi/libata-scsi.c2614rc1err_ai renamed from drivers/scsi/libata-scsi.c +18 -0
+
+unsigned int ata_scsiop_inq_89(struct ata_scsi_args *args, u8 *rbuf,
+                   unsigned int buflen)
+{
+    struct ata_port *ap;
+    struct ata_device *dev;
+    struct scsi_cmnd *cmd = args->cmd;
+    struct scsi_device *scsidev = cmd->device;
+    u8 *scsicmd = cmd->cmnd;
+    unsigned int out_len;
+    int res;
+    const int spec_page_len = 568;
+    u8 b[60];
+    int is_atapi_dev = 0;
+
+    out_len = (scsicmd[3] << 8) + scsicmd[4];
+    out_len = (buflen < out_len) ? buflen : out_len;
+    memset(b, 0, sizeof(b));
Quoted 8 lines

drivers/scsi/libata-scsi.c2614rc1err_ai line 1172

Quoted 4 lines
drivers/scsi/libata-scsi.c2614rc1err_ai renamed from drivers/scsi/libata-scsi.c +4 -0
+    ap = (struct ata_port *)&scsidev->host->hostdata[0];
+    if (ap) {
+        dev = ata_scsi_find_dev(ap, scsidev);
+        if (dev && (dev->class != ATA_DEV_ATA)) {

test dev->class == ATA_DEV_ATAPI, as we may soon add port multiplier device classes, breaking the assumption you're making here.

Ok. Perhaps you could add the appropriate code here since I'm not familiar with what is planned. I just looked at existing ATA code for guidance.

Quoted 12 lines

drivers/scsi/libata-scsi.c2614rc1err_ai line 1182

Quoted 10 lines
drivers/scsi/libata-scsi.c2614rc1err_ai renamed from drivers/scsi/libata-scsi.c +10 -0
+            is_atapi_dev = 1;
+            b[0] = 0x5;    /* assume MMC device, dubious */
+        }
+    } else
+        dev = NULL;
+    b[1] = 0x89;            /* this page code */
+    b[2] = (spec_page_len >> 8) & 0xff;
+    b[3] = spec_page_len & 0xff;
+    strncpy(b + 8, "linux   ", 8);
+    strncpy(b + 16, "libata          ", 16);
Quoted 1 line

can we stuff DRV_VERSION in there too?

Sure ...

Quoted 10 lines

drivers/scsi/libata-scsi.c2614rc1err_ai line 1190

Quoted 8 lines
drivers/scsi/libata-scsi.c2614rc1err_ai renamed from drivers/scsi/libata-scsi.c +8 -0
+    strncpy(b + 32, "0001", 4);
+    /* signature stuff goes here, where to fetch it from? */
+    b[36] = 0x34;        /* FIS type */
+    b[36 + 1] = 0x0;    /* interrupt + PM port */
+    b[36 + 4] = 0x1;    /* lba low */
+    b[36 + 5] = is_atapi_dev ? 0x14 : 0x0;    /* lba mid */
+    b[36 + 6] = is_atapi_dev ? 0xeb : 0x0;    /* lba high */
+    b[36 + 12] = 0x1;    /* sector count */
Quoted 3 lines

this is a sufficient simulation for now. for the future, when other devices such as enclosure, port multipliers, and such are supported, we'll probably want to cache the signature returned by the device.

What the draft wanted was a copy of those registers just after the most recent device reset. I do not know how to do this (or if that information is held) so I filled those in from a table of indicative values in the draft.

Quoted 16 lines
Quoted 4 lines
drivers/scsi/libata-scsi.c2614rc1err_ai renamed from drivers/scsi/libata-scsi.c +4 -0
+    b[56] = is_atapi_dev ? 0xa1 : 0xec;    /* command code */
+
+    memcpy(rbuf, b, ((sizeof(b) < out_len) ? sizeof(b) : out_len));
+    if ((out_len <= sizeof(b)) || (! ap) || (! dev))

drivers/scsi/libata-scsi.c2614rc1err_ai line 1205

Quoted 10 lines
drivers/scsi/libata-scsi.c2614rc1err_ai renamed from drivers/scsi/libata-scsi.c +10 -0
+        return 0;
+   
+    spin_unlock_irq(&ap->host_set->lock);
+    res = ata_dev_redo_identify(ap, scsidev->id);
+    spin_lock_irq(&ap->host_set->lock);
+    if (res) {
+        /* sat-r05 says ok, leave IDENTIFY response all zeroes */
+        DPRINTK("ata_dev_redo_identify failed\n");
+        return 0;
+    }
Quoted 2 lines

Just eliminate this. dev->id should be considered always current. If it is not, that should be fixed elsewhere in libata.

"Since some fields within the IDENTIFY DEVICE and IDENTIFY PACKET DEVICE may change depending on the state of the attached ATA device the SATL shall issue the IDENTIFY DEVICE or IDENTIFY PACKET DEVICE command to retrieve the updated data whenever the ATA Information VPD page is requested." [sat-r06, section 10.3.5 page 84, describing that field]

Well that seems pretty clear to me. When I write apps like sdparm and sg_inq then I would feel confident documenting that the most up to date response data will be fetched irrespective of the the transport and the topology. As I have pointed out on another thread, the presence of multiple initiators in SAS with the STP makes caching device state problematic. STP affiliations seem only to span a single connection which at best stops co-incident ATA commands interfering with one another, however they won't help if 2 hosts (initiators) send contradictory SET FEATURE commands, one soon after the other.

So if you disagree with that then that is your decision as the maintainer. However I don't want to sign off on code that I am aware violates a draft that I claim to follow unless I agree with the change. If I disagree with the SAT draft then I take it up with [email protected] , just as you have in the past.

As discussed in relation to the implementation of the MODE SELECT SCSI command (to change SATA disk attributes), there is a need for a function like ata_dev_redo_identify() anyway. If there is another function to do this (and for that matter a way for the userspace to resync the dev->id array), perhaps you could point it out to me.

Is anyone aware if other OSes have a SAT layer that we might study? It seems as if SAT layers are appearing in silicon judging from the SR-1216 and the BR-2401 FC to SATA bridge/routers from Sierra Logic.

Doug Gilbert

#3 lore raw
Quoted 32 lines · Douglas Gilbert
Quoted 4 lines · Jeff Garzik
Quoted 1 line · Douglas Gilbert

Jeff,

<snip>

Quoted 9 lines
Quoted 5 lines

Changelog:
- add support for the ATA Information VPD page [0x89]
- add function to redo IDENTIFY (PACKET) DEVICE ATA command to update the array dev->id, assumes ata_dev_identify() has been called.

Basic idea OK, patch rejected due to the following concerns. Please resend an updated patch.

Jeff,
This patch was sent on 19 September 2005.
Why does a response take so long? [Has Luben's
"I request inclusion ..." thread been going that
long :-) ] With the passage of time, it takes me
longer to rework and retest. Please also consider
the submitter's time.

IMO you are quite capable of making the changes
that you are now requesting me to make ... but
I'll have another shot ...

Quoted 13 lines

In the last year or so I have suggested around twenty libata patches and bug fixes which have resulted in large signal_to_noise and ignore_or_reject ratios. Progress seems glacial in terms of SCSI support. Others have submitted more code than me. What happened to the ATA PASS THROUGH SCSI commands? [I would like to use them in smartmontools, hdparm is coded for them.] My suspicion is that SAT layer in libata is being maintained "half-baked" so it can be displaced more easily when the "emulation is bad" policy is implemented some time in the future. Makes it hard to get motivated to (re-)roll another SAT libata patch.

SAT is the primary interface to libata from the outside world. It's not half-baked at all.

ATA passthru support has been in Andrew Morton's -mm kernels for months, by virtue of being automatically pulled in from the 'passthru' branch I actively maintained in rsync://rsync.kernel.org/pub/scm/linux/kernel/git/jgarzik/libata-dev.git

WRT patches, I just don't have time to hand-edit the useful parts out of everybody's patches. I review, and then run a script to merge your patch into the git repository. This is exactly how Linus works as well.
He hand-edits maybe one or two patches a month, and only minor edits
to get the patches to apply.

This is why I am _begging_ you to split up your patches. When each one of your submissions contains <N> changes, that exponentially increases the review time of that patch, and as such, increases the likelihood that it will need revisions.

If you will split up your changes into very fine-grained pieces, the entire process goes a lot faster, and your changes will make it into the kernel with a minimum of fuss, and a maximum speed.

Examples of patches (one patch per email) you could create:

* create ata_scsi_set_sense()
* use ata_scsi_set_sense() in existing code
* simplifications found after using ata_scsi_set_sense()
* move MODE SENSE control page defaults into individual variables (def_control_mode_page, etc.) for use by future patches

The issue is not patch size. The issue is splitting up logical changes into multiple successive patches.

If you split up your libata patches, I promise I will merge them rapidly.

Quoted 44 lines
Quoted 34 lines
Quoted 34 lines
drivers/scsi/libata-scsi.c2614rc1err_ai renamed from drivers/scsi/libata-scsi.c +3 -0
@@ -1059,6 +1137,79 @@
 }
 
 /**
+ *    ata_scsiop_inq_89 - Simulate INQUIRY EVPD page 83, ATA information
+ *    @args: device IDENTIFY data / SCSI command of interest.
+ *    @rbuf: Response buffer, to which simulated SCSI cmd output is

sent.
+ * @buflen: Response buffer length.
+ *
+ * Yields ATA (and SAT layer) information. Defined per sat-r05
+ * This function should also be called for S-ATAPI devices.
+ *
+ * LOCKING:
+ * spin_lock_irqsave(host_set lock)
+ */

drivers/scsi/libata-scsi.c2614rc1err_ai renamed from drivers/scsi/libata-scsi.c +18 -0
+
+unsigned int ata_scsiop_inq_89(struct ata_scsi_args *args, u8 *rbuf,
+                   unsigned int buflen)
+{
+    struct ata_port *ap;
+    struct ata_device *dev;
+    struct scsi_cmnd *cmd = args->cmd;
+    struct scsi_device *scsidev = cmd->device;
+    u8 *scsicmd = cmd->cmnd;
+    unsigned int out_len;
+    int res;
+    const int spec_page_len = 568;
+    u8 b[60];
+    int is_atapi_dev = 0;
+
+    out_len = (scsicmd[3] << 8) + scsicmd[4];
+    out_len = (buflen < out_len) ? buflen : out_len;
+    memset(b, 0, sizeof(b));
Quoted 8 lines

drivers/scsi/libata-scsi.c2614rc1err_ai line 1172

Quoted 4 lines
drivers/scsi/libata-scsi.c2614rc1err_ai renamed from drivers/scsi/libata-scsi.c +4 -0
+    ap = (struct ata_port *)&scsidev->host->hostdata[0];
+    if (ap) {
+        dev = ata_scsi_find_dev(ap, scsidev);
+        if (dev && (dev->class != ATA_DEV_ATA)) {

test dev->class == ATA_DEV_ATAPI, as we may soon add port multiplier device classes, breaking the assumption you're making here.

Quoted 3 lines

Ok. Perhaps you could add the appropriate code here since I'm not familiar with what is planned. I just looked at existing ATA code for guidance.

Change the test to be as I described:

replace

	dev->class != ATA_DEV_ATA
with
	dev->class == ATA_DEV_ATAPI
Quoted 15 lines
Quoted 10 lines

drivers/scsi/libata-scsi.c2614rc1err_ai line 1190

Quoted 8 lines
drivers/scsi/libata-scsi.c2614rc1err_ai renamed from drivers/scsi/libata-scsi.c +8 -0
+    strncpy(b + 32, "0001", 4);
+    /* signature stuff goes here, where to fetch it from? */
+    b[36] = 0x34;        /* FIS type */
+    b[36 + 1] = 0x0;    /* interrupt + PM port */
+    b[36 + 4] = 0x1;    /* lba low */
+    b[36 + 5] = is_atapi_dev ? 0x14 : 0x0;    /* lba mid */
+    b[36 + 6] = is_atapi_dev ? 0xeb : 0x0;    /* lba high */
+    b[36 + 12] = 0x1;    /* sector count */
Quoted 3 lines

this is a sufficient simulation for now. for the future, when other devices such as enclosure, port multipliers, and such are supported, we'll probably want to cache the signature returned by the device.

Quoted 4 lines

What the draft wanted was a copy of those registers just after the most recent device reset. I do not know how to do this (or if that information is held) so I filled those in from a table of indicative values in the draft.

As I said, that's fine.

In order to do what the draft suggests, we should store a copy of struct ata_taskfile in struct ata_device, caching the register values. That's also fine, if you are motivated to create a further patch.

Quoted 6 lines

"Since some fields within the IDENTIFY DEVICE and IDENTIFY PACKET DEVICE may change depending on the state of the attached ATA device the SATL shall issue the IDENTIFY DEVICE or IDENTIFY PACKET DEVICE command to retrieve the updated data whenever the ATA Information VPD page is requested." [sat-r06, section 10.3.5 page 84, describing that field]

Each time the ATA device's state changes, dev->id needs to be updated. And it doesn't change randomly, only when initiated by further libata actions.

Thus, dev->id is defined as always containing the most current IDENTIFY [PACKET] DEVICE page. If it does not, that problem should be addressed in the area of libata that initiated the state change, _not_ papered over by SATL.

Quoted 10 lines

Well that seems pretty clear to me. When I write apps like sdparm and sg_inq then I would feel confident documenting that the most up to date response data will be fetched irrespective of the the transport and the topology. As I have pointed out on another thread, the presence of multiple initiators in SAS with the STP makes caching device state problematic. STP affiliations seem only to span a single connection which at best stops co-incident ATA commands interfering with one another, however they won't help if 2 hosts (initiators) send contradictory SET FEATURE commands, one soon after the other.

If ATA device state is not accurately cached, you get data corruption. By definition it -must- be accurate at all times, otherwise the hardware will be programmed incorrectly. This is why we must snoop the ATA passthru command.

Welcome to ATA :)

Quoted 6 lines

As discussed in relation to the implementation of the MODE SELECT SCSI command (to change SATA disk attributes), there is a need for a function like ata_dev_redo_identify() anyway. If there is another function to do this (and for that matter a way for the userspace to resync the dev->id array), perhaps you could point it out to me.

Tejun posted a patch that illustrates doing multiple ATA commands from within SATL. Message id <[email protected]>, and you were even in the To: header.

Jeff

#4 lore raw
Quoted 7 lines · Jeff Garzik
Quoted 7 lines

Jeff,
This patch was sent on 19 September 2005.
Why does a response take so long? [Has Luben's
"I request inclusion ..." thread been going that
long :-) ] With the passage of time, it takes me
longer to rework and retest. Please also consider
the submitter's time.

Yes, consider the submitter's name.

Quoted 2 lines

SAT is the primary interface to libata from the outside world. It's not half-baked at all.

:-^

Related to this patch by Doug:

Jeff, any plans on dropping the SAS code into the kernel now, so that that big company can start using it, plus people would start seeing the code, using it and themselves submitting patches alongside James B, Christoph and you?

I just think it would be more fare this way to the big company, and the SCSI community? I don't mind everyone submitting patches.

Latest Linus' Git tree + SAS, patches, whatnot:
http://linux.adaptec.com/sas/

Plus there is a lot of relevant information in the SAS code in reference to _this_ patch by Doug. (see below)

Quoted 32 lines
Quoted 27 lines
Quoted 17 lines
Quoted 17 lines
drivers/scsi/libata-scsi.c2614rc1err_ai renamed from drivers/scsi/libata-scsi.c +17 -0
+unsigned int ata_scsiop_inq_89(struct ata_scsi_args *args, u8 *rbuf,
+                   unsigned int buflen)
+{
+    struct ata_port *ap;
+    struct ata_device *dev;
+    struct scsi_cmnd *cmd = args->cmd;
+    struct scsi_device *scsidev = cmd->device;
+    u8 *scsicmd = cmd->cmnd;
+    unsigned int out_len;
+    int res;
+    const int spec_page_len = 568;
+    u8 b[60];
+    int is_atapi_dev = 0;
+
+    out_len = (scsicmd[3] << 8) + scsicmd[4];
+    out_len = (buflen < out_len) ? buflen : out_len;
+    memset(b, 0, sizeof(b));
Quoted 8 lines

drivers/scsi/libata-scsi.c2614rc1err_ai line 1172

Quoted 4 lines
drivers/scsi/libata-scsi.c2614rc1err_ai renamed from drivers/scsi/libata-scsi.c +4 -0
+    ap = (struct ata_port *)&scsidev->host->hostdata[0];
+    if (ap) {
+        dev = ata_scsi_find_dev(ap, scsidev);
+        if (dev && (dev->class != ATA_DEV_ATA)) {

test dev->class == ATA_DEV_ATAPI, as we may soon add port multiplier device classes, breaking the assumption you're making here.

Quoted 3 lines

Ok. Perhaps you could add the appropriate code here since I'm not familiar with what is planned. I just looked at existing ATA code for guidance.

Quoted 6 lines

Change the test to be as I described:

replace

	dev->class != ATA_DEV_ATA
with
	dev->class == ATA_DEV_ATAPI

Doug, Port Multipliers and other exotic ATA devices are supported in the SAS code. You can take a look in there and do similar here.

Quoted 21 lines
Quoted 15 lines
Quoted 10 lines

drivers/scsi/libata-scsi.c2614rc1err_ai line 1190

Quoted 8 lines
drivers/scsi/libata-scsi.c2614rc1err_ai renamed from drivers/scsi/libata-scsi.c +8 -0
+    strncpy(b + 32, "0001", 4);
+    /* signature stuff goes here, where to fetch it from? */
+    b[36] = 0x34;        /* FIS type */
+    b[36 + 1] = 0x0;    /* interrupt + PM port */
+    b[36 + 4] = 0x1;    /* lba low */
+    b[36 + 5] = is_atapi_dev ? 0x14 : 0x0;    /* lba mid */
+    b[36 + 6] = is_atapi_dev ? 0xeb : 0x0;    /* lba high */
+    b[36 + 12] = 0x1;    /* sector count */
Quoted 3 lines

this is a sufficient simulation for now. for the future, when other devices such as enclosure, port multipliers, and such are supported, we'll probably want to cache the signature returned by the device.

Quoted 4 lines

What the draft wanted was a copy of those registers just after the most recent device reset. I do not know how to do this (or if that information is held) so I filled those in from a table of indicative values in the draft.

Quoted 5 lines

As I said, that's fine.

In order to do what the draft suggests, we should store a copy of struct ata_taskfile in struct ata_device, caching the register values. That's also fine, if you are motivated to create a further patch.

In the SAS code, upon Domain Discovery, the IDENTIFY (SAS) or the initial Device-to-host FIS (ATA) is kept in

struct domain_device::frame_rcvd[32]

This is in drivers/include/scsi/sas/sas_discover.h.

struct domain_device describes any device on the domain.

If the ATA device is not directly attached, but behind an expaner (SAS/SATA bridge), then this info is gotten from REPORT PHY SATA, in which case struct domain_device::frame_rcvd[32] keeps the initial d2h FIS (from the RPS), plus the REPORT PHY SATA is kept in struct domain_device::ata_dev::rps_resp.

Similar functionality can be had in "libata" (ala SATL).

Luben