Patchwork make ata_exec_internal_sg honor DMADIR

login
register
mail settings
Submitter Vincent Pelletier
Date May 12, 2013, 10:13 a.m.
Message ID <201305121213.47294.plr.vincent@gmail.com>
Download mbox | patch
Permalink /patch/243167/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Vincent Pelletier - May 12, 2013, 10:13 a.m.
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,
Tejun Heo - May 14, 2013, 7:06 p.m.
(cc'ing Sergei)

On Sun, May 12, 2013 at 12:13:46PM +0200, Vincent Pelletier wrote:
> 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 ?

Patch looks correct to me but can you please put more detail into the
description preferably with a link to this thread?  As for why
atapi_dmadir isn't enabled by default, my memory is extremely fuzzy
now but ISTR it to be deprecated and cause issues with some devices.

Thanks.

Patch

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