[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <757018c2d9bd235cd2dad4363fb9a54354c9a372.camel@redhat.com>
Date: Tue, 30 Jan 2024 13:37:25 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Geoff Levand <geoff@...radead.org>, sambat goson <sombat3960@...il.com>,
Christophe Leroy <christophe.leroy@...roup.eu>, "David S. Miller"
<davem@...emloft.net>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net] ps3/gelic: Fix SKB allocation
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@...il.com>
> Fixes: 3ce4f9c3fbb3 ("net/ps3_gelic_net: Add gelic_descr structures")
> Signed-off-by: Geoff Levand <geoff@...radead.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
Powered by blists - more mailing lists