[PATCH] sata_nv: Don't attempt using ADMA for (READ|SET)_MAX commands

[PATCH] sata_nv: Don't attempt using ADMA for (READ|SET)_MAX commands

[Kyle McMartin]

READ_NATIVE_MAX and SET_MAX were causing timeouts on sata_nv controllers.
Disabling ADMA helped, but that is quite a large hammer to use. Reverting
382a6652e91b34d5480cfc0ed840c196650493d4 also helped, but we might as well
fix it right, instead of disabling the performance gain on cache flushes
by using ADMA mode.

Signed-off-by: Kyle McMartin <kyle@canonical.com>

---

This patch depends on the Host Protected Area patch Alan sent to linux-ide
this week.

diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
index 9d9670a..eaf9b76 100644
--- a/drivers/ata/sata_nv.c
+++ b/drivers/ata/sata_nv.c
@@ -1162,6 +1162,20 @@ static void nv_adma_fill_sg(struct ata_queued_cmd *qc, struct nv_adma_cpb *cpb)
 		cpb->next_aprd = cpu_to_le64(0);
 }
 
+static int nv_blacklist_adma_for_hpa_cmds(struct ata_taskfile *tf)
+{
+	switch(tf->command) {
+	case ATA_CMD_READ_NATIVE_MAX:
+	case ATA_CMD_READ_NATIVE_MAX_EXT:
+	case ATA_CMD_SET_MAX:
+	case ATA_CMD_SET_MAX_EXT:
+		return 1;
+
+	default:
+		return 0;
+	}
+}
+
 static int nv_adma_use_reg_mode(struct ata_queued_cmd *qc)
 {
 	struct nv_adma_port_priv *pp = qc->ap->private_data;
@@ -1173,8 +1187,12 @@ static int nv_adma_use_reg_mode(struct ata_queued_cmd *qc)
 		return 1;
 
 	if((qc->flags & ATA_QCFLAG_DMAMAP) ||
-	   (qc->tf.protocol == ATA_PROT_NODATA))
-		return 0;
+	   (qc->tf.protocol == ATA_PROT_NODATA)) {
+		if (nv_blacklist_adma_for_hpa_cmds(&qc->tf))
+			return 1;	/* (SET|READ)_NATIVE_MAX time out in ADMA mdoe */
+		else
+			return 0;
+	}
 
 	return 1;
 }

Re: [PATCH] sata_nv: Don't attempt using ADMA for (READ|SET)_MAX commands

[Alan Cox]

On Fri, 13 Apr 2007 13:08:31 -0400
Kyle McMartin <kyle@canonical.com> wrote:

> READ_NATIVE_MAX and SET_MAX were causing timeouts on sata_nv controllers.
> Disabling ADMA helped, but that is quite a large hammer to use. Reverting
> 382a6652e91b34d5480cfc0ed840c196650493d4 also helped, but we might as well
> fix it right, instead of disabling the performance gain on cache flushes
> by using ADMA mode.

Probably not going to make any performance difference to blacklist all
non-data commands or all but a few like cache-flush. 

Alan

Re: [PATCH] sata_nv: Don't attempt using ADMA for (READ|SET)_MAX commands

[Mark Lord]

Alan Cox wrote:
> On Fri, 13 Apr 2007 13:08:31 -0400
> Kyle McMartin <kyle@canonical.com> wrote:
> 
>> READ_NATIVE_MAX and SET_MAX were causing timeouts on sata_nv controllers.
>> Disabling ADMA helped, but that is quite a large hammer to use. Reverting
>> 382a6652e91b34d5480cfc0ed840c196650493d4 also helped, but we might as well
>> fix it right, instead of disabling the performance gain on cache flushes
>> by using ADMA mode.
> 
> Probably not going to make any performance difference to blacklist all
> non-data commands or all but a few like cache-flush. 

I agree.  The Pacific Digital ADMA stuff had some quirks for various non-data
commands as well, and the sensible thing was to just not use ADMA for anything
other than READs/WRITEs and possible CACHE FLUSHes.

Cheers

Re: [PATCH] sata_nv: Don't attempt using ADMA for (READ|SET)_MAX commands

[Robert Hancock]

Mark Lord wrote:
> Alan Cox wrote:
>> On Fri, 13 Apr 2007 13:08:31 -0400
>> Kyle McMartin <kyle@canonical.com> wrote:
>>
>>> READ_NATIVE_MAX and SET_MAX were causing timeouts on sata_nv 
>>> controllers.
>>> Disabling ADMA helped, but that is quite a large hammer to use. 
>>> Reverting
>>> 382a6652e91b34d5480cfc0ed840c196650493d4 also helped, but we might as 
>>> well
>>> fix it right, instead of disabling the performance gain on cache flushes
>>> by using ADMA mode.
>>
>> Probably not going to make any performance difference to blacklist all
>> non-data commands or all but a few like cache-flush. 
> 
> I agree.  The Pacific Digital ADMA stuff had some quirks for various 
> non-data
> commands as well, and the sensible thing was to just not use ADMA for 
> anything
> other than READs/WRITEs and possible CACHE FLUSHes.

I'm told by NVidia that non-data commands should be fine in ADMA mode, 
except for those that require reading back a result taskfile (which 
can't be done in ADMA mode). The patch in libata-dev which I mentioned 
in another email will ensure that we don't try to use ADMA for such 
commands, so this patch should not be needed.

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/