aic94xx: fix smartctl utility problem

Fixed the problem that "smartctl -a /dev/some_sata_disk -d ata" does not work on SATA device. ( The smartctl v5.38 does need "-d ata" option.)
The root cause is the aic94xx driver does not return ATA output register due to performance reason. The aic94xx need check ATA command which need ATA output register and turn on internal flag to enable firmware to return ATA output register to top layer.
It also add new ATA commands to ata.h sush as ATA_CMD_CHK_MEDIA_TYPE and ATA_CMD_SMART.

diff -urN a/drivers/scsi/aic94xx/aic94xx_task.c b/drivers/scsi/aic94xx/aic94xx_task.c
@@ -25,6 +25,7 @@
  */
 
 #include <linux/spinlock.h>
+#include <linux/hdreg.h>
 #include "aic94xx.h"
 #include "aic94xx_sas.h"
 #include "aic94xx_hwi.h"

@@ -152,6 +153,22 @@
 		pci_unmap_sg(asd_ha->pcidev, task->scatter, task->num_scatter,
 			     task->data_dir);
 }
+static const struct ata_command_table {
+	u8 command;
+	u8 sub_command;
+} ata_command_tbl[] = {
+	{ATA_CMD_CHK_MEDIA_TYPE,         0},
+	{ATA_CMD_DEV_RESET,              0},
+	{ATA_CMD_EDD,                    0},
+	{ATA_CMD_IDLEIMMEDIATE,          0},
+	{ATA_CMD_READ_NATIVE_MAX,        0},
+	{ATA_CMD_READ_NATIVE_MAX_EXT,    0},
+	{ATA_CMD_SET_MAX,                0},
+	{ATA_CMD_SET_MAX_EXT,            0},
+	{ATA_CMD_SMART,                  SMART_STATUS},
+	{ATA_CMD_SMART,                  SMART_IMMEDIATE_OFFLINE}, 
+	{0,0} /* terminate list */
+};
 
 /* ---------- Task complete tasklet ---------- */
 

@@ -253,6 +270,7 @@
 		break;
 	case TC_SSP_RESP:
 	case TC_ATA_RESP:
+	case TC_CSMI:
 		ts->resp = SAS_TASK_COMPLETE;
 		ts->stat = SAS_PROTO_RESPONSE;
 		asd_get_response_tasklet(ascb, dl);

@@ -375,6 +393,21 @@
 
 /* ---------- ATA ---------- */
 
+int need_ata_output_reg(u8 command, u8 sub_command)
+{
+
+	int i;
+		
+	for (i = 0; ata_command_tbl[i].command != 0 ; i++) {
+
+		if (ata_command_tbl[i].command == command &&
+			(!ata_command_tbl[i].sub_command || 
+			  ata_command_tbl[i].sub_command == sub_command )) 
+		return 1;
+	}
+	return 0;
+}
+
 static int asd_build_ata_ascb(struct asd_ascb *ascb, struct sas_task *task,
 			      gfp_t gfp_flags)
 {

@@ -427,6 +460,9 @@
 			flags |= STP_AFFIL_POLICY;
 		scb->ata_task.flags = flags;
 	}
+	if (need_ata_output_reg(scb->ata_task.fis.command,scb->ata_task.fis.features))
+		scb->ata_task.ata_flags|=CSMI_TASK;
+
 	ascb->tasklet_complete = asd_task_tasklet_complete;
 
 	if (likely(!task->ata_task.device_control_reg_update))

diff -urN a/include/linux/ata.h b/include/linux/ata.h
@@ -175,6 +175,8 @@
 	ATA_CMD_READ_LOG_EXT	= 0x2f,
 	ATA_CMD_PMP_READ	= 0xE4,
 	ATA_CMD_PMP_WRITE	= 0xE8,
+	ATA_CMD_CHK_MEDIA_TYPE	= 0xD1,
+	ATA_CMD_SMART		= 0xB0,
 
 	/* READ_LOG_EXT pages */
 	ATA_LOG_SATA_NCQ	= 0x10,

