| Message ID | 20191120114647.88967-1-nbd@nbd.name (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Felix Fietkau |
| Headers | show |
| Series | mt76: use rcu_read_lock_bh in mt76_dma_rx_poll | expand |
On 11/20/19 12:46 PM, Felix Fietkau wrote: > Fixes potential RCU issues and avoids calling ieee80211_rx_napi with softirq > enabled. > > Reported-by: Markus Theil <markus.theil@tu-ilmenau.de> > Signed-off-by: Felix Fietkau <nbd@nbd.name> > --- > drivers/net/wireless/mediatek/mt76/dma.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/mediatek/mt76/dma.c b/drivers/net/wireless/mediatek/mt76/dma.c > index 6173c80189ba..ed0ee2d4a452 100644 > --- a/drivers/net/wireless/mediatek/mt76/dma.c > +++ b/drivers/net/wireless/mediatek/mt76/dma.c > @@ -528,7 +528,7 @@ mt76_dma_rx_poll(struct napi_struct *napi, int budget) > dev = container_of(napi->dev, struct mt76_dev, napi_dev); > qid = napi - dev->napi; > > - rcu_read_lock(); > + rcu_read_lock_bh(); > > do { > cur = mt76_dma_rx_process(dev, &dev->q_rx[qid], budget - done); > @@ -536,7 +536,7 @@ mt76_dma_rx_poll(struct napi_struct *napi, int budget) > done += cur; > } while (cur && done < budget); > > - rcu_read_unlock(); > + rcu_read_unlock_bh(); > > if (done < budget && napi_complete(napi)) > dev->drv->rx_poll_complete(dev, qid); This patch is incomplete. The same problem exists for usb. I'll send a fix for that. Regards, Markus
On 2019-11-20 12:59, Markus Theil wrote: > On 11/20/19 12:46 PM, Felix Fietkau wrote: >> Fixes potential RCU issues and avoids calling ieee80211_rx_napi with softirq >> enabled. >> >> Reported-by: Markus Theil <markus.theil@tu-ilmenau.de> >> Signed-off-by: Felix Fietkau <nbd@nbd.name> >> --- >> drivers/net/wireless/mediatek/mt76/dma.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/wireless/mediatek/mt76/dma.c b/drivers/net/wireless/mediatek/mt76/dma.c >> index 6173c80189ba..ed0ee2d4a452 100644 >> --- a/drivers/net/wireless/mediatek/mt76/dma.c >> +++ b/drivers/net/wireless/mediatek/mt76/dma.c >> @@ -528,7 +528,7 @@ mt76_dma_rx_poll(struct napi_struct *napi, int budget) >> dev = container_of(napi->dev, struct mt76_dev, napi_dev); >> qid = napi - dev->napi; >> >> - rcu_read_lock(); >> + rcu_read_lock_bh(); >> >> do { >> cur = mt76_dma_rx_process(dev, &dev->q_rx[qid], budget - done); >> @@ -536,7 +536,7 @@ mt76_dma_rx_poll(struct napi_struct *napi, int budget) >> done += cur; >> } while (cur && done < budget); >> >> - rcu_read_unlock(); >> + rcu_read_unlock_bh(); >> >> if (done < budget && napi_complete(napi)) >> dev->drv->rx_poll_complete(dev, qid); > > This patch is incomplete. The same problem exists for usb. I'll send a fix for that. How so? On usb, it is called from a tasklet already. - Felix
On 11/20/19 1:03 PM, Felix Fietkau wrote: > On 2019-11-20 12:59, Markus Theil wrote: >> On 11/20/19 12:46 PM, Felix Fietkau wrote: >>> Fixes potential RCU issues and avoids calling ieee80211_rx_napi with softirq >>> enabled. >>> >>> Reported-by: Markus Theil <markus.theil@tu-ilmenau.de> >>> Signed-off-by: Felix Fietkau <nbd@nbd.name> >>> --- >>> drivers/net/wireless/mediatek/mt76/dma.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/wireless/mediatek/mt76/dma.c b/drivers/net/wireless/mediatek/mt76/dma.c >>> index 6173c80189ba..ed0ee2d4a452 100644 >>> --- a/drivers/net/wireless/mediatek/mt76/dma.c >>> +++ b/drivers/net/wireless/mediatek/mt76/dma.c >>> @@ -528,7 +528,7 @@ mt76_dma_rx_poll(struct napi_struct *napi, int budget) >>> dev = container_of(napi->dev, struct mt76_dev, napi_dev); >>> qid = napi - dev->napi; >>> >>> - rcu_read_lock(); >>> + rcu_read_lock_bh(); >>> >>> do { >>> cur = mt76_dma_rx_process(dev, &dev->q_rx[qid], budget - done); >>> @@ -536,7 +536,7 @@ mt76_dma_rx_poll(struct napi_struct *napi, int budget) >>> done += cur; >>> } while (cur && done < budget); >>> >>> - rcu_read_unlock(); >>> + rcu_read_unlock_bh(); >>> >>> if (done < budget && napi_complete(napi)) >>> dev->drv->rx_poll_complete(dev, qid); >> This patch is incomplete. The same problem exists for usb. I'll send a fix for that. > How so? On usb, it is called from a tasklet already. > > - Felix I've noticed the bug I reported on USB devices. I've only seen that mt76_rx_aggr_reorder_work also accesses mt76_rx_complete. Maybe it only worked for me by accident. Of course, the tasklet should not run in parallel with multiple instances. Markus
diff --git a/drivers/net/wireless/mediatek/mt76/dma.c b/drivers/net/wireless/mediatek/mt76/dma.c index 6173c80189ba..ed0ee2d4a452 100644 --- a/drivers/net/wireless/mediatek/mt76/dma.c +++ b/drivers/net/wireless/mediatek/mt76/dma.c @@ -528,7 +528,7 @@ mt76_dma_rx_poll(struct napi_struct *napi, int budget) dev = container_of(napi->dev, struct mt76_dev, napi_dev); qid = napi - dev->napi; - rcu_read_lock(); + rcu_read_lock_bh(); do { cur = mt76_dma_rx_process(dev, &dev->q_rx[qid], budget - done); @@ -536,7 +536,7 @@ mt76_dma_rx_poll(struct napi_struct *napi, int budget) done += cur; } while (cur && done < budget); - rcu_read_unlock(); + rcu_read_unlock_bh(); if (done < budget && napi_complete(napi)) dev->drv->rx_poll_complete(dev, qid);
Fixes potential RCU issues and avoids calling ieee80211_rx_napi with softirq enabled. Reported-by: Markus Theil <markus.theil@tu-ilmenau.de> Signed-off-by: Felix Fietkau <nbd@nbd.name> --- drivers/net/wireless/mediatek/mt76/dma.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)