Re: [PATCH] make ata_exec_internal_sg honor DMADIR

[Vincent Pelletier <plr.vincent@gmail.com>]

[-- Attachment #1: Type: Text/Plain, Size: 1697 bytes --]

Le lundi 18 février 2013 19:17:54, Csaba Halász a écrit :
> kernel: ata5.00: qc timeout (cmd 0xa0)
> kernel: ata5.00: failed to clear UNIT ATTENTION (err_mask=0x5)
> kernel: ata5.00: disabled
> 
> At least for the bridge I have (an Abit Serillel 2) setting DMADIR
> option and making sure even the internal commands use it seems to fix
> the issue.

I confirm this issue, as of 3.8.12 (current debian sid) with the same bridge:
"Abit Serillel" marked on the outside, PCB marked with "serillel2".
The most relevant IC has these markings:
  Silicon Image
  SataLink
  SiL3611CT80
  Q31844.1
  Q329
  1.4

> I have copied the code from atapi_xlat. Maybe some refactoring would
> be in order, because apparently some other things might have to be
> done too (such as setting lbam/lbah).
> Also I am not sure whether we need to check that it's in fact an ATAPI
> command (maybe by putting this in the if (cdb) block).

In my understanding, it should indeed go in the "if (cdb)" block, as it should 
only be needed for ATAPI commands. I don't think lbam/lbah need to be set (or 
if they do, it's a different issue), because they are set independently from 
DMADIR in atapi_xlat (so ata_exec_internal_sg would have to set them 
independently too, probably in the "if (cdb)" block).

I've modified original patch to the attached one, and tested it: drive is 
correctly recognised and data can be read from it.

What would be needed to integrate this patch into the kernel ?
Also, why does atapi_dmadir default to disabled ? I'm very unfamiliar with 
ata[pi], if there any drawback from enabling it by default to fix such
devices ?

Regards,
-- 
Vincent Pelletier

[-- Attachment #2: 0001-libata-make-ata_exec_internal_sg-honor-DMADIR.patch --]
[-- Type: text/x-patch, Size: 1285 bytes --]

From 7de32c38eb2633fc324852c0a239d067c1b4f9ea Mon Sep 17 00:00:00 2001
Message-Id: <7de32c38eb2633fc324852c0a239d067c1b4f9ea.1368353364.git.plr.vincent@gmail.com>
From: Vincent Pelletier <plr.vincent@gmail.com>
Date: Sun, 12 May 2013 12:09:18 +0200
Subject: libata: make ata_exec_internal_sg honor DMADIR
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Based on a patch by Csaba Halász <csaba.halasz@gmail.com>

Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
---
 drivers/ata/libata-core.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 63c743b..d121db7 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1600,8 +1600,13 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
 
 	/* prepare & issue qc */
 	qc->tf = *tf;
-	if (cdb)
+	if (cdb) {
 		memcpy(qc->cdb, cdb, ATAPI_CDB_LEN);
+		if ((dev->flags & ATA_DFLAG_DMADIR) &&
+		    (dma_dir == DMA_FROM_DEVICE))
+			/* some SATA bridges need us to indicate data xfer direction */
+			qc->tf.feature |= ATAPI_DMADIR;
+	}
 	qc->flags |= ATA_QCFLAG_RESULT_TF;
 	qc->dma_dir = dma_dir;
 	if (dma_dir != DMA_NONE) {
-- 
1.7.10.4