| Message ID | de8eacb3bb238f40ce69882e425bd83c6180d671.1676221818.git.geoff@infradead.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/ps3_gelic_net: DMA related fixes | expand |
+Alex On Sun, 2023-02-12 at 18:00 +0000, Geoff Levand wrote: > The Gelic Ethernet device needs to have the RX sk_buffs aligned to > GELIC_NET_RXBUF_ALIGN and the length of the RX sk_buffs must be a > multiple of GELIC_NET_RXBUF_ALIGN. > > The current Gelic Ethernet driver was not allocating sk_buffs large > enough to allow for this alignment. > > Fixes various randomly occurring runtime network errors. > > Fixes: 02c1889166b4 (ps3: gigabit ethernet driver for PS3, take3) Please use the correct format for the Fixes tag: <hash> ("<msg>"). Note the missing quotes. > Signed-off-by: Geoff Levand <geoff@infradead.org> > --- > drivers/net/ethernet/toshiba/ps3_gelic_net.c | 55 ++++++++++++-------- > 1 file changed, 33 insertions(+), 22 deletions(-) > > diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c > index cf8de8a7a8a1..2bb68e60d0d5 100644 > --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c > +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c > @@ -365,51 +365,62 @@ 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 > + * > + * Gelic RX sk_buffs must be aligned to GELIC_NET_RXBUF_ALIGN and the length > + * must be a multiple of GELIC_NET_RXBUF_ALIGN. > */ > 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 { > + const unsigned int buffer_bytes; > + const unsigned int total_bytes; > + unsigned int offset; > + } aligned_buf = { > + .buffer_bytes = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN), > + .total_bytes = (GELIC_NET_RXBUF_ALIGN - 1) + > + ALIGN(GELIC_NET_MAX_MTU, 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); > > - /* 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); > + descr->skb = dev_alloc_skb(aligned_buf.total_bytes); > + > 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); > + > + aligned_buf.offset = > + PTR_ALIGN(descr->skb->data, GELIC_NET_RXBUF_ALIGN) - > + descr->skb->data; > + > + descr->buf_size = aligned_buf.buffer_bytes; > 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)); > + skb_reserve(descr->skb, aligned_buf.offset); > + > + descr->buf_addr = dma_map_single(dev, descr->skb->data, descr->buf_size, > + DMA_FROM_DEVICE); As already noted by Alex, you should preserve the cpu_to_be32(). If the running arch is be32, it has 0 performance and/or code size overhead, and it helps readability and maintainability. Please be sure to check the indentation of new code with checkpatch. When reposting, please be sure to CC people that gave feedback on previous iterations. Cheers, Paolo
From: Geoff Levand <geoff@infradead.org> Date: Sun, 12 Feb 2023 18:00:58 +0000 > The Gelic Ethernet device needs to have the RX sk_buffs aligned to > GELIC_NET_RXBUF_ALIGN and the length of the RX sk_buffs must be a > multiple of GELIC_NET_RXBUF_ALIGN. > > The current Gelic Ethernet driver was not allocating sk_buffs large > enough to allow for this alignment. > > Fixes various randomly occurring runtime network errors. > > Fixes: 02c1889166b4 (ps3: gigabit ethernet driver for PS3, take3) > Signed-off-by: Geoff Levand <geoff@infradead.org> > --- > drivers/net/ethernet/toshiba/ps3_gelic_net.c | 55 ++++++++++++-------- > 1 file changed, 33 insertions(+), 22 deletions(-) > > diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c > index cf8de8a7a8a1..2bb68e60d0d5 100644 > --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c > +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c > @@ -365,51 +365,62 @@ 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 > + * > + * Gelic RX sk_buffs must be aligned to GELIC_NET_RXBUF_ALIGN and the length > + * must be a multiple of GELIC_NET_RXBUF_ALIGN. > */ > 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 { > + const unsigned int buffer_bytes; > + const unsigned int total_bytes; > + unsigned int offset; > + } aligned_buf = { > + .buffer_bytes = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN), > + .total_bytes = (GELIC_NET_RXBUF_ALIGN - 1) + > + ALIGN(GELIC_NET_MAX_MTU, 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); > > - /* 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); > + descr->skb = dev_alloc_skb(aligned_buf.total_bytes); I highly recommend using {napi,netdev}_alloc_frag_align() + {napi_,}build_skb() to not waste memory. It will align everything for ye, so you won't need all this. Also, dunno why create an onstack struct for 3 integers :D > + > 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); > + > + aligned_buf.offset = > + PTR_ALIGN(descr->skb->data, GELIC_NET_RXBUF_ALIGN) - > + descr->skb->data; > + > + descr->buf_size = aligned_buf.buffer_bytes; > 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)); > + skb_reserve(descr->skb, aligned_buf.offset); > + > + descr->buf_addr = dma_map_single(dev, descr->skb->data, descr->buf_size, > + DMA_FROM_DEVICE); > + > if (!descr->buf_addr) { > dev_kfree_skb_any(descr->skb); > + descr->buf_addr = 0; > + descr->buf_size = 0; > descr->skb = NULL; > - dev_info(ctodev(card), > + dev_info(dev, > "%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; > } > > /** Thanks, Olek
Hi, On 2/14/23 09:34, Alexander Lobakin wrote: >> The Gelic Ethernet device needs to have the RX sk_buffs aligned to >> GELIC_NET_RXBUF_ALIGN and the length of the RX sk_buffs must be a >> multiple of GELIC_NET_RXBUF_ALIGN. >> >> The current Gelic Ethernet driver was not allocating sk_buffs large >> enough to allow for this alignment. >> >> Fixes various randomly occurring runtime network errors. >> >> Fixes: 02c1889166b4 (ps3: gigabit ethernet driver for PS3, take3) >> Signed-off-by: Geoff Levand <geoff@infradead.org> >> --- >> drivers/net/ethernet/toshiba/ps3_gelic_net.c | 55 ++++++++++++-------- >> 1 file changed, 33 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c >> index cf8de8a7a8a1..2bb68e60d0d5 100644 >> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c >> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c >> @@ -365,51 +365,62 @@ 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 >> + * >> + * Gelic RX sk_buffs must be aligned to GELIC_NET_RXBUF_ALIGN and the length >> + * must be a multiple of GELIC_NET_RXBUF_ALIGN. >> */ >> 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 { >> + const unsigned int buffer_bytes; >> + const unsigned int total_bytes; >> + unsigned int offset; >> + } aligned_buf = { >> + .buffer_bytes = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN), >> + .total_bytes = (GELIC_NET_RXBUF_ALIGN - 1) + >> + ALIGN(GELIC_NET_MAX_MTU, 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); >> >> - /* 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); >> + descr->skb = dev_alloc_skb(aligned_buf.total_bytes); > > I highly recommend using {napi,netdev}_alloc_frag_align() + > {napi_,}build_skb() to not waste memory. It will align everything for > ye, so you won't need all this. I converted this over to use napi_alloc_frag_align and napi_build_skb, and then cleanup with skb_free_frag. I couldn't find any good documentation for those napi routines though. For napi_alloc_frag_align, it seems the align parameter should be the alignment required by the gelic hardware device, so GELIC_NET_RXBUF_ALIGN. But for the fragsz parameter I couldn't quite figure out from the existing users of it what exactly it should be. I assumed it needed to be the maximum number of bytes the hardware device can fill, so I set it to be ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN), but with that I got skb_over_panic errors (skb->tail > skb->end). I added some debugging code and found that when it hit the panic skb->end was always 384 bytes short. I changed GELIC_NET_MAX_MTU to be 1920 which is ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN) + 384, and it seems to be working OK. Does this seem OK? Maybe the current GELIC_NET_MAX_MTU value is incorrect. I did not write that driver, and I don't have any good documentation for the gelic device. Thanks for any help you can give. -Geoff
Hi, On 2/14/23 02:46, Paolo Abeni wrote: > +Alex > On Sun, 2023-02-12 at 18:00 +0000, Geoff Levand wrote: >> The Gelic Ethernet device needs to have the RX sk_buffs aligned to >> GELIC_NET_RXBUF_ALIGN and the length of the RX sk_buffs must be a >> multiple of GELIC_NET_RXBUF_ALIGN. >> >> The current Gelic Ethernet driver was not allocating sk_buffs large >> enough to allow for this alignment. >> >> Fixes various randomly occurring runtime network errors. >> >> Fixes: 02c1889166b4 (ps3: gigabit ethernet driver for PS3, take3) > > Please use the correct format for the Fixes tag: <hash> ("<msg>"). Note > the missing quotes. I'll add those. >> - /* 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)); >> + skb_reserve(descr->skb, aligned_buf.offset); >> + >> + descr->buf_addr = dma_map_single(dev, descr->skb->data, descr->buf_size, >> + DMA_FROM_DEVICE); > > As already noted by Alex, you should preserve the cpu_to_be32(). If the > running arch is be32, it has 0 performance and/or code size overhead, > and it helps readability and maintainability. I'll add those in for the next patch set. > Please be sure to check the indentation of new code with checkpatch. checkpatch reports a CHECK for my indentation. As is discussed in the kernel's coding style guide, coding style is about maintainability, and as the maintainer of this driver I think the indentation I have used is more readable and hence easier to maintain. Thanks for the review. -Geoff
diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c index cf8de8a7a8a1..2bb68e60d0d5 100644 --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c @@ -365,51 +365,62 @@ 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 + * + * Gelic RX sk_buffs must be aligned to GELIC_NET_RXBUF_ALIGN and the length + * must be a multiple of GELIC_NET_RXBUF_ALIGN. */ 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 { + const unsigned int buffer_bytes; + const unsigned int total_bytes; + unsigned int offset; + } aligned_buf = { + .buffer_bytes = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN), + .total_bytes = (GELIC_NET_RXBUF_ALIGN - 1) + + ALIGN(GELIC_NET_MAX_MTU, 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); - /* 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); + descr->skb = dev_alloc_skb(aligned_buf.total_bytes); + 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); + + aligned_buf.offset = + PTR_ALIGN(descr->skb->data, GELIC_NET_RXBUF_ALIGN) - + descr->skb->data; + + descr->buf_size = aligned_buf.buffer_bytes; 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)); + skb_reserve(descr->skb, aligned_buf.offset); + + descr->buf_addr = dma_map_single(dev, descr->skb->data, descr->buf_size, + DMA_FROM_DEVICE); + if (!descr->buf_addr) { dev_kfree_skb_any(descr->skb); + descr->buf_addr = 0; + descr->buf_size = 0; descr->skb = NULL; - dev_info(ctodev(card), + dev_info(dev, "%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; } /**
The Gelic Ethernet device needs to have the RX sk_buffs aligned to GELIC_NET_RXBUF_ALIGN and the length of the RX sk_buffs must be a multiple of GELIC_NET_RXBUF_ALIGN. The current Gelic Ethernet driver was not allocating sk_buffs large enough to allow for this alignment. Fixes various randomly occurring runtime network errors. Fixes: 02c1889166b4 (ps3: gigabit ethernet driver for PS3, take3) Signed-off-by: Geoff Levand <geoff@infradead.org> --- drivers/net/ethernet/toshiba/ps3_gelic_net.c | 55 ++++++++++++-------- 1 file changed, 33 insertions(+), 22 deletions(-)