* Re: [PATCH net v2 1/2] net/ps3_gelic_net: Fix RX skbuff length
[not found] ` <c9a523347acc2d399b667472e158b5fdfbadc941.1674436603.git.geoff@infradead.org>
@ 2023-01-25 2:31 ` Jakub Kicinski
0 siblings, 0 replies; 2+ messages in thread
From: Jakub Kicinski @ 2023-01-25 2:31 UTC (permalink / raw)
To: Geoff Levand; +Cc: David S. Miller, netdev
The patches didn't make it to patchwork or the lore archive.
Keep an eye for any irregularities when reposting.
On Mon, 23 Jan 2023 01:24:25 +0000 Geoff Levand wrote:
> The Gelic Etherenet device needs to have the RX skbuffs aligned to 128 bytes.
> The current Gelic Etherenet driver was not allocating skbuffs large enough to
> allow for this alignment.
>
> This change adds a new local structure named aligned_buff to help calculate a
> buffer size large enough to allow for this alignment.
>
> Fixes various randomly occurring runtime network errors.
>
> Signed-off-by: Geoff Levand <geoff@infradead.org>
Please add a Fixes tag.
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> index cf8de8a7a8a1..43e438f8f595 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> @@ -366,50 +366,67 @@ static int gelic_card_init_chain(struct gelic_card *card,
> * allocates a new rx skb, iommu-maps it and attaches it to the descriptor.
> * Activate the descriptor state-wise
> */
> +
Why this new line?
> static int gelic_descr_prepare_rx(struct gelic_card *card,
> struct gelic_descr *descr)
> {
> - int offset;
> - unsigned int bufsize;
> + struct device *dev = ctodev(card);
> + struct aligned_buff {
> + unsigned int total_bytes;
> + unsigned int offset;
> + };
> + struct aligned_buff a_buf;
You can declare this as a anonymous struct:
struct {
unsigned int total_bytes;
unsigned int offset;
} a_buf;
> + dma_addr_t cpu_addr;
> +
> + if (gelic_descr_get_status(descr) != GELIC_DESCR_DMA_NOT_IN_USE) {
> + dev_err(dev, "%s:%d: ERROR status\n", __func__, __LINE__);
> + }
The fixes should be minimal, please don't change prints or reformat
the code unless it makes it a lot easier to understand.
> + a_buf.total_bytes = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN)
> + + GELIC_NET_RXBUF_ALIGN;
>
> - if (gelic_descr_get_status(descr) != GELIC_DESCR_DMA_NOT_IN_USE)
> - dev_info(ctodev(card), "%s: ERROR status\n", __func__);
> - /* we need to round up the buffer size to a multiple of 128 */
> - bufsize = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN);
> + descr->skb = dev_alloc_skb(a_buf.total_bytes);
>
> - /* and we need to have it 128 byte aligned, therefore we allocate a
> - * bit more */
> - descr->skb = dev_alloc_skb(bufsize + GELIC_NET_RXBUF_ALIGN - 1);
So what did you change? This is hard to read.
Allocating ALIGN - 1 more space should be enough to align the pointer.
> if (!descr->skb) {
> - descr->buf_addr = 0; /* tell DMAC don't touch memory */
> + descr->buf_addr = 0;
> return -ENOMEM;
> }
> - descr->buf_size = cpu_to_be32(bufsize);
> +
> + a_buf.offset = PTR_ALIGN(descr->skb->data, GELIC_NET_RXBUF_ALIGN)
> + - descr->skb->data;
> +
> + if (a_buf.offset) {
> + dev_dbg(dev, "%s:%d: offset=%u\n", __func__, __LINE__,
> + a_buf.offset);
> + skb_reserve(descr->skb, a_buf.offset);
> + }
> +
> + descr->buf_size = a_buf.total_bytes - a_buf.offset;
> descr->dmac_cmd_status = 0;
> descr->result_size = 0;
> descr->valid_size = 0;
> descr->data_error = 0;
>
> - 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 */
> - descr->buf_addr = cpu_to_be32(dma_map_single(ctodev(card),
> - descr->skb->data,
> - GELIC_NET_MAX_MTU,
> - DMA_FROM_DEVICE));
> - if (!descr->buf_addr) {
> + cpu_addr = dma_map_single(dev, descr->skb->data,
> + descr->buf_size, DMA_FROM_DEVICE);
> + descr->buf_addr = cpu_to_be32(cpu_addr);
> +
> + if (unlikely(dma_mapping_error(dev, cpu_addr))) {
adding dma mapping error handling should be a separate fix
> + dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, __LINE__);
> +
> + descr->buf_addr = 0;
> + descr->buf_size = 0;
> +
> dev_kfree_skb_any(descr->skb);
> descr->skb = NULL;
> - dev_info(ctodev(card),
> - "%s:Could not iommu-map rx buffer\n", __func__);
> +
> gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
> +
> return -ENOMEM;
> - } else {
> - gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
> - return 0;
> }
> +
> + gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
> + return 0;
> }
>
> /**
^ permalink raw reply [flat|nested] 2+ messages in thread