| Message ID | 1f5ffc7d-4b2e-4f07-bc7e-97d49ccff28c@infradead.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ps3/gelic: Fix SKB allocation | expand |
Hi, On Fri, 2024-01-26 at 18:25 +0900, Geoff Levand wrote: > Commit 3ce4f9c3fbb3 ("net/ps3_gelic_net: Add gelic_descr structures") > of 6.8-rc1 did not set up the ps3 gelic network SKB's correctly, > resulting in a kernel panic. > > This fix changes the way the napi buffer and corresponding SKB are > allocated and managed. > > Reported-by: sambat goson <sombat3960@gmail.com> > Fixes: 3ce4f9c3fbb3 ("net/ps3_gelic_net: Add gelic_descr structures") > Signed-off-by: Geoff Levand <geoff@infradead.org> The patch overall looks correct to me, but there are a few formal issues worthy to be addressed, see below. > > diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c > index d5b75af163d3..1870f173e850 100644 > --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c > +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c > @@ -375,20 +375,16 @@ static int gelic_card_init_chain(struct gelic_card *card, > static int gelic_descr_prepare_rx(struct gelic_card *card, > struct gelic_descr *descr) > { > - static const unsigned int rx_skb_size = > - ALIGN(GELIC_NET_MAX_FRAME, GELIC_NET_RXBUF_ALIGN) + > - GELIC_NET_RXBUF_ALIGN - 1; > + static const unsigned int napi_buff_size = > + round_up(GELIC_NET_MAX_FRAME, GELIC_NET_RXBUF_ALIGN); > + No empty line in the declaration area. > + struct device *dev = ctodev(card); > dma_addr_t cpu_addr; > - int offset; > + void *napi_buff; > > if (gelic_descr_get_status(descr) != GELIC_DESCR_DMA_NOT_IN_USE) > - dev_info(ctodev(card), "%s: ERROR status\n", __func__); > + dev_info(dev, "%s: ERROR status\n", __func__); > > - descr->skb = netdev_alloc_skb(*card->netdev, rx_skb_size); > - if (!descr->skb) { > - descr->hw_regs.payload.dev_addr = 0; /* tell DMAC don't touch memory */ > - return -ENOMEM; > - } > descr->hw_regs.dmac_cmd_status = 0; > descr->hw_regs.result_size = 0; > descr->hw_regs.valid_size = 0; > @@ -397,24 +393,33 @@ static int gelic_descr_prepare_rx(struct gelic_card *card, > descr->hw_regs.payload.size = 0; > descr->skb = NULL; > > - offset = ((unsigned long)descr->skb->data) & > - (GELIC_NET_RXBUF_ALIGN - 1); > - if (offset) > - skb_reserve(descr->skb, GELIC_NET_RXBUF_ALIGN - offset); > - /* io-mmu-map the skb */ > - cpu_addr = dma_map_single(ctodev(card), descr->skb->data, > - GELIC_NET_MAX_FRAME, DMA_FROM_DEVICE); > - descr->hw_regs.payload.dev_addr = cpu_to_be32(cpu_addr); > - if (dma_mapping_error(ctodev(card), cpu_addr)) { > - dev_kfree_skb_any(descr->skb); > + napi_buff = napi_alloc_frag_align(napi_buff_size, > + GELIC_NET_RXBUF_ALIGN); Please align with the above bracket: napi_buff = napi_alloc_frag_align(napi_buff_size, GELIC_NET_RXBUF_ALIGN); > + > + if (unlikely(!napi_buff)) { > + return -ENOMEM; > + } The brackets are not needed here. > + > + descr->skb = napi_build_skb(napi_buff, napi_buff_size); > + > + if (unlikely(!descr->skb)) { > + skb_free_frag(napi_buff); > + return -ENOMEM; > + } > + > + cpu_addr = dma_map_single(dev, napi_buff, napi_buff_size, > + DMA_FROM_DEVICE); Please align with the above bracket > + > + if (dma_mapping_error(dev, cpu_addr)) { > + skb_free_frag(napi_buff); > descr->skb = NULL; > - dev_info(ctodev(card), > - "%s:Could not iommu-map rx buffer\n", __func__); > + dev_err_once(dev, "%s:Could not iommu-map rx buffer\n", > + __func__); Same here. Side note: as a nice net-next follow-up you could move the skb allocation into gelic_net_pass_skb_up() - so that the skbuff is hot in the cache at rx time. Cheers, Paolo
Hi Paolo, On 1/30/24 21:37, Paolo Abeni wrote: > Hi, > > On Fri, 2024-01-26 at 18:25 +0900, Geoff Levand wrote: >> Commit 3ce4f9c3fbb3 ("net/ps3_gelic_net: Add gelic_descr structures") >> of 6.8-rc1 did not set up the ps3 gelic network SKB's correctly, >> resulting in a kernel panic. >> >> This fix changes the way the napi buffer and corresponding SKB are >> allocated and managed. >> >> Reported-by: sambat goson <sombat3960@gmail.com> >> Fixes: 3ce4f9c3fbb3 ("net/ps3_gelic_net: Add gelic_descr structures") >> Signed-off-by: Geoff Levand <geoff@infradead.org> > > The patch overall looks correct to me, but there are a few formal > issues worthy to be addressed, see below. Thanks for the review. I'll send out an updated patch sometime soon. -Geoff
diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c index d5b75af163d3..1870f173e850 100644 --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c @@ -375,20 +375,16 @@ static int gelic_card_init_chain(struct gelic_card *card, static int gelic_descr_prepare_rx(struct gelic_card *card, struct gelic_descr *descr) { - static const unsigned int rx_skb_size = - ALIGN(GELIC_NET_MAX_FRAME, GELIC_NET_RXBUF_ALIGN) + - GELIC_NET_RXBUF_ALIGN - 1; + static const unsigned int napi_buff_size = + round_up(GELIC_NET_MAX_FRAME, GELIC_NET_RXBUF_ALIGN); + + struct device *dev = ctodev(card); dma_addr_t cpu_addr; - int offset; + void *napi_buff; if (gelic_descr_get_status(descr) != GELIC_DESCR_DMA_NOT_IN_USE) - dev_info(ctodev(card), "%s: ERROR status\n", __func__); + dev_info(dev, "%s: ERROR status\n", __func__); - descr->skb = netdev_alloc_skb(*card->netdev, rx_skb_size); - if (!descr->skb) { - descr->hw_regs.payload.dev_addr = 0; /* tell DMAC don't touch memory */ - return -ENOMEM; - } descr->hw_regs.dmac_cmd_status = 0; descr->hw_regs.result_size = 0; descr->hw_regs.valid_size = 0; @@ -397,24 +393,33 @@ static int gelic_descr_prepare_rx(struct gelic_card *card, descr->hw_regs.payload.size = 0; descr->skb = NULL; - offset = ((unsigned long)descr->skb->data) & - (GELIC_NET_RXBUF_ALIGN - 1); - if (offset) - skb_reserve(descr->skb, GELIC_NET_RXBUF_ALIGN - offset); - /* io-mmu-map the skb */ - cpu_addr = dma_map_single(ctodev(card), descr->skb->data, - GELIC_NET_MAX_FRAME, DMA_FROM_DEVICE); - descr->hw_regs.payload.dev_addr = cpu_to_be32(cpu_addr); - if (dma_mapping_error(ctodev(card), cpu_addr)) { - dev_kfree_skb_any(descr->skb); + napi_buff = napi_alloc_frag_align(napi_buff_size, + GELIC_NET_RXBUF_ALIGN); + + if (unlikely(!napi_buff)) { + return -ENOMEM; + } + + descr->skb = napi_build_skb(napi_buff, napi_buff_size); + + if (unlikely(!descr->skb)) { + skb_free_frag(napi_buff); + return -ENOMEM; + } + + cpu_addr = dma_map_single(dev, napi_buff, napi_buff_size, + DMA_FROM_DEVICE); + + if (dma_mapping_error(dev, cpu_addr)) { + skb_free_frag(napi_buff); descr->skb = NULL; - dev_info(ctodev(card), - "%s:Could not iommu-map rx buffer\n", __func__); + dev_err_once(dev, "%s:Could not iommu-map rx buffer\n", + __func__); gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE); return -ENOMEM; } - descr->hw_regs.payload.size = cpu_to_be32(GELIC_NET_MAX_FRAME); + descr->hw_regs.payload.size = cpu_to_be32(napi_buff_size); descr->hw_regs.payload.dev_addr = cpu_to_be32(cpu_addr); gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
Commit 3ce4f9c3fbb3 ("net/ps3_gelic_net: Add gelic_descr structures") of 6.8-rc1 did not set up the ps3 gelic network SKB's correctly, resulting in a kernel panic. This fix changes the way the napi buffer and corresponding SKB are allocated and managed. Reported-by: sambat goson <sombat3960@gmail.com> Fixes: 3ce4f9c3fbb3 ("net/ps3_gelic_net: Add gelic_descr structures") Signed-off-by: Geoff Levand <geoff@infradead.org>