2020-04-09 15:59:24

by Felix Fietkau

[permalink] [raw]
Subject: [PATCH] mt76: mt7615: rework IRQ handling to prepare for MSI support

With MSI interrupts, IRQs must not be enabled from within the IRQ handler,
because that can lead to lost events.
Defer IRQ processing to a tasklet, which is also responsible for enabling
IRQs (to avoid race conditions against the handler)

Co-developed-by: Soul Huang <[email protected]>
Signed-off-by: Felix Fietkau <[email protected]>
---
drivers/net/wireless/mediatek/mt76/mmio.c | 3 +-
.../net/wireless/mediatek/mt76/mt7615/init.c | 2 ++
.../net/wireless/mediatek/mt76/mt7615/mmio.c | 30 +++++++++++++------
.../wireless/mediatek/mt76/mt7615/mt7615.h | 9 +++---
4 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mmio.c b/drivers/net/wireless/mediatek/mt76/mmio.c
index 7ead6620bb8b..26353b6bce97 100644
--- a/drivers/net/wireless/mediatek/mt76/mmio.c
+++ b/drivers/net/wireless/mediatek/mt76/mmio.c
@@ -73,7 +73,8 @@ void mt76_set_irq_mask(struct mt76_dev *dev, u32 addr,
spin_lock_irqsave(&dev->mmio.irq_lock, flags);
dev->mmio.irqmask &= ~clear;
dev->mmio.irqmask |= set;
- mt76_mmio_wr(dev, addr, dev->mmio.irqmask);
+ if (addr)
+ mt76_mmio_wr(dev, addr, dev->mmio.irqmask);
spin_unlock_irqrestore(&dev->mmio.irq_lock, flags);
}
EXPORT_SYMBOL_GPL(mt76_set_irq_mask);
diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/init.c b/drivers/net/wireless/mediatek/mt76/mt7615/init.c
index 96b7c6284833..cb626a2d9197 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7615/init.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7615/init.c
@@ -561,5 +561,7 @@ void mt7615_unregister_device(struct mt7615_dev *dev)
spin_unlock_bh(&dev->token_lock);
idr_destroy(&dev->token);

+ tasklet_disable(&dev->irq_tasklet);
+
mt76_free_device(&dev->mt76);
}
diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/mmio.c b/drivers/net/wireless/mediatek/mt76/mt7615/mmio.c
index 3849bb6b49d0..58506dc423ab 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7615/mmio.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7615/mmio.c
@@ -80,30 +80,41 @@ mt7615_rx_poll_complete(struct mt76_dev *mdev, enum mt76_rxq_id q)
static irqreturn_t mt7615_irq_handler(int irq, void *dev_instance)
{
struct mt7615_dev *dev = dev_instance;
- u32 intr;
-
- intr = mt76_rr(dev, MT_INT_SOURCE_CSR);
- mt76_wr(dev, MT_INT_SOURCE_CSR, intr);

if (!test_bit(MT76_STATE_INITIALIZED, &dev->mphy.state))
return IRQ_NONE;

- trace_dev_irq(&dev->mt76, intr, dev->mt76.mmio.irqmask);
+ mt76_wr(dev, MT_INT_MASK_CSR, 0);
+ tasklet_schedule(&dev->irq_tasklet);

+ return IRQ_HANDLED;
+}
+
+static void mt7615_irq_tasklet(unsigned long data)
+{
+ struct mt7615_dev *dev = (struct mt7615_dev *)data;
+ u32 intr, mask = 0;
+
+ mt76_wr(dev, MT_INT_MASK_CSR, 0);
+
+ intr = mt76_rr(dev, MT_INT_SOURCE_CSR);
+ mt76_wr(dev, MT_INT_SOURCE_CSR, intr);
+
+ trace_dev_irq(&dev->mt76, intr, dev->mt76.mmio.irqmask);
intr &= dev->mt76.mmio.irqmask;

if (intr & MT_INT_TX_DONE_ALL) {
- mt7615_irq_disable(dev, MT_INT_TX_DONE_ALL);
+ mask |= MT_INT_TX_DONE_ALL;
napi_schedule(&dev->mt76.tx_napi);
}

if (intr & MT_INT_RX_DONE(0)) {
- mt7615_irq_disable(dev, MT_INT_RX_DONE(0));
+ mask |= MT_INT_RX_DONE(0);
napi_schedule(&dev->mt76.napi[0]);
}

if (intr & MT_INT_RX_DONE(1)) {
- mt7615_irq_disable(dev, MT_INT_RX_DONE(1));
+ mask |= MT_INT_RX_DONE(1);
napi_schedule(&dev->mt76.napi[1]);
}

@@ -117,7 +128,7 @@ static irqreturn_t mt7615_irq_handler(int irq, void *dev_instance)
}
}

- return IRQ_HANDLED;
+ mt76_set_irq_mask(&dev->mt76, MT_INT_MASK_CSR, mask, 0);
}

int mt7615_mmio_probe(struct device *pdev, void __iomem *mem_base,
@@ -154,6 +165,7 @@ int mt7615_mmio_probe(struct device *pdev, void __iomem *mem_base,

dev = container_of(mdev, struct mt7615_dev, mt76);
mt76_mmio_init(&dev->mt76, mem_base);
+ tasklet_init(&dev->irq_tasklet, mt7615_irq_tasklet, (unsigned long)dev);

dev->reg_map = map;
dev->ops = ops;
diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/mt7615.h b/drivers/net/wireless/mediatek/mt76/mt7615/mt7615.h
index 4f0d29e5e595..10a98d38f77e 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7615/mt7615.h
+++ b/drivers/net/wireless/mediatek/mt76/mt7615/mt7615.h
@@ -229,6 +229,8 @@ struct mt7615_dev {
struct mt76_phy mphy;
};

+ struct tasklet_struct irq_tasklet;
+
struct mt7615_phy phy;
u32 vif_mask;
u32 omac_mask;
@@ -404,12 +406,9 @@ static inline bool is_mt7663(struct mt76_dev *dev)

static inline void mt7615_irq_enable(struct mt7615_dev *dev, u32 mask)
{
- mt76_set_irq_mask(&dev->mt76, MT_INT_MASK_CSR, 0, mask);
-}
+ mt76_set_irq_mask(&dev->mt76, 0, 0, mask);

-static inline void mt7615_irq_disable(struct mt7615_dev *dev, u32 mask)
-{
- mt76_set_irq_mask(&dev->mt76, MT_INT_MASK_CSR, mask, 0);
+ tasklet_schedule(&dev->irq_tasklet);
}

static inline bool mt7615_firmware_offload(struct mt7615_dev *dev)
--
2.24.0


2020-04-09 17:08:17

by Lorenzo Bianconi

[permalink] [raw]
Subject: Re: [PATCH] mt76: mt7615: rework IRQ handling to prepare for MSI support

> With MSI interrupts, IRQs must not be enabled from within the IRQ handler,
> because that can lead to lost events.
> Defer IRQ processing to a tasklet, which is also responsible for enabling
> IRQs (to avoid race conditions against the handler)
>
> Co-developed-by: Soul Huang <[email protected]>
> Signed-off-by: Felix Fietkau <[email protected]>

Acked-by: Lorenzo Bianconi <[email protected]>

> ---
> drivers/net/wireless/mediatek/mt76/mmio.c | 3 +-
> .../net/wireless/mediatek/mt76/mt7615/init.c | 2 ++
> .../net/wireless/mediatek/mt76/mt7615/mmio.c | 30 +++++++++++++------
> .../wireless/mediatek/mt76/mt7615/mt7615.h | 9 +++---
> 4 files changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/wireless/mediatek/mt76/mmio.c b/drivers/net/wireless/mediatek/mt76/mmio.c
> index 7ead6620bb8b..26353b6bce97 100644
> --- a/drivers/net/wireless/mediatek/mt76/mmio.c
> +++ b/drivers/net/wireless/mediatek/mt76/mmio.c
> @@ -73,7 +73,8 @@ void mt76_set_irq_mask(struct mt76_dev *dev, u32 addr,
> spin_lock_irqsave(&dev->mmio.irq_lock, flags);
> dev->mmio.irqmask &= ~clear;
> dev->mmio.irqmask |= set;
> - mt76_mmio_wr(dev, addr, dev->mmio.irqmask);
> + if (addr)
> + mt76_mmio_wr(dev, addr, dev->mmio.irqmask);
> spin_unlock_irqrestore(&dev->mmio.irq_lock, flags);
> }
> EXPORT_SYMBOL_GPL(mt76_set_irq_mask);
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/init.c b/drivers/net/wireless/mediatek/mt76/mt7615/init.c
> index 96b7c6284833..cb626a2d9197 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7615/init.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7615/init.c
> @@ -561,5 +561,7 @@ void mt7615_unregister_device(struct mt7615_dev *dev)
> spin_unlock_bh(&dev->token_lock);
> idr_destroy(&dev->token);
>
> + tasklet_disable(&dev->irq_tasklet);
> +
> mt76_free_device(&dev->mt76);
> }
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/mmio.c b/drivers/net/wireless/mediatek/mt76/mt7615/mmio.c
> index 3849bb6b49d0..58506dc423ab 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7615/mmio.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7615/mmio.c
> @@ -80,30 +80,41 @@ mt7615_rx_poll_complete(struct mt76_dev *mdev, enum mt76_rxq_id q)
> static irqreturn_t mt7615_irq_handler(int irq, void *dev_instance)
> {
> struct mt7615_dev *dev = dev_instance;
> - u32 intr;
> -
> - intr = mt76_rr(dev, MT_INT_SOURCE_CSR);
> - mt76_wr(dev, MT_INT_SOURCE_CSR, intr);
>
> if (!test_bit(MT76_STATE_INITIALIZED, &dev->mphy.state))
> return IRQ_NONE;
>
> - trace_dev_irq(&dev->mt76, intr, dev->mt76.mmio.irqmask);
> + mt76_wr(dev, MT_INT_MASK_CSR, 0);
> + tasklet_schedule(&dev->irq_tasklet);
>
> + return IRQ_HANDLED;
> +}
> +
> +static void mt7615_irq_tasklet(unsigned long data)
> +{
> + struct mt7615_dev *dev = (struct mt7615_dev *)data;
> + u32 intr, mask = 0;
> +
> + mt76_wr(dev, MT_INT_MASK_CSR, 0);
> +
> + intr = mt76_rr(dev, MT_INT_SOURCE_CSR);
> + mt76_wr(dev, MT_INT_SOURCE_CSR, intr);
> +
> + trace_dev_irq(&dev->mt76, intr, dev->mt76.mmio.irqmask);
> intr &= dev->mt76.mmio.irqmask;
>
> if (intr & MT_INT_TX_DONE_ALL) {
> - mt7615_irq_disable(dev, MT_INT_TX_DONE_ALL);
> + mask |= MT_INT_TX_DONE_ALL;
> napi_schedule(&dev->mt76.tx_napi);
> }
>
> if (intr & MT_INT_RX_DONE(0)) {
> - mt7615_irq_disable(dev, MT_INT_RX_DONE(0));
> + mask |= MT_INT_RX_DONE(0);
> napi_schedule(&dev->mt76.napi[0]);
> }
>
> if (intr & MT_INT_RX_DONE(1)) {
> - mt7615_irq_disable(dev, MT_INT_RX_DONE(1));
> + mask |= MT_INT_RX_DONE(1);
> napi_schedule(&dev->mt76.napi[1]);
> }
>
> @@ -117,7 +128,7 @@ static irqreturn_t mt7615_irq_handler(int irq, void *dev_instance)
> }
> }
>
> - return IRQ_HANDLED;
> + mt76_set_irq_mask(&dev->mt76, MT_INT_MASK_CSR, mask, 0);
> }
>
> int mt7615_mmio_probe(struct device *pdev, void __iomem *mem_base,
> @@ -154,6 +165,7 @@ int mt7615_mmio_probe(struct device *pdev, void __iomem *mem_base,
>
> dev = container_of(mdev, struct mt7615_dev, mt76);
> mt76_mmio_init(&dev->mt76, mem_base);
> + tasklet_init(&dev->irq_tasklet, mt7615_irq_tasklet, (unsigned long)dev);
>
> dev->reg_map = map;
> dev->ops = ops;
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/mt7615.h b/drivers/net/wireless/mediatek/mt76/mt7615/mt7615.h
> index 4f0d29e5e595..10a98d38f77e 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7615/mt7615.h
> +++ b/drivers/net/wireless/mediatek/mt76/mt7615/mt7615.h
> @@ -229,6 +229,8 @@ struct mt7615_dev {
> struct mt76_phy mphy;
> };
>
> + struct tasklet_struct irq_tasklet;
> +
> struct mt7615_phy phy;
> u32 vif_mask;
> u32 omac_mask;
> @@ -404,12 +406,9 @@ static inline bool is_mt7663(struct mt76_dev *dev)
>
> static inline void mt7615_irq_enable(struct mt7615_dev *dev, u32 mask)
> {
> - mt76_set_irq_mask(&dev->mt76, MT_INT_MASK_CSR, 0, mask);
> -}
> + mt76_set_irq_mask(&dev->mt76, 0, 0, mask);
>
> -static inline void mt7615_irq_disable(struct mt7615_dev *dev, u32 mask)
> -{
> - mt76_set_irq_mask(&dev->mt76, MT_INT_MASK_CSR, mask, 0);
> + tasklet_schedule(&dev->irq_tasklet);
> }
>
> static inline bool mt7615_firmware_offload(struct mt7615_dev *dev)
> --
> 2.24.0
>

Attachments:
(No filename) (5.22 kB)
 signature.asc (235.00 B)
 Download all attachments

2020-04-15 10:30:34

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH] mt76: mt7615: rework IRQ handling to prepare for MSI support

Felix Fietkau <[email protected]> writes:

> With MSI interrupts, IRQs must not be enabled from within the IRQ handler,
> because that can lead to lost events.
> Defer IRQ processing to a tasklet, which is also responsible for enabling
> IRQs (to avoid race conditions against the handler)
>
> Co-developed-by: Soul Huang <[email protected]>

Soul's s-o-b is missing.

--
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches