[PATCH] ath9k: Fix PCI FATAL interrupts by restoring RETRY_TIMEOUT disabling

[PATCH] ath9k: Fix PCI FATAL interrupts by restoring RETRY_TIMEOUT disabling

[Jouni Malinen]

An earlier commit, b572b24c578ab1be9d1fcb11d2d8244878757a66, removed
code that was documented to disable RETRY_TIMEOUT register (PCI reg
0x41) since it was claimed to be a no-op. However, it turns out that
there are some combinations of hosts and ath9k-supported cards for
which this is not a no-op (reg 0x41 has value 0x80, not 0) and this
code (or something similar) is needed. In such cases, the driver may
be next to unusable due to very frequent PCI FATAL interrupts from the
card.

Reverting the earlier commit, i.e., restoring the RETRY_TIMEOUT
disabling, seems to resolve the issue. Since the removal of this code
was not based on any known issue and was purely a cleanup change, the
safest option here is to just revert that commit. Should there be
desire to clean this up in the future, the change will need to be
tested with a more complete coverage of cards and host systems.

Cc: stable@kernel.org
Signed-off-by: Jouni Malinen <jouni.malinen@atheros.com>

---
 drivers/net/wireless/ath/ath9k/pci.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

This issue was a regression in 2.6.30 and this patch should be applied
to wireless-testing.git and 2.6.31 as-is and to a 2.6.30.x stable
release as-is but without the "ath/" subdirectory on the path.

--- wireless-testing.orig/drivers/net/wireless/ath/ath9k/pci.c	2009-06-16 11:38:16.000000000 +0300
+++ wireless-testing/drivers/net/wireless/ath/ath9k/pci.c	2009-06-16 11:38:29.000000000 +0300
@@ -87,6 +87,7 @@ static int ath_pci_probe(struct pci_dev 
 	struct ath_softc *sc;
 	struct ieee80211_hw *hw;
 	u8 csz;
+	u32 val;
 	int ret = 0;
 	struct ath_hw *ah;
 
@@ -133,6 +134,14 @@ static int ath_pci_probe(struct pci_dev 
 
 	pci_set_master(pdev);
 
+	/*
+	 * Disable the RETRY_TIMEOUT register (0x41) to keep
+	 * PCI Tx retries from interfering with C3 CPU state.
+	 */
+	pci_read_config_dword(pdev, 0x40, &val);
+	if ((val & 0x0000ff00) != 0)
+		pci_write_config_dword(pdev, 0x40, val & 0xffff00ff);
+
 	ret = pci_request_region(pdev, 0, "ath9k");
 	if (ret) {
 		dev_err(&pdev->dev, "PCI memory region reserve error\n");
@@ -239,12 +248,21 @@ static int ath_pci_resume(struct pci_dev
 	struct ieee80211_hw *hw = pci_get_drvdata(pdev);
 	struct ath_wiphy *aphy = hw->priv;
 	struct ath_softc *sc = aphy->sc;
+	u32 val;
 	int err;
 
 	err = pci_enable_device(pdev);
 	if (err)
 		return err;
 	pci_restore_state(pdev);
+	/*
+	 * Suspend/Resume resets the PCI configuration space, so we have to
+	 * re-disable the RETRY_TIMEOUT register (0x41) to keep
+	 * PCI Tx retries from interfering with C3 CPU state
+	 */
+	pci_read_config_dword(pdev, 0x40, &val);
+	if ((val & 0x0000ff00) != 0)
+		pci_write_config_dword(pdev, 0x40, val & 0xffff00ff);
 
 	/* Enable LED */
 	ath9k_hw_cfg_output(sc->sc_ah, ATH_LED_PIN,

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH] ath9k: Fix PCI FATAL interrupts by restoring RETRY_TIMEOUT disabling

[Bob Copeland]

On Tue, Jun 16, 2009 at 4:59 AM, Jouni Malinen<jouni.malinen@atheros.com> wrote:
> An earlier commit, b572b24c578ab1be9d1fcb11d2d8244878757a66, removed
> code that was documented to disable RETRY_TIMEOUT register (PCI reg
> 0x41) since it was claimed to be a no-op.

Jouni, can you confirm that e23a9014... also needs reverting?
(In this case only affects resume path).

I seem to remember the provenance of this code was copy-paste from
an intel driver, so while it does "something," the comment may not
match the code, 0x41 being vendor-defined.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH] ath9k: Fix PCI FATAL interrupts by restoring RETRY_TIMEOUT disabling

[Jouni Malinen]

On Tue, Jun 16, 2009 at 10:33:29AM -0400, Bob Copeland wrote:
> > An earlier commit, b572b24c578ab1be9d1fcb11d2d8244878757a66, removed
> > code that was documented to disable RETRY_TIMEOUT register (PCI reg
> > 0x41) since it was claimed to be a no-op.
> 
> Jouni, can you confirm that e23a9014... also needs reverting?
> (In this case only affects resume path).

I wish I could, but I can only guess that it probably should be
reverted.

> I seem to remember the provenance of this code was copy-paste from
> an intel driver, so while it does "something," the comment may not
> match the code, 0x41 being vendor-defined.

The exact story behind this has been a bit more than trivial task to
figure out ;-). I would assume this comment is referring to a madwifi
changeset: http://madwifi-project.org/changeset/584

I've yet to find documentation that would explain the exact meaning of
PCI register 0x41 in case of ath9k/ath5k-supported devices, but maybe I
will some day ;-). Or well, I've seen documentation for said register,
but that does not match with the comment (but seems to indicate that 0
is a reasonable value to write there).

At this point, I can really only say that I can now easily reproduce
this with ath9k and AR5416 and zeroing the 0x41 value does make the
problem disappear. Since that code has been in use for five years, I do
not expect problems with it and we can just eventually fix the comment
if somebody actually find out what that register is supposed to control
;-).
 
-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH] ath9k: Fix PCI FATAL interrupts by restoring RETRY_TIMEOUT disabling

[Bob Copeland]

On Tue, Jun 16, 2009 at 11:12 AM, Jouni Malinen<j@w1.fi> wrote:
>> Jouni, can you confirm that e23a9014... also needs reverting?
>
> At this point, I can really only say that I can now easily reproduce
> this with ath9k and AR5416 and zeroing the 0x41 value does make the
> problem disappear. Since that code has been in use for five years, I do
> not expect problems with it and we can just eventually fix the comment
> if somebody actually find out what that register is supposed to control
> ;-).

Heh, fair enough.  That's good enough for me, we know it's been
in madwifi for ages and doesn't hurt anything, so let's go ahead
and revert e23a9014... as well.

-- 
Bob Copeland %% www.bobcopeland.com