diff mbox

make ata_exec_internal_sg honor DMADIR

Message ID 201305171920.11127.plr.vincent@gmail.com
State Not Applicable
Delegated to: David Miller
Headers show

Commit Message

Vincent Pelletier May 17, 2013, 5:20 p.m. UTC
Le mardi 14 mai 2013 21:06:16, Tejun Heo a écrit :
> Patch looks correct to me but can you please put more detail into the
> description preferably with a link to this thread?

Updated & attached. I write one (trivial) kernel commit message every 3 years, 
so please correct me if there is a more consistent way to present it.

> 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.

As atapi_id_dmadir() doesn't detect the bridge needs DMADIR, for now I need to 
enable it globally.
This means it doesn't work out of the box, and needs a reboot to work as 
atapi_dmadir is read-only in sysfs. Also, if it causes regression with other 
drives, maybe one would gain a drive by enabling DMADIR but loose another.

From my (very limited) understanding, the bridge just passes the drive's "id" 
(as in "atapi_id_dmadir(dev->id)") through. Is there another way to detect 
such bridge ? Other things atapi_id_dmadir() should look for in "id" ?

If not, would it be possible to have a rw sysfs pseudofile per-device (...per 
port ?) to enable DMADIR ?

If not, what about making atapi_dmadir sysfs pseudofile rw, to save a reboot ?

Would you have more ideas on how it could be solved ?

I'm willing to give a try to any of these options if they have a chance to get 
somewhere.

Regards,

Comments

Tejun Heo May 17, 2013, 6:47 p.m. UTC | #1
Hello, Vincent.

On Fri, May 17, 2013 at 07:20:10PM +0200, Vincent Pelletier wrote:
> From my (very limited) understanding, the bridge just passes the drive's "id" 
> (as in "atapi_id_dmadir(dev->id)") through. Is there another way to detect 
> such bridge ? Other things atapi_id_dmadir() should look for in "id" ?

I don't think there's any way to detect bridges in a reliable way.

> If not, would it be possible to have a rw sysfs pseudofile per-device (...per 
> port ?) to enable DMADIR ?

Yeap, that sounds like the best we can do at this point.  Care to
write up a patch?

> From beca064485e3c86e4abe08b9ce5c89b33ed8c780 Mon Sep 17 00:00:00 2001
> Message-Id: <beca064485e3c86e4abe08b9ce5c89b33ed8c780.1368810901.git.plr.vincent@gmail.com>
> From: Vincent Pelletier <plr.vincent@gmail.com>
> Date: Fri, 17 May 2013 19:09:05 +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
> 
> Fixes SATA-to-PATA bridge "Abit Serillel 2" when used on an ATAPI device,
> which otherwise fails several tries with a timeout until it gets disabled:
> 
>   kernel: ata5.00: qc timeout (cmd 0xa0)
>   kernel: ata5.00: failed to clear UNIT ATTENTION (err_mask=0x5)
>   kernel: ata5.00: disabled
> 
> Based on a patch by Csaba Halász <csaba.halasz@gmail.com> on linux-ide:
> http://marc.info/?l=linux-ide&m=136121147832295&w=2

While better, please go into more details.  The problem here is that
the bridge requires DMADIR and while libata makes use of DMADIR for
regular commands, it doesn't do that for internal commands which are
used during EH, right?  Please go into full details of what's going on
and be verbose.

Thanks!
diff mbox

Patch

From beca064485e3c86e4abe08b9ce5c89b33ed8c780 Mon Sep 17 00:00:00 2001
Message-Id: <beca064485e3c86e4abe08b9ce5c89b33ed8c780.1368810901.git.plr.vincent@gmail.com>
From: Vincent Pelletier <plr.vincent@gmail.com>
Date: Fri, 17 May 2013 19:09:05 +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

Fixes SATA-to-PATA bridge "Abit Serillel 2" when used on an ATAPI device,
which otherwise fails several tries with a timeout until it gets disabled:

  kernel: ata5.00: qc timeout (cmd 0xa0)
  kernel: ata5.00: failed to clear UNIT ATTENTION (err_mask=0x5)
  kernel: ata5.00: disabled

Based on a patch by Csaba Halász <csaba.halasz@gmail.com> on linux-ide:
http://marc.info/?l=linux-ide&m=136121147832295&w=2

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