Skip to contentSkip to conversation

libata: always use polling SETXFER

PATCH In review Tejun Heo 6 participants 9 messages May 27, 2007 – Jun 4, 2007 libata

lore
Share & tools
lore

Tejun Heo

author
#0 lore raw

PATCH 1/1 libata: always use polling SETXFER

Several people have reported LITE-ON LTR-48246S detection failed because SETXFER fails. It seems the device raises IRQ too early after SETXFER. This is controller independent. The same problem has been reported for different controllers.

So, now we have pata_via where the controller raises IRQ before it's ready after SETXFER and a device which does similar thing. This patch makes libata always execute SETXFER via polling. As this only happens during EH, performance impact is nil. Setting ATA_TFLAG_POLLING is also moved from issue hot path to ata_dev_set_xfermode() - the only place where SETXFER can be issued.

Note that ATA_TFLAG_POLLING applies only to drivers which implement SFF TF interface and use libata HSM. More advanced controllers ignore the flag. This doesn't matter for this fix as SFF TF controllers are the problematic ones.

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/libata-core.c |   13 ++++---------
 drivers/ata/pata_via.c    |   12 ++++++------
 include/linux/libata.h    |    1 -
 3 files changed, 10 insertions(+), 16 deletions(-)
drivers/ata/libata-core.c +4 -9
index 3ca9c61..4d6de65 100644
@@ -3932,10 +3932,13 @@ static unsigned int ata_dev_set_xfermode
 	/* set up set-features taskfile */
 	DPRINTK("set features - xfer mode\n");
 
+	/* Some controllers and ATAPI devices show flaky interrupt
+	 * behavior after setting xfer mode.  Use polling instead.
+	 */
 	ata_tf_init(dev, &tf);
 	tf.command = ATA_CMD_SET_FEATURES;
 	tf.feature = SETFEATURES_XFER;
-	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE | ATA_TFLAG_POLLING;
 	tf.protocol = ATA_PROT_NODATA;
 	tf.nsect = dev->xfer_mode;
 

@@ -5413,14 +5416,6 @@ unsigned int ata_qc_issue_prot(struct at
 		}
 	}
 
-	/* Some controllers show flaky interrupt behavior after
-	 * setting xfer mode.  Use polling instead.
-	 */
-	if (unlikely(qc->tf.command == ATA_CMD_SET_FEATURES &&
-		     qc->tf.feature == SETFEATURES_XFER) &&
-	    (ap->flags & ATA_FLAG_SETXFER_POLLING))
-		qc->tf.flags |= ATA_TFLAG_POLLING;
-
 	/* select the device */
 	ata_dev_select(ap, qc->dev->devno, 1, 0);
drivers/ata/pata_via.c +6 -6
index a8462f1..63eca29 100644
@@ -452,7 +452,7 @@ static int via_init_one(struct pci_dev *
 	/* Early VIA without UDMA support */
 	static const struct ata_port_info via_mwdma_info = {
 		.sht = &via_sht,
-		.flags = ATA_FLAG_SLAVE_POSS | ATA_FLAG_SETXFER_POLLING,
+		.flags = ATA_FLAG_SLAVE_POSS,
 		.pio_mask = 0x1f,
 		.mwdma_mask = 0x07,
 		.port_ops = &via_port_ops

@@ -460,7 +460,7 @@ static int via_init_one(struct pci_dev *
 	/* Ditto with IRQ masking required */
 	static const struct ata_port_info via_mwdma_info_borked = {
 		.sht = &via_sht,
-		.flags = ATA_FLAG_SLAVE_POSS | ATA_FLAG_SETXFER_POLLING,
+		.flags = ATA_FLAG_SLAVE_POSS,
 		.pio_mask = 0x1f,
 		.mwdma_mask = 0x07,
 		.port_ops = &via_port_ops_noirq,

@@ -468,7 +468,7 @@ static int via_init_one(struct pci_dev *
 	/* VIA UDMA 33 devices (and borked 66) */
 	static const struct ata_port_info via_udma33_info = {
 		.sht = &via_sht,
-		.flags = ATA_FLAG_SLAVE_POSS | ATA_FLAG_SETXFER_POLLING,
+		.flags = ATA_FLAG_SLAVE_POSS,
 		.pio_mask = 0x1f,
 		.mwdma_mask = 0x07,
 		.udma_mask = 0x7,

@@ -477,7 +477,7 @@ static int via_init_one(struct pci_dev *
 	/* VIA UDMA 66 devices */
 	static const struct ata_port_info via_udma66_info = {
 		.sht = &via_sht,
-		.flags = ATA_FLAG_SLAVE_POSS | ATA_FLAG_SETXFER_POLLING,
+		.flags = ATA_FLAG_SLAVE_POSS,
 		.pio_mask = 0x1f,
 		.mwdma_mask = 0x07,
 		.udma_mask = 0x1f,

@@ -486,7 +486,7 @@ static int via_init_one(struct pci_dev *
 	/* VIA UDMA 100 devices */
 	static const struct ata_port_info via_udma100_info = {
 		.sht = &via_sht,
-		.flags = ATA_FLAG_SLAVE_POSS | ATA_FLAG_SETXFER_POLLING,
+		.flags = ATA_FLAG_SLAVE_POSS,
 		.pio_mask = 0x1f,
 		.mwdma_mask = 0x07,
 		.udma_mask = 0x3f,

@@ -495,7 +495,7 @@ static int via_init_one(struct pci_dev *
 	/* UDMA133 with bad AST (All current 133) */
 	static const struct ata_port_info via_udma133_info = {
 		.sht = &via_sht,
-		.flags = ATA_FLAG_SLAVE_POSS | ATA_FLAG_SETXFER_POLLING,
+		.flags = ATA_FLAG_SLAVE_POSS,
 		.pio_mask = 0x1f,
 		.mwdma_mask = 0x07,
 		.udma_mask = 0x7f,	/* FIXME: should check north bridge */
include/linux/libata.h +0 -1
index 85f7b1b..a6a3113 100644
@@ -171,7 +171,6 @@ enum {
 	ATA_FLAG_SKIP_D2H_BSY	= (1 << 12), /* can't wait for the first D2H
 					      * Register FIS clearing BSY */
 	ATA_FLAG_DEBUGMSG	= (1 << 13),
-	ATA_FLAG_SETXFER_POLLING= (1 << 14), /* use polling for SETXFER */
 	ATA_FLAG_IGN_SIMPLEX	= (1 << 15), /* ignore SIMPLEX */
 	ATA_FLAG_NO_IORDY	= (1 << 16), /* controller lacks iordy */
 	ATA_FLAG_ACPI_SATA	= (1 << 17), /* need native SATA ACPI layout */
#1 lore raw
Quoted 17 lines · Tejun Heo

Several people have reported LITE-ON LTR-48246S detection failed because SETXFER fails. It seems the device raises IRQ too early after SETXFER. This is controller independent. The same problem has been reported for different controllers.

So, now we have pata_via where the controller raises IRQ before it's ready after SETXFER and a device which does similar thing. This patch makes libata always execute SETXFER via polling. As this only happens during EH, performance impact is nil. Setting ATA_TFLAG_POLLING is also moved from issue hot path to ata_dev_set_xfermode() - the only place where SETXFER can be issued.

Note that ATA_TFLAG_POLLING applies only to drivers which implement SFF TF interface and use libata HSM. More advanced controllers ignore the flag. This doesn't matter for this fix as SFF TF controllers are the problematic ones.

Quoted 6 lines
Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/libata-core.c |   13 ++++---------
 drivers/ata/pata_via.c    |   12 ++++++------
 include/linux/libata.h    |    1 -
 3 files changed, 10 insertions(+), 16 deletions(-)

To Linus and Andrew:

After discussion and a minor revision (mostly to the description), I am OK with this patch.

At this point, I am only reluctant to push it for 2.6.22 since it is so late in the -rc series.

If we have another -rc, I would probably be OK with pushing it for 2.6.22, otherwise I would prefer to wait for 2.6.23.

Comments solicited, from all involved...

Jeff

#2 lore raw

Hi,

Quoted 30 lines · Jeff Garzik
Quoted 17 lines · Tejun Heo

Several people have reported LITE-ON LTR-48246S detection failed because SETXFER fails. It seems the device raises IRQ too early after SETXFER. This is controller independent. The same problem has been reported for different controllers.

So, now we have pata_via where the controller raises IRQ before it's ready after SETXFER and a device which does similar thing. This patch makes libata always execute SETXFER via polling. As this only happens during EH, performance impact is nil. Setting ATA_TFLAG_POLLING is also moved from issue hot path to ata_dev_set_xfermode() - the only place where SETXFER can be issued.

Note that ATA_TFLAG_POLLING applies only to drivers which implement SFF TF interface and use libata HSM. More advanced controllers ignore the flag. This doesn't matter for this fix as SFF TF controllers are the problematic ones.

Quoted 6 lines
Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/libata-core.c |   13 ++++---------
 drivers/ata/pata_via.c    |   12 ++++++------
 include/linux/libata.h    |    1 -
 3 files changed, 10 insertions(+), 16 deletions(-)

To Linus and Andrew:

After discussion and a minor revision (mostly to the description), I am OK with this patch.

Quoted 7 lines

At this point, I am only reluctant to push it for 2.6.22 since it is so late in the -rc series.

If we have another -rc, I would probably be OK with pushing it for 2.6.22, otherwise I would prefer to wait for 2.6.23.

Comments solicited, from all involved...

I have a feeling that we are needlessly wasting time on this.

The patch should be safe (drivers/ide has been doing SETXFER this way for ages) and fixes real user problems (which becomes even more important now as distros are converting to libata PATA).

So.. can we get this almost 3 months old patch merged finally?

http://thread.gmane.org/gmane.linux.ide/17096/focus=17116

Thanks,
Bart

#3 lore raw
Quoted 4 lines · Bartlomiej Zolnierkiewicz

The patch should be safe (drivers/ide has been doing SETXFER this way for ages) and fixes real user problems (which becomes even more important now as distros are converting to libata PATA).

I hadn't realized that Fedora 7 had converted entirely to libata, but considering that it apparently has, I can almost guarantee that the Fedora people would need to merge that patch, whether it's in the standard kernel or not. If it hit me with an all-intel chipset (and not even a very strange drive), it will hit others.

Of course, maybe that TDK drive really is more unusual than I thought it was, so I can't really back that gut feel up. But yeah, I think we might as well merge it sooner rather than later - we'll have to bite the bullet at some point anyway.

Linus

#4 lore raw
In reply to Jeff Garzik
Quoted 30 lines · On Sun, 03 Jun 2007 12:00:51 -0400 Jeff Garzik <[email protected]> wrote:
Quoted 17 lines · Tejun Heo

Several people have reported LITE-ON LTR-48246S detection failed because SETXFER fails. It seems the device raises IRQ too early after SETXFER. This is controller independent. The same problem has been reported for different controllers.

So, now we have pata_via where the controller raises IRQ before it's ready after SETXFER and a device which does similar thing. This patch makes libata always execute SETXFER via polling. As this only happens during EH, performance impact is nil. Setting ATA_TFLAG_POLLING is also moved from issue hot path to ata_dev_set_xfermode() - the only place where SETXFER can be issued.

Note that ATA_TFLAG_POLLING applies only to drivers which implement SFF TF interface and use libata HSM. More advanced controllers ignore the flag. This doesn't matter for this fix as SFF TF controllers are the problematic ones.

Quoted 6 lines
Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/libata-core.c |   13 ++++---------
 drivers/ata/pata_via.c    |   12 ++++++------
 include/linux/libata.h    |    1 -
 3 files changed, 10 insertions(+), 16 deletions(-)

To Linus and Andrew:

After discussion and a minor revision (mostly to the description), I am OK with this patch.

Quoted 8 lines

At this point, I am only reluctant to push it for 2.6.22 since it is so late in the -rc series.

If we have another -rc, I would probably be OK with pushing it for 2.6.22, otherwise I would prefer to wait for 2.6.23.

Comments solicited, from all involved...

I'd expect there will be two or three more -rcs - the regression list is pretty big.

Another option would be to merge into 2.6.23, backport into 2.6.22.x later on.

But I'd say the thing to do is to merge it immediately is we're reasonably confident - we can always take it out again or fix it if it breaks.

#5 lore raw
In reply to Jeff Garzik
Quoted 3 lines · Jeff Garzik

If we have another -rc, I would probably be OK with pushing it for 2.6.22, otherwise I would prefer to wait for 2.6.23.

We'll definitely have another -rc. I'll push -rc4 tonight, and while I'm hoping that we'll have resolved a number of the regressions, I can guarantee an -rc5 and I'd be very surprised if we don't have an -rc6 too.

(In fact, -rc6 tends to be what I consider the sweet spot for when I start thinking that I'm ready for a release).

Linus

#6 lore raw
In reply to Jeff Garzik
Quoted 7 lines

At this point, I am only reluctant to push it for 2.6.22 since it is so late in the -rc series.

If we have another -rc, I would probably be OK with pushing it for 2.6.22, otherwise I would prefer to wait for 2.6.23.

Comments solicited, from all involved...

Without a doubt it should be merged

#7 lore raw
Quoted 9 lines · Alan Cox
Quoted 7 lines

At this point, I am only reluctant to push it for 2.6.22 since it is so late in the -rc series.

If we have another -rc, I would probably be OK with pushing it for 2.6.22, otherwise I would prefer to wait for 2.6.23.

Comments solicited, from all involved...

Without a doubt it should be merged

Ok, I think everybody agreed.

Jeff, do you want me to just take the patch you sent earlier if I can still find it, or should I pull it from somewhere (btw, the changelog should probably be fixed, since there were at least two people who were confused by the fact that it mentioned VIA controllers and LITE-ON devices, even though it definitely wasn't restricted to that set).

I was going to do an -rc4 yesterday, but slipped, so since I slipped anyway, I would prefer to just get this in before doing the -rc..

Linus

#8 lore raw
Quoted 10 lines · Linus Torvalds

Ok, I think everybody agreed.

Jeff, do you want me to just take the patch you sent earlier if I can still find it, or should I pull it from somewhere (btw, the changelog should probably be fixed, since there were at least two people who were confused by the fact that it mentioned VIA controllers and LITE-ON devices, even though it definitely wasn't restricted to that set).

I was going to do an -rc4 yesterday, but slipped, so since I slipped anyway, I would prefer to just get this in before doing the -rc..

I'll send the patch in an hour or so.

I'd like to get in the updated patch description, that documents we intentionally do not apply these workarounds in non-SFF drivers.

That will eventually be relevant for, e.g. the upcoming Marvell PATA driver (libata-dev.git#mv-ahci-pata) that needs to care about this problem, but where the workaround does not exist at all, since the controller is ultra-modern.

Jeff