Message ID | 201305191531.23110.plr.vincent@gmail.com |
---|---|
State | Not Applicable |
Delegated to: | David Miller |
Headers | show |
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.
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,
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.
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,
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.
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