RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

[Alan Cox]

(Jeff would you please take a look at this: Its #4 or #5 top OOPS on Arjan's
 oops tracker, and it generally causes the boot to fail. First sent 20th May)

From: Alan Cox <alan@redhat.com>

This fixes most common oops #5 on Arjan's kerneloops.org site

Not all controllers have ctl/altstatus. In particular many ISAPnP controllers
omit them. While libata should handle this it generally only got the ctl port
(the write side) correct.

Functions
- ata_sff_maybe_altstatus - this uses the status if altstatus is not available.
  In the longer term I believe each use of it is in fact removable but don't
  want to perturb anything during the -rc releases
- ata_sff_pause - don't use altstatus I/O for fencing if we don't have one
- ata_sff_sync - add a fencing call so we can distinguish fencing from real
		 altstatus usage. All non ctl/altstatus controllers are PIO
		 so do not need a fence

Code wise we then use maybe_altstatus in the IRQ path (where we should only
check status and the current code is actually I think buggy), and for DMA
completion (no non ctl/altstatus controller does DMA but need to finish
verifying the calling cases).

This has been tested with ctl/altstatus faked to be unavailable on controllers
I have and works. You get some spew about failed identify but that is another
issue that can be fixed later and it does all work. Also the spew only occurs
on controllers that currently just go wrong.



Signed-off-by: Alan Cox <alan@redhat.com>
Acked-by: Tejun Heo <htejun@gmail.com>

---

 drivers/ata/libata-sff.c  |   81 +++++++++++++++++++++++++++++++++++++++++----
 drivers/ata/pata_icside.c |    2 +
 include/linux/libata.h    |   14 +-------
 3 files changed, 76 insertions(+), 21 deletions(-)


diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 3c2d228..25bcb58 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -256,6 +256,72 @@ u8 ata_sff_altstatus(struct ata_port *ap)
 }
 
 /**
+ *	ata_sff_maybe_altstatus - Read device alternate status reg
+ *	@ap: port where the device is
+ *
+ *	TEMPORARY:
+ *
+ *	Reads ATA taskfile alternate status register for
+ *	currently-selected device and return its value. Uses the
+ *	status register as an alternative.
+ *
+ *	Note: may NOT be used as the check_altstatus() entry in
+ *	ata_port_operations.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ */
+static u8 ata_sff_maybe_altstatus(struct ata_port *ap)
+{
+	if (ap->ops->sff_check_altstatus)
+		return ap->ops->sff_check_altstatus(ap);
+	else if (ap->ioaddr.altstatus_addr)
+		return ioread8(ap->ioaddr.altstatus_addr);
+	else
+		return ata_sff_check_status(ap);
+}
+
+/**
+ *	ata_sff_sync - Flush writes
+ *	@ap: Port to wait for.
+ *
+ *	CAUTION:
+ *	If we have an mmio device with no ctl and no altstatus
+ *	method this will fail. No such devices are known to exist.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ */
+
+static void ata_sff_sync(struct ata_port *ap)
+{
+	if (ap->ops->sff_check_altstatus)
+		ap->ops->sff_check_altstatus(ap);
+	else if (ap->ioaddr.altstatus_addr)
+		ioread8(ap->ioaddr.altstatus_addr);
+	ndelay(400);
+}
+
+/**
+ *	ata_sff_pause - Flush writes and wait 400nS
+ *	@ap: Port to wait for.
+ *
+ *	CAUTION:
+ *	If we have an mmio device with no ctl and no altstatus
+ *	method this will fail. No such devices are known to exist.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ */
+
+void ata_sff_pause(struct ata_port *ap)
+{
+	ata_sff_sync(ap);
+	ndelay(400);
+}
+
+
+/**
  *	ata_sff_busy_sleep - sleep until BSY clears, or timeout
  *	@ap: port containing status register to be polled
  *	@tmout_pat: impatience timeout
@@ -742,7 +808,7 @@ static void ata_pio_sectors(struct ata_queued_cmd *qc)
 	} else
 		ata_pio_sector(qc);
 
-	ata_sff_altstatus(qc->ap); /* flush */
+	ata_sff_sync(qc->ap); /* flush */
 }
 
 /**
@@ -763,7 +829,7 @@ static void atapi_send_cdb(struct ata_port *ap, struct ata_queued_cmd *qc)
 	WARN_ON(qc->dev->cdb_len < 12);
 
 	ap->ops->sff_data_xfer(qc->dev, qc->cdb, qc->dev->cdb_len, 1);
-	ata_sff_altstatus(ap); /* flush */
+	ata_sff_pause(ap);
 
 	switch (qc->tf.protocol) {
 	case ATAPI_PROT_PIO:
@@ -905,7 +971,7 @@ static void atapi_pio_bytes(struct ata_queued_cmd *qc)
 
 	if (unlikely(__atapi_pio_bytes(qc, bytes)))
 		goto err_out;
-	ata_sff_altstatus(ap); /* flush */
+	ata_sff_pause(ap); /* flush */
 
 	return;
 
@@ -1489,8 +1555,8 @@ inline unsigned int ata_sff_host_intr(struct ata_port *ap,
 		goto idle_irq;
 	}
 
-	/* check altstatus */
-	status = ata_sff_altstatus(ap);
+	/* check altstatus: We don't need to do this here */
+	status = ata_sff_maybe_altstatus(ap);
 	if (status & ATA_BUSY)
 		goto idle_irq;
 
@@ -2030,7 +2096,7 @@ void ata_sff_error_handler(struct ata_port *ap)
 		ap->ops->bmdma_stop(qc);
 	}
 
-	ata_sff_altstatus(ap);
+	ata_sff_sync(ap);		/* FIXME: We don't need this */
 	ap->ops->sff_check_status(ap);
 	ap->ops->sff_irq_clear(ap);
 
@@ -2203,7 +2269,7 @@ void ata_bmdma_stop(struct ata_queued_cmd *qc)
 		 mmio + ATA_DMA_CMD);
 
 	/* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */
-	ata_sff_altstatus(ap);        /* dummy read */
+	ata_sff_maybe_altstatus(ap);        /* dummy read */
 }
 
 /**
@@ -2723,6 +2789,7 @@ EXPORT_SYMBOL_GPL(ata_sff_dumb_qc_prep);
 EXPORT_SYMBOL_GPL(ata_sff_dev_select);
 EXPORT_SYMBOL_GPL(ata_sff_check_status);
 EXPORT_SYMBOL_GPL(ata_sff_altstatus);
+EXPORT_SYMBOL_GPL(ata_sff_pause);
 EXPORT_SYMBOL_GPL(ata_sff_busy_sleep);
 EXPORT_SYMBOL_GPL(ata_sff_wait_ready);
 EXPORT_SYMBOL_GPL(ata_sff_tf_load);
diff --git a/drivers/ata/pata_icside.c b/drivers/ata/pata_icside.c
index 1713843..bc685cd 100644
--- a/drivers/ata/pata_icside.c
+++ b/drivers/ata/pata_icside.c
@@ -269,7 +269,7 @@ static void pata_icside_bmdma_stop(struct ata_queued_cmd *qc)
 
 	disable_dma(state->dma);
 
-	/* see ata_bmdma_stop */
+	/* see ata_bmdma_stop: we know we have a ctl port in this case */
 	ata_sff_altstatus(ap);
 }
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 4a92fba..8eb5022 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1435,6 +1435,7 @@ extern void ata_sff_qc_prep(struct ata_queued_cmd *qc);
 extern void ata_sff_dumb_qc_prep(struct ata_queued_cmd *qc);
 extern void ata_sff_dev_select(struct ata_port *ap, unsigned int device);
 extern u8 ata_sff_check_status(struct ata_port *ap);
+extern void ata_sff_pause(struct ata_port *ap);
 extern u8 ata_sff_altstatus(struct ata_port *ap);
 extern int ata_sff_busy_sleep(struct ata_port *ap,
 			      unsigned long timeout_pat, unsigned long timeout);
@@ -1496,19 +1497,6 @@ extern int ata_pci_sff_init_one(struct pci_dev *pdev,
 #endif /* CONFIG_PCI */
 
 /**
- *	ata_sff_pause - Flush writes and pause 400 nanoseconds.
- *	@ap: Port to wait for.
- *
- *	LOCKING:
- *	Inherited from caller.
- */
-static inline void ata_sff_pause(struct ata_port *ap)
-{
-	ata_sff_altstatus(ap);
-	ndelay(400);
-}
-
-/**
  *	ata_sff_busy_wait - Wait for a port status register
  *	@ap: Port to wait for.
  *	@bits: bits that must be clear

Re: RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

[Linus Torvalds]

On Thu, 29 May 2008, Alan Cox wrote:
>
> (Jeff would you please take a look at this: Its #4 or #5 top OOPS on Arjan's
>  oops tracker, and it generally causes the boot to fail. First sent 20th May)

Quite frankly, if I was Jeff, I'd have refused to apply this patch as "too 
damn ugly to live".

Why the *hell* doesn't it just fix "ata_sff_altstatus()" instead? Why does 
it introduce a ludicrously named stupid "maybe" version of it that doesn't 
oops?

In other words: in *any* case where the old "ata_sff_altstatus()" function 
worked correctly, the new "ata_sff_maybe_altstatus()" function does THE 
EXACT SAME THING. And in any case where the old "ata_sff_altstatus()" 
function oopsed, the new "maybe" version at least is _better_.

In other words: there is absolutely no excuse for keeping the old (and 
known-to-be-broken) "ata_sff_altstatus()" function at all. It should be 
removed, not left around with an alternate function that works.

I also think your "ata_sff_sync()" thing is buggy. It has a "ndelay(400)" 
that is almost certainly buggy (it's the one that is already in 
ata_sff_pause()).

It may be that you meant to make it an "else if" case, ie if there was no 
IO-read, then you do a ndelay(400) as a last desperate case, but that's 
not how your ata_sdd_sync() is actually written.

		Linus

Re: RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

[Jeff Garzik]

Linus Torvalds wrote:
> 
> On Thu, 29 May 2008, Alan Cox wrote:
>> (Jeff would you please take a look at this: Its #4 or #5 top OOPS on Arjan's
>>  oops tracker, and it generally causes the boot to fail. First sent 20th May)
> 
> Quite frankly, if I was Jeff, I'd have refused to apply this patch as "too 
> damn ugly to live".
> 
> Why the *hell* doesn't it just fix "ata_sff_altstatus()" instead? Why does 
> it introduce a ludicrously named stupid "maybe" version of it that doesn't 
> oops?
> 
> In other words: in *any* case where the old "ata_sff_altstatus()" function 
> worked correctly, the new "ata_sff_maybe_altstatus()" function does THE 
> EXACT SAME THING. And in any case where the old "ata_sff_altstatus()" 
> function oopsed, the new "maybe" version at least is _better_.
> 
> In other words: there is absolutely no excuse for keeping the old (and 
> known-to-be-broken) "ata_sff_altstatus()" function at all. It should be 
> removed, not left around with an alternate function that works.

That's my general feeling on the issue.  It was ugly and seemed to 
needlessly avoid the existing one, which we would probably have to 
bugfix later on...


> I also think your "ata_sff_sync()" thing is buggy. It has a "ndelay(400)" 
> that is almost certainly buggy (it's the one that is already in 
> ata_sff_pause()).
> 
> It may be that you meant to make it an "else if" case, ie if there was no 
> IO-read, then you do a ndelay(400) as a last desperate case, but that's 
> not how your ata_sdd_sync() is actually written.

The double-ndelay is definitely wrong, but we do need one.  Technically 
it should -only- be a 400ns delay, but we also have a register read in 
there to make sure any posted writes are flushed.

	Jeff

Re: RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

[Alan Cox]

> Why the *hell* doesn't it just fix "ata_sff_altstatus()" instead? Why does 
> it introduce a ludicrously named stupid "maybe" version of it that doesn't 
> oops?

Ok the problem is we have three cases to distinguish

- altstatus must be used - in which case we want to blow up anyway if we
touch it (the usual case)
- altstatus should be used if available - shared IRQ check
- altstatus being used for flushing - altstatus irrelevant, it just has
to flush somehow.

So the _maybe naming sucks, but the reasoning I think is sound. The other
way to do it would be to replace it and the bit of irq handler logic with
an ata_sff_busy() that did the status checks correctly for both ctl and
non-ctl capable devices.

> It may be that you meant to make it an "else if" case, ie if there was no 
> IO-read, then you do a ndelay(400) as a last desperate case, but that's 
> not how your ata_sdd_sync() is actually written.

The ndelay(400) is correct. The IO-read is Jeff being paranoid and
actually hurts us materially for the usual PIO case (bus PIO not disk
PIO) to the tune of about 1mS a command in many cases, but is needed for
MMIO (which we almost never do for any SFF hardware). That itself is a
different problem that can be fixed later (and not in -rc5). It wants
fixing as its a key reason that old IDE is still faster for PATA.

maybe_altstatus is crap naming but simply making ata_sff_altstatus fake a
reply in arbitary cases risks not catching mistakes and could mean we
don't catch corrupting mistakes which would be very bad indeed.

Re: RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

[Linus Torvalds]

On Thu, 29 May 2008, Jeff Garzik wrote:
> > 
> > It may be that you meant to make it an "else if" case, ie if there was no
> > IO-read, then you do a ndelay(400) as a last desperate case, but that's not
> > how your ata_sdd_sync() is actually written.
> 
> The double-ndelay is definitely wrong, but we do need one.  Technically it
> should -only- be a 400ns delay, but we also have a register read in there to
> make sure any posted writes are flushed.

Well, but the "read + delay" is already in ata_sdd_pause(). 

So it's "ata_sdd_sync()" that I think is bogus. Based on its name alone, 
it shouldn't have a delay in it (except, as mentioned, possibly for the 
fallback case where no port can be used for reading).

		Linus

Re: RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

[Jeff Garzik]

Linus Torvalds wrote:
> 
> On Thu, 29 May 2008, Jeff Garzik wrote:
>>> It may be that you meant to make it an "else if" case, ie if there was no
>>> IO-read, then you do a ndelay(400) as a last desperate case, but that's not
>>> how your ata_sdd_sync() is actually written.
>> The double-ndelay is definitely wrong, but we do need one.  Technically it
>> should -only- be a 400ns delay, but we also have a register read in there to
>> make sure any posted writes are flushed.
> 
> Well, but the "read + delay" is already in ata_sdd_pause(). 

Right, that's what I meant by double-ndelay.


> So it's "ata_sdd_sync()" that I think is bogus. Based on its name alone, 
> it shouldn't have a delay in it (except, as mentioned, possibly for the 
> fallback case where no port can be used for reading).

Agreed,

	Jeff

Re: RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

[Alan Cox]

And this is how a revised one would look.

libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

From: Alan Cox <alan@redhat.com>


---

 drivers/ata/libata-sff.c  |   88 +++++++++++++++++++++++++++++++++++++++------
 drivers/ata/pata_icside.c |    2 +
 include/linux/libata.h    |   14 +------
 3 files changed, 78 insertions(+), 26 deletions(-)


diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 3c2d228..7958da7 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -256,6 +256,73 @@ u8 ata_sff_altstatus(struct ata_port *ap)
 }
 
 /**
+ *	ata_sff_irq_status - Check if the device is busy
+ *	@ap: port where the device is
+ *
+ *	Determine if the port is currently busy. Uses altstatus
+ *	if available in order to avoid clearing shared IRQ status
+ *	when finding an IRQ source. Non ctl capable devices don't
+ *	share interrupt lines fortunately for us.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ */
+static u8 ata_sff_irq_status(struct ata_port *ap)
+{
+	u8 status;
+
+	if (ap->ops->sff_check_altstatus || ap->ioaddr.altstatus_addr) {
+		status = ata_sff_altstatus(ap);
+		/* Not us: We are busy */
+		if (status & ATA_BUSY)
+		    	return status;
+	}
+	/* Clear INTRQ latch */
+	status = ata_sff_check_status(ap);
+	return status;
+}
+
+/**
+ *	ata_sff_sync - Flush writes
+ *	@ap: Port to wait for.
+ *
+ *	CAUTION:
+ *	If we have an mmio device with no ctl and no altstatus
+ *	method this will fail. No such devices are known to exist.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ */
+
+static void ata_sff_sync(struct ata_port *ap)
+{
+	if (ap->ops->sff_check_altstatus)
+		ap->ops->sff_check_altstatus(ap);
+	else if (ap->ioaddr.altstatus_addr)
+		ioread8(ap->ioaddr.altstatus_addr);
+	ndelay(400);
+}
+
+/**
+ *	ata_sff_pause - Flush writes and wait 400nS
+ *	@ap: Port to wait for.
+ *
+ *	CAUTION:
+ *	If we have an mmio device with no ctl and no altstatus
+ *	method this will fail. No such devices are known to exist.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ */
+
+void ata_sff_pause(struct ata_port *ap)
+{
+	ata_sff_sync(ap);
+	ndelay(400);
+}
+
+
+/**
  *	ata_sff_busy_sleep - sleep until BSY clears, or timeout
  *	@ap: port containing status register to be polled
  *	@tmout_pat: impatience timeout
@@ -742,7 +809,7 @@ static void ata_pio_sectors(struct ata_queued_cmd *qc)
 	} else
 		ata_pio_sector(qc);
 
-	ata_sff_altstatus(qc->ap); /* flush */
+	ata_sff_sync(qc->ap); /* flush */
 }
 
 /**
@@ -763,7 +830,7 @@ static void atapi_send_cdb(struct ata_port *ap, struct ata_queued_cmd *qc)
 	WARN_ON(qc->dev->cdb_len < 12);
 
 	ap->ops->sff_data_xfer(qc->dev, qc->cdb, qc->dev->cdb_len, 1);
-	ata_sff_altstatus(ap); /* flush */
+	ata_sff_pause(ap);
 
 	switch (qc->tf.protocol) {
 	case ATAPI_PROT_PIO:
@@ -905,7 +972,7 @@ static void atapi_pio_bytes(struct ata_queued_cmd *qc)
 
 	if (unlikely(__atapi_pio_bytes(qc, bytes)))
 		goto err_out;
-	ata_sff_altstatus(ap); /* flush */
+	ata_sff_pause(ap); /* flush */
 
 	return;
 
@@ -1489,14 +1556,10 @@ inline unsigned int ata_sff_host_intr(struct ata_port *ap,
 		goto idle_irq;
 	}
 
-	/* check altstatus */
-	status = ata_sff_altstatus(ap);
-	if (status & ATA_BUSY)
-		goto idle_irq;
 
-	/* check main status, clearing INTRQ */
-	status = ap->ops->sff_check_status(ap);
-	if (unlikely(status & ATA_BUSY))
+	/* check main status, clearing INTRQ if needed */
+	status = ata_sff_irq_status(ap);
+	if (status & ATA_BUSY)
 		goto idle_irq;
 
 	/* ack bmdma irq events */
@@ -2030,7 +2093,7 @@ void ata_sff_error_handler(struct ata_port *ap)
 		ap->ops->bmdma_stop(qc);
 	}
 
-	ata_sff_altstatus(ap);
+	ata_sff_sync(ap);		/* FIXME: We don't need this */
 	ap->ops->sff_check_status(ap);
 	ap->ops->sff_irq_clear(ap);
 
@@ -2203,7 +2266,7 @@ void ata_bmdma_stop(struct ata_queued_cmd *qc)
 		 mmio + ATA_DMA_CMD);
 
 	/* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */
-	ata_sff_altstatus(ap);        /* dummy read */
+	ata_sff_sync(ap);        /* dummy read */
 }
 
 /**
@@ -2723,6 +2786,7 @@ EXPORT_SYMBOL_GPL(ata_sff_dumb_qc_prep);
 EXPORT_SYMBOL_GPL(ata_sff_dev_select);
 EXPORT_SYMBOL_GPL(ata_sff_check_status);
 EXPORT_SYMBOL_GPL(ata_sff_altstatus);
+EXPORT_SYMBOL_GPL(ata_sff_pause);
 EXPORT_SYMBOL_GPL(ata_sff_busy_sleep);
 EXPORT_SYMBOL_GPL(ata_sff_wait_ready);
 EXPORT_SYMBOL_GPL(ata_sff_tf_load);
diff --git a/drivers/ata/pata_icside.c b/drivers/ata/pata_icside.c
index 1713843..bc685cd 100644
--- a/drivers/ata/pata_icside.c
+++ b/drivers/ata/pata_icside.c
@@ -269,7 +269,7 @@ static void pata_icside_bmdma_stop(struct ata_queued_cmd *qc)
 
 	disable_dma(state->dma);
 
-	/* see ata_bmdma_stop */
+	/* see ata_bmdma_stop: we know we have a ctl port in this case */
 	ata_sff_altstatus(ap);
 }
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 4a92fba..8eb5022 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1435,6 +1435,7 @@ extern void ata_sff_qc_prep(struct ata_queued_cmd *qc);
 extern void ata_sff_dumb_qc_prep(struct ata_queued_cmd *qc);
 extern void ata_sff_dev_select(struct ata_port *ap, unsigned int device);
 extern u8 ata_sff_check_status(struct ata_port *ap);
+extern void ata_sff_pause(struct ata_port *ap);
 extern u8 ata_sff_altstatus(struct ata_port *ap);
 extern int ata_sff_busy_sleep(struct ata_port *ap,
 			      unsigned long timeout_pat, unsigned long timeout);
@@ -1496,19 +1497,6 @@ extern int ata_pci_sff_init_one(struct pci_dev *pdev,
 #endif /* CONFIG_PCI */
 
 /**
- *	ata_sff_pause - Flush writes and pause 400 nanoseconds.
- *	@ap: Port to wait for.
- *
- *	LOCKING:
- *	Inherited from caller.
- */
-static inline void ata_sff_pause(struct ata_port *ap)
-{
-	ata_sff_altstatus(ap);
-	ndelay(400);
-}
-
-/**
  *	ata_sff_busy_wait - Wait for a port status register
  *	@ap: Port to wait for.
  *	@bits: bits that must be clear

Re: RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

[Jeff Garzik]

Alan Cox wrote:
> maybe_altstatus is crap naming but simply making ata_sff_altstatus fake a
> reply in arbitary cases risks not catching mistakes and could mean we
> don't catch corrupting mistakes which would be very bad indeed.

Please grep the remaining users, IMO you will find that's not really 
true.  After your patch no core users remain, only ones in a few 
scattered drivers that most likely want your logic (or a 
simplified-for-that-controller version thereof).

	Jeff

Re: RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

[Alan Cox]

> So it's "ata_sdd_sync()" that I think is bogus. Based on its name alone, 
> it shouldn't have a delay in it (except, as mentioned, possibly for the 
> fallback case where no port can be used for reading).

It should indeed not have the delay in it.

Alan

Re: RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

[Alan Cox]

On Thu, 29 May 2008 14:25:58 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> Alan Cox wrote:
> > maybe_altstatus is crap naming but simply making ata_sff_altstatus fake a
> > reply in arbitary cases risks not catching mistakes and could mean we
> > don't catch corrupting mistakes which would be very bad indeed.
> 
> Please grep the remaining users, IMO you will find that's not really 
> true.  After your patch no core users remain, only ones in a few 
> scattered drivers that most likely want your logic (or a 
> simplified-for-that-controller version thereof).

I did check them all and review them and read the documentation right
back to the original 1010 controller behaviour. I stand by my comment.

Alan

Re: RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

[Alan Cox]

> Please grep the remaining users, IMO you will find that's not really 
> true.  After your patch no core users remain, only ones in a few 
> scattered drivers that most likely want your logic (or a 
> simplified-for-that-controller version thereof).

That said if you want to be clearer we can hide the altstatus function
internally and only export an ata_sff_dma_sync() which does go *BANG* if
you call it with no altstatus (where you would immediately violate DMA
timing rules)

Want me to roll a patch in that style ?

Alan

Re: RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

[Jeff Garzik]

Alan Cox wrote:
>> Please grep the remaining users, IMO you will find that's not really 
>> true.  After your patch no core users remain, only ones in a few 
>> scattered drivers that most likely want your logic (or a 
>> simplified-for-that-controller version thereof).
> 
> That said if you want to be clearer we can hide the altstatus function
> internally and only export an ata_sff_dma_sync() which does go *BANG* if
> you call it with no altstatus (where you would immediately violate DMA
> timing rules)
> 
> Want me to roll a patch in that style ?

Works for me...

Re: RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

[Linus Torvalds]

On Thu, 29 May 2008, Alan Cox wrote:
> 
> I did check them all and review them and read the documentation right
> back to the original 1010 controller behaviour. I stand by my comment.

Your comment is pointless.

The old function oopsed for the case that you worry about. That is WORSE 
than anything else you can come up with. There is absolutely zero upside 
to using that old and broken thing.

		Linus

Re: RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

[Alan Cox]

> The old function oopsed for the case that you worry about. That is WORSE 
> than anything else you can come up with. There is absolutely zero upside 
> to using that old and broken thing.

The worst case is uncaught data corruption. An oops is harmless in
comparison. Someone using altstatus to enforce DMA timing barriers and
getting nothing else will find their disk contents 'degraded'.

I'm just putting together a patch that removes ata_altstatus entirely
from public use, and that actually looks far nicer.

[RFC PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

[Alan Cox]

> > Want me to roll a patch in that style ?
> 
> Works for me...

Ok

- Make ata_sff_altstatus private so nobody uses it by mistake
- Drop the 400nS delay from it

Add

ata_sff_irq_status	-	encapsulates the IRQ check logic

This function keeps the existing behaviour for altstatus using devices. I
actually suspect the logic was wrong before the changes but -rc isn't the
time to play with that

ata_sff_sync		-	ensure writes hit the device

Really we want an io* operation for 'is posted' eg ioisposted(ioaddr) so
that we can fix the nasty delay this causes on most systems.

- ata_sff_pause		-	400nS delay

Ensure the command hit the device and delay 400nS

- ata_sff_dma_pause

Ensure the I/O hit the device and enforce an HDMA1:0 transition delay.
Requires altstatus register exists, BUG if not so we don't risk
corruption in MWDMA modes. (UDMA the checksum will save your backside in
theory)

The only other complication then is devices with their own handlers.
rb532 can use dma_pause but scc needs to access its own altstatus
register for internal errata workarounds so directly call the drivers own
altstatus function.


Signed-off-by: Alan Cox <alan@redhat.com>

 drivers/ata/libata-sff.c    |  115 +++++++++++++++++++++++++++++++++++++------
 drivers/ata/pata_icside.c   |    2 -
 drivers/ata/pata_rb532_cf.c |    4 +
 drivers/ata/pata_scc.c      |    5 +-
 include/linux/libata.h      |   16 +-----
 5 files changed, 109 insertions(+), 33 deletions(-)


diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 3c2d228..90d20c6 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -247,7 +247,7 @@ u8 ata_sff_check_status(struct ata_port *ap)
  *	LOCKING:
  *	Inherited from caller.
  */
-u8 ata_sff_altstatus(struct ata_port *ap)
+static u8 ata_sff_altstatus(struct ata_port *ap)
 {
 	if (ap->ops->sff_check_altstatus)
 		return ap->ops->sff_check_altstatus(ap);
@@ -256,6 +256,93 @@ u8 ata_sff_altstatus(struct ata_port *ap)
 }
 
 /**
+ *	ata_sff_irq_status - Check if the device is busy
+ *	@ap: port where the device is
+ *
+ *	Determine if the port is currently busy. Uses altstatus
+ *	if available in order to avoid clearing shared IRQ status
+ *	when finding an IRQ source. Non ctl capable devices don't
+ *	share interrupt lines fortunately for us.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ */
+static u8 ata_sff_irq_status(struct ata_port *ap)
+{
+	u8 status;
+
+	if (ap->ops->sff_check_altstatus || ap->ioaddr.altstatus_addr) {
+		status = ata_sff_altstatus(ap);
+		/* Not us: We are busy */
+		if (status & ATA_BUSY)
+		    	return status;
+	}
+	/* Clear INTRQ latch */
+	status = ata_sff_check_status(ap);
+	return status;
+}
+
+/**
+ *	ata_sff_sync - Flush writes
+ *	@ap: Port to wait for.
+ *
+ *	CAUTION:
+ *	If we have an mmio device with no ctl and no altstatus
+ *	method this will fail. No such devices are known to exist.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ */
+
+static void ata_sff_sync(struct ata_port *ap)
+{
+	if (ap->ops->sff_check_altstatus)
+		ap->ops->sff_check_altstatus(ap);
+	else if (ap->ioaddr.altstatus_addr)
+		ioread8(ap->ioaddr.altstatus_addr);
+}
+
+/**
+ *	ata_sff_pause		-	Flush writes and wait 400nS
+ *	@ap: Port to pause for.
+ *
+ *	CAUTION:
+ *	If we have an mmio device with no ctl and no altstatus
+ *	method this will fail. No such devices are known to exist.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ */
+
+void ata_sff_pause(struct ata_port *ap)
+{
+	ata_sff_sync(ap);
+	ndelay(400);
+}
+
+/**
+ *	ata_sff_dma_pause	-	Pause before commencing DMA
+ *	@ap: Port to pause for.
+ *
+ *	Perform I/O fencing and ensure sufficient cycle delays occur
+ *	for the HDMA1:0 transition
+ */
+ 
+void ata_sff_dma_pause(struct ata_port *ap)
+{
+	if (ap->ops->sff_check_altstatus || ap->ioaddr.altstatus_addr) {
+		/* An altstatus read will cause the needed delay without
+		   messing up the IRQ status */
+		ata_sff_altstatus(ap);
+		return;
+	}
+	/* There are no DMA controllers without ctl. BUG here to ensure
+	   we never violate the HDMA1:0 transition timing and risk
+	   corruption. */
+	BUG();
+}
+
+/**
  *	ata_sff_busy_sleep - sleep until BSY clears, or timeout
  *	@ap: port containing status register to be polled
  *	@tmout_pat: impatience timeout
@@ -742,7 +829,7 @@ static void ata_pio_sectors(struct ata_queued_cmd *qc)
 	} else
 		ata_pio_sector(qc);
 
-	ata_sff_altstatus(qc->ap); /* flush */
+	ata_sff_sync(qc->ap); /* flush */
 }
 
 /**
@@ -763,8 +850,9 @@ static void atapi_send_cdb(struct ata_port *ap, struct ata_queued_cmd *qc)
 	WARN_ON(qc->dev->cdb_len < 12);
 
 	ap->ops->sff_data_xfer(qc->dev, qc->cdb, qc->dev->cdb_len, 1);
-	ata_sff_altstatus(ap); /* flush */
-
+	ata_sff_sync(ap);
+	/* FIXME: If the CDB is for DMA do we need to do the transition delay
+	   or is bmdma_start guaranteed to do it ? */
 	switch (qc->tf.protocol) {
 	case ATAPI_PROT_PIO:
 		ap->hsm_task_state = HSM_ST;
@@ -905,7 +993,7 @@ static void atapi_pio_bytes(struct ata_queued_cmd *qc)
 
 	if (unlikely(__atapi_pio_bytes(qc, bytes)))
 		goto err_out;
-	ata_sff_altstatus(ap); /* flush */
+	ata_sff_sync(ap); /* flush */
 
 	return;
 
@@ -1489,14 +1577,10 @@ inline unsigned int ata_sff_host_intr(struct ata_port *ap,
 		goto idle_irq;
 	}
 
-	/* check altstatus */
-	status = ata_sff_altstatus(ap);
-	if (status & ATA_BUSY)
-		goto idle_irq;
 
-	/* check main status, clearing INTRQ */
-	status = ap->ops->sff_check_status(ap);
-	if (unlikely(status & ATA_BUSY))
+	/* check main status, clearing INTRQ if needed */
+	status = ata_sff_irq_status(ap);
+	if (status & ATA_BUSY)
 		goto idle_irq;
 
 	/* ack bmdma irq events */
@@ -2030,7 +2114,7 @@ void ata_sff_error_handler(struct ata_port *ap)
 		ap->ops->bmdma_stop(qc);
 	}
 
