[PATCH] make ata_exec_internal_sg honor DMADIR

[PATCH] make ata_exec_internal_sg honor DMADIR

[Csaba Halász]

[-- Attachment #1: Type: text/plain, Size: 793 bytes --]

Hi!

There seems to be a longstanding problem with certain ATAPI drives and
SATA bridges. This usually manifests itself in the device being
disabled on boot, with log messages similar to this:

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

Cheers,
Csaba Halasz

[-- Attachment #2: dmadir.diff --]
[-- Type: application/octet-stream, Size: 557 bytes --]

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 46cd3f4..140d671 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1614,6 +1614,10 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
 		ata_sg_init(qc, sgl, n_elem);
 		qc->nbytes = buflen;
 	}
+	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->private_data = &wait;
 	qc->complete_fn = ata_qc_complete_internal;

Re: [PATCH] make ata_exec_internal_sg honor DMADIR

[Vincent Pelletier]

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

Re: [PATCH] make ata_exec_internal_sg honor DMADIR

[Tejun Heo]

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

-- 
tejun

Re: [PATCH] make ata_exec_internal_sg honor DMADIR

[Vincent Pelletier]

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

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,
-- 
Vincent Pelletier

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

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

Re: [PATCH] make ata_exec_internal_sg honor DMADIR

[Tejun Heo]

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!

-- 
tejun

Re: [PATCH] make ata_exec_internal_sg honor DMADIR

[Vincent Pelletier]

[-- Attachment #1: Type: text/plain, Size: 1642 bytes --]

Hi.

Le vendredi 17 mai 2013 20:47:32, vous avez écrit :
> On Fri, May 17, 2013 at 07:20:10PM +0200, Vincent Pelletier wrote:
> > 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?

First attempt attached (2/2).

Now that I got it working, I'm thining it would be better to automatically 
fallback to enabling DMADIR per-device level during initialisation (just like 
current fallbacks to 1.5Gbps and UDMA/33:PIO3, as visible in 1/2 commit 
message).
I believe it will slow down boot when such device is plugged in, though, any 
idea on how this can be avoided ?

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

Just to be clear about EH: the timeout happens during device  initialisation 
(both at boot or on hotplug), not during error handling (or, maybe it happens 
in both places, but for the same reason).
I'm not comfortable with calling device discovery/initialisation as "error 
handling", but maybe it's unambiguous when used to libata.

I assume the failure to clear UNIT ATTENTION is just an error-on-error 
which gets fixed both because it wouldn't fail to clear (I didn't check) and 
because there is no error state to clear.

> Please go into full details of what's going on and be verbose.

3rd try attached (1/2).

Regards,
-- 
Vincent Pelletier

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

From 4fd4c88648d151e6c790c8ce49bcc128492f018b Mon Sep 17 00:00:00 2001
Message-Id: <4fd4c88648d151e6c790c8ce49bcc128492f018b.1368970061.git.plr.vincent@gmail.com>
From: Vincent Pelletier <plr.vincent@gmail.com>
Date: Sat, 18 May 2013 18:44:04 +0200
Subject: [1/2] libata: make ata_exec_internal_sg honor DMADIR
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

libata honors DMADIR for regular commands, but not for internal commands
used (among other) during device initialisation.

This makes SATA-host-to-PATA-device bridges based on Silicon Image SiL3611
(such as "Abit Serillel 2") end up disabled when used with an ATAPI device
after a few tries.

Log output of the bridge being hot-plugged with an ATAPI drive:

  [ 9631.212901] ata1: exception Emask 0x10 SAct 0x0 SErr 0x40c0000 action 0xe frozen
  [ 9631.212913] ata1: irq_stat 0x00000040, connection status changed
  [ 9631.212923] ata1: SError: { CommWake 10B8B DevExch }
  [ 9631.212939] ata1: hard resetting link
  [ 9632.104962] ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
  [ 9632.106393] ata1.00: ATAPI: PIONEER DVD-RW  DVR-115, 1.06, max UDMA/33
  [ 9632.106407] ata1.00: applying bridge limits
  [ 9632.108151] ata1.00: configured for UDMA/33
  [ 9637.105303] ata1.00: qc timeout (cmd 0xa0)
  [ 9637.105324] ata1.00: failed to clear UNIT ATTENTION (err_mask=0x5)
  [ 9637.105335] ata1: hard resetting link
  [ 9638.044599] ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
  [ 9638.047878] ata1.00: configured for UDMA/33
  [ 9643.044933] ata1.00: qc timeout (cmd 0xa0)
  [ 9643.044953] ata1.00: failed to clear UNIT ATTENTION (err_mask=0x5)
  [ 9643.044963] ata1: limiting SATA link speed to 1.5 Gbps
  [ 9643.044971] ata1.00: limiting speed to UDMA/33:PIO3
  [ 9643.044979] ata1: hard resetting link
  [ 9643.984225] ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 310)
  [ 9643.987471] ata1.00: configured for UDMA/33
  [ 9648.984591] ata1.00: qc timeout (cmd 0xa0)
  [ 9648.984612] ata1.00: failed to clear UNIT ATTENTION (err_mask=0x5)
  [ 9648.984619] ata1.00: disabled
  [ 9649.000593] ata1: hard resetting link
  [ 9649.939902] ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 310)
  [ 9649.955864] ata1: EH complete

With this patch, the drive enumerates correctly when libata is loaded with
atapi_dmadir=1:

  [ 9891.810863] ata1: exception Emask 0x10 SAct 0x0 SErr 0x40c0000 action 0xe frozen
  [ 9891.810874] ata1: irq_stat 0x00000040, connection status changed
  [ 9891.810884] ata1: SError: { CommWake 10B8B DevExch }
  [ 9891.810900] ata1: hard resetting link
  [ 9892.762105] ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
  [ 9892.763544] ata1.00: ATAPI: PIONEER DVD-RW  DVR-115, 1.06, max UDMA/33, DMADIR
  [ 9892.763558] ata1.00: applying bridge limits
  [ 9892.765393] ata1.00: configured for UDMA/33
  [ 9892.786063] ata1: EH complete
  [ 9892.792062] scsi 0:0:0:0: CD-ROM            PIONEER  DVD-RW  DVR-115  1.06 PQ: 0 ANSI: 5
  [ 9892.798455] sr2: scsi3-mmc drive: 12x/12x writer dvd-ram cd/rw xa/form2 cdda tray
  [ 9892.798837] sr 0:0:0:0: Attached scsi CD-ROM sr2
  [ 9892.799109] sr 0:0:0:0: Attached scsi generic sg6 type 5

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


[-- Attachment #3: 0002-RFC-libata-Add-a-per-device-sysfs-control-for-atapi_.patch --]
[-- Type: text/x-patch, Size: 5474 bytes --]

From 62ce0a157f245929f3b7471a3668e03792d14420 Mon Sep 17 00:00:00 2001
Message-Id: <62ce0a157f245929f3b7471a3668e03792d14420.1368970061.git.plr.vincent@gmail.com>
In-Reply-To: <4fd4c88648d151e6c790c8ce49bcc128492f018b.1368970061.git.plr.vincent@gmail.com>
References: <4fd4c88648d151e6c790c8ce49bcc128492f018b.1368970061.git.plr.vincent@gmail.com>
From: Vincent Pelletier <plr.vincent@gmail.com>
Date: Sun, 19 May 2013 15:10:41 +0200
Subject: [2/2] [RFC] libata: Add a per-device sysfs control for atapi_dmadir

Some device require DMADIR to be enabled, but are not detected as such by
atapi_id_dmadir.
One such example is "Asus Serillel 2" SATA-host-to-PATA-device bridge: the
bridge itself requires DMADIR, even if the bridged device does not.

As atapi_dmadir module parameter can cause problems with some devices
(as per Tejun Heo's memory), enabling it may not be possible depending on
the hardware.

This patch implements a per-device sysfs control knob on port level, as
port is present before devices are attached, so configuration can happen
before device initialisation.

Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
---
 Documentation/ABI/testing/sysfs-ata |    8 ++++++++
 drivers/ata/libata-core.c           |    3 ++-
 drivers/ata/libata-transport.c      |   27 ++++++++++++++++++++++-----
 include/linux/libata.h              |    2 ++
 4 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-ata b/Documentation/ABI/testing/sysfs-ata
index 0a93215..c2dbe1a 100644
--- a/Documentation/ABI/testing/sysfs-ata
+++ b/Documentation/ABI/testing/sysfs-ata
@@ -20,6 +20,14 @@ nr_pmp_links (read)
 
 	If a SATA Port Multiplier (PM) is connected, number of link behind it.
 
+atapi_dmadir
+
+	Bitmask enabling dmadir for corresponding device if ATAPI.
+	1:	Enable dmadir for port's device 0
+	2:	Enable dmadir for port's device 1
+	(etc)
+	See also libata's atapi_dmadir module parameter.
+
 Files under /sys/class/ata_link
 -------------------------------
 
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index d121db7..1b4fcee 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2400,7 +2400,7 @@ int ata_dev_configure(struct ata_device *dev)
 			cdb_intr_string = ", CDB intr";
 		}
 
-		if (atapi_dmadir || atapi_id_dmadir(dev->id)) {
+		if (atapi_dmadir || ap->atapi_dmadir & (1 << dev->devno) || atapi_id_dmadir(dev->id)) {
 			dev->flags |= ATA_DFLAG_DMADIR;
 			dma_dir_string = ", DMADIR";
 		}
@@ -5643,6 +5643,7 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
 	ap->print_id = -1;
 	ap->host = host;
 	ap->dev = host->dev;
+	ap->atapi_dmadir = 0;
 
 #if defined(ATA_VERBOSE_DEBUG)
 	/* turn on all debugging levels */
diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index c04d393..6624d1d 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -37,7 +37,7 @@
 #include "libata.h"
 #include "libata-transport.h"
 
-#define ATA_PORT_ATTRS		2
+#define ATA_PORT_ATTRS		3
 #define ATA_LINK_ATTRS		3
 #define ATA_DEV_ATTRS		9
 
@@ -217,6 +217,22 @@ static DEVICE_ATTR(name, S_IRUGO, show_ata_port_##name, NULL)
 ata_port_simple_attr(nr_pmp_links, nr_pmp_links, "%d\n", int);
 ata_port_simple_attr(stats.idle_irq, idle_irq, "%ld\n", unsigned long);
 
+static ssize_t
+store_ata_port_atapi_dmadir(struct device *dev,
+			    struct device_attribute *attr,
+			    const char *buf, size_t count)
+{
+	struct ata_port *ap = transport_class_to_port(dev);
+	if (kstrtouint(buf, 0, &ap->atapi_dmadir) < 0)
+		return -EINVAL;
+	return count;
+}
+
+ata_port_show_simple(atapi_dmadir, atapi_dmadir, "%d\n", (int))
+static DEVICE_ATTR(atapi_dmadir, S_IRUGO | S_IWUSR,
+		   show_ata_port_atapi_dmadir,
+		   store_ata_port_atapi_dmadir);
+
 static DECLARE_TRANSPORT_CLASS(ata_port_class,
 			       "ata_port", NULL, NULL, NULL);
 
@@ -669,8 +685,8 @@ static int ata_tdev_add(struct ata_device *ata_dev)
 #define SETUP_LINK_ATTRIBUTE(field)					\
 	SETUP_TEMPLATE(link_attrs, field, S_IRUGO, 1)
 
-#define SETUP_PORT_ATTRIBUTE(field)					\
-	SETUP_TEMPLATE(port_attrs, field, S_IRUGO, 1)
+#define SETUP_PORT_ATTRIBUTE(field, mode)				\
+	SETUP_TEMPLATE(port_attrs, field, mode, 1)
 
 #define SETUP_DEV_ATTRIBUTE(field)					\
 	SETUP_TEMPLATE(dev_attrs, field, S_IRUGO, 1)
@@ -707,8 +723,9 @@ struct scsi_transport_template *ata_attach_transport(void)
 	transport_container_register(&i->dev_attr_cont);
 
 	count = 0;
-	SETUP_PORT_ATTRIBUTE(nr_pmp_links);
-	SETUP_PORT_ATTRIBUTE(idle_irq);
+	SETUP_PORT_ATTRIBUTE(nr_pmp_links, S_IRUGO);
+	SETUP_PORT_ATTRIBUTE(idle_irq, S_IRUGO);
+	SETUP_PORT_ATTRIBUTE(atapi_dmadir, S_IRUGO | S_IWUSR);
 	BUG_ON(count > ATA_PORT_ATTRS);
 	i->port_attrs[count] = NULL;
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index eae7a05..0f598c5 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -773,6 +773,8 @@ struct ata_port {
 	struct ata_link		link;		/* host default link */
 	struct ata_link		*slave_link;	/* see ata_slave_link_init() */
 
+	unsigned int		atapi_dmadir;   /* bitmask of dmadir-enabled device numbers */
+
 	int			nr_pmp_links;	/* nr of available PMP links */
 	struct ata_link		*pmp_link;	/* array of PMP links */
 	struct ata_link		*excl_link;	/* for PMP qc exclusion */
-- 
1.7.10.4

Re: [PATCH] make ata_exec_internal_sg honor DMADIR

[Tejun Heo]

Hello,

On Sun, May 19, 2013 at 03:31:22PM +0200, Vincent Pelletier wrote:
> Now that I got it working, I'm thining it would be better to automatically 
> fallback to enabling DMADIR per-device level during initialisation (just like 
> current fallbacks to 1.5Gbps and UDMA/33:PIO3, as visible in 1/2 commit 
> message).
> I believe it will slow down boot when such device is plugged in, though, any 
> idea on how this can be avoided ?

I don't really wanna go that way.  Those bridges always have been
something fringe and broken in ways which aren't fundamentally
fixable.  Fixing one would break another without anyway to properly
detect them.

So, I'm okay with having a knob for cases where the user knows what to
do but I don't think even that is something of much importance, and
I'm definitely not gonna do anything which may affect !bridge case
adversely.  Those bridges have always been a second-class citizen and
their importance has waned a lot.

> > 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?
> 
> Just to be clear about EH: the timeout happens during device  initialisation 
> (both at boot or on hotplug), not during error handling (or, maybe it happens 
> in both places, but for the same reason).
> I'm not comfortable with calling device discovery/initialisation as "error 
> handling", but maybe it's unambiguous when used to libata.

EH stands for exception handling and discovery / init definitely are
part of exception handling in libata speak.

> Subject: [1/2] libata: make ata_exec_internal_sg honor DMADIR
...
> ---
>  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;
> +	}

So, nope, I really don't want this.

> Subject: [2/2] [RFC] libata: Add a per-device sysfs control for atapi_dmadir
...
> +atapi_dmadir
> +
> +	Bitmask enabling dmadir for corresponding device if ATAPI.
> +	1:	Enable dmadir for port's device 0
> +	2:	Enable dmadir for port's device 1
> +	(etc)
> +	See also libata's atapi_dmadir module parameter.

Shouldn't this be a device property?

Thanks.

-- 
tejun

Re: [PATCH] make ata_exec_internal_sg honor DMADIR

[Vincent Pelletier]

Le lundi 20 mai 2013 01:38:18, Tejun Heo a écrit :
> I don't really wanna go that way.  Those bridges always have been
> something fringe and broken in ways which aren't fundamentally
> fixable.  Fixing one would break another without anyway to properly
> detect them.
> 
> So, I'm okay with having a knob for cases where the user knows what to
> do but I don't think even that is something of much importance, and
> I'm definitely not gonna do anything which may affect !bridge case
> adversely.

I understand.

> Those bridges have always been a second-class citizen and
> their importance has waned a lot.

Just a bit of background on why I got interested in Csaba's patch:

I never used my bridges when I originally got them (serillel was bundled with 
some motherboards I bought circa 2003), because a PATA controller was 
available anyway.
Since then, I changed all my hard drives progressively to SATA ones because of 
the capacity increase. But I still have many DVD drives from that time, 
because there was just no motivation to change them, and I don't have any 
optical SATA drive.
Then I bought a new motherboards a few weeks ago, without paying enough 
attention: there is no PATA controller on it. So I remembered those bridges 
and finally tried to use them.

Now, given that those bridges are old and were originally IMHO very close to 
useless, I'm indeed probably part of a fringe who didn't throw them away and 
remember them. SATA optical drives are quite cheap, maybe there is not even a 
financial motivation to look for such bridge.

> EH stands for exception handling and discovery / init definitely are
> part of exception handling in libata speak.

Thanks for the clarification.

> So, nope, I really don't want this.

Err, the body of this patch didn't change from my original submission, only 
the commit message has changed.

> > +atapi_dmadir
> > +
> > +	Bitmask enabling dmadir for corresponding device if ATAPI.
> > +	1:	Enable dmadir for port's device 0
> > +	2:	Enable dmadir for port's device 1
> > +	(etc)
> > +	See also libata's atapi_dmadir module parameter.
> 
> Shouldn't this be a device property?

Unplugging the drive would, in my understanding, loose the setting if stored 
at the device level. Is there another way to trigger a new initialisation 
attempt after changing the setting ?
Should I add a "rescan" device attribute ?

Regards,
-- 
Vincent Pelletier

Re: [PATCH] make ata_exec_internal_sg honor DMADIR

[Tejun Heo]

Hello,

On Mon, May 20, 2013 at 08:20:04AM +0200, Vincent Pelletier wrote:
> > So, nope, I really don't want this.
> 
> Err, the body of this patch didn't change from my original submission, only 
> the commit message has changed.

Heh, that's my jet-lagged brain thinking it was something else. :)
Sorry about that.

> > > +atapi_dmadir
> > > +
> > > +	Bitmask enabling dmadir for corresponding device if ATAPI.
> > > +	1:	Enable dmadir for port's device 0
> > > +	2:	Enable dmadir for port's device 1
> > > +	(etc)
> > > +	See also libata's atapi_dmadir module parameter.
> > 
> > Shouldn't this be a device property?
> 
> Unplugging the drive would, in my understanding, loose the setting if stored 
> at the device level. Is there another way to trigger a new initialisation 
> attempt after changing the setting ?
> Should I add a "rescan" device attribute ?

But isn't that what we want?  We don't really know to which side the
bridge is attached but given SATA supports hotplug while PATA doesn't,
it's natural to assume the bridge to be part of the device rather than
bus and reset the state on device hotplug, no?

Thanks.

-- 
tejun

Re: [PATCH] make ata_exec_internal_sg honor DMADIR

[Vincent Pelletier]

Hello,

Le lundi 20 mai 2013 09:30:12, Tejun Heo a écrit :
> > Unplugging the drive would, in my understanding, loose the setting if
> > stored at the device level. Is there another way to trigger a new
> > initialisation attempt after changing the setting ?
> > Should I add a "rescan" device attribute ?
> 
> But isn't that what we want?  We don't really know to which side the
> bridge is attached but given SATA supports hotplug while PATA doesn't,
> it's natural to assume the bridge to be part of the device rather than
> bus and reset the state on device hotplug, no?

Semantically, I agree that the bridge is part of the device.

Putting the knob on the port is just a way I thought about to hold the 
configuration before the drive is plugged into the system (because so far I 
was focussed on host-side being hot-pluggable, I indeed didn't consider the 
opposite situation) so it can be used before libata tries to access the 
device.

So it would be something like:
- plug device (or boot up)
  -> detection times out, device "half" configured (sysfs node present, drive
     not usable)
- cd $DEVICE_IN_SYSFS
- echo 1 > atapi_dmadir
- echo 1 > (rescan|reinit|...)

If it's ok, I'll write a patch to add a rescan write-only file (will also be 
independent from the 2 other patches).
I'll certainly need a few days to get it though: I feel I'll have some 
difficulty finding the right function to call, and to call it correctly (I 
feel interrupt handler & locking, which I'm not familiar with at kernel 
level). Pointers welcome !

Regards,
-- 
Vincent Pelletier

Re: [PATCH] make ata_exec_internal_sg honor DMADIR

[Tejun Heo]

Hello,

On Mon, May 20, 2013 at 12:51:20PM +0200, Vincent Pelletier wrote:
> Putting the knob on the port is just a way I thought about to hold the 
> configuration before the drive is plugged into the system (because so far I 
> was focussed on host-side being hot-pluggable, I indeed didn't consider the 
> opposite situation) so it can be used before libata tries to access the 
> device.

Right, we don't even have the sysfs node before probing.  I forgot
about that.

> So it would be something like:
> - plug device (or boot up)
>   -> detection times out, device "half" configured (sysfs node present, drive
>      not usable)
> - cd $DEVICE_IN_SYSFS
> - echo 1 > atapi_dmadir
> - echo 1 > (rescan|reinit|...)
> 
> If it's ok, I'll write a patch to add a rescan write-only file (will also be 
> independent from the 2 other patches).

Ugh... so, this is inherently racy between the probing code and admin.
Maybe we should just implement a new libata.force param and forget
about dynamic configuration?

One more thing.  In the ata_exec_internal_sg(), DMADIR should be set
iff DMA is being used, right?  So, it should also check tf->protocol.
It prolly should test tf->protocol == ATAPI_PROT_DMA instead of cdb.

Thanks.

-- 
tejun

Re: [PATCH] make ata_exec_internal_sg honor DMADIR

[Vincent Pelletier]

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

Hello,

Le lundi 20 mai 2013 20:59:29, Tejun Heo a écrit :
> Ugh... so, this is inherently racy between the probing code and admin.
> Maybe we should just implement a new libata.force param and forget
> about dynamic configuration?

Something like this ? (2/2 - untested)
Is "horkage" just another way to say "quirks" in this context ? Google 
translate doesn't help, and urbandictionary has too many entries for "hork" to 
make me confident.
Alternatively, I would add a "dflags" field to struct ata_force_param, and 
reuse ATA_DFLAG_DMADIR instead of defining a new enum item.

As this completely supersedes the atapi_dmadir module argument, is there a way 
to deprecate it (if at all a good practice) ?

> One more thing.  In the ata_exec_internal_sg(), DMADIR should be set
> iff DMA is being used, right?  So, it should also check tf->protocol.
> It prolly should test tf->protocol == ATAPI_PROT_DMA instead of cdb.

Sounds logical. I lack ATA[PI] background a lot, so I'm guessing a lot.
I updated the patch (1/2 - untested).

I should find the time to test both patches tomorrow.

Regards,
-- 
Vincent Pelletier

[-- Attachment #2: 0002-RFC-libata-Add-no-atapi_dmadir-horkage.patch --]
[-- Type: text/x-patch, Size: 2714 bytes --]

From 1b491075d03ba3830296331e3c5d0e259f6803b3 Mon Sep 17 00:00:00 2001
Message-Id: <1b491075d03ba3830296331e3c5d0e259f6803b3.1369081552.git.plr.vincent@gmail.com>
In-Reply-To: <a2fe028e6736afa9d6b6612c713844c411ffca5c.1369081552.git.plr.vincent@gmail.com>
References: <a2fe028e6736afa9d6b6612c713844c411ffca5c.1369081552.git.plr.vincent@gmail.com>
From: Vincent Pelletier <plr.vincent@gmail.com>
Date: Mon, 20 May 2013 22:24:25 +0200
Subject: [2/2] [RFC] libata: Add [no]atapi_dmadir horkage

Some device require DMADIR to be enabled, but are not detected as such by
atapi_id_dmadir.
One such example is "Asus Serillel 2" SATA-host-to-PATA-device bridge: the
bridge itself requires DMADIR, even if the bridged device does not.

As atapi_dmadir module parameter can cause problems with some devices
(as per Tejun Heo's memory), enabling it globally may not be possible
depending on the hardware.

This patch adds atapi_dmadir in the form of a "force" horkage value,
allowing global, per-bus and per-device control.

Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
---
 drivers/ata/libata-core.c |    4 +++-
 include/linux/libata.h    |    1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 16c3345..e6ed443 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2399,7 +2399,7 @@ int ata_dev_configure(struct ata_device *dev)
 			cdb_intr_string = ", CDB intr";
 		}
 
-		if (atapi_dmadir || atapi_id_dmadir(dev->id)) {
+		if (atapi_dmadir || (dev->horkage & ATA_HORKAGE_ATAPI_DMADIR) || atapi_id_dmadir(dev->id)) {
 			dev->flags |= ATA_DFLAG_DMADIR;
 			dma_dir_string = ", DMADIR";
 		}
@@ -6498,6 +6498,8 @@ static int __init ata_parse_force_one(char **cur,
 		{ "nosrst",	.lflags		= ATA_LFLAG_NO_SRST },
 		{ "norst",	.lflags		= ATA_LFLAG_NO_HRST | ATA_LFLAG_NO_SRST },
 		{ "rstonce",	.lflags		= ATA_LFLAG_RST_ONCE },
+		{ "atapi_dmadir", .horkage_on	= ATA_HORKAGE_ATAPI_DMADIR },
+		{ "noatapi_dmadir", .horkage_off = ATA_HORKAGE_ATAPI_DMADIR },
 	};
 	char *start = *cur, *p = *cur;
 	char *id, *val, *endp;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index eae7a05..9a4c194 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -399,6 +399,7 @@ enum {
 	ATA_HORKAGE_BROKEN_FPDMA_AA	= (1 << 15),	/* skip AA */
 	ATA_HORKAGE_DUMP_ID	= (1 << 16),	/* dump IDENTIFY data */
 	ATA_HORKAGE_MAX_SEC_LBA48 = (1 << 17),	/* Set max sects to 65535 */
+	ATA_HORKAGE_ATAPI_DMADIR = (1 << 18),	/* device requires dmadir */
 
 	 /* DMA mask for user DMA control: User visible values; DO NOT
 	    renumber */
-- 
1.7.10.4


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

From a2fe028e6736afa9d6b6612c713844c411ffca5c Mon Sep 17 00:00:00 2001
Message-Id: <a2fe028e6736afa9d6b6612c713844c411ffca5c.1369081552.git.plr.vincent@gmail.com>
From: Vincent Pelletier <plr.vincent@gmail.com>
Date: Sat, 18 May 2013 18:44:04 +0200
Subject: [1/2] libata: make ata_exec_internal_sg honor DMADIR
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

libata honors DMADIR for regular commands, but not for internal commands
used (among other) during device initialisation.

This makes SATA-host-to-PATA-device bridges based on Silicon Image SiL3611
(such as "Abit Serillel 2") end up disabled when used with an ATAPI device
after a few tries.

Log output of the bridge being hot-plugged with an ATAPI drive:

  [ 9631.212901] ata1: exception Emask 0x10 SAct 0x0 SErr 0x40c0000 action 0xe frozen
  [ 9631.212913] ata1: irq_stat 0x00000040, connection status changed
  [ 9631.212923] ata1: SError: { CommWake 10B8B DevExch }
  [ 9631.212939] ata1: hard resetting link
  [ 9632.104962] ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
  [ 9632.106393] ata1.00: ATAPI: PIONEER DVD-RW  DVR-115, 1.06, max UDMA/33
  [ 9632.106407] ata1.00: applying bridge limits
  [ 9632.108151] ata1.00: configured for UDMA/33
  [ 9637.105303] ata1.00: qc timeout (cmd 0xa0)
  [ 9637.105324] ata1.00: failed to clear UNIT ATTENTION (err_mask=0x5)
  [ 9637.105335] ata1: hard resetting link
  [ 9638.044599] ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
  [ 9638.047878] ata1.00: configured for UDMA/33
  [ 9643.044933] ata1.00: qc timeout (cmd 0xa0)
  [ 9643.044953] ata1.00: failed to clear UNIT ATTENTION (err_mask=0x5)
  [ 9643.044963] ata1: limiting SATA link speed to 1.5 Gbps
  [ 9643.044971] ata1.00: limiting speed to UDMA/33:PIO3
  [ 9643.044979] ata1: hard resetting link
  [ 9643.984225] ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 310)
  [ 9643.987471] ata1.00: configured for UDMA/33
  [ 9648.984591] ata1.00: qc timeout (cmd 0xa0)
  [ 9648.984612] ata1.00: failed to clear UNIT ATTENTION (err_mask=0x5)
  [ 9648.984619] ata1.00: disabled
  [ 9649.000593] ata1: hard resetting link
  [ 9649.939902] ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 310)
  [ 9649.955864] ata1: EH complete

With this patch, the drive enumerates correctly when libata is loaded with
atapi_dmadir=1:

  [ 9891.810863] ata1: exception Emask 0x10 SAct 0x0 SErr 0x40c0000 action 0xe frozen
  [ 9891.810874] ata1: irq_stat 0x00000040, connection status changed
  [ 9891.810884] ata1: SError: { CommWake 10B8B DevExch }
  [ 9891.810900] ata1: hard resetting link
  [ 9892.762105] ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
  [ 9892.763544] ata1.00: ATAPI: PIONEER DVD-RW  DVR-115, 1.06, max UDMA/33, DMADIR
  [ 9892.763558] ata1.00: applying bridge limits
  [ 9892.765393] ata1.00: configured for UDMA/33
  [ 9892.786063] ata1: EH complete
  [ 9892.792062] scsi 0:0:0:0: CD-ROM            PIONEER  DVD-RW  DVR-115  1.06 PQ: 0 ANSI: 5
  [ 9892.798455] sr2: scsi3-mmc drive: 12x/12x writer dvd-ram cd/rw xa/form2 cdda tray
  [ 9892.798837] sr 0:0:0:0: Attached scsi CD-ROM sr2
  [ 9892.799109] sr 0:0:0:0: Attached scsi generic sg6 type 5

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 |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 63c743b..16c3345 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1602,6 +1602,10 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
 	qc->tf = *tf;
 	if (cdb)
 		memcpy(qc->cdb, cdb, ATAPI_CDB_LEN);
+	if (tf->protocol == ATAPI_PROT_DMA && 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

Re: [PATCH] make ata_exec_internal_sg honor DMADIR

[Tejun Heo]

Hello,

On Mon, May 20, 2013 at 10:43:14PM +0200, Vincent Pelletier wrote:
> Something like this ? (2/2 - untested)
> Is "horkage" just another way to say "quirks" in this context ? Google 
> translate doesn't help, and urbandictionary has too many entries for "hork" to 
> make me confident.

I think so.  It was originally named by Alan Cox who sometimes plays
with old English dialects, Yiddish, whatnot.

> Alternatively, I would add a "dflags" field to struct ata_force_param, and 
> reuse ATA_DFLAG_DMADIR instead of defining a new enum item.
> 
> As this completely supersedes the atapi_dmadir module argument, is there a way 
> to deprecate it (if at all a good practice) ?

I'd just leave it alone.  It's not like it adds a lot of complexity.

> @@ -6498,6 +6498,8 @@ static int __init ata_parse_force_one(char **cur,
>  		{ "nosrst",	.lflags		= ATA_LFLAG_NO_SRST },
>  		{ "norst",	.lflags		= ATA_LFLAG_NO_HRST | ATA_LFLAG_NO_SRST },
>  		{ "rstonce",	.lflags		= ATA_LFLAG_RST_ONCE },
> +		{ "atapi_dmadir", .horkage_on	= ATA_HORKAGE_ATAPI_DMADIR },
> +		{ "noatapi_dmadir", .horkage_off = ATA_HORKAGE_ATAPI_DMADIR },

I don't think we need noatapi_dmadir as the default is off anyway.

Thanks.

-- 
tejun

Re: [PATCH] make ata_exec_internal_sg honor DMADIR

[Vincent Pelletier]

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

Hi.

Attached should be the final patches, unless someone has objections.

Changes since previous version:
- noatapi_dmadir dropped
- flag documented
- slight change to 2/2 commit message
- tested

Regards,
-- 
Vincent Pelletier

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

From a2fe028e6736afa9d6b6612c713844c411ffca5c Mon Sep 17 00:00:00 2001
Message-Id: <a2fe028e6736afa9d6b6612c713844c411ffca5c.1369168262.git.plr.vincent@gmail.com>
From: Vincent Pelletier <plr.vincent@gmail.com>
Date: Sat, 18 May 2013 18:44:04 +0200
Subject: [1/2] libata: make ata_exec_internal_sg honor DMADIR
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

libata honors DMADIR for regular commands, but not for internal commands
used (among other) during device initialisation.

This makes SATA-host-to-PATA-device bridges based on Silicon Image SiL3611
(such as "Abit Serillel 2") end up disabled when used with an ATAPI device
after a few tries.

Log output of the bridge being hot-plugged with an ATAPI drive:

  [ 9631.212901] ata1: exception Emask 0x10 SAct 0x0 SErr 0x40c0000 action 0xe frozen
  [ 9631.212913] ata1: irq_stat 0x00000040, connection status changed
  [ 9631.212923] ata1: SError: { CommWake 10B8B DevExch }
  [ 9631.212939] ata1: hard resetting link
  [ 9632.104962] ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
  [ 9632.106393] ata1.00: ATAPI: PIONEER DVD-RW  DVR-115, 1.06, max UDMA/33
  [ 9632.106407] ata1.00: applying bridge limits
  [ 9632.108151] ata1.00: configured for UDMA/33
  [ 9637.105303] ata1.00: qc timeout (cmd 0xa0)
  [ 9637.105324] ata1.00: failed to clear UNIT ATTENTION (err_mask=0x5)
  [ 9637.105335] ata1: hard resetting link
  [ 9638.044599] ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
  [ 9638.047878] ata1.00: configured for UDMA/33
  [ 9643.044933] ata1.00: qc timeout (cmd 0xa0)
  [ 9643.044953] ata1.00: failed to clear UNIT ATTENTION (err_mask=0x5)
  [ 9643.044963] ata1: limiting SATA link speed to 1.5 Gbps
  [ 9643.044971] ata1.00: limiting speed to UDMA/33:PIO3
  [ 9643.044979] ata1: hard resetting link
  [ 9643.984225] ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 310)
  [ 9643.987471] ata1.00: configured for UDMA/33
  [ 9648.984591] ata1.00: qc timeout (cmd 0xa0)
  [ 9648.984612] ata1.00: failed to clear UNIT ATTENTION (err_mask=0x5)
  [ 9648.984619] ata1.00: disabled
  [ 9649.000593] ata1: hard resetting link
  [ 9649.939902] ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 310)
  [ 9649.955864] ata1: EH complete

With this patch, the drive enumerates correctly when libata is loaded with
atapi_dmadir=1:

  [ 9891.810863] ata1: exception Emask 0x10 SAct 0x0 SErr 0x40c0000 action 0xe frozen
  [ 9891.810874] ata1: irq_stat 0x00000040, connection status changed
  [ 9891.810884] ata1: SError: { CommWake 10B8B DevExch }
  [ 9891.810900] ata1: hard resetting link
  [ 9892.762105] ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
  [ 9892.763544] ata1.00: ATAPI: PIONEER DVD-RW  DVR-115, 1.06, max UDMA/33, DMADIR
  [ 9892.763558] ata1.00: applying bridge limits
  [ 9892.765393] ata1.00: configured for UDMA/33
  [ 9892.786063] ata1: EH complete
  [ 9892.792062] scsi 0:0:0:0: CD-ROM            PIONEER  DVD-RW  DVR-115  1.06 PQ: 0 ANSI: 5
  [ 9892.798455] sr2: scsi3-mmc drive: 12x/12x writer dvd-ram cd/rw xa/form2 cdda tray
  [ 9892.798837] sr 0:0:0:0: Attached scsi CD-ROM sr2
  [ 9892.799109] sr 0:0:0:0: Attached scsi generic sg6 type 5

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 |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 63c743b..16c3345 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1602,6 +1602,10 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
 	qc->tf = *tf;
 	if (cdb)
 		memcpy(qc->cdb, cdb, ATAPI_CDB_LEN);
+	if (tf->protocol == ATAPI_PROT_DMA && 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


[-- Attachment #3: 0002-libata-Add-atapi_dmadir-force-flag.patch --]
[-- Type: text/x-patch, Size: 3217 bytes --]

From 0fbfe1b12661030142a91cbae4d452d97440b2f6 Mon Sep 17 00:00:00 2001
Message-Id: <0fbfe1b12661030142a91cbae4d452d97440b2f6.1369168262.git.plr.vincent@gmail.com>
In-Reply-To: <a2fe028e6736afa9d6b6612c713844c411ffca5c.1369168262.git.plr.vincent@gmail.com>
References: <a2fe028e6736afa9d6b6612c713844c411ffca5c.1369168262.git.plr.vincent@gmail.com>
From: Vincent Pelletier <plr.vincent@gmail.com>
Date: Tue, 21 May 2013 22:30:58 +0200
Subject: [2/2] libata: Add atapi_dmadir force flag

Some device require DMADIR to be enabled, but are not detected as such by
atapi_id_dmadir.
One such example is "Asus Serillel 2" SATA-host-to-PATA-device bridge: the
bridge itself requires DMADIR, even if the bridged device does not.

As atapi_dmadir module parameter can cause problems with some devices
(as per Tejun Heo's memory), enabling it globally may not be possible
depending on the hardware.

This patch adds atapi_dmadir in the form of a "force" horkage value,
allowing global, per-bus and per-device control.

Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
---
 Documentation/kernel-parameters.txt |    2 ++
 drivers/ata/libata-core.c           |    3 ++-
 include/linux/libata.h              |    1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 8ccbf27..c679714 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1414,6 +1414,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 
 			* dump_id: dump IDENTIFY data.
 
+			* atapi_dmadir: Enable ATAPI DMADIR bridge support
+
 			If there are multiple matching configurations changing
 			the same attribute, the last one is used.
 
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 16c3345..7db6375 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2399,7 +2399,7 @@ int ata_dev_configure(struct ata_device *dev)
 			cdb_intr_string = ", CDB intr";
 		}
 
-		if (atapi_dmadir || atapi_id_dmadir(dev->id)) {
+		if (atapi_dmadir || (dev->horkage & ATA_HORKAGE_ATAPI_DMADIR) || atapi_id_dmadir(dev->id)) {
 			dev->flags |= ATA_DFLAG_DMADIR;
 			dma_dir_string = ", DMADIR";
 		}
@@ -6498,6 +6498,7 @@ static int __init ata_parse_force_one(char **cur,
 		{ "nosrst",	.lflags		= ATA_LFLAG_NO_SRST },
 		{ "norst",	.lflags		= ATA_LFLAG_NO_HRST | ATA_LFLAG_NO_SRST },
 		{ "rstonce",	.lflags		= ATA_LFLAG_RST_ONCE },
+		{ "atapi_dmadir", .horkage_on	= ATA_HORKAGE_ATAPI_DMADIR },
 	};
 	char *start = *cur, *p = *cur;
 	char *id, *val, *endp;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index eae7a05..9a4c194 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -399,6 +399,7 @@ enum {
 	ATA_HORKAGE_BROKEN_FPDMA_AA	= (1 << 15),	/* skip AA */
 	ATA_HORKAGE_DUMP_ID	= (1 << 16),	/* dump IDENTIFY data */
 	ATA_HORKAGE_MAX_SEC_LBA48 = (1 << 17),	/* Set max sects to 65535 */
+	ATA_HORKAGE_ATAPI_DMADIR = (1 << 18),	/* device requires dmadir */
 
 	 /* DMA mask for user DMA control: User visible values; DO NOT
 	    renumber */
-- 
1.7.10.4

[PATCH 1/2] libata: make ata_exec_internal_sg honor DMADIR

[Tejun Heo]

Applied to libata/for-3.10-fixes w/ stable cc'd.

Thanks a lot!

From e771451c0a831d96a7c14b0ca8a8ec671d98567b Mon Sep 17 00:00:00 2001
From: Vincent Pelletier <plr.vincent@gmail.com>
Date: Sat, 18 May 2013 18:44:04 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

libata honors DMADIR for regular commands, but not for internal commands
used (among other) during device initialisation.

This makes SATA-host-to-PATA-device bridges based on Silicon Image SiL3611
(such as "Abit Serillel 2") end up disabled when used with an ATAPI device
after a few tries.

Log output of the bridge being hot-plugged with an ATAPI drive:

  [ 9631.212901] ata1: exception Emask 0x10 SAct 0x0 SErr 0x40c0000 action 0xe frozen
  [ 9631.212913] ata1: irq_stat 0x00000040, connection status changed
  [ 9631.212923] ata1: SError: { CommWake 10B8B DevExch }
  [ 9631.212939] ata1: hard resetting link
  [ 9632.104962] ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
  [ 9632.106393] ata1.00: ATAPI: PIONEER DVD-RW  DVR-115, 1.06, max UDMA/33
  [ 9632.106407] ata1.00: applying bridge limits
  [ 9632.108151] ata1.00: configured for UDMA/33
  [ 9637.105303] ata1.00: qc timeout (cmd 0xa0)
  [ 9637.105324] ata1.00: failed to clear UNIT ATTENTION (err_mask=0x5)
  [ 9637.105335] ata1: hard resetting link
  [ 9638.044599] ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
  [ 9638.047878] ata1.00: configured for UDMA/33
  [ 9643.044933] ata1.00: qc timeout (cmd 0xa0)
  [ 9643.044953] ata1.00: failed to clear UNIT ATTENTION (err_mask=0x5)
  [ 9643.044963] ata1: limiting SATA link speed to 1.5 Gbps
  [ 9643.044971] ata1.00: limiting speed to UDMA/33:PIO3
  [ 9643.044979] ata1: hard resetting link
  [ 9643.984225] ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 310)
  [ 9643.987471] ata1.00: configured for UDMA/33
  [ 9648.984591] ata1.00: qc timeout (cmd 0xa0)
  [ 9648.984612] ata1.00: failed to clear UNIT ATTENTION (err_mask=0x5)
  [ 9648.984619] ata1.00: disabled
  [ 9649.000593] ata1: hard resetting link
  [ 9649.939902] ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 310)
  [ 9649.955864] ata1: EH complete

With this patch, the drive enumerates correctly when libata is loaded with
atapi_dmadir=1:

  [ 9891.810863] ata1: exception Emask 0x10 SAct 0x0 SErr 0x40c0000 action 0xe frozen
  [ 9891.810874] ata1: irq_stat 0x00000040, connection status changed
  [ 9891.810884] ata1: SError: { CommWake 10B8B DevExch }
  [ 9891.810900] ata1: hard resetting link
  [ 9892.762105] ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
  [ 9892.763544] ata1.00: ATAPI: PIONEER DVD-RW  DVR-115, 1.06, max UDMA/33, DMADIR
  [ 9892.763558] ata1.00: applying bridge limits
  [ 9892.765393] ata1.00: configured for UDMA/33
  [ 9892.786063] ata1: EH complete
  [ 9892.792062] scsi 0:0:0:0: CD-ROM            PIONEER  DVD-RW  DVR-115  1.06 PQ: 0 ANSI: 5
  [ 9892.798455] sr2: scsi3-mmc drive: 12x/12x writer dvd-ram cd/rw xa/form2 cdda tray
  [ 9892.798837] sr 0:0:0:0: Attached scsi CD-ROM sr2
  [ 9892.799109] sr 0:0:0:0: Attached scsi generic sg6 type 5

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

tj: minor formatting changes.

Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: stable@vger.kernel.org
---
 drivers/ata/libata-core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index d35524c..f218427 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1602,6 +1602,12 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
 	qc->tf = *tf;
 	if (cdb)
 		memcpy(qc->cdb, cdb, ATAPI_CDB_LEN);
+
+	/* some SATA bridges need us to indicate data xfer direction */
+	if (tf->protocol == ATAPI_PROT_DMA && (dev->flags & ATA_DFLAG_DMADIR) &&
+	    dma_dir == DMA_FROM_DEVICE)
+		qc->tf.feature |= ATAPI_DMADIR;
+
 	qc->flags |= ATA_QCFLAG_RESULT_TF;
 	qc->dma_dir = dma_dir;
 	if (dma_dir != DMA_NONE) {
-- 
1.8.1.4

[PATCH 2/2] libata: Add atapi_dmadir force flag

[Tejun Heo]

Applied to libata/for-3.11.  Thanks a lot!

>From 966fbe193f47c68e70a80ec9991098e88e7959cb Mon Sep 17 00:00:00 2001
From: Vincent Pelletier <plr.vincent@gmail.com>
Date: Tue, 21 May 2013 22:30:58 +0200

Some device require DMADIR to be enabled, but are not detected as such
by atapi_id_dmadir.  One such example is "Asus Serillel 2"
SATA-host-to-PATA-device bridge: the bridge itself requires DMADIR,
even if the bridged device does not.

As atapi_dmadir module parameter can cause problems with some devices
(as per Tejun Heo's memory), enabling it globally may not be possible
depending on the hardware.

This patch adds atapi_dmadir in the form of a "force" horkage value,
allowing global, per-bus and per-device control.

Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
Signed-off-by: Tejun Heo <tj@kernel.org>
---
 Documentation/kernel-parameters.txt | 2 ++
 drivers/ata/libata-core.c           | 3 ++-
 include/linux/libata.h              | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index c3bfacb..489815e 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1456,6 +1456,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 
 			* dump_id: dump IDENTIFY data.
 
+			* atapi_dmadir: Enable ATAPI DMADIR bridge support
+
 			If there are multiple matching configurations changing
 			the same attribute, the last one is used.
 
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 5f7d5f9..c97a244 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2395,7 +2395,7 @@ int ata_dev_configure(struct ata_device *dev)
 			cdb_intr_string = ", CDB intr";
 		}
 
-		if (atapi_dmadir || atapi_id_dmadir(dev->id)) {
+		if (atapi_dmadir || (dev->horkage & ATA_HORKAGE_ATAPI_DMADIR) || atapi_id_dmadir(dev->id)) {
 			dev->flags |= ATA_DFLAG_DMADIR;
 			dma_dir_string = ", DMADIR";
 		}
@@ -6496,6 +6496,7 @@ static int __init ata_parse_force_one(char **cur,
 		{ "nosrst",	.lflags		= ATA_LFLAG_NO_SRST },
 		{ "norst",	.lflags		= ATA_LFLAG_NO_HRST | ATA_LFLAG_NO_SRST },
 		{ "rstonce",	.lflags		= ATA_LFLAG_RST_ONCE },
+		{ "atapi_dmadir", .horkage_on	= ATA_HORKAGE_ATAPI_DMADIR },
 	};
 	char *start = *cur, *p = *cur;
 	char *id, *val, *endp;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 47e0292..c886dc87 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -399,6 +399,7 @@ enum {
 	ATA_HORKAGE_BROKEN_FPDMA_AA	= (1 << 15),	/* skip AA */
 	ATA_HORKAGE_DUMP_ID	= (1 << 16),	/* dump IDENTIFY data */
 	ATA_HORKAGE_MAX_SEC_LBA48 = (1 << 17),	/* Set max sects to 65535 */
+	ATA_HORKAGE_ATAPI_DMADIR = (1 << 18),	/* device requires dmadir */
 
 	 /* DMA mask for user DMA control: User visible values; DO NOT
 	    renumber */
-- 
1.8.1.4