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

RFC PATCH Applied Alan Cox 3 participants 22 messages May 29, 2008 – Jun 4, 2008 libata

Share & tools

Alan Cox

author

2008-05-29 16:39 UTC #0 lore raw

(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 <[email protected]>

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 <[email protected]>

Acked-by Tejun Heo <[email protected]>

---

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

drivers/ata/libata-sff.c +74 -7

index 3c2d228..25bcb58 100644
@@ -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);

drivers/ata/pata_icside.c +1 -1

index 1713843..bc685cd 100644
@@ -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);
 }

include/linux/libata.h +1 ~~-13~~

index 4a92fba..8eb5022 100644
@@ -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

Linus Torvalds

2008-05-29 17:04 UTC #1 lore raw

Quoted 3 lines · 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)

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

Jeff Garzik

2008-05-29 17:46 UTC #2 lore raw

Quoted 5 lines · Linus Torvalds

Quoted 2 lines · 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)

Quoted 7 lines

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

Quoted 3 lines

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

Linus Torvalds

2008-05-29 18:04 UTC #3 lore raw

Quoted 8 lines · Jeff Garzik

Quoted 4 lines
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

Jeff Garzik

2008-05-29 18:14 UTC #4 lore raw

Quoted 10 lines · Linus Torvalds

Quoted 6 lines · Jeff Garzik
Quoted 3 lines
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.

Quoted 1 line

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

Alan Cox

author

2008-05-29 18:43 UTC #5 lore raw

In reply to Linus Torvalds

Quoted 3 lines

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

Alan Cox

author

2008-05-29 18:33 UTC #6 lore raw

In reply to Jeff Garzik

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 <[email protected]>

---

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

drivers/ata/libata-sff.c +76 ~~-12~~

index 3c2d228..7958da7 100644
@@ -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);

drivers/ata/pata_icside.c +1 -1

index 1713843..bc685cd 100644
@@ -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);
 }

include/linux/libata.h +1 ~~-13~~

index 4a92fba..8eb5022 100644
@@ -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

Linus Torvalds

2008-05-29 21:14 UTC #7 lore raw

Quoted 2 lines · Alan Cox

And this is how a revised one would look.

Ok, this looks nicer, but how about:

drivers/ata/libata-sff.c u8 ata_sff_altstatus(struct ata_port *ap) · line 275

Quoted 6 lines

drivers/ata/libata-sff.c +6 -0

+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

Alan Cox

author

2008-05-29 21:34 UTC #8 lore raw

Quoted 8 lines

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

Alan Cox

author

2008-05-29 18:17 UTC #9 lore raw

In reply to Linus Torvalds

Quoted 3 lines

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.

Quoted 3 lines

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.

Jeff Garzik

2008-05-29 18:26 UTC #10 lore raw

Quoted 3 lines · Alan Cox

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

Alan Cox

author

2008-05-29 18:53 UTC #11 lore raw

Quoted 9 lines · On Thu, 29 May 2008 14:25:58 -0400 Jeff Garzik <[email protected]> wrote:

Quoted 3 lines · Alan Cox
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

Linus Torvalds

2008-05-29 19:46 UTC #12 lore raw

Quoted 3 lines · Alan Cox

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

Alan Cox

author

2008-05-29 20:33 UTC #13 lore raw

Quoted 3 lines

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.

Alan Cox

author

2008-05-29 18:57 UTC #14 lore raw

In reply to Jeff Garzik

Quoted 4 lines

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

Jeff Garzik

2008-05-29 19:31 UTC #15 lore raw

Quoted 8 lines · Alan Cox

Quoted 1 line
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...

Alan Cox

author

2008-05-29 21:25 UTC #16 lore raw

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

Quoted 3 lines

Quoted 1 line
Want me to roll a patch in that style ?
Works for me...

- 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 <[email protected]>

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

drivers/ata/libata-sff.c +100 ~~-15~~

index 3c2d228..90d20c6 100644
@@ -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);

drivers/ata/pata_icside.c +1 -1

index 1713843..cf9e984 100644
@@ -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)

drivers/ata/pata_rb532_cf.c +3 -1

index a108d25..f8b3ffc 100644
@@ -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);

drivers/ata/pata_scc.c +3 -2

index e965b25..bbf5aa3 100644
@@ -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 */

include/linux/libata.h +2 ~~-14~~

index 4a92fba..57cfd92 100644
@@ -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

Alan Cox

author

2008-05-29 21:52 UTC #17 lore raw

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

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

Signed-off-by Alan Cox <[email protected]>

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

drivers/ata/libata-sff.c +100 ~~-17~~

index 3c2d228..43049ba 100644
@@ -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);

drivers/ata/pata_icside.c +1 -1

index 1713843..cf9e984 100644
@@ -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)

drivers/ata/pata_rb532_cf.c +3 -1

index a108d25..f8b3ffc 100644
@@ -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);

drivers/ata/pata_scc.c +3 -2

index e965b25..bbf5aa3 100644
@@ -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 */

include/linux/libata.h +2 ~~-14~~

index 4a92fba..57cfd92 100644
@@ -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

Jeff Garzik

2008-05-29 21:57 UTC #18 lore raw

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

Quoted 29 lines · 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)

Quoted 6 lines

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 <[email protected]>

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

Alan Cox

author

2008-05-29 22:11 UTC #19 lore raw

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

Quoted 4 lines

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

Quoted 3 lines

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

Jeff Garzik

2008-05-30 22:15 UTC #20 lore raw

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

In reply to Alan Cox

Quoted 38 lines · Alan Cox

Quoted 2 lines
Quoted 1 line
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.

Quoted 8 lines

Signed-off-by Alan Cox <[email protected]>

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

Jeff Garzik

Applied

2008-06-04 10:45 UTC #21 lore raw

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

In reply to Alan Cox

Quoted 38 lines · Alan Cox

Quoted 2 lines
Quoted 1 line
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.

Quoted 7 lines

Signed-off-by Alan Cox <[email protected]>

 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