[PATCH] mt76: disable softirqs while calling ieee80211_rx_napi

[PATCH] mt76: disable softirqs while calling ieee80211_rx_napi

[Markus Theil]

mac80211 assumes ieee80211_rx_napi to be called with disabled softirqs.

ieee80211_rx_napi in mac80211.c can be called from aggregation reordering work queue
or from mt76_rx_poll_complete. mt76_rx_poll_complete does currently not disable softirq
processing.

This patch fixes this by disabling softirqs before calling ieee80211_rx_napi.
It should be no problem to disable them twice, if mt76_aggr_reorder_work calls ieee80211_rx_napi
and has already called local_bh_disable, as local_bh_disable/local_bh_enable are reentrant.

I became aware of this issue by the following dmesg output:
  NOHZ: local_softirq_pending 08

Signed-off-by: Markus Theil <markus.theil@tu-ilmenau.de>
---
 drivers/net/wireless/mediatek/mt76/mac80211.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mac80211.c b/drivers/net/wireless/mediatek/mt76/mac80211.c
index 1a2c143b34d0..43c050660fc7 100644
--- a/drivers/net/wireless/mediatek/mt76/mac80211.c
+++ b/drivers/net/wireless/mediatek/mt76/mac80211.c
@@ -628,7 +628,7 @@ void mt76_rx_complete(struct mt76_dev *dev, struct sk_buff_head *frames,
 	struct ieee80211_sta *sta;
 	struct sk_buff *skb;

-	spin_lock(&dev->rx_lock);
+	spin_lock_bh(&dev->rx_lock);
 	while ((skb = __skb_dequeue(frames)) != NULL) {
 		if (mt76_check_ccmp_pn(skb)) {
 			dev_kfree_skb(skb);
@@ -638,7 +638,7 @@ void mt76_rx_complete(struct mt76_dev *dev, struct sk_buff_head *frames,
 		sta = mt76_rx_convert(skb);
 		ieee80211_rx_napi(dev->hw, sta, skb, napi);
 	}
-	spin_unlock(&dev->rx_lock);
+	spin_unlock_bh(&dev->rx_lock);
 }

 void mt76_rx_poll_complete(struct mt76_dev *dev, enum mt76_rxq_id q,
--
2.20.1

Re: [PATCH] mt76: disable softirqs while calling ieee80211_rx_napi

[Felix Fietkau]

On 2019-11-04 14:45, Markus Theil wrote:
> mac80211 assumes ieee80211_rx_napi to be called with disabled softirqs.
> 
> ieee80211_rx_napi in mac80211.c can be called from aggregation reordering work queue
> or from mt76_rx_poll_complete. mt76_rx_poll_complete does currently not disable softirq
> processing.
> 
> This patch fixes this by disabling softirqs before calling ieee80211_rx_napi.
> It should be no problem to disable them twice, if mt76_aggr_reorder_work calls ieee80211_rx_napi
> and has already called local_bh_disable, as local_bh_disable/local_bh_enable are reentrant.
> 
> I became aware of this issue by the following dmesg output:
>   NOHZ: local_softirq_pending 08
> 
> Signed-off-by: Markus Theil <markus.theil@tu-ilmenau.de>
I believe this fix is incomplete. If we run with softirq enabled at this
point, it also implies that we've taken the RCU lock with softirq
enabled, which we really shouldn't do.
I believe this should be fixed by changing rcu_read_lock/unlock to the
_bh variant in mt76_dma_rx_poll(). I will send a patch for that.

Thanks,

- Felix