diff mbox

make ata_exec_internal_sg honor DMADIR

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

Commit Message

Vincent Pelletier May 19, 2013, 1:31 p.m. UTC 
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,

Comments

Tejun Heo May 19, 2013, 11:38 p.m. UTC | #1 
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.
Vincent Pelletier May 20, 2013, 6:20 a.m. UTC | #2 
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,
Tejun Heo May 20, 2013, 7:30 a.m. UTC | #3 
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.
Vincent Pelletier May 20, 2013, 10:51 a.m. UTC | #4 
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,
Tejun Heo May 20, 2013, 6:59 p.m. UTC | #5 
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.
diff mbox

Patch

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