* [PATCH net v5 0/2] net/ps3_gelic_net: DMA related fixes @ 2023-02-12 18:00 Geoff Levand 2023-02-12 18:00 ` [PATCH net v5 2/2] net/ps3_gelic_net: Use dma_mapping_error Geoff Levand 2023-02-12 18:00 ` [PATCH net v5 1/2] net/ps3_gelic_net: Fix RX sk_buff length Geoff Levand 0 siblings, 2 replies; 11+ messages in thread From: Geoff Levand @ 2023-02-12 18:00 UTC (permalink / raw) To: netdev, Jakub Kicinski, David S. Miller v5: Some additional patch cleanups. v4: More patch cleanups. v3: Cleaned up patches as requested. Geoff Levand (2): net/ps3_gelic_net: Fix RX sk_buff length net/ps3_gelic_net: Use dma_mapping_error drivers/net/ethernet/toshiba/ps3_gelic_net.c | 94 +++++++++++--------- 1 file changed, 52 insertions(+), 42 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net v5 2/2] net/ps3_gelic_net: Use dma_mapping_error 2023-02-12 18:00 [PATCH net v5 0/2] net/ps3_gelic_net: DMA related fixes Geoff Levand @ 2023-02-12 18:00 ` Geoff Levand 2023-02-14 10:58 ` Paolo Abeni 2023-02-14 17:28 ` Alexander Lobakin 2023-02-12 18:00 ` [PATCH net v5 1/2] net/ps3_gelic_net: Fix RX sk_buff length Geoff Levand 1 sibling, 2 replies; 11+ messages in thread From: Geoff Levand @ 2023-02-12 18:00 UTC (permalink / raw) To: netdev, Jakub Kicinski, David S. Miller The current Gelic Etherenet driver was checking the return value of its dma_map_single call, and not using the dma_mapping_error() routine. Fixes runtime problems like these: DMA-API: ps3_gelic_driver sb_05: device driver failed to check map error WARNING: CPU: 0 PID: 0 at kernel/dma/debug.c:1027 .check_unmap+0x888/0x8dc 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 | 41 ++++++++++---------- 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c index 2bb68e60d0d5..0e52bb99e344 100644 --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c @@ -309,22 +309,30 @@ static int gelic_card_init_chain(struct gelic_card *card, struct gelic_descr_chain *chain, struct gelic_descr *start_descr, int no) { - int i; + struct device *dev = ctodev(card); struct gelic_descr *descr; + int i; - descr = start_descr; - memset(descr, 0, sizeof(*descr) * no); + memset(start_descr, 0, no * sizeof(*start_descr)); /* set up the hardware pointers in each descriptor */ - for (i = 0; i < no; i++, descr++) { + for (i = 0, descr = start_descr; i < no; i++, descr++) { gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE); descr->bus_addr = dma_map_single(ctodev(card), descr, GELIC_DESCR_SIZE, DMA_BIDIRECTIONAL); - if (!descr->bus_addr) - goto iommu_error; + if (unlikely(dma_mapping_error(dev, descr->bus_addr))) { + dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, + __LINE__); + + for (i--, descr--; i >= 0; i--, descr--) { + dma_unmap_single(ctodev(card), descr->bus_addr, + GELIC_DESCR_SIZE, DMA_BIDIRECTIONAL); + } + return -ENOMEM; + } descr->next = descr + 1; descr->prev = descr - 1; @@ -346,14 +354,6 @@ static int gelic_card_init_chain(struct gelic_card *card, (descr - 1)->next_descr_addr = 0; return 0; - -iommu_error: - for (i--, descr--; 0 <= i; i--, descr--) - if (descr->bus_addr) - dma_unmap_single(ctodev(card), descr->bus_addr, - GELIC_DESCR_SIZE, - DMA_BIDIRECTIONAL); - return -ENOMEM; } /** @@ -408,13 +408,12 @@ static int gelic_descr_prepare_rx(struct gelic_card *card, descr->buf_addr = dma_map_single(dev, descr->skb->data, descr->buf_size, DMA_FROM_DEVICE); - if (!descr->buf_addr) { + if (unlikely(dma_mapping_error(dev, descr->buf_addr))) { + dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, __LINE__); dev_kfree_skb_any(descr->skb); descr->buf_addr = 0; descr->buf_size = 0; descr->skb = NULL; - 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; } @@ -774,6 +773,7 @@ static int gelic_descr_prepare_tx(struct gelic_card *card, struct gelic_descr *descr, struct sk_buff *skb) { + struct device *dev = ctodev(card); dma_addr_t buf; if (card->vlan_required) { @@ -788,11 +788,10 @@ static int gelic_descr_prepare_tx(struct gelic_card *card, skb = skb_tmp; } - buf = dma_map_single(ctodev(card), skb->data, skb->len, DMA_TO_DEVICE); + buf = dma_map_single(dev, skb->data, skb->len, DMA_TO_DEVICE); - if (!buf) { - dev_err(ctodev(card), - "dma map 2 failed (%p, %i). Dropping packet\n", + if (unlikely(dma_mapping_error(dev, buf))) { + dev_err(dev, "dma map 2 failed (%p, %i). Dropping packet\n", skb->data, skb->len); return -ENOMEM; } -- 2.34.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net v5 2/2] net/ps3_gelic_net: Use dma_mapping_error 2023-02-12 18:00 ` [PATCH net v5 2/2] net/ps3_gelic_net: Use dma_mapping_error Geoff Levand @ 2023-02-14 10:58 ` Paolo Abeni 2023-02-26 0:16 ` Geoff Levand 2023-02-14 17:28 ` Alexander Lobakin 1 sibling, 1 reply; 11+ messages in thread From: Paolo Abeni @ 2023-02-14 10:58 UTC (permalink / raw) To: Geoff Levand, netdev, Jakub Kicinski, David S. Miller; +Cc: Alexander H Duyck +Alex On Sun, 2023-02-12 at 18:00 +0000, Geoff Levand wrote: > The current Gelic Etherenet driver was checking the return value of its > dma_map_single call, and not using the dma_mapping_error() routine. > > Fixes runtime problems like these: > > DMA-API: ps3_gelic_driver sb_05: device driver failed to check map error > WARNING: CPU: 0 PID: 0 at kernel/dma/debug.c:1027 .check_unmap+0x888/0x8dc > > Fixes: 02c1889166b4 (ps3: gigabit ethernet driver for PS3, take3) Please use the correct format for the above tag. > Signed-off-by: Geoff Levand <geoff@infradead•org> > --- > drivers/net/ethernet/toshiba/ps3_gelic_net.c | 41 ++++++++++---------- > 1 file changed, 20 insertions(+), 21 deletions(-) > > diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c > index 2bb68e60d0d5..0e52bb99e344 100644 > --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c > +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c > @@ -309,22 +309,30 @@ static int gelic_card_init_chain(struct gelic_card *card, > struct gelic_descr_chain *chain, > struct gelic_descr *start_descr, int no) > { > - int i; > + struct device *dev = ctodev(card); > struct gelic_descr *descr; > + int i; > > - descr = start_descr; > - memset(descr, 0, sizeof(*descr) * no); > + memset(start_descr, 0, no * sizeof(*start_descr)); > > /* set up the hardware pointers in each descriptor */ > - for (i = 0; i < no; i++, descr++) { > + for (i = 0, descr = start_descr; i < no; i++, descr++) { > gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE); > descr->bus_addr = > dma_map_single(ctodev(card), descr, > GELIC_DESCR_SIZE, > DMA_BIDIRECTIONAL); > > - if (!descr->bus_addr) > - goto iommu_error; > + if (unlikely(dma_mapping_error(dev, descr->bus_addr))) { Not a big issue, but I think the existing goto is preferable to the following indentation > + dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, > + __LINE__); > + > + for (i--, descr--; i >= 0; i--, descr--) { Again not a big deal, but I think the construct suggested by Alex in the previous patch is more clear. > + dma_unmap_single(ctodev(card), descr->bus_addr, > + GELIC_DESCR_SIZE, DMA_BIDIRECTIONAL); > + } > + return -ENOMEM; > + } > > descr->next = descr + 1; > descr->prev = descr - 1; > @@ -346,14 +354,6 @@ static int gelic_card_init_chain(struct gelic_card *card, > (descr - 1)->next_descr_addr = 0; > > return 0; > - > -iommu_error: > - for (i--, descr--; 0 <= i; i--, descr--) > - if (descr->bus_addr) > - dma_unmap_single(ctodev(card), descr->bus_addr, > - GELIC_DESCR_SIZE, > - DMA_BIDIRECTIONAL); > - return -ENOMEM; > } > > /** > @@ -408,13 +408,12 @@ static int gelic_descr_prepare_rx(struct gelic_card *card, > descr->buf_addr = dma_map_single(dev, descr->skb->data, descr->buf_size, > DMA_FROM_DEVICE); > > - if (!descr->buf_addr) { > + if (unlikely(dma_mapping_error(dev, descr->buf_addr))) { > + dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, __LINE__); > dev_kfree_skb_any(descr->skb); > descr->buf_addr = 0; > descr->buf_size = 0; > descr->skb = NULL; > - dev_info(dev, > - "%s:Could not iommu-map rx buffer\n", __func__); You touched the above line in the previous patch. Since it does lot look functional-related to the fix here you can as well drop the message in the previous patch. Cheers, Paolo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net v5 2/2] net/ps3_gelic_net: Use dma_mapping_error 2023-02-14 10:58 ` Paolo Abeni @ 2023-02-26 0:16 ` Geoff Levand 0 siblings, 0 replies; 11+ messages in thread From: Geoff Levand @ 2023-02-26 0:16 UTC (permalink / raw) To: Paolo Abeni, netdev, Jakub Kicinski, David S. Miller; +Cc: Alexander H Duyck On 2/14/23 02:58, Paolo Abeni wrote: > +Alex > On Sun, 2023-02-12 at 18:00 +0000, Geoff Levand wrote: >> The current Gelic Etherenet driver was checking the return value of its >> dma_map_single call, and not using the dma_mapping_error() routine. >> >> Fixes runtime problems like these: >> >> DMA-API: ps3_gelic_driver sb_05: device driver failed to check map error >> WARNING: CPU: 0 PID: 0 at kernel/dma/debug.c:1027 .check_unmap+0x888/0x8dc >> >> Fixes: 02c1889166b4 (ps3: gigabit ethernet driver for PS3, take3) > > Please use the correct format for the above tag. > >> Signed-off-by: Geoff Levand <geoff@infradead•org> >> --- >> drivers/net/ethernet/toshiba/ps3_gelic_net.c | 41 ++++++++++---------- >> 1 file changed, 20 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c >> index 2bb68e60d0d5..0e52bb99e344 100644 >> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c >> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c >> @@ -309,22 +309,30 @@ static int gelic_card_init_chain(struct gelic_card *card, >> struct gelic_descr_chain *chain, >> struct gelic_descr *start_descr, int no) >> { >> - int i; >> + struct device *dev = ctodev(card); >> struct gelic_descr *descr; >> + int i; >> >> - descr = start_descr; >> - memset(descr, 0, sizeof(*descr) * no); >> + memset(start_descr, 0, no * sizeof(*start_descr)); >> >> /* set up the hardware pointers in each descriptor */ >> - for (i = 0; i < no; i++, descr++) { >> + for (i = 0, descr = start_descr; i < no; i++, descr++) { >> gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE); >> descr->bus_addr = >> dma_map_single(ctodev(card), descr, >> GELIC_DESCR_SIZE, >> DMA_BIDIRECTIONAL); >> >> - if (!descr->bus_addr) >> - goto iommu_error; >> + if (unlikely(dma_mapping_error(dev, descr->bus_addr))) { > > Not a big issue, but I think the existing goto is preferable to the > following indentation In a latter patch I put the common cleanup code into a separate static routine and call that here. >> + dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, >> + __LINE__); >> + >> + for (i--, descr--; i >= 0; i--, descr--) { > > Again not a big deal, but I think the construct suggested by Alex in > the previous patch is more clear. Well, I like this better... >> + dma_unmap_single(ctodev(card), descr->bus_addr, >> + GELIC_DESCR_SIZE, DMA_BIDIRECTIONAL); >> + } >> + return -ENOMEM; >> + } >> >> descr->next = descr + 1; >> descr->prev = descr - 1; >> @@ -346,14 +354,6 @@ static int gelic_card_init_chain(struct gelic_card *card, >> (descr - 1)->next_descr_addr = 0; >> >> return 0; >> - >> -iommu_error: >> - for (i--, descr--; 0 <= i; i--, descr--) >> - if (descr->bus_addr) >> - dma_unmap_single(ctodev(card), descr->bus_addr, >> - GELIC_DESCR_SIZE, >> - DMA_BIDIRECTIONAL); >> - return -ENOMEM; >> } >> >> /** >> @@ -408,13 +408,12 @@ static int gelic_descr_prepare_rx(struct gelic_card *card, >> descr->buf_addr = dma_map_single(dev, descr->skb->data, descr->buf_size, >> DMA_FROM_DEVICE); >> >> - if (!descr->buf_addr) { >> + if (unlikely(dma_mapping_error(dev, descr->buf_addr))) { >> + dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, __LINE__); >> dev_kfree_skb_any(descr->skb); >> descr->buf_addr = 0; >> descr->buf_size = 0; >> descr->skb = NULL; >> - dev_info(dev, >> - "%s:Could not iommu-map rx buffer\n", __func__); > > You touched the above line in the previous patch. Since it does lot > look functional-related to the fix here you can as well drop the > message in the previous patch. Sure, I'll consider it. Thanks for the review. -Geoff ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net v5 2/2] net/ps3_gelic_net: Use dma_mapping_error 2023-02-12 18:00 ` [PATCH net v5 2/2] net/ps3_gelic_net: Use dma_mapping_error Geoff Levand 2023-02-14 10:58 ` Paolo Abeni @ 2023-02-14 17:28 ` Alexander Lobakin 2023-02-26 0:16 ` Geoff Levand 1 sibling, 1 reply; 11+ messages in thread From: Alexander Lobakin @ 2023-02-14 17:28 UTC (permalink / raw) To: Geoff Levand; +Cc: Jakub Kicinski, David S. Miller, netdev From: Geoff Levand <geoff@infradead•org> Date: Sun, 12 Feb 2023 18:00:58 +0000 > The current Gelic Etherenet driver was checking the return value of its > dma_map_single call, and not using the dma_mapping_error() routine. > > Fixes runtime problems like these: > > DMA-API: ps3_gelic_driver sb_05: device driver failed to check map error > WARNING: CPU: 0 PID: 0 at kernel/dma/debug.c:1027 .check_unmap+0x888/0x8dc > > 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 | 41 ++++++++++---------- > 1 file changed, 20 insertions(+), 21 deletions(-) > > diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c > index 2bb68e60d0d5..0e52bb99e344 100644 > --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c > +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c > @@ -309,22 +309,30 @@ static int gelic_card_init_chain(struct gelic_card *card, > struct gelic_descr_chain *chain, > struct gelic_descr *start_descr, int no) > { > - int i; > + struct device *dev = ctodev(card); > struct gelic_descr *descr; > + int i; > > - descr = start_descr; > - memset(descr, 0, sizeof(*descr) * no); > + memset(start_descr, 0, no * sizeof(*start_descr)); > > /* set up the hardware pointers in each descriptor */ > - for (i = 0; i < no; i++, descr++) { > + for (i = 0, descr = start_descr; i < no; i++, descr++) { > gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE); > descr->bus_addr = > dma_map_single(ctodev(card), descr, > GELIC_DESCR_SIZE, > DMA_BIDIRECTIONAL); > > - if (!descr->bus_addr) > - goto iommu_error; > + if (unlikely(dma_mapping_error(dev, descr->bus_addr))) { dma_mapping_error() already has unlikely() inside. > + dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, > + __LINE__); > + > + for (i--, descr--; i >= 0; i--, descr--) { > + dma_unmap_single(ctodev(card), descr->bus_addr, > + GELIC_DESCR_SIZE, DMA_BIDIRECTIONAL); > + } > + return -ENOMEM; > + } > > descr->next = descr + 1; > descr->prev = descr - 1; > @@ -346,14 +354,6 @@ static int gelic_card_init_chain(struct gelic_card *card, > (descr - 1)->next_descr_addr = 0; > > return 0; > - > -iommu_error: > - for (i--, descr--; 0 <= i; i--, descr--) > - if (descr->bus_addr) > - dma_unmap_single(ctodev(card), descr->bus_addr, > - GELIC_DESCR_SIZE, > - DMA_BIDIRECTIONAL); > - return -ENOMEM; > } > > /** > @@ -408,13 +408,12 @@ static int gelic_descr_prepare_rx(struct gelic_card *card, > descr->buf_addr = dma_map_single(dev, descr->skb->data, descr->buf_size, > DMA_FROM_DEVICE); > > - if (!descr->buf_addr) { > + if (unlikely(dma_mapping_error(dev, descr->buf_addr))) { Same. > + dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, __LINE__); It is fast path. You're not allowed to use plain printk on fast path, since you may generate then thousands of messages per second. Consider looking at _ratelimit family of functions. > dev_kfree_skb_any(descr->skb); > descr->buf_addr = 0; > descr->buf_size = 0; > descr->skb = NULL; > - 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; > } > @@ -774,6 +773,7 @@ static int gelic_descr_prepare_tx(struct gelic_card *card, > struct gelic_descr *descr, > struct sk_buff *skb) > { > + struct device *dev = ctodev(card); > dma_addr_t buf; > > if (card->vlan_required) { > @@ -788,11 +788,10 @@ static int gelic_descr_prepare_tx(struct gelic_card *card, > skb = skb_tmp; > } > > - buf = dma_map_single(ctodev(card), skb->data, skb->len, DMA_TO_DEVICE); > + buf = dma_map_single(dev, skb->data, skb->len, DMA_TO_DEVICE); > > - if (!buf) { > - dev_err(ctodev(card), > - "dma map 2 failed (%p, %i). Dropping packet\n", > + if (unlikely(dma_mapping_error(dev, buf))) { > + dev_err(dev, "dma map 2 failed (%p, %i). Dropping packet\n", Same, same (for both lines). > skb->data, skb->len); > return -ENOMEM; > } Thanks, Olek ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net v5 2/2] net/ps3_gelic_net: Use dma_mapping_error 2023-02-14 17:28 ` Alexander Lobakin @ 2023-02-26 0:16 ` Geoff Levand 0 siblings, 0 replies; 11+ messages in thread From: Geoff Levand @ 2023-02-26 0:16 UTC (permalink / raw) To: Alexander Lobakin; +Cc: Jakub Kicinski, David S. Miller, netdev Hi, On 2/14/23 09:28, Alexander Lobakin wrote: > From: Geoff Levand <geoff@infradead•org> > Date: Sun, 12 Feb 2023 18:00:58 +0000 > >> The current Gelic Etherenet driver was checking the return value of its >> dma_map_single call, and not using the dma_mapping_error() routine. >> >> Fixes runtime problems like these: >> >> DMA-API: ps3_gelic_driver sb_05: device driver failed to check map error >> WARNING: CPU: 0 PID: 0 at kernel/dma/debug.c:1027 .check_unmap+0x888/0x8dc >> >> 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 | 41 ++++++++++---------- >> 1 file changed, 20 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c >> index 2bb68e60d0d5..0e52bb99e344 100644 >> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c >> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c >> - if (!descr->bus_addr) >> - goto iommu_error; >> + if (unlikely(dma_mapping_error(dev, descr->bus_addr))) { > > dma_mapping_error() already has unlikely() inside. OK, I'll remove that in the next patch set. >> + dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, >> + __LINE__); > It is fast path. You're not allowed to use plain printk on fast path, > since you may generate then thousands of messages per second. > Consider looking at _ratelimit family of functions. OK, I'll fix that. Thanks for the review. -Geoff ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net v5 1/2] net/ps3_gelic_net: Fix RX sk_buff length 2023-02-12 18:00 [PATCH net v5 0/2] net/ps3_gelic_net: DMA related fixes Geoff Levand 2023-02-12 18:00 ` [PATCH net v5 2/2] net/ps3_gelic_net: Use dma_mapping_error Geoff Levand @ 2023-02-12 18:00 ` Geoff Levand 2023-02-14 10:46 ` Paolo Abeni 2023-02-14 17:34 ` Alexander Lobakin 1 sibling, 2 replies; 11+ messages in thread From: Geoff Levand @ 2023-02-12 18:00 UTC (permalink / raw) To: netdev, Jakub Kicinski, David S. Miller 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); + 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; } /** -- 2.34.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net v5 1/2] net/ps3_gelic_net: Fix RX sk_buff length 2023-02-12 18:00 ` [PATCH net v5 1/2] net/ps3_gelic_net: Fix RX sk_buff length Geoff Levand @ 2023-02-14 10:46 ` Paolo Abeni 2023-02-26 0:16 ` Geoff Levand 2023-02-14 17:34 ` Alexander Lobakin 1 sibling, 1 reply; 11+ messages in thread From: Paolo Abeni @ 2023-02-14 10:46 UTC (permalink / raw) To: Geoff Levand, netdev, Jakub Kicinski, David S. Miller; +Cc: Alexander Duyck +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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net v5 1/2] net/ps3_gelic_net: Fix RX sk_buff length 2023-02-14 10:46 ` Paolo Abeni @ 2023-02-26 0:16 ` Geoff Levand 0 siblings, 0 replies; 11+ messages in thread From: Geoff Levand @ 2023-02-26 0:16 UTC (permalink / raw) To: Paolo Abeni, netdev, Jakub Kicinski, David S. Miller; +Cc: Alexander Duyck 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net v5 1/2] net/ps3_gelic_net: Fix RX sk_buff length 2023-02-12 18:00 ` [PATCH net v5 1/2] net/ps3_gelic_net: Fix RX sk_buff length Geoff Levand 2023-02-14 10:46 ` Paolo Abeni @ 2023-02-14 17:34 ` Alexander Lobakin 2023-02-25 20:46 ` Geoff Levand 1 sibling, 1 reply; 11+ messages in thread From: Alexander Lobakin @ 2023-02-14 17:34 UTC (permalink / raw) To: Geoff Levand; +Cc: netdev, Jakub Kicinski, David S. Miller 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net v5 1/2] net/ps3_gelic_net: Fix RX sk_buff length 2023-02-14 17:34 ` Alexander Lobakin @ 2023-02-25 20:46 ` Geoff Levand 0 siblings, 0 replies; 11+ messages in thread From: Geoff Levand @ 2023-02-25 20:46 UTC (permalink / raw) To: Alexander Lobakin; +Cc: netdev, Jakub Kicinski, David S. Miller 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-02-26 0:16 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-02-12 18:00 [PATCH net v5 0/2] net/ps3_gelic_net: DMA related fixes Geoff Levand 2023-02-12 18:00 ` [PATCH net v5 2/2] net/ps3_gelic_net: Use dma_mapping_error Geoff Levand 2023-02-14 10:58 ` Paolo Abeni 2023-02-26 0:16 ` Geoff Levand 2023-02-14 17:28 ` Alexander Lobakin 2023-02-26 0:16 ` Geoff Levand 2023-02-12 18:00 ` [PATCH net v5 1/2] net/ps3_gelic_net: Fix RX sk_buff length Geoff Levand 2023-02-14 10:46 ` Paolo Abeni 2023-02-26 0:16 ` Geoff Levand 2023-02-14 17:34 ` Alexander Lobakin 2023-02-25 20:46 ` Geoff Levand
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox