| Message ID | 45545484eadcf15a3ef06e35ccf34981cda2e867.1677981671.git.geoff@infradead.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/ps3_gelic_net: DMA related fixes | expand |
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.
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
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);
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(-)