| Message ID | 1523368459-32128-1-git-send-email-baijiaju1990@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6e1d8d1470b2c9335715f7d52e864f0bd91a5f59 |
| Delegated to: | Kalle Valo |
| Headers | show |
On Tue, 10 Apr 2018 21:54:19 +0800 Jia-Ju Bai <baijiaju1990@gmail.com> wrote: > dma_tx_fragment() is never called in atomic context. > > dma_tx_fragment() is only called by b43legacy_dma_tx(), which is > only called by b43legacy_tx_work(). > b43legacy_tx_work() is only set a parameter of INIT_WORK() in > b43legacy_wireless_init(). > > Despite never getting called from atomic context, > dma_tx_fragment() calls alloc_skb() with GFP_ATOMIC, > which does not sleep for allocation. > GFP_ATOMIC is not necessary and can be replaced with GFP_KERNEL, > which can sleep and improve the possibility of sucessful allocation. > > This is found by a static analysis tool named DCNS written by myself. > And I also manually check it. > > Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> > --- > drivers/net/wireless/broadcom/b43legacy/dma.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/broadcom/b43legacy/dma.c b/drivers/net/wireless/broadcom/b43legacy/dma.c > index cfa617d..2f0c64c 100644 > --- a/drivers/net/wireless/broadcom/b43legacy/dma.c > +++ b/drivers/net/wireless/broadcom/b43legacy/dma.c > @@ -1064,7 +1064,7 @@ static int dma_tx_fragment(struct b43legacy_dmaring *ring, > meta->dmaaddr = map_descbuffer(ring, skb->data, skb->len, 1); > /* create a bounce buffer in zone_dma on mapping failure. */ > if (b43legacy_dma_mapping_error(ring, meta->dmaaddr, skb->len, 1)) { > - bounce_skb = alloc_skb(skb->len, GFP_ATOMIC | GFP_DMA); > + bounce_skb = alloc_skb(skb->len, GFP_KERNEL | GFP_DMA); > if (!bounce_skb) { > ring->current_slot = old_top_slot; > ring->used_slots = old_used_slots; Ack. I think the GFP_ATOMIC came from the days where we did DMA operations under spinlock instead of mutex. The same thing can be done in b43. Also setup_rx_descbuffer(ring, desc, meta, GFP_ATOMIC) could be GFP_KERNEL in dma_rx(). This function is called from IRQ thread context.
Jia-Ju Bai <baijiaju1990@gmail.com> wrote: > dma_tx_fragment() is never called in atomic context. > > dma_tx_fragment() is only called by b43legacy_dma_tx(), which is > only called by b43legacy_tx_work(). > b43legacy_tx_work() is only set a parameter of INIT_WORK() in > b43legacy_wireless_init(). > > Despite never getting called from atomic context, > dma_tx_fragment() calls alloc_skb() with GFP_ATOMIC, > which does not sleep for allocation. > GFP_ATOMIC is not necessary and can be replaced with GFP_KERNEL, > which can sleep and improve the possibility of sucessful allocation. > > This is found by a static analysis tool named DCNS written by myself. > And I also manually check it. > > Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> Patch applied to wireless-drivers-next.git, thanks. 6e1d8d1470b2 net: wireless: b43legacy: Replace GFP_ATOMIC with GFP_KERNEL in dma_tx_fragment
diff --git a/drivers/net/wireless/broadcom/b43legacy/dma.c b/drivers/net/wireless/broadcom/b43legacy/dma.c index cfa617d..2f0c64c 100644 --- a/drivers/net/wireless/broadcom/b43legacy/dma.c +++ b/drivers/net/wireless/broadcom/b43legacy/dma.c @@ -1064,7 +1064,7 @@ static int dma_tx_fragment(struct b43legacy_dmaring *ring, meta->dmaaddr = map_descbuffer(ring, skb->data, skb->len, 1); /* create a bounce buffer in zone_dma on mapping failure. */ if (b43legacy_dma_mapping_error(ring, meta->dmaaddr, skb->len, 1)) { - bounce_skb = alloc_skb(skb->len, GFP_ATOMIC | GFP_DMA); + bounce_skb = alloc_skb(skb->len, GFP_KERNEL | GFP_DMA); if (!bounce_skb) { ring->current_slot = old_top_slot; ring->used_slots = old_used_slots;
dma_tx_fragment() is never called in atomic context. dma_tx_fragment() is only called by b43legacy_dma_tx(), which is only called by b43legacy_tx_work(). b43legacy_tx_work() is only set a parameter of INIT_WORK() in b43legacy_wireless_init(). Despite never getting called from atomic context, dma_tx_fragment() calls alloc_skb() with GFP_ATOMIC, which does not sleep for allocation. GFP_ATOMIC is not necessary and can be replaced with GFP_KERNEL, which can sleep and improve the possibility of sucessful allocation. This is found by a static analysis tool named DCNS written by myself. And I also manually check it. Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> --- drivers/net/wireless/broadcom/b43legacy/dma.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)