* [PATCH net v7 0/2] net/ps3_gelic_net: DMA related fixes @ 2023-03-05 2:08 Geoff Levand 2023-03-05 2:08 ` [PATCH net v7 1/2] net/ps3_gelic_net: Fix RX sk_buff length Geoff Levand 2023-03-05 2:08 ` [PATCH net v7 2/2] net/ps3_gelic_net: Use dma_mapping_error Geoff Levand 0 siblings, 2 replies; 7+ messages in thread From: Geoff Levand @ 2023-03-05 2:08 UTC (permalink / raw) To: netdev, Jakub Kicinski, David S. Miller Cc: Alexander Lobakin, Alexander Duyck, Paolo Abeni v7: Remove all cleanups, sync to spider net. v6: Reworked and cleaned up patches. 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 | 16 +++++++++------- drivers/net/ethernet/toshiba/ps3_gelic_net.h | 5 +++-- 2 files changed, 12 insertions(+), 9 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net v7 1/2] net/ps3_gelic_net: Fix RX sk_buff length 2023-03-05 2:08 [PATCH net v7 0/2] net/ps3_gelic_net: DMA related fixes Geoff Levand @ 2023-03-05 2:08 ` Geoff Levand 2023-03-06 15:54 ` Alexander H Duyck 2023-03-05 2:08 ` [PATCH net v7 2/2] net/ps3_gelic_net: Use dma_mapping_error Geoff Levand 1 sibling, 1 reply; 7+ messages in thread From: Geoff Levand @ 2023-03-05 2:08 UTC (permalink / raw) To: netdev, Jakub Kicinski, David S. Miller Cc: Alexander Lobakin, Alexander Duyck, Paolo Abeni The Gelic Ethernet device needs to have the RX sk_buffs aligned to GELIC_NET_RXBUF_ALIGN, and also 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. Also, correct the maximum and minimum MTU sizes, and add a new preprocessor macro for the maximum frame size, GELIC_NET_MAX_FRAME. 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 | 10 ++++++---- drivers/net/ethernet/toshiba/ps3_gelic_net.h | 5 +++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c index cf8de8a7a8a1..b0ebe0e603b4 100644 --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c @@ -375,11 +375,13 @@ static int gelic_descr_prepare_rx(struct gelic_card *card, 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); + bufsize = (GELIC_NET_MAX_FRAME + GELIC_NET_RXBUF_ALIGN - 1) & + (~(GELIC_NET_RXBUF_ALIGN - 1)); /* 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 = netdev_alloc_skb(*card->netdev, bufsize + + GELIC_NET_RXBUF_ALIGN - 1); if (!descr->skb) { descr->buf_addr = 0; /* tell DMAC don't touch memory */ return -ENOMEM; @@ -397,7 +399,7 @@ static int gelic_descr_prepare_rx(struct gelic_card *card, /* io-mmu-map the skb */ descr->buf_addr = cpu_to_be32(dma_map_single(ctodev(card), descr->skb->data, - GELIC_NET_MAX_MTU, + GELIC_NET_MAX_FRAME, DMA_FROM_DEVICE)); if (!descr->buf_addr) { dev_kfree_skb_any(descr->skb); @@ -915,7 +917,7 @@ static void gelic_net_pass_skb_up(struct gelic_descr *descr, data_error = be32_to_cpu(descr->data_error); /* unmap skb buffer */ dma_unmap_single(ctodev(card), be32_to_cpu(descr->buf_addr), - GELIC_NET_MAX_MTU, + GELIC_NET_MAX_FRAME, DMA_FROM_DEVICE); skb_put(skb, be32_to_cpu(descr->valid_size)? diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.h b/drivers/net/ethernet/toshiba/ps3_gelic_net.h index 68f324ed4eaf..0d98defb011e 100644 --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.h +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.h @@ -19,8 +19,9 @@ #define GELIC_NET_RX_DESCRIPTORS 128 /* num of descriptors */ #define GELIC_NET_TX_DESCRIPTORS 128 /* num of descriptors */ -#define GELIC_NET_MAX_MTU VLAN_ETH_FRAME_LEN -#define GELIC_NET_MIN_MTU VLAN_ETH_ZLEN +#define GELIC_NET_MAX_FRAME 2312 +#define GELIC_NET_MAX_MTU 2294 +#define GELIC_NET_MIN_MTU 64 #define GELIC_NET_RXBUF_ALIGN 128 #define GELIC_CARD_RX_CSUM_DEFAULT 1 /* hw chksum */ #define GELIC_NET_WATCHDOG_TIMEOUT 5*HZ -- 2.34.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net v7 1/2] net/ps3_gelic_net: Fix RX sk_buff length 2023-03-05 2:08 ` [PATCH net v7 1/2] net/ps3_gelic_net: Fix RX sk_buff length Geoff Levand @ 2023-03-06 15:54 ` Alexander H Duyck 2023-03-11 21:40 ` Geoff Levand 0 siblings, 1 reply; 7+ messages in thread From: Alexander H Duyck @ 2023-03-06 15:54 UTC (permalink / raw) To: Geoff Levand, netdev, Jakub Kicinski, David S. Miller Cc: Alexander Lobakin, Paolo Abeni On Sun, 2023-03-05 at 02:08 +0000, Geoff Levand wrote: > The Gelic Ethernet device needs to have the RX sk_buffs aligned to > GELIC_NET_RXBUF_ALIGN, and also 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. > > Also, correct the maximum and minimum MTU sizes, and add a new > preprocessor macro for the maximum frame size, GELIC_NET_MAX_FRAME. > > 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 | 10 ++++++---- > drivers/net/ethernet/toshiba/ps3_gelic_net.h | 5 +++-- > 2 files changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c > index cf8de8a7a8a1..b0ebe0e603b4 100644 > --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c > +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c > @@ -375,11 +375,13 @@ static int gelic_descr_prepare_rx(struct gelic_card *card, > 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); > + bufsize = (GELIC_NET_MAX_FRAME + GELIC_NET_RXBUF_ALIGN - 1) & > + (~(GELIC_NET_RXBUF_ALIGN - 1)); Why did you stop using ALIGN? What you coded looks exactly like what the code for ALIGN does. From what I can tell you just need to replace GELIC_NET_MAX_MTU with GELIC_NET_MAX_FRAME. > > /* 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 = netdev_alloc_skb(*card->netdev, bufsize + > + GELIC_NET_RXBUF_ALIGN - 1); This wrapping doesn't look right to me. Also why add the align value again here? I would think that it being added above should have taken care of what you needed. Are you adding any data beyond the end of what is DMAed into the frame? > if (!descr->skb) { > descr->buf_addr = 0; /* tell DMAC don't touch memory */ > return -ENOMEM; > @@ -397,7 +399,7 @@ static int gelic_descr_prepare_rx(struct gelic_card *card, > /* io-mmu-map the skb */ > descr->buf_addr = cpu_to_be32(dma_map_single(ctodev(card), > descr->skb->data, > - GELIC_NET_MAX_MTU, > + GELIC_NET_MAX_FRAME, > DMA_FROM_DEVICE)); Rather than using the define GELIC_NET_MAX_FRAME, why not just use bufsize since that is what you actually have allocated for receive? > if (!descr->buf_addr) { > dev_kfree_skb_any(descr->skb); > @@ -915,7 +917,7 @@ static void gelic_net_pass_skb_up(struct gelic_descr *descr, > data_error = be32_to_cpu(descr->data_error); > /* unmap skb buffer */ > dma_unmap_single(ctodev(card), be32_to_cpu(descr->buf_addr), > - GELIC_NET_MAX_MTU, > + GELIC_NET_MAX_FRAME, > DMA_FROM_DEVICE); I suppose my suggestion above would cause a problem here since you would need to also define buffer size here. However since it looks like you are adding a define below anyway maybe you should just look at adding a new RX_BUFSZ define and just drop bufsize completely. > > skb_put(skb, be32_to_cpu(descr->valid_size)? > diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.h b/drivers/net/ethernet/toshiba/ps3_gelic_net.h > index 68f324ed4eaf..0d98defb011e 100644 > --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.h > +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.h > @@ -19,8 +19,9 @@ > #define GELIC_NET_RX_DESCRIPTORS 128 /* num of descriptors */ > #define GELIC_NET_TX_DESCRIPTORS 128 /* num of descriptors */ > > -#define GELIC_NET_MAX_MTU VLAN_ETH_FRAME_LEN > -#define GELIC_NET_MIN_MTU VLAN_ETH_ZLEN > +#define GELIC_NET_MAX_FRAME 2312 > +#define GELIC_NET_MAX_MTU 2294 > +#define GELIC_NET_MIN_MTU 64 > #define GELIC_NET_RXBUF_ALIGN 128 > #define GELIC_CARD_RX_CSUM_DEFAULT 1 /* hw chksum */ > #define GELIC_NET_WATCHDOG_TIMEOUT 5*HZ Since you are adding defines why not just add: #define GELIC_NET_RX_BUFSZ \ ALIGN(GELIC_NET_MAX_FRAME, GELIC_NET_RXBUF_ALIGN) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net v7 1/2] net/ps3_gelic_net: Fix RX sk_buff length 2023-03-06 15:54 ` Alexander H Duyck @ 2023-03-11 21:40 ` Geoff Levand 0 siblings, 0 replies; 7+ messages in thread From: Geoff Levand @ 2023-03-11 21:40 UTC (permalink / raw) To: Alexander H Duyck, netdev, Jakub Kicinski, David S. Miller Cc: Alexander Lobakin, Paolo Abeni Hi, On 3/6/23 07:54, Alexander H Duyck wrote: > On Sun, 2023-03-05 at 02:08 +0000, Geoff Levand wrote: >> The Gelic Ethernet device needs to have the RX sk_buffs aligned to >> GELIC_NET_RXBUF_ALIGN, and also 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. >> >> Also, correct the maximum and minimum MTU sizes, and add a new >> preprocessor macro for the maximum frame size, GELIC_NET_MAX_FRAME. >> >> 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 | 10 ++++++---- >> drivers/net/ethernet/toshiba/ps3_gelic_net.h | 5 +++-- >> 2 files changed, 9 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c >> index cf8de8a7a8a1..b0ebe0e603b4 100644 >> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c >> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c >> @@ -375,11 +375,13 @@ static int gelic_descr_prepare_rx(struct gelic_card *card, >> 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); >> + bufsize = (GELIC_NET_MAX_FRAME + GELIC_NET_RXBUF_ALIGN - 1) & >> + (~(GELIC_NET_RXBUF_ALIGN - 1)); > > Why did you stop using ALIGN? What you coded looks exactly like what > the code for ALIGN does. From what I can tell you just need to replace > GELIC_NET_MAX_MTU with GELIC_NET_MAX_FRAME. OK, I'll use ALIGN for the next patch set. >> /* 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 = netdev_alloc_skb(*card->netdev, bufsize + >> + GELIC_NET_RXBUF_ALIGN - 1); > > This wrapping doesn't look right to me. Also why add the align value > again here? I would think that it being added above should have taken > care of what you needed. Are you adding any data beyond the end of what > is DMAed into the frame? This is just a straight copy of what is done in the spider net driver. As I mentioned in the comment of this patch, the DMA buffer must be a multiple of GELIC_NET_RXBUF_ALIGN, and that is what that extra 'GELIC_NET_RXBUF_ALIGN - 1' is for. >> if (!descr->skb) { >> descr->buf_addr = 0; /* tell DMAC don't touch memory */ >> return -ENOMEM; >> @@ -397,7 +399,7 @@ static int gelic_descr_prepare_rx(struct gelic_card *card, >> /* io-mmu-map the skb */ >> descr->buf_addr = cpu_to_be32(dma_map_single(ctodev(card), >> descr->skb->data, >> - GELIC_NET_MAX_MTU, >> + GELIC_NET_MAX_FRAME, >> DMA_FROM_DEVICE)); > > Rather than using the define GELIC_NET_MAX_FRAME, why not just use > bufsize since that is what you actually have allocated for receive? Again, this is a straight copy of what is done in the spider net driver. It does seem like using bufsize would be better, so I'll change that. >> if (!descr->buf_addr) { >> dev_kfree_skb_any(descr->skb); >> @@ -915,7 +917,7 @@ static void gelic_net_pass_skb_up(struct gelic_descr *descr, >> data_error = be32_to_cpu(descr->data_error); >> /* unmap skb buffer */ >> dma_unmap_single(ctodev(card), be32_to_cpu(descr->buf_addr), >> - GELIC_NET_MAX_MTU, >> + GELIC_NET_MAX_FRAME, >> DMA_FROM_DEVICE); > > I suppose my suggestion above would cause a problem here since you > would need to also define buffer size here. However since it looks like > you are adding a define below anyway maybe you should just look at > adding a new RX_BUFSZ define and just drop bufsize completely. I think making bufsize a static const would be better, since it would avoid using the pre-processor. >> skb_put(skb, be32_to_cpu(descr->valid_size)? >> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.h b/drivers/net/ethernet/toshiba/ps3_gelic_net.h >> index 68f324ed4eaf..0d98defb011e 100644 >> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.h >> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.h >> @@ -19,8 +19,9 @@ >> #define GELIC_NET_RX_DESCRIPTORS 128 /* num of descriptors */ >> #define GELIC_NET_TX_DESCRIPTORS 128 /* num of descriptors */ >> >> -#define GELIC_NET_MAX_MTU VLAN_ETH_FRAME_LEN >> -#define GELIC_NET_MIN_MTU VLAN_ETH_ZLEN >> +#define GELIC_NET_MAX_FRAME 2312 >> +#define GELIC_NET_MAX_MTU 2294 >> +#define GELIC_NET_MIN_MTU 64 >> #define GELIC_NET_RXBUF_ALIGN 128 >> #define GELIC_CARD_RX_CSUM_DEFAULT 1 /* hw chksum */ >> #define GELIC_NET_WATCHDOG_TIMEOUT 5*HZ > > Since you are adding defines why not just add: > #define GELIC_NET_RX_BUFSZ \ > ALIGN(GELIC_NET_MAX_FRAME, GELIC_NET_RXBUF_ALIGN) Thanks for the review. -Geoff ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net v7 2/2] net/ps3_gelic_net: Use dma_mapping_error 2023-03-05 2:08 [PATCH net v7 0/2] net/ps3_gelic_net: DMA related fixes Geoff Levand 2023-03-05 2:08 ` [PATCH net v7 1/2] net/ps3_gelic_net: Fix RX sk_buff length Geoff Levand @ 2023-03-05 2:08 ` Geoff Levand 2023-03-06 16:01 ` Alexander H Duyck 1 sibling, 1 reply; 7+ messages in thread From: Geoff Levand @ 2023-03-05 2:08 UTC (permalink / raw) To: netdev, Jakub Kicinski, David S. Miller Cc: Alexander Lobakin, Alexander Duyck, Paolo Abeni 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 | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c index b0ebe0e603b4..40261947e0ea 100644 --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c @@ -323,7 +323,7 @@ static int gelic_card_init_chain(struct gelic_card *card, GELIC_DESCR_SIZE, DMA_BIDIRECTIONAL); - if (!descr->bus_addr) + if (dma_mapping_error(ctodev(card), descr->bus_addr)) goto iommu_error; descr->next = descr + 1; @@ -401,7 +401,7 @@ static int gelic_descr_prepare_rx(struct gelic_card *card, descr->skb->data, GELIC_NET_MAX_FRAME, DMA_FROM_DEVICE)); - if (!descr->buf_addr) { + if (dma_mapping_error(ctodev(card), descr->buf_addr)) { dev_kfree_skb_any(descr->skb); descr->skb = NULL; dev_info(ctodev(card), @@ -781,7 +781,7 @@ static int gelic_descr_prepare_tx(struct gelic_card *card, buf = dma_map_single(ctodev(card), skb->data, skb->len, DMA_TO_DEVICE); - if (!buf) { + if (dma_mapping_error(ctodev(card), buf)) { dev_err(ctodev(card), "dma map 2 failed (%p, %i). Dropping packet\n", skb->data, skb->len); -- 2.34.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net v7 2/2] net/ps3_gelic_net: Use dma_mapping_error 2023-03-05 2:08 ` [PATCH net v7 2/2] net/ps3_gelic_net: Use dma_mapping_error Geoff Levand @ 2023-03-06 16:01 ` Alexander H Duyck 2023-03-11 21:41 ` Geoff Levand 0 siblings, 1 reply; 7+ messages in thread From: Alexander H Duyck @ 2023-03-06 16:01 UTC (permalink / raw) To: Geoff Levand, netdev, Jakub Kicinski, David S. Miller Cc: Alexander Lobakin, Paolo Abeni On Sun, 2023-03-05 at 02:08 +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") > Signed-off-by: Geoff Levand <geoff@infradead.org> > --- > drivers/net/ethernet/toshiba/ps3_gelic_net.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c > index b0ebe0e603b4..40261947e0ea 100644 > --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c > +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c > @@ -323,7 +323,7 @@ static int gelic_card_init_chain(struct gelic_card *card, > GELIC_DESCR_SIZE, > DMA_BIDIRECTIONAL); > > - if (!descr->bus_addr) > + if (dma_mapping_error(ctodev(card), descr->bus_addr)) > goto iommu_error; > > descr->next = descr + 1; The bus_addr value is __be32 and the dma_mapping_error should be CPU ordered. I think there was a byteswap using cpu_to_be32 missing here. In addition you will probably need to have an intermediate variable to store it in to test the DMA address before you byte swap it and store it in the descriptor. > @@ -401,7 +401,7 @@ static int gelic_descr_prepare_rx(struct gelic_card *card, > descr->skb->data, > GELIC_NET_MAX_FRAME, > DMA_FROM_DEVICE)); > - if (!descr->buf_addr) { > + if (dma_mapping_error(ctodev(card), descr->buf_addr)) { > dev_kfree_skb_any(descr->skb); > descr->skb = NULL; > dev_info(ctodev(card), This is happening AFTER the DMA is passed through a cpu_to_be32 right? The test should be on the raw value, not the byteswapped value. > @@ -781,7 +781,7 @@ static int gelic_descr_prepare_tx(struct gelic_card *card, > > buf = dma_map_single(ctodev(card), skb->data, skb->len, DMA_TO_DEVICE); > > - if (!buf) { > + if (dma_mapping_error(ctodev(card), buf)) { > dev_err(ctodev(card), > "dma map 2 failed (%p, %i). Dropping packet\n", > skb->data, skb->len); This one is correct from what I can tell. I would recommend using it as a template and applying it to the two above so that you can sort out the byte ordering issues and perform the test and the CPU ordered DMA variable. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net v7 2/2] net/ps3_gelic_net: Use dma_mapping_error 2023-03-06 16:01 ` Alexander H Duyck @ 2023-03-11 21:41 ` Geoff Levand 0 siblings, 0 replies; 7+ messages in thread From: Geoff Levand @ 2023-03-11 21:41 UTC (permalink / raw) To: Alexander H Duyck, netdev, Jakub Kicinski, David S. Miller Cc: Alexander Lobakin, Paolo Abeni Hi, On 3/6/23 08:01, Alexander H Duyck wrote: > On Sun, 2023-03-05 at 02:08 +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") >> Signed-off-by: Geoff Levand <geoff@infradead.org> >> --- >> drivers/net/ethernet/toshiba/ps3_gelic_net.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c >> index b0ebe0e603b4..40261947e0ea 100644 >> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c >> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c >> @@ -323,7 +323,7 @@ static int gelic_card_init_chain(struct gelic_card *card, >> GELIC_DESCR_SIZE, >> DMA_BIDIRECTIONAL); >> >> - if (!descr->bus_addr) >> + if (dma_mapping_error(ctodev(card), descr->bus_addr)) >> goto iommu_error; >> >> descr->next = descr + 1; > > The bus_addr value is __be32 and the dma_mapping_error should be CPU > ordered. I think there was a byteswap using cpu_to_be32 missing here. > In addition you will probably need to have an intermediate variable to > store it in to test the DMA address before you byte swap it and store > it in the descriptor. I added a local variable 'cpu_addr' as you recommend. >> @@ -401,7 +401,7 @@ static int gelic_descr_prepare_rx(struct gelic_card *card, >> descr->skb->data, >> GELIC_NET_MAX_FRAME, >> DMA_FROM_DEVICE)); >> - if (!descr->buf_addr) { >> + if (dma_mapping_error(ctodev(card), descr->buf_addr)) { >> dev_kfree_skb_any(descr->skb); >> descr->skb = NULL; >> dev_info(ctodev(card), > > This is happening AFTER the DMA is passed through a cpu_to_be32 right? > The test should be on the raw value, not the byteswapped value. Did the same here. >> @@ -781,7 +781,7 @@ static int gelic_descr_prepare_tx(struct gelic_card *card, >> >> buf = dma_map_single(ctodev(card), skb->data, skb->len, DMA_TO_DEVICE); >> >> - if (!buf) { >> + if (dma_mapping_error(ctodev(card), buf)) { >> dev_err(ctodev(card), >> "dma map 2 failed (%p, %i). Dropping packet\n", >> skb->data, skb->len); > > This one is correct from what I can tell. I would recommend using it as > a template and applying it to the two above so that you can sort out > the byte ordering issues and perform the test and the CPU ordered DMA > variable. Thanks for the review. -Geoff ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-03-11 21:41 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-05 2:08 [PATCH net v7 0/2] net/ps3_gelic_net: DMA related fixes Geoff Levand 2023-03-05 2:08 ` [PATCH net v7 1/2] net/ps3_gelic_net: Fix RX sk_buff length Geoff Levand 2023-03-06 15:54 ` Alexander H Duyck 2023-03-11 21:40 ` Geoff Levand 2023-03-05 2:08 ` [PATCH net v7 2/2] net/ps3_gelic_net: Use dma_mapping_error Geoff Levand 2023-03-06 16:01 ` Alexander H Duyck 2023-03-11 21:41 ` Geoff Levand
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.