-	ata_sff_altstatus(ap);
+	ata_sff_sync(ap);		/* FIXME: We don't need this */
 	ap->ops->sff_check_status(ap);
 	ap->ops->sff_irq_clear(ap);
 
@@ -2203,7 +2287,7 @@ void ata_bmdma_stop(struct ata_queued_cmd *qc)
 		 mmio + ATA_DMA_CMD);
 
 	/* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */
-	ata_sff_altstatus(ap);        /* dummy read */
+	ata_sff_dma_pause(ap);
 }
 
 /**
@@ -2722,7 +2806,8 @@ EXPORT_SYMBOL_GPL(ata_sff_qc_prep);
 EXPORT_SYMBOL_GPL(ata_sff_dumb_qc_prep);
 EXPORT_SYMBOL_GPL(ata_sff_dev_select);
 EXPORT_SYMBOL_GPL(ata_sff_check_status);
-EXPORT_SYMBOL_GPL(ata_sff_altstatus);
+EXPORT_SYMBOL_GPL(ata_sff_dma_pause);
+EXPORT_SYMBOL_GPL(ata_sff_pause);
 EXPORT_SYMBOL_GPL(ata_sff_busy_sleep);
 EXPORT_SYMBOL_GPL(ata_sff_wait_ready);
 EXPORT_SYMBOL_GPL(ata_sff_tf_load);
diff --git a/drivers/ata/pata_icside.c b/drivers/ata/pata_icside.c
index 1713843..cf9e984 100644
--- a/drivers/ata/pata_icside.c
+++ b/drivers/ata/pata_icside.c
@@ -270,7 +270,7 @@ static void pata_icside_bmdma_stop(struct ata_queued_cmd *qc)
 	disable_dma(state->dma);
 
 	/* see ata_bmdma_stop */
-	ata_sff_altstatus(ap);
+	ata_sff_dma_pause(ap);
 }
 
 static u8 pata_icside_bmdma_status(struct ata_port *ap)
diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index a108d25..f8b3ffc 100644
--- a/drivers/ata/pata_rb532_cf.c
+++ b/drivers/ata/pata_rb532_cf.c
@@ -57,7 +57,9 @@ static inline void rb532_pata_finish_io(struct ata_port *ap)
 	struct ata_host *ah = ap->host;
 	struct rb532_cf_info *info = ah->private_data;
 
-	ata_sff_altstatus(ap);
+	/* FIXME: Keep previous delay. If this is merely a fence then
+	   ata_sff_sync might be sufficient. */
+	ata_sff_dma_pause(ap);
 	ndelay(RB500_CF_IO_DELAY);
 
 	set_irq_type(info->irq, IRQ_TYPE_LEVEL_HIGH);
diff --git a/drivers/ata/pata_scc.c b/drivers/ata/pata_scc.c
index e965b25..bbf5aa3 100644
--- a/drivers/ata/pata_scc.c
+++ b/drivers/ata/pata_scc.c
@@ -726,7 +726,7 @@ static void scc_bmdma_stop (struct ata_queued_cmd *qc)
 		 in_be32(bmid_base + SCC_DMA_CMD) & ~ATA_DMA_START);
 
 	/* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */
-	ata_sff_altstatus(ap);	/* dummy read */
+	ata_sff_dma_pause(ap);	/* dummy read */
 }
 
 /**
@@ -747,7 +747,8 @@ static u8 scc_bmdma_status (struct ata_port *ap)
 		return host_stat;
 
 	/* errata A252,A308 workaround: Step4 */
-	if ((ata_sff_altstatus(ap) & ATA_ERR) && (int_status & INTSTS_INTRQ))
+	if ((scc_check_altstatus(ap) & ATA_ERR)
+					&& (int_status & INTSTS_INTRQ))
 		return (host_stat | ATA_DMA_INTR);
 
 	/* errata A308 workaround Step5 */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 4a92fba..57cfd92 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1435,7 +1435,8 @@ extern void ata_sff_qc_prep(struct ata_queued_cmd *qc);
 extern void ata_sff_dumb_qc_prep(struct ata_queued_cmd *qc);
 extern void ata_sff_dev_select(struct ata_port *ap, unsigned int device);
 extern u8 ata_sff_check_status(struct ata_port *ap);