All commands except for hot path READ/WRITE should be returning ATA output registers.

Additionally, READ/WRITE should be returning ATA output registers, if the READ/WRITE command did not succeed.

Any code that requires an "accepted list of commands" is problematic, because it does not take into account vendor-specific commands.

Jeff

HI Jeff,

I was thinking the checking "READ/WRITE" command table is larger than
my current table. This does not cover vendor-specific command. Do you wan me just check READ/WRITE command?
The aic94xx default implementation is all ATA command will be returning ATA output register if the command did not succeed.

Thanks!
Gilbert

-----Original Message-----
From: Jeff Garzik [mailto:[email protected]]
Sent: Thursday, September 06, 2007 3:06 AM
To: Wu, Gilbert
Cc: [email protected]
Subject: Re: [PATCH] aic94xx: fix smartctl utility problem

does not work on SATA device.

register due to performance reason.

and turn on internal flag to

ATA_CMD_CHK_MEDIA_TYPE and ATA_CMD_SMART.

All commands except for hot path READ/WRITE should be returning ATA output registers.

Additionally, READ/WRITE should be returning ATA output registers, if the READ/WRITE command did not succeed.

Any code that requires an "accepted list of commands" is problematic, because it does not take into account vendor-specific commands.

Jeff

You can implement the check in a _far_ more optimal manner:

Possibility 1:

	static const u8 ata_rw_command_table[256] = {
		[ATA_CMD_READ]		= 1,
		[ATA_CMD_READ_EXT]	= 1,
		... other READ/WRITE commands here, always value==1 ...
	};

	...

	u8 ata_command = ... ;

	if (ata_rw_command_table[ata_command]) {
		/* it is a read/write command */
	} else {
		/* it is NOT a read/write command */
	}

Possibility 2:

	static inline int is_ata_rw_cmd(u8 ata_cmd)
	{
		switch (ata_cmd) {
		case ATA_CMD_READ:
		case ATA_CMD_READ_EXT:
		... other READ/WRITE commands here ...
			return 1;
		}

		return 0;
	}

Either way you avoid the iteration, and simplify things down to a single test.

Once that is done, it should be self-evident that testing -any- list of commands is O(1), rather than O(n) for the case of table iteration. And therefore, the cost of checking "is it a READ/WRITE command?" is equal to the cost of checking for any other commands.

Yes, please.

Great!

Jeff

Hi Jeff,

Thanks for your suggestions. I have done some experiment to return all
ATA output register except for Read/Write command. But the aic94xx discovery machine just can not find the SATA device. Probably I will fix the new issue by next week since I have the other hot issue here.

Thanks!
Gilbert

-----Original Message-----
From: Jeff Garzik [mailto:[email protected]]
Sent: Friday, September 07, 2007 4:01 PM
To: Wu, Gilbert
Cc: [email protected]
Subject: Re: [PATCH] aic94xx: fix smartctl utility problem

than

You can implement the check in a _far_ more optimal manner:

Possibility 1:

	static const u8 ata_rw_command_table[256] = {
		[ATA_CMD_READ]		= 1,
		[ATA_CMD_READ_EXT]	= 1,
		... other READ/WRITE commands here, always value==1 ...
	};

	...

	u8 ata_command = ... ;

	if (ata_rw_command_table[ata_command]) {
		/* it is a read/write command */
	} else {
		/* it is NOT a read/write command */
	}

Possibility 2:

	static inline int is_ata_rw_cmd(u8 ata_cmd)
	{
		switch (ata_cmd) {
		case ATA_CMD_READ:
		case ATA_CMD_READ_EXT:
		... other READ/WRITE commands here ...
			return 1;
		}

		return 0;
	}

Either way you avoid the iteration, and simplify things down to a single

test.

Once that is done, it should be self-evident that testing -any- list of commands is O(1), rather than O(n) for the case of table iteration. And

therefore, the cost of checking "is it a READ/WRITE command?" is equal to the cost of checking for any other commands.

Yes, please.

returning

Great!

Jeff