[PATCH] libata: always use polling SETXFER

[PATCH] libata: always use polling SETXFER

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

Jeff Garzik suggests that, in the long term, it might be better to
modify libata HSM implementation such that we're more tolerant of
erratic ATAPI IRQ behavior - e.g. default to IRQ but falling back to
polling if the device doesn't seem ready at the point of interrupt.
Such change might be necessary to support ancient/weird ATAPI devices.

Signed-off-by: Tejun Heo <htejun@gmail.com>

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 14629a3..3a8da9d 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3575,10 +3575,13 @@ static unsigned int ata_dev_set_xfermode(struct ata_device *dev)
 	/* 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;
 
@@ -5036,14 +5039,6 @@ unsigned int ata_qc_issue_prot(struct ata_queued_cmd *qc)
 		}
 	}
 
-	/* 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);
 
diff --git a/drivers/ata/pata_via.c b/drivers/ata/pata_via.c
index 96b7179..377e792 100644
--- a/drivers/ata/pata_via.c
+++ b/drivers/ata/pata_via.c
@@ -426,7 +426,7 @@ static int via_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	/* Early VIA without UDMA support */
 	static 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
@@ -434,7 +434,7 @@ static int via_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	/* Ditto with IRQ masking required */
 	static 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,
@@ -442,7 +442,7 @@ static int via_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	/* VIA UDMA 33 devices (and borked 66) */
 	static 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,
@@ -451,7 +451,7 @@ static int via_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	/* VIA UDMA 66 devices */
 	static 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,
@@ -460,7 +460,7 @@ static int via_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	/* VIA UDMA 100 devices */
 	static 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,
@@ -469,7 +469,7 @@ static int via_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	/* UDMA133 with bad AST (All current 133) */
 	static 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 */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 0ad28a3..b9fc57c 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -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 */
 

Re: [PATCH] libata: always use polling SETXFER

[Bartlomiej Zolnierkiewicz]

On Wednesday 14 March 2007, Tejun Heo wrote:
> 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.
> 
> Jeff Garzik suggests that, in the long term, it might be better to
> modify libata HSM implementation such that we're more tolerant of
> erratic ATAPI IRQ behavior - e.g. default to IRQ but falling back to
> polling if the device doesn't seem ready at the point of interrupt.
> Such change might be necessary to support ancient/weird ATAPI devices.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

Acked-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

Re: [PATCH] libata: always use polling SETXFER

[Tejun Heo]

Tejun Heo wrote:
> 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.
> 
> Jeff Garzik suggests that, in the long term, it might be better to
> modify libata HSM implementation such that we're more tolerant of
> erratic ATAPI IRQ behavior - e.g. default to IRQ but falling back to
> polling if the device doesn't seem ready at the point of interrupt.
> Such change might be necessary to support ancient/weird ATAPI devices.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

Jeff, ping.

-- 
tejun

Re: [PATCH] libata: always use polling SETXFER

[Jeff Garzik]

Tejun Heo wrote:
> 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.
> 
> Jeff Garzik suggests that, in the long term, it might be better to
> modify libata HSM implementation such that we're more tolerant of
> erratic ATAPI IRQ behavior - e.g. default to IRQ but falling back to
> polling if the device doesn't seem ready at the point of interrupt.
> Such change might be necessary to support ancient/weird ATAPI devices.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

Since I wrote them up in IRC, I might as well post them here and get it 
archived:

We need to figure out a better polling solution.

For SAS and advanced SATA, polling really has no meaning at all, when 
you consider what polling IDENTIFY DEVICE and polling SET FEATURES are 
trying to solve.  To the advanced hardware, it's all a bunch of packets. 
  An event that appears "late" to the eyes of the PATA world is now 
presented as changing data fields in the packet stream.

We are going to have to deal with the HSM issue underlying the need to 
do SET FEATURES - XFER MODE polling, and ultimately IDENTIFY DEVICE 
polling too.

This is the main reason why I have resisted applying "[PATCH] libata: 
always use polling SETXFER" -- polling implies a model that does not 
exist on SAS/SATA and advanced SATA.  It's only luck that AHCI includes 
a real register to poll.

To illustrate:  Fixing this problem The Right Way(tm) will yield a 
result that would allow ahci.c to operate in an interrupt-driven mode, 
examining the contents of the FIS's returned. Polling status can already 
be replaced by examining the D2H and SDB FIS areas.

And by definition, on AHCI (and sata_sil24, IIRC) the status will not 
change unless a new FIS has arrived.

Polling is still fine on PCI IDE-like controllers (older ones), but 
advanced controllers require us to coalesce the polling bandaid into a 
test for a sequence of events.

We cannot escape the "hard part."  :)

	Jeff

Re: [PATCH] libata: always use polling SETXFER

[Tejun Heo]

Hello, Jeff.

Jeff Garzik wrote:
> Since I wrote them up in IRC, I might as well post them here and get it
> archived:

Just about to reply on IRC.  :-)

> We need to figure out a better polling solution.
> 
> For SAS and advanced SATA, polling really has no meaning at all, when
> you consider what polling IDENTIFY DEVICE and polling SET FEATURES are
> trying to solve.  To the advanced hardware, it's all a bunch of packets.
>  An event that appears "late" to the eyes of the PATA world is now
> presented as changing data fields in the packet stream.
> 
> We are going to have to deal with the HSM issue underlying the need to
> do SET FEATURES - XFER MODE polling, and ultimately IDENTIFY DEVICE
> polling too.
> 
> This is the main reason why I have resisted applying "[PATCH] libata:
> always use polling SETXFER" -- polling implies a model that does not
> exist on SAS/SATA and advanced SATA.  It's only luck that AHCI includes
> a real register to poll.
> 
> To illustrate:  Fixing this problem The Right Way(tm) will yield a
> result that would allow ahci.c to operate in an interrupt-driven mode,
> examining the contents of the FIS's returned. Polling status can already
> be replaced by examining the D2H and SDB FIS areas.
> 
> And by definition, on AHCI (and sata_sil24, IIRC) the status will not
> change unless a new FIS has arrived.
> 
> Polling is still fine on PCI IDE-like controllers (older ones), but
> advanced controllers require us to coalesce the polling bandaid into a
> test for a sequence of events.
> 
> We cannot escape the "hard part."  :)

I don't think the "hard part" exists at all.

1. There are only a handful of PATA devices which raise IRQ too early.
For native SATA devices, it's much more difficult to get it wrong if you
consider the SATA non-data and PIO transport protocol.  For PATA devices
bridged to SATA, again, there's nothing much we can do.  The bridge
implements HSM and would send D2H Reg FIS on command completion IRQ.  If
the PATA shows incorrect register values at that stage, well, that's it.

3. Intelligent controllers such as AHCI and sil24 implement some part of
HSM in the silicon.  sil24 implements most of it, ahci a bit less, but,
even for ahci, the too early interrupt can trigger internal HSM failure.
 I don't think we can do much in such cases.  sil24 doesn't even update
the TF area if command is not in progress.  In the intelligent
controllers, the problem polling SETXFER tries to solve is in lower
layer than OS driver.

So, I don't think the problem exists for SATA in the first place.  At
least there hasn't been any report of it and doing SETXFER by polling
can handle all the existing cases.  We can and probably should deal with
such SATA devices when and if they come up.  How are we gonna verify the
controller doesn't crap itself and ahci TF register monitoring HSM can
work around the weirdo when we don't have any such device?  Even if we
determine that we need to do HSM over intelligent SATA controller now, I
think we still need to push polling SETXFER first to take care of the
existing cases.

Thanks.

-- 
tejun

Re: [PATCH] libata: always use polling SETXFER

[Jeff Garzik]

Tejun Heo wrote:
> So, I don't think the problem exists for SATA in the first place.  At
> least there hasn't been any report of it and doing SETXFER by polling
> can handle all the existing cases.  We can and probably should deal with
> such SATA devices when and if they come up.  How are we gonna verify the
> controller doesn't crap itself and ahci TF register monitoring HSM can
> work around the weirdo when we don't have any such device?  Even if we
> determine that we need to do HSM over intelligent SATA controller now, I
> think we still need to push polling SETXFER first to take care of the
> existing cases.

Doing SETXFER by polling only handles the cases where the driver 
actually honors ATA_TFLAG_POLLING, which is /not/ always the case.

If the new policy ensures that it continues to be OK to /not/ honor 
ATA_TFLAG_POLLING -- thus limiting SETXFER polling assumptions to older 
hardware -- that's fine, and it merely needs to be documented.

But let us not make the assumption that this bandaid fixes all cases, 
because the bandaid is not applied in all cases.

	Jeff

Re: [PATCH] libata: always use polling SETXFER

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> So, I don't think the problem exists for SATA in the first place.  At
>> least there hasn't been any report of it and doing SETXFER by polling
>> can handle all the existing cases.  We can and probably should deal with
>> such SATA devices when and if they come up.  How are we gonna verify the
>> controller doesn't crap itself and ahci TF register monitoring HSM can
>> work around the weirdo when we don't have any such device?  Even if we
>> determine that we need to do HSM over intelligent SATA controller now, I
>> think we still need to push polling SETXFER first to take care of the
>> existing cases.
> 
> Doing SETXFER by polling only handles the cases where the driver
> actually honors ATA_TFLAG_POLLING, which is /not/ always the case.
> 
> If the new policy ensures that it continues to be OK to /not/ honor
> ATA_TFLAG_POLLING -- thus limiting SETXFER polling assumptions to older
> hardware -- that's fine, and it merely needs to be documented.

Basically this flag applies to drivers which is SFF compliant, at least
at TF interface level.  There also are other flags/callbacks which only
apply to SFF or BMDMA.  It would be nice to separate them out in the
long term and yeah it needs documentation.

> But let us not make the assumption that this bandaid fixes all cases,
> because the bandaid is not applied in all cases.

It covers all the known cases but I agree that SFF specific things
certainly need documentation.

Thanks.

-- 
tejun

[PATCH] libata: always use polling SETXFER

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

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-core.c |   13 ++++---------
 drivers/ata/pata_via.c    |   12 ++++++------
 include/linux/libata.h    |    1 -
 3 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 3ca9c61..4d6de65 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -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);
 
diff --git a/drivers/ata/pata_via.c b/drivers/ata/pata_via.c
index a8462f1..63eca29 100644
--- a/drivers/ata/pata_via.c
+++ b/drivers/ata/pata_via.c
@@ -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 */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 85f7b1b..a6a3113 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -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 */

Re: [PATCH] libata: always use polling SETXFER

[Jeff Garzik]

Tejun Heo wrote:
> 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 <htejun@gmail.com>
> ---
>  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

Re: [PATCH] libata: always use polling SETXFER

[Bartlomiej Zolnierkiewicz]

Hi,

On Sunday 03 June 2007, Jeff Garzik wrote:
> Tejun Heo wrote:
> > 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 <htejun@gmail.com>
> > ---
> >  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...

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

Re: [PATCH] libata: always use polling SETXFER

[Linus Torvalds]

On Sun, 3 Jun 2007, Bartlomiej Zolnierkiewicz wrote:
> 
> 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

Re: [PATCH] libata: always use polling SETXFER

[Andrew Morton]

On Sun, 03 Jun 2007 12:00:51 -0400 Jeff Garzik <jeff@garzik.org> wrote:

> Tejun Heo wrote:
> > 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 <htejun@gmail.com>
> > ---
> >  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...
> 

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.

Re: [PATCH] libata: always use polling SETXFER

[Linus Torvalds]

On Sun, 3 Jun 2007, Jeff Garzik wrote:
> 
> 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

Re: [PATCH] libata: always use polling SETXFER

[Alan Cox]

> 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

Re: [PATCH] libata: always use polling SETXFER

[Linus Torvalds]

On Mon, 4 Jun 2007, Alan Cox wrote:

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

Re: [PATCH] libata: always use polling SETXFER

[Jeff Garzik]

On Mon, Jun 04, 2007 at 01:34:21PM -0700, Linus Torvalds wrote:
> 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