-extern u8 ata_sff_altstatus(struct ata_port *ap);
+extern void ata_sff_pause(struct ata_port *ap);
+extern void ata_sff_dma_pause(struct ata_port *ap);
 extern int ata_sff_busy_sleep(struct ata_port *ap,
 			      unsigned long timeout_pat, unsigned long timeout);
 extern int ata_sff_wait_ready(struct ata_link *link, unsigned long deadline);
@@ -1496,19 +1497,6 @@ extern int ata_pci_sff_init_one(struct pci_dev *pdev,
 #endif /* CONFIG_PCI */
 
 /**
- *	ata_sff_pause - Flush writes and pause 400 nanoseconds.
- *	@ap: Port to wait for.
- *
- *	LOCKING:
- *	Inherited from caller.
- */
-static inline void ata_sff_pause(struct ata_port *ap)
-{
-	ata_sff_altstatus(ap);
-	ndelay(400);
-}
-
-/**
  *	ata_sff_busy_wait - Wait for a port status register
  *	@ap: Port to wait for.
  *	@bits: bits that must be clear

Re: RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

[Linus Torvalds]

On Thu, 29 May 2008, Alan Cox wrote:
>
> And this is how a revised one would look.

Ok, this looks nicer, but how about:

> +static u8 ata_sff_irq_status(struct ata_port *ap)
> +{
> +	u8 status;
> +
> +	if (ap->ops->sff_check_altstatus || ap->ioaddr.altstatus_addr) {
> +		status = ata_sff_altstatus(ap);

This here is all about not oopsing in ata_sff_altstatus(), ie just knowing 
about the bug.

Why not just fix ata_sff_altstatus(), instead of causing these kinds of 
problems downstream?

If you don't want to read the status register, fine. But at least do 
something like

 - make ata_sff_altstatus static, since it's useless and dangerous to call 
   otherwise (ie make the exported interfaces be the nicer higher-level 
   ones that are sane)

 - and make it just return 0 if the altstatus register doesn't exist.

At that point, both 'ata_sff_irq_status()' and 'ata_sff_sync()' can just 
use ata_sff_altstatus() directly, *without* having to check that altstatus 
setup by hand, or re-implement the function just because it was buggy and 
broken to begin with.

So now ata_sff_irq_status() just becomes

	static u8 ata_sff_irq_status(struct ata_port *ap)
	{
		u8 status;

		status = ata_sff_altstatus(ap);
		if (status & ATA_BUSY)
			return status;

		/* Clear INTRQ latch */
		status = ata_sff_check_status(ap);
		return status;
	}

and if there was no altstatus register, everything is fine because 
"ata_sff_altstatus()" was safe and returned 0 (and not ATA_BUSY).

Or feel free and make it return ATA_INVALID, which has a value of 0x100, 
or something - it still won't be busy, and it will clearly not be a byte 
read, so people *can* check for "no altstatus existed" if they want to.

Similarly, 'ata_sff_sync()' just becomes

	void ata_sff_sync(struct ata_port *ap)
	{
		ata_sff_altstatus();
	}

and 'ata_sff_pause()' becomes

	void ata_sff_pause(struct ata_port *ap)
	{
		ata_sff_altstatus();
		ndelay(400);
	}

and again, if there is no altstatus register, that's a low-level driver 
issue.

		Linus

Re: RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

[Alan Cox]

> 	void ata_sff_pause(struct ata_port *ap)
> 	{
> 		ata_sff_altstatus();
> 		ndelay(400);
> 	}
> 
> and again, if there is no altstatus register, that's a low-level driver 
> issue.

This takes away the ability to shoot yourself directly in the foot and
replaces it with the ability to shoot yourself in the foot in a more
abstract way. The dma_pause case still needs to check as that is the one
where you can make an actual mess but yes it certainly cleans up the
others.

(Next rev brewing ;))

Re: [RFC PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

[Alan Cox]

The Linusified version:

- Make ata_sff_altstatus private so nobody uses it by mistake
- Drop the 400nS delay from it

Add

ata_sff_irq_status	-	encapsulates the IRQ check logic

This function keeps the existing behaviour for altstatus using devices. I
actually suspect the logic was wrong before the changes but -rc isn't the
time to play with that

ata_sff_sync		-	ensure writes hit the device

Really we want an io* operation for 'is posted' eg ioisposted(ioaddr) so
that we can fix the nasty delay this causes on most systems.

- ata_sff_pause		-	400nS delay

Ensure the command hit the device and delay 400nS

- ata_sff_dma_pause
 
Ensure the I/O hit the device and enforce an HDMA1:0 transition delay.
Requires altstatus register exists, BUG if not so we don't risk
corruption in MWDMA modes. (UDMA the checksum will save your backside in
theory)

The only other complication then is devices with their own handlers.
rb532 can use dma_pause but scc needs to access its own altstatus
register for internal errata workarounds so directly call the drivers own
altstatus function.

Signed-off-by: Alan Cox <alan@redhat.com>

 drivers/ata/libata-sff.c    |  117 +++++++++++++++++++++++++++++++++++++------
 drivers/ata/pata_icside.c   |    2 -
 drivers/ata/pata_rb532_cf.c |    4 +
 drivers/ata/pata_scc.c      |    5 +-
 include/linux/libata.h      |   16 +-----
 5 files changed, 109 insertions(+), 35 deletions(-)


diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 3c2d228..43049ba 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -239,7 +239,8 @@ u8 ata_sff_check_status(struct ata_port *ap)
  *	@ap: port where the device is
  *
  *	Reads ATA taskfile alternate status register for
- *	currently-selected device and return its value.
+ *	currently-selected device and return its value. Returns
+ *	NO_ALTSTATUS (0x100) if no status is present.
  *
  *	Note: may NOT be used as the check_altstatus() entry in
  *	ata_port_operations.
@@ -247,12 +248,96 @@ u8 ata_sff_check_status(struct ata_port *ap)
  *	LOCKING:
  *	Inherited from caller.
  */
-u8 ata_sff_altstatus(struct ata_port *ap)
+static unsigned int ata_sff_altstatus(struct ata_port *ap)
 {
 	if (ap->ops->sff_check_altstatus)
 		return ap->ops->sff_check_altstatus(ap);
+	if (ap->ioaddr.altstatus_addr)
+		return ioread8(ap->ioaddr.altstatus_addr);
+#define NO_ALTSTATUS 0x100
+	return NO_ALTSTATUS;
+}
+
+/**
+ *	ata_sff_irq_status - Check if the device is busy
+ *	@ap: port where the device is
+ *
+ *	Determine if the port is currently busy. Uses altstatus
+ *	if available in order to avoid clearing shared IRQ status
+ *	when finding an IRQ source. Non ctl capable devices don't
+ *	share interrupt lines fortunately for us.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ */
+static u8 ata_sff_irq_status(struct ata_port *ap)
+{
+	u8 status;
+
+	status = ata_sff_altstatus(ap);
+	/* Not us: We are busy */
+	if (status & ATA_BUSY)
+	    	return status;
+	/* Clear INTRQ latch */
+	status = ata_sff_check_status(ap);
+	return status;
+}
+
+/**
+ *	ata_sff_sync - Flush writes
+ *	@ap: Port to wait for.
+ *
+ *	CAUTION:
+ *	If we have an mmio device with no ctl and no altstatus
+ *	method this will fail. No such devices are known to exist.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ */
 
-	return ioread8(ap->ioaddr.altstatus_addr);
+static void ata_sff_sync(struct ata_port *ap)
+{
+	/* For the moment we read altstatus. Ideally we should only
+	   do this if we *know* a fence is needed - eg with MMIO */
+	ata_sff_altstatus(ap);
+}
+
+/**
+ *	ata_sff_pause		-	Flush writes and wait 400nS
+ *	@ap: Port to pause for.
+ *
+ *	CAUTION:
+ *	If we have an mmio device with no ctl and no altstatus
+ *	method this will fail. No such devices are known to exist.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ */
+
+void ata_sff_pause(struct ata_port *ap)
+{
+	ata_sff_sync(ap);
+	ndelay(400);
+}
+
+/**
+ *	ata_sff_dma_pause	-	Pause before commencing DMA
+ *	@ap: Port to pause for.
+ *
+ *	Perform I/O fencing and ensure sufficient cycle delays occur
+ *	for the HDMA1:0 transition
+ */
+ 
+void ata_sff_dma_pause(struct ata_port *ap)
+{
+	/* An altstatus read will cause the needed delay without
+	   messing up the IRQ status */
+	unsigned int v = ata_sff_altstatus(ap);
+	if (unlikely(v == NO_ALTSTATUS))
+		/* There are no DMA controllers without ctl. BUG here to
+		   ensure we never violate the HDMA1:0 transition timing
+		   and risk corruption. */
+		BUG();
 }
 
 /**
@@ -742,7 +827,7 @@ static void ata_pio_sectors(struct ata_queued_cmd *qc)
 	} else
 		ata_pio_sector(qc);
 
-	ata_sff_altstatus(qc->ap); /* flush */
+	ata_sff_sync(qc->ap); /* flush */
 }
 
 /**
@@ -763,8 +848,9 @@ static void atapi_send_cdb(struct ata_port *ap, struct ata_queued_cmd *qc)
 	WARN_ON(qc->dev->cdb_len < 12);
 
 	ap->ops->sff_data_xfer(qc->dev, qc->cdb, qc->dev->cdb_len, 1);
-	ata_sff_altstatus(ap); /* flush */
-
+	ata_sff_sync(ap);
+	/* FIXME: If the CDB is for DMA do we need to do the transition delay
+	   or is bmdma_start guaranteed to do it ? */
 	switch (qc->tf.protocol) {
 	case ATAPI_PROT_PIO:
 		ap->hsm_task_state = HSM_ST;
@@ -905,7 +991,7 @@ static void atapi_pio_bytes(struct ata_queued_cmd *qc)
 
 	if (unlikely(__atapi_pio_bytes(qc, bytes)))
 		goto err_out;
-	ata_sff_altstatus(ap); /* flush */
+	ata_sff_sync(ap); /* flush */
 
 	return;
 
@@ -1489,14 +1575,10 @@ inline unsigned int ata_sff_host_intr(struct ata_port *ap,
 		goto idle_irq;
 	}
 
-	/* check altstatus */
-	status = ata_sff_altstatus(ap);
-	if (status & ATA_BUSY)
-		goto idle_irq;
 
-	/* check main status, clearing INTRQ */
-	status = ap->ops->sff_check_status(ap);
-	if (unlikely(status & ATA_BUSY))
+	/* check main status, clearing INTRQ if needed */
+	status = ata_sff_irq_status(ap);
+	if (status & ATA_BUSY)
 		goto idle_irq;
 
 	/* ack bmdma irq events */
@@ -2030,7 +2112,7 @@ void ata_sff_error_handler(struct ata_port *ap)
 		ap->ops->bmdma_stop(qc);
 	}
 
-	ata_sff_altstatus(ap);
+	ata_sff_sync(ap);		/* FIXME: We don't need this */
 	ap->ops->sff_check_status(ap);
 	ap->ops->sff_irq_clear(ap);
 
@@ -2203,7 +2285,7 @@ void ata_bmdma_stop(struct ata_queued_cmd *qc)
 		 mmio + ATA_DMA_CMD);
 
 	/* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */
-	ata_sff_altstatus(ap);        /* dummy read */
+	ata_sff_dma_pause(ap);
 }
 
 /**
@@ -2722,7 +2804,8 @@ EXPORT_SYMBOL_GPL(ata_sff_qc_prep);
 EXPORT_SYMBOL_GPL(ata_sff_dumb_qc_prep);
 EXPORT_SYMBOL_GPL(ata_sff_dev_select);
 EXPORT_SYMBOL_GPL(ata_sff_check_status);
-EXPORT_SYMBOL_GPL(ata_sff_altstatus);
+EXPORT_SYMBOL_GPL(ata_sff_dma_pause);
+EXPORT_SYMBOL_GPL(ata_sff_pause);
 EXPORT_SYMBOL_GPL(ata_sff_busy_sleep);
 EXPORT_SYMBOL_GPL(ata_sff_wait_ready);
 EXPORT_SYMBOL_GPL(ata_sff_tf_load);
diff --git a/drivers/ata/pata_icside.c b/drivers/ata/pata_icside.c
index 1713843..cf9e984 100644
--- a/drivers/ata/pata_icside.c
+++ b/drivers/ata/pata_icside.c
@@ -270,7 +270,7 @@ static void pata_icside_bmdma_stop(struct ata_queued_cmd *qc)
 	disable_dma(state->dma);
 
 	/* see ata_bmdma_stop */
-	ata_sff_altstatus(ap);
+	ata_sff_dma_pause(ap);
 }
 
 static u8 pata_icside_bmdma_status(struct ata_port *ap)
diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index a108d25..f8b3ffc 100644
--- a/drivers/ata/pata_rb532_cf.c
+++ b/drivers/ata/pata_rb532_cf.c
@@ -57,7 +57,9 @@ static inline void rb532_pata_finish_io(struct ata_port *ap)
 	struct ata_host *ah = ap->host;
 	struct rb532_cf_info *info = ah->private_data;
 
-	ata_sff_altstatus(ap);
+	/* FIXME: Keep previous delay. If this is merely a fence then
+	   ata_sff_sync might be sufficient. */
+	ata_sff_dma_pause(ap);
 	ndelay(RB500_CF_IO_DELAY);
 
 	set_irq_type(info->irq, IRQ_TYPE_LEVEL_HIGH);
diff --git a/drivers/ata/pata_scc.c b/drivers/ata/pata_scc.c
index e965b25..bbf5aa3 100644
--- a/drivers/ata/pata_scc.c
+++ b/drivers/ata/pata_scc.c
@@ -726,7 +726,7 @@ static void scc_bmdma_stop (struct ata_queued_cmd *qc)
 		 in_be32(bmid_base + SCC_DMA_CMD) & ~ATA_DMA_START);
 
 	/* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */
-	ata_sff_altstatus(ap);	/* dummy read */
+	ata_sff_dma_pause(ap);	/* dummy read */
 }
 
 /**
@@ -747,7 +747,8 @@ static u8 scc_bmdma_status (struct ata_port *ap)
 		return host_stat;
 
 	/* errata A252,A308 workaround: Step4 */
-	if ((ata_sff_altstatus(ap) & ATA_ERR) && (int_status & INTSTS_INTRQ))
+	if ((scc_check_altstatus(ap) & ATA_ERR)
+					&& (int_status & INTSTS_INTRQ))
 		return (host_stat | ATA_DMA_INTR);
 
 	/* errata A308 workaround Step5 */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 4a92fba..57cfd92 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1435,7 +1435,8 @@ extern void ata_sff_qc_prep(struct ata_queued_cmd *qc);
 extern void ata_sff_dumb_qc_prep(struct ata_queued_cmd *qc);
 extern void ata_sff_dev_select(struct ata_port *ap, unsigned int device);
 extern u8 ata_sff_check_status(struct ata_port *ap);
-extern u8 ata_sff_altstatus(struct ata_port *ap);
+extern void ata_sff_pause(struct ata_port *ap);
+extern void ata_sff_dma_pause(struct ata_port *ap);
 extern int ata_sff_busy_sleep(struct ata_port *ap,
 			      unsigned long timeout_pat, unsigned long timeout);
 extern int ata_sff_wait_ready(struct ata_link *link, unsigned long deadline);
@@ -1496,19 +1497,6 @@ extern int ata_pci_sff_init_one(struct pci_dev *pdev,
 #endif /* CONFIG_PCI */
 
 /**
- *	ata_sff_pause - Flush writes and pause 400 nanoseconds.
- *	@ap: Port to wait for.
- *
- *	LOCKING:
- *	Inherited from caller.
- */
-static inline void ata_sff_pause(struct ata_port *ap)
-{
-	ata_sff_altstatus(ap);
-	ndelay(400);
-}
-
-/**
  *	ata_sff_busy_wait - Wait for a port status register
  *	@ap: Port to wait for.
  *	@bits: bits that must be clear

Re: [RFC PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

[Jeff Garzik]

Alan Cox wrote:
> The Linusified version:
> 
> - Make ata_sff_altstatus private so nobody uses it by mistake
> - Drop the 400nS delay from it
> 
> Add
> 
> ata_sff_irq_status	-	encapsulates the IRQ check logic
> 
> This function keeps the existing behaviour for altstatus using devices. I
> actually suspect the logic was wrong before the changes but -rc isn't the
> time to play with that
> 
> ata_sff_sync		-	ensure writes hit the device
> 
> Really we want an io* operation for 'is posted' eg ioisposted(ioaddr) so
> that we can fix the nasty delay this causes on most systems.
> 
> - ata_sff_pause		-	400nS delay
> 
> Ensure the command hit the device and delay 400nS
> 
> - ata_sff_dma_pause
>  
> Ensure the I/O hit the device and enforce an HDMA1:0 transition delay.
> Requires altstatus register exists, BUG if not so we don't risk
> corruption in MWDMA modes. (UDMA the checksum will save your backside in
> theory)
> 
> The only other complication then is devices with their own handlers.
> rb532 can use dma_pause but scc needs to access its own altstatus
> register for internal errata workarounds so directly call the drivers own
> altstatus function.
> 
> Signed-off-by: Alan Cox <alan@redhat.com>

Honestly I think your first RFC PATCH (dated Thu, 29 May 2008 22:10:58 
+0100) turned out better than this.

NO_ALTSTATUS is a bit ugly, don't you think?

In any case, based on a quick review I would certainly ack the first RFC 
PATCH.

	Jeff

Re: [RFC PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

[Alan Cox]

> Honestly I think your first RFC PATCH (dated Thu, 29 May 2008 22:10:58 
> +0100) turned out better than this.
> 
> NO_ALTSTATUS is a bit ugly, don't you think?

I do , but Linus seemed to like it ;)
> 
> In any case, based on a quick review I would certainly ack the first RFC 
> PATCH.

Ok. I'll run a full test set on that one tomorrow

Re: [RFC PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

[Jeff Garzik]

Alan Cox wrote:
>>> Want me to roll a patch in that style ?
>> Works for me...
> 
> Ok
> 
> - Make ata_sff_altstatus private so nobody uses it by mistake
> - Drop the 400nS delay from it
> 
> Add
> 
> ata_sff_irq_status	-	encapsulates the IRQ check logic
> 
> This function keeps the existing behaviour for altstatus using devices. I
> actually suspect the logic was wrong before the changes but -rc isn't the
> time to play with that
> 
> ata_sff_sync		-	ensure writes hit the device
> 
> Really we want an io* operation for 'is posted' eg ioisposted(ioaddr) so
> that we can fix the nasty delay this causes on most systems.
> 
> - ata_sff_pause		-	400nS delay
> 
> Ensure the command hit the device and delay 400nS
> 
> - ata_sff_dma_pause
> 
> Ensure the I/O hit the device and enforce an HDMA1:0 transition delay.
> Requires altstatus register exists, BUG if not so we don't risk
> corruption in MWDMA modes. (UDMA the checksum will save your backside in
> theory)
> 
> The only other complication then is devices with their own handlers.
> rb532 can use dma_pause but scc needs to access its own altstatus
> register for internal errata workarounds so directly call the drivers own
> altstatus function.
> 
> 
> Signed-off-by: Alan Cox <alan@redhat.com>
> 
>  drivers/ata/libata-sff.c    |  115 +++++++++++++++++++++++++++++++++++++------
>  drivers/ata/pata_icside.c   |    2 -
>  drivers/ata/pata_rb532_cf.c |    4 +
>  drivers/ata/pata_scc.c      |    5 +-
>  include/linux/libata.h      |   16 +-----
>  5 files changed, 109 insertions(+), 33 deletions(-)

ACK

If no further comments, I'm OK with this version...

Re: [RFC PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

[Jeff Garzik]

Alan Cox wrote:
>>> Want me to roll a patch in that style ?
>> Works for me...
> 
> Ok
> 
> - Make ata_sff_altstatus private so nobody uses it by mistake
> - Drop the 400nS delay from it
> 
> Add
> 
> ata_sff_irq_status	-	encapsulates the IRQ check logic
> 
> This function keeps the existing behaviour for altstatus using devices. I
> actually suspect the logic was wrong before the changes but -rc isn't the
> time to play with that
> 
> ata_sff_sync		-	ensure writes hit the device
> 
> Really we want an io* operation for 'is posted' eg ioisposted(ioaddr) so
> that we can fix the nasty delay this causes on most systems.
> 
> - ata_sff_pause		-	400nS delay
> 
> Ensure the command hit the device and delay 400nS
> 
> - ata_sff_dma_pause
> 
> Ensure the I/O hit the device and enforce an HDMA1:0 transition delay.
> Requires altstatus register exists, BUG if not so we don't risk
> corruption in MWDMA modes. (UDMA the checksum will save your backside in
> theory)
> 
> The only other complication then is devices with their own handlers.
> rb532 can use dma_pause but scc needs to access its own altstatus
> register for internal errata workarounds so directly call the drivers own
> altstatus function.
> 
> 
> Signed-off-by: Alan Cox <alan@redhat.com>
> 
>  drivers/ata/libata-sff.c    |  115 +++++++++++++++++++++++++++++++++++++------
>  drivers/ata/pata_icside.c   |    2 -
>  drivers/ata/pata_rb532_cf.c |    4 +
>  drivers/ata/pata_scc.c      |    5 +-
>  include/linux/libata.h      |   16 +-----

applied