rt2800pci: fix spurious interrupts generation

PATCH In review Stanislaw Gruszka 4 participants 4 messages Jan 13, 2012 – Jan 15, 2012 Networking drivers (wireless)

Share & tools

Stanislaw Gruszka

author

2012-01-13 12:00 UTC #0 lore raw

PATCH 1/1 rt2800pci: fix spurious interrupts generation

Same devices can generate interrupt without properly setting bit in INT_SOURCE_CSR register (spurious interrupt), what will cause IRQ line will be disabled by interrupts controller driver.

We discovered that clearing INT_MASK_CSR stops such behaviour. We previously first read that register, and then clear all know interrupt sources bits and do not touch reserved bits. After this patch, we write to all register content (I believe writing to reserved bits on that register will not cause any problems, I tested that on my rt2800pci device).

This fix very bad performance problem, practically making device unusable (since worked without interrupts), reported in:
https://bugzilla.redhat.com/show_bug.cgi?id=658451

We previously tried to workaround that issue in commit 4ba7d9997869d25bd223dea7536fc1ce9fab3b3b "rt2800pci: handle spurious interrupts", but it was reverted in commit 82e5fc2a34fa9ffea38f00c4066b7e600a0ca5e6
as thing, that will prevent to detect real spurious interrupts.

Reported-and-tested-by Amir Hedayaty <[email protected]>

Cc [email protected]

Signed-off-by Stanislaw Gruszka <[email protected]>

---
 drivers/net/wireless/rt2x00/rt2800pci.c |   28 ++++++++--------------------
 1 files changed, 8 insertions(+), 20 deletions(-)

drivers/net/wireless/rt2x00/rt2800pci.c +8 ~~-20~~

index 4941a1a..dc88bae 100644
@@ -422,7 +422,6 @@ static int rt2800pci_init_queues(struct rt2x00_dev *rt2x00dev)
 static void rt2800pci_toggle_irq(struct rt2x00_dev *rt2x00dev,
 				 enum dev_state state)
 {
-	int mask = (state == STATE_RADIO_IRQ_ON);
 	u32 reg;
 	unsigned long flags;

@@ -436,25 +435,14 @@ static void rt2800pci_toggle_irq(struct rt2x00_dev *rt2x00dev,
 	}
 
 	spin_lock_irqsave(&rt2x00dev->irqmask_lock, flags);
-	rt2x00pci_register_read(rt2x00dev, INT_MASK_CSR, &reg);
-	rt2x00_set_field32(&reg, INT_MASK_CSR_RXDELAYINT, 0);
-	rt2x00_set_field32(&reg, INT_MASK_CSR_TXDELAYINT, 0);
-	rt2x00_set_field32(&reg, INT_MASK_CSR_RX_DONE, mask);
-	rt2x00_set_field32(&reg, INT_MASK_CSR_AC0_DMA_DONE, 0);
-	rt2x00_set_field32(&reg, INT_MASK_CSR_AC1_DMA_DONE, 0);
-	rt2x00_set_field32(&reg, INT_MASK_CSR_AC2_DMA_DONE, 0);
-	rt2x00_set_field32(&reg, INT_MASK_CSR_AC3_DMA_DONE, 0);
-	rt2x00_set_field32(&reg, INT_MASK_CSR_HCCA_DMA_DONE, 0);
-	rt2x00_set_field32(&reg, INT_MASK_CSR_MGMT_DMA_DONE, 0);
-	rt2x00_set_field32(&reg, INT_MASK_CSR_MCU_COMMAND, 0);
-	rt2x00_set_field32(&reg, INT_MASK_CSR_RXTX_COHERENT, 0);
-	rt2x00_set_field32(&reg, INT_MASK_CSR_TBTT, mask);
-	rt2x00_set_field32(&reg, INT_MASK_CSR_PRE_TBTT, mask);
-	rt2x00_set_field32(&reg, INT_MASK_CSR_TX_FIFO_STATUS, mask);
-	rt2x00_set_field32(&reg, INT_MASK_CSR_AUTO_WAKEUP, mask);
-	rt2x00_set_field32(&reg, INT_MASK_CSR_GPTIMER, 0);
-	rt2x00_set_field32(&reg, INT_MASK_CSR_RX_COHERENT, 0);
-	rt2x00_set_field32(&reg, INT_MASK_CSR_TX_COHERENT, 0);
+	reg = 0;
+	if (state == STATE_RADIO_IRQ_ON) {
+		rt2x00_set_field32(&reg, INT_MASK_CSR_RX_DONE, 1);
+		rt2x00_set_field32(&reg, INT_MASK_CSR_TBTT, 1);
+		rt2x00_set_field32(&reg, INT_MASK_CSR_PRE_TBTT, 1);
+		rt2x00_set_field32(&reg, INT_MASK_CSR_TX_FIFO_STATUS, 1);
+		rt2x00_set_field32(&reg, INT_MASK_CSR_AUTO_WAKEUP, 1);
+	}
 	rt2x00pci_register_write(rt2x00dev, INT_MASK_CSR, reg);
 	spin_unlock_irqrestore(&rt2x00dev->irqmask_lock, flags);

Signature · 1 line

1.7.1

Helmut Schaa

2012-01-13 12:19 UTC #1 lore raw

Quoted 63 lines · Stanislaw Gruszka

Same devices can generate interrupt without properly setting bit in INT_SOURCE_CSR register (spurious interrupt), what will cause IRQ line will be disabled by interrupts controller driver.
We discovered that clearing INT_MASK_CSR stops such behaviour. We previously first read that register, and then clear all know interrupt sources bits and do not touch reserved bits. After this patch, we write to all register content (I believe writing to reserved bits on that register will not cause any problems, I tested that on my rt2800pci device).
This fix very bad performance problem, practically making device unusable (since worked without interrupts), reported in:
https://bugzilla.redhat.com/show_bug.cgi?id=658451
We previously tried to workaround that issue in commit 4ba7d9997869d25bd223dea7536fc1ce9fab3b3b "rt2800pci: handle spurious interrupts", but it was reverted in commit 82e5fc2a34fa9ffea38f00c4066b7e600a0ca5e6
as thing, that will prevent to detect real spurious interrupts.
Reported-and-tested-by Amir Hedayaty <[email protected]>
Cc [email protected]
Signed-off-by Stanislaw Gruszka <[email protected]>
---
drivers/net/wireless/rt2x00/rt2800pci.c | 28 ++++++++--------------------
 1 files changed, 8 insertions(+), 20 deletions(-)
drivers/net/wireless/rt2x00/rt2800pci.c +0 ~~-20~~
index 4941a1a..dc88bae 100644
@@ -422,7 +422,6 @@ static int rt2800pci_init_queues(struct rt2x00_dev *rt2x00dev)
  static void rt2800pci_toggle_irq(struct rt2x00_dev *rt2x00dev,
                                 enum dev_state state)
  {
-       int mask = (state == STATE_RADIO_IRQ_ON);
        u32 reg;
        unsigned long flags;
 
@@ -436,25 +435,14 @@ static void rt2800pci_toggle_irq(struct rt2x00_dev *rt2x00dev,
        }
 
        spin_lock_irqsave(&rt2x00dev->irqmask_lock, flags);
-       rt2x00pci_register_read(rt2x00dev, INT_MASK_CSR, &reg);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_RXDELAYINT, 0);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_TXDELAYINT, 0);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_RX_DONE, mask);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_AC0_DMA_DONE, 0);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_AC1_DMA_DONE, 0);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_AC2_DMA_DONE, 0);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_AC3_DMA_DONE, 0);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_HCCA_DMA_DONE, 0);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_MGMT_DMA_DONE, 0);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_MCU_COMMAND, 0);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_RXTX_COHERENT, 0);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_TBTT, mask);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_PRE_TBTT, mask);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_TX_FIFO_STATUS, mask);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_AUTO_WAKEUP, mask);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_GPTIMER, 0);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_RX_COHERENT, 0);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_TX_COHERENT, 0);

drivers/net/wireless/rt2x00/rt2800pci.c static void rt2800pci_toggle_irq(struct rt2x00_dev *rt2x00dev, · line 447

Quoted 10 lines

drivers/net/wireless/rt2x00/rt2800pci.c +8 -0

+       reg = 0;
+       if (state == STATE_RADIO_IRQ_ON) {
+               rt2x00_set_field32(&reg, INT_MASK_CSR_RX_DONE, 1);
+               rt2x00_set_field32(&reg, INT_MASK_CSR_TBTT, 1);
+               rt2x00_set_field32(&reg, INT_MASK_CSR_PRE_TBTT, 1);
+               rt2x00_set_field32(&reg, INT_MASK_CSR_TX_FIFO_STATUS, 1);
+               rt2x00_set_field32(&reg, INT_MASK_CSR_AUTO_WAKEUP, 1);
+       }
        rt2x00pci_register_write(rt2x00dev, INT_MASK_CSR, reg);
        spin_unlock_irqrestore(&rt2x00dev->irqmask_lock, flags);

Looks good to me. Thanks for finding the root cause for this issue!

Helmut

Gertjan van Wingerde

Acked-by on v1 1/1

2012-01-13 17:47 UTC #2 lore raw

In reply to Stanislaw Gruszka

Quoted 21 lines · Stanislaw Gruszka

Same devices can generate interrupt without properly setting bit in INT_SOURCE_CSR register (spurious interrupt), what will cause IRQ line will be disabled by interrupts controller driver.
We discovered that clearing INT_MASK_CSR stops such behaviour. We previously first read that register, and then clear all know interrupt sources bits and do not touch reserved bits. After this patch, we write to all register content (I believe writing to reserved bits on that register will not cause any problems, I tested that on my rt2800pci device).
This fix very bad performance problem, practically making device unusable (since worked without interrupts), reported in:
https://bugzilla.redhat.com/show_bug.cgi?id=658451
We previously tried to workaround that issue in commit 4ba7d9997869d25bd223dea7536fc1ce9fab3b3b "rt2800pci: handle spurious interrupts", but it was reverted in commit 82e5fc2a34fa9ffea38f00c4066b7e600a0ca5e6
as thing, that will prevent to detect real spurious interrupts.

Quoted 3 lines

Reported-and-tested-by Amir Hedayaty <[email protected]>
Cc [email protected]
Signed-off-by Stanislaw Gruszka <[email protected]>

Acked-by Gertjan van Wingerde <[email protected]>

Quoted 53 lines

---
drivers/net/wireless/rt2x00/rt2800pci.c | 28 ++++++++--------------------

1 files changed, 8 insertions(+), 20 deletions(-)

drivers/net/wireless/rt2x00/rt2800pci.c +8 ~~-20~~

index 4941a1a..dc88bae 100644
@@ -422,7 +422,6 @@ static int rt2800pci_init_queues(struct rt2x00_dev *rt2x00dev)
 static void rt2800pci_toggle_irq(struct rt2x00_dev *rt2x00dev,
                enum dev_state state)
 {
-    int mask = (state == STATE_RADIO_IRQ_ON);
   u32 reg;
   unsigned long flags;

@@ -436,25 +435,14 @@ static void rt2800pci_toggle_irq(struct rt2x00_dev *rt2x00dev,
   }
 
   spin_lock_irqsave(&rt2x00dev->irqmask_lock, flags);
-    rt2x00pci_register_read(rt2x00dev, INT_MASK_CSR, &reg);
-    rt2x00_set_field32(&reg, INT_MASK_CSR_RXDELAYINT, 0);
-    rt2x00_set_field32(&reg, INT_MASK_CSR_TXDELAYINT, 0);
-    rt2x00_set_field32(&reg, INT_MASK_CSR_RX_DONE, mask);
-    rt2x00_set_field32(&reg, INT_MASK_CSR_AC0_DMA_DONE, 0);
-    rt2x00_set_field32(&reg, INT_MASK_CSR_AC1_DMA_DONE, 0);
-    rt2x00_set_field32(&reg, INT_MASK_CSR_AC2_DMA_DONE, 0);
-    rt2x00_set_field32(&reg, INT_MASK_CSR_AC3_DMA_DONE, 0);
-    rt2x00_set_field32(&reg, INT_MASK_CSR_HCCA_DMA_DONE, 0);
-    rt2x00_set_field32(&reg, INT_MASK_CSR_MGMT_DMA_DONE, 0);
-    rt2x00_set_field32(&reg, INT_MASK_CSR_MCU_COMMAND, 0);
-    rt2x00_set_field32(&reg, INT_MASK_CSR_RXTX_COHERENT, 0);
-    rt2x00_set_field32(&reg, INT_MASK_CSR_TBTT, mask);
-    rt2x00_set_field32(&reg, INT_MASK_CSR_PRE_TBTT, mask);
-    rt2x00_set_field32(&reg, INT_MASK_CSR_TX_FIFO_STATUS, mask);
-    rt2x00_set_field32(&reg, INT_MASK_CSR_AUTO_WAKEUP, mask);
-    rt2x00_set_field32(&reg, INT_MASK_CSR_GPTIMER, 0);
-    rt2x00_set_field32(&reg, INT_MASK_CSR_RX_COHERENT, 0);
-    rt2x00_set_field32(&reg, INT_MASK_CSR_TX_COHERENT, 0);
+    reg = 0;
+    if (state == STATE_RADIO_IRQ_ON) {
+        rt2x00_set_field32(&reg, INT_MASK_CSR_RX_DONE, 1);
+        rt2x00_set_field32(&reg, INT_MASK_CSR_TBTT, 1);
+        rt2x00_set_field32(&reg, INT_MASK_CSR_PRE_TBTT, 1);
+        rt2x00_set_field32(&reg, INT_MASK_CSR_TX_FIFO_STATUS, 1);
+        rt2x00_set_field32(&reg, INT_MASK_CSR_AUTO_WAKEUP, 1);
+    }
   rt2x00pci_register_write(rt2x00dev, INT_MASK_CSR, reg);
   spin_unlock_irqrestore(&rt2x00dev->irqmask_lock, flags);

Signature · 1 line

1.7.1

Ivo Van Doorn

Acked-by on v1 1/1

2012-01-15 09:20 UTC #3 lore raw

In reply to Stanislaw Gruszka

Quoted 21 lines · Stanislaw Gruszka

Same devices can generate interrupt without properly setting bit in INT_SOURCE_CSR register (spurious interrupt), what will cause IRQ line will be disabled by interrupts controller driver.
We discovered that clearing INT_MASK_CSR stops such behaviour. We previously first read that register, and then clear all know interrupt sources bits and do not touch reserved bits. After this patch, we write to all register content (I believe writing to reserved bits on that register will not cause any problems, I tested that on my rt2800pci device).
This fix very bad performance problem, practically making device unusable (since worked without interrupts), reported in:
https://bugzilla.redhat.com/show_bug.cgi?id=658451
We previously tried to workaround that issue in commit 4ba7d9997869d25bd223dea7536fc1ce9fab3b3b "rt2800pci: handle spurious interrupts", but it was reverted in commit 82e5fc2a34fa9ffea38f00c4066b7e600a0ca5e6
as thing, that will prevent to detect real spurious interrupts.

Quoted 3 lines

Reported-and-tested-by Amir Hedayaty <[email protected]>
Cc [email protected]
Signed-off-by Stanislaw Gruszka <[email protected]>

Acked-by Ivo van Doorn <[email protected]>

Quoted 53 lines

---
drivers/net/wireless/rt2x00/rt2800pci.c | 28 ++++++++--------------------

 1 files changed, 8 insertions(+), 20 deletions(-)

drivers/net/wireless/rt2x00/rt2800pci.c +8 ~~-21~~

index 4941a1a..dc88bae 100644
@@ -422,7 +422,6 @@ static int rt2800pci_init_queues(struct rt2x00_dev *rt2x00dev)
  static void rt2800pci_toggle_irq(struct rt2x00_dev *rt2x00dev,
                                 enum dev_state state)
  {
-       int mask = (state == STATE_RADIO_IRQ_ON);
        u32 reg;
        unsigned long flags;

@@ -436,25 +435,14 @@ static void rt2800pci_toggle_irq(struct rt2x00_dev *rt2x00dev,
        }
 
        spin_lock_irqsave(&rt2x00dev->irqmask_lock, flags);
-       rt2x00pci_register_read(rt2x00dev, INT_MASK_CSR, &reg);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_RXDELAYINT, 0);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_TXDELAYINT, 0);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_RX_DONE, mask);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_AC0_DMA_DONE, 0);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_AC1_DMA_DONE, 0);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_AC2_DMA_DONE, 0);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_AC3_DMA_DONE, 0);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_HCCA_DMA_DONE, 0);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_MGMT_DMA_DONE, 0);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_MCU_COMMAND, 0);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_RXTX_COHERENT, 0);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_TBTT, mask);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_PRE_TBTT, mask);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_TX_FIFO_STATUS, mask);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_AUTO_WAKEUP, mask);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_GPTIMER, 0);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_RX_COHERENT, 0);
-       rt2x00_set_field32(&reg, INT_MASK_CSR_TX_COHERENT, 0);
+       reg = 0;
+       if (state == STATE_RADIO_IRQ_ON) {
+               rt2x00_set_field32(&reg, INT_MASK_CSR_RX_DONE, 1);
+               rt2x00_set_field32(&reg, INT_MASK_CSR_TBTT, 1);
+               rt2x00_set_field32(&reg, INT_MASK_CSR_PRE_TBTT, 1);
+               rt2x00_set_field32(&reg, INT_MASK_CSR_TX_FIFO_STATUS, 1);
+               rt2x00_set_field32(&reg, INT_MASK_CSR_AUTO_WAKEUP, 1);
+       }
        rt2x00pci_register_write(rt2x00dev, INT_MASK_CSR, reg);
        spin_unlock_irqrestore(&rt2x00dev->irqmask_lock, flags);
 
--

1.7.1