| Message ID | 20170116030535.GA32238@makrotopia.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
On Mon, Jan 16, 2017 at 04:05:47AM +0100, Daniel Golle wrote: > irqreturn_t rt2800mmio_interrupt(int irq, void *dev_instance) > { > struct rt2x00_dev *rt2x00dev = dev_instance; > u32 reg, mask; > + u32 txstatus = 0; > > - /* Read status and ACK all interrupts */ > + /* Read status */ > rt2x00mmio_register_read(rt2x00dev, INT_SOURCE_CSR, ®); > + > + if (rt2x00_get_field32(reg, INT_SOURCE_CSR_TX_FIFO_STATUS)) { > + /* Due to unknown reason the hardware generates a > + * TX_FIFO_STATUS interrupt before the TX_STA_FIFO > + * register contain valid data. Read the TX status > + * here to see if we have to process the actual > + * request. > + */ > + rt2x00mmio_register_read(rt2x00dev, TX_STA_FIFO, &txstatus); > + if (rt2800mmio_txstatus_is_spurious(rt2x00dev, txstatus)) { > + /* Remove the TX_FIFO_STATUS bit so it won't be > + * processed in this turn. The hardware will > + * generate another IRQ for us. > + */ > + rt2x00_set_field32(®, > + INT_SOURCE_CSR_TX_FIFO_STATUS, 0); > + } > + } > + > + /* ACK interrupts */ > rt2x00mmio_register_write(rt2x00dev, INT_SOURCE_CSR, reg); I think spurious TX_STA_FIFO problem happen because we first ACK interrupt and then read in bulk all statuses from TX_STA_FIFO register, while hardware generate new interrupt (as previous was ACKed), then in new interrupt we have no more statues to read. This is inherently racy situation and first ACK interrupt and then read statuses is safer than reverse order which make risk we will have pending status and not get interrupt about that. Hence I think we should not apply this patch. Stanislaw
On Wed, Jan 18, 2017 at 03:44:46PM +0100, Stanislaw Gruszka wrote: > On Mon, Jan 16, 2017 at 04:05:47AM +0100, Daniel Golle wrote: > > irqreturn_t rt2800mmio_interrupt(int irq, void *dev_instance) > > { > > struct rt2x00_dev *rt2x00dev = dev_instance; > > u32 reg, mask; > > + u32 txstatus = 0; > > > > - /* Read status and ACK all interrupts */ > > + /* Read status */ > > rt2x00mmio_register_read(rt2x00dev, INT_SOURCE_CSR, ®); > > + > > + if (rt2x00_get_field32(reg, INT_SOURCE_CSR_TX_FIFO_STATUS)) { > > + /* Due to unknown reason the hardware generates a > > + * TX_FIFO_STATUS interrupt before the TX_STA_FIFO > > + * register contain valid data. Read the TX status > > + * here to see if we have to process the actual > > + * request. > > + */ > > + rt2x00mmio_register_read(rt2x00dev, TX_STA_FIFO, &txstatus); > > + if (rt2800mmio_txstatus_is_spurious(rt2x00dev, txstatus)) { > > + /* Remove the TX_FIFO_STATUS bit so it won't be > > + * processed in this turn. The hardware will > > + * generate another IRQ for us. > > + */ > > + rt2x00_set_field32(®, > > + INT_SOURCE_CSR_TX_FIFO_STATUS, 0); > > + } > > + } > > + > > + /* ACK interrupts */ > > rt2x00mmio_register_write(rt2x00dev, INT_SOURCE_CSR, reg); > > I think spurious TX_STA_FIFO problem happen because we first ACK > interrupt and then read in bulk all statuses from TX_STA_FIFO > register, while hardware generate new interrupt (as previous > was ACKed), then in new interrupt we have no more statues to > read. > > This is inherently racy situation and first ACK interrupt and > then read statuses is safer than reverse order which make risk we > will have pending status and not get interrupt about that. > > Hence I think we should not apply this patch. Actually patch is safe in regard that we first ACK interrupt and then read all statuses TX_STA_FIFO. We only do not ACK interrupt if we do not have valid status in TX_STA_FIFO . However I don't really see point of the patch, we should get next interrupt when new status will be places in TX_STA_FIFO regardless we ACK this interrupt or don't and if we don't ACK we possibly can get series of spurious interrupts. Stanislaw
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800mmio.c b/drivers/net/wireless/ralink/rt2x00/rt2800mmio.c index 5f1936aa8fa7..750a9425b5be 100644 --- a/drivers/net/wireless/ralink/rt2x00/rt2800mmio.c +++ b/drivers/net/wireless/ralink/rt2x00/rt2800mmio.c @@ -415,9 +415,9 @@ void rt2800mmio_autowake_tasklet(unsigned long data) } EXPORT_SYMBOL_GPL(rt2800mmio_autowake_tasklet); -static void rt2800mmio_txstatus_interrupt(struct rt2x00_dev *rt2x00dev) +static void rt2800mmio_txstatus_interrupt(struct rt2x00_dev *rt2x00dev, + u32 status) { - u32 status; int i; /* @@ -438,29 +438,77 @@ static void rt2800mmio_txstatus_interrupt(struct rt2x00_dev *rt2x00dev) * Since we have only one producer and one consumer we don't * need to lock the kfifo. */ - for (i = 0; i < rt2x00dev->tx->limit; i++) { - rt2x00mmio_register_read(rt2x00dev, TX_STA_FIFO, &status); - - if (!rt2x00_get_field32(status, TX_STA_FIFO_VALID)) - break; - + i = 0; + do { if (!kfifo_put(&rt2x00dev->txstatus_fifo, status)) { - rt2x00_warn(rt2x00dev, "TX status FIFO overrun, drop tx status report\n"); + rt2x00_warn(rt2x00dev, + "TX status FIFO overrun, drop TX status report\n"); break; } - } + + if (++i >= rt2x00dev->tx->limit) + break; + + rt2x00mmio_register_read(rt2x00dev, TX_STA_FIFO, &status); + } while (rt2x00_get_field32(status, TX_STA_FIFO_VALID)); /* Schedule the tasklet for processing the tx status. */ tasklet_schedule(&rt2x00dev->txstatus_tasklet); } +#define RT2800MMIO_TXSTATUS_IRQ_MAX_RETRIES 4 + +static bool rt2800mmio_txstatus_is_spurious(struct rt2x00_dev *rt2x00dev, + u32 txstatus) +{ + if (likely(rt2x00_get_field32(txstatus, TX_STA_FIFO_VALID))) { + rt2x00dev->txstatus_irq_retries = 0; + return false; + } + + rt2x00dev->txstatus_irq_retries++; + + /* Ensure that we don't go into an infinite IRQ loop. */ + if (rt2x00dev->txstatus_irq_retries >= + RT2800MMIO_TXSTATUS_IRQ_MAX_RETRIES) { + rt2x00_warn(rt2x00dev, + "%u spurious TX_FIFO_STATUS interrupt(s)\n", + rt2x00dev->txstatus_irq_retries); + rt2x00dev->txstatus_irq_retries = 0; + return false; + } + + return true; +} + irqreturn_t rt2800mmio_interrupt(int irq, void *dev_instance) { struct rt2x00_dev *rt2x00dev = dev_instance; u32 reg, mask; + u32 txstatus = 0; - /* Read status and ACK all interrupts */ + /* Read status */ rt2x00mmio_register_read(rt2x00dev, INT_SOURCE_CSR, ®); + + if (rt2x00_get_field32(reg, INT_SOURCE_CSR_TX_FIFO_STATUS)) { + /* Due to unknown reason the hardware generates a + * TX_FIFO_STATUS interrupt before the TX_STA_FIFO + * register contain valid data. Read the TX status + * here to see if we have to process the actual + * request. + */ + rt2x00mmio_register_read(rt2x00dev, TX_STA_FIFO, &txstatus); + if (rt2800mmio_txstatus_is_spurious(rt2x00dev, txstatus)) { + /* Remove the TX_FIFO_STATUS bit so it won't be + * processed in this turn. The hardware will + * generate another IRQ for us. + */ + rt2x00_set_field32(®, + INT_SOURCE_CSR_TX_FIFO_STATUS, 0); + } + } + + /* ACK interrupts */ rt2x00mmio_register_write(rt2x00dev, INT_SOURCE_CSR, reg); if (!reg) @@ -477,7 +525,7 @@ irqreturn_t rt2800mmio_interrupt(int irq, void *dev_instance) mask = ~reg; if (rt2x00_get_field32(reg, INT_SOURCE_CSR_TX_FIFO_STATUS)) { - rt2800mmio_txstatus_interrupt(rt2x00dev); + rt2800mmio_txstatus_interrupt(rt2x00dev, txstatus); /* * Never disable the TX_FIFO_STATUS interrupt. */ diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00.h b/drivers/net/wireless/ralink/rt2x00/rt2x00.h index 034a07273038..c27408b494ef 100644 --- a/drivers/net/wireless/ralink/rt2x00/rt2x00.h +++ b/drivers/net/wireless/ralink/rt2x00/rt2x00.h @@ -995,6 +995,11 @@ struct rt2x00_dev { int rf_channel; /* + * Counter for tx status irq retries (rt2800pci). + */ + unsigned int txstatus_irq_retries; + + /* * Protect the interrupt mask register. */ spinlock_t irqmask_lock;