* [PATCH RFC 0/2] net: macb: Fixing DMA usage @ 2014-02-21 19:30 Soren Brinkmann 2014-02-21 19:30 ` [PATCH RFC 1/2] net: macb: Check DMA mappings for error Soren Brinkmann 2014-02-21 19:30 ` [PATCH RFC 2/2] net: macb: DMA-unmap full rx-buffer Soren Brinkmann 0 siblings, 2 replies; 5+ messages in thread From: Soren Brinkmann @ 2014-02-21 19:30 UTC (permalink / raw) To: Nicolas Ferre Cc: netdev, linux-kernel, Vinod Koul, Dan Williams, David S. Miller, Michal Simek, Soren Brinkmann Hi, when I enabled CONFIG_DMA_API_DEBUG recently, I started seeing warnings caused by the macb's DMA usage. I read trough Documentation/DMA-API_HOWTO.txt and tried solving these to the best of my knowledge of the networking and DMA systems. But that knowledge is not very deep. So, there might be fundamental flaws with these patches, hence as RFC only. Thanks, Sören Soren Brinkmann (2): net: macb: Check DMA mappings for error net: macb: DMA-unmap full rx-buffer drivers/net/ethernet/cadence/macb.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) -- 1.9.0.1.g4196000 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH RFC 1/2] net: macb: Check DMA mappings for error 2014-02-21 19:30 [PATCH RFC 0/2] net: macb: Fixing DMA usage Soren Brinkmann @ 2014-02-21 19:30 ` Soren Brinkmann 2014-02-23 0:13 ` Ben Hutchings 2014-02-21 19:30 ` [PATCH RFC 2/2] net: macb: DMA-unmap full rx-buffer Soren Brinkmann 1 sibling, 1 reply; 5+ messages in thread From: Soren Brinkmann @ 2014-02-21 19:30 UTC (permalink / raw) To: Nicolas Ferre Cc: netdev, linux-kernel, Vinod Koul, Dan Williams, David S. Miller, Michal Simek, Soren Brinkmann With CONFIG_DMA_API_DEBUG enabled the following warning is printed: WARNING: CPU: 0 PID: 619 at lib/dma-debug.c:1101 check_unmap+0x758/0x894() macb e000b000.ethernet: DMA-API: device driver failed to check map error[device address=0x000000002d171c02] [size=322 bytes] [mapped as single] Modules linked in: CPU: 0 PID: 619 Comm: udhcpc Not tainted 3.14.0-rc3-xilinx-00219-gd158fc7f36a2 #63 [<c001516c>] (unwind_backtrace) from [<c0011df8>] (show_stack+0x10/0x14) [<c0011df8>] (show_stack) from [<c03c7714>] (dump_stack+0x7c/0xc8) [<c03c7714>] (dump_stack) from [<c00245cc>] (warn_slowpath_common+0x60/0x84) [<c00245cc>] (warn_slowpath_common) from [<c0024670>] (warn_slowpath_fmt+0x2c/0x3c) [<c0024670>] (warn_slowpath_fmt) from [<c0228244>] (check_unmap+0x758/0x894) [<c0228244>] (check_unmap) from [<c0228588>] (debug_dma_unmap_page+0x64/0x70) [<c0228588>] (debug_dma_unmap_page) from [<c02aba64>] (macb_interrupt+0x1f8/0x2dc) [<c02aba64>] (macb_interrupt) from [<c006c6e4>] (handle_irq_event_percpu+0x2c/0x178) [<c006c6e4>] (handle_irq_event_percpu) from [<c006c86c>] (handle_irq_event+0x3c/0x5c) [<c006c86c>] (handle_irq_event) from [<c006f548>] (handle_fasteoi_irq+0xb8/0x100) [<c006f548>] (handle_fasteoi_irq) from [<c006c148>] (generic_handle_irq+0x20/0x30) [<c006c148>] (generic_handle_irq) from [<c000f35c>] (handle_IRQ+0x64/0x8c) [<c000f35c>] (handle_IRQ) from [<c0008528>] (gic_handle_irq+0x3c/0x60) [<c0008528>] (gic_handle_irq) from [<c0012904>] (__irq_svc+0x44/0x78) Exception stack(0xed197f60 to 0xed197fa8) 7f60: 00000134 60000013 bd94362e bd94362e be96b37c 00000014 fffffd72 00000122 7f80: c000ebe4 ed196000 00000000 00000011 c032c0d8 ed197fa8 c0064008 c000ea20 7fa0: 60000013 ffffffff [<c0012904>] (__irq_svc) from [<c000ea20>] (ret_fast_syscall+0x0/0x48) ---[ end trace 478f921d0d542d1e ]--- Mapped at: [<c0227184>] debug_dma_map_page+0x48/0x11c [<c02aaca0>] macb_start_xmit+0x184/0x2a8 [<c03143c0>] dev_hard_start_xmit+0x334/0x470 [<c032c09c>] sch_direct_xmit+0x78/0x2f8 [<c0314814>] __dev_queue_xmit+0x318/0x708 due to missing checks of the dma mapping. Add the appropriate checks to fix this. Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> --- drivers/net/ethernet/cadence/macb.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c index 3190d38e16fb..64eeab6f9971 100644 --- a/drivers/net/ethernet/cadence/macb.c +++ b/drivers/net/ethernet/cadence/macb.c @@ -632,11 +632,16 @@ static void gem_rx_refill(struct macb *bp) "Unable to allocate sk_buff\n"); break; } - bp->rx_skbuff[entry] = skb; /* now fill corresponding descriptor entry */ paddr = dma_map_single(&bp->pdev->dev, skb->data, bp->rx_buffer_size, DMA_FROM_DEVICE); + if (dma_mapping_error(&bp->pdev->dev, paddr)) { + dev_kfree_skb(skb); + break; + } + + bp->rx_skbuff[entry] = skb; if (entry == RX_RING_SIZE - 1) paddr |= MACB_BIT(RX_WRAP); @@ -1040,6 +1045,10 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev) netdev_vdbg(bp->dev, "Allocated ring entry %u\n", entry); mapping = dma_map_single(&bp->pdev->dev, skb->data, len, DMA_TO_DEVICE); + if (dma_mapping_error(&bp->pdev->dev, mapping)) { + dev_kfree_skb(skb); + return NETDEV_TX_OK; + } tx_skb = &bp->tx_skb[entry]; tx_skb->skb = skb; -- 1.9.0.1.g4196000 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH RFC 1/2] net: macb: Check DMA mappings for error 2014-02-21 19:30 ` [PATCH RFC 1/2] net: macb: Check DMA mappings for error Soren Brinkmann @ 2014-02-23 0:13 ` Ben Hutchings 2014-02-26 18:36 ` Sören Brinkmann 0 siblings, 1 reply; 5+ messages in thread From: Ben Hutchings @ 2014-02-23 0:13 UTC (permalink / raw) To: Soren Brinkmann Cc: Nicolas Ferre, netdev, linux-kernel, Vinod Koul, Dan Williams, David S. Miller, Michal Simek [-- Attachment #1: Type: text/plain, Size: 1458 bytes --] On Fri, 2014-02-21 at 11:30 -0800, Soren Brinkmann wrote: [...] > --- a/drivers/net/ethernet/cadence/macb.c > +++ b/drivers/net/ethernet/cadence/macb.c > @@ -632,11 +632,16 @@ static void gem_rx_refill(struct macb *bp) > "Unable to allocate sk_buff\n"); > break; > } > - bp->rx_skbuff[entry] = skb; > > /* now fill corresponding descriptor entry */ > paddr = dma_map_single(&bp->pdev->dev, skb->data, > bp->rx_buffer_size, DMA_FROM_DEVICE); > + if (dma_mapping_error(&bp->pdev->dev, paddr)) { > + dev_kfree_skb(skb); > + break; > + } > + > + bp->rx_skbuff[entry] = skb; This bit looks good. > if (entry == RX_RING_SIZE - 1) > paddr |= MACB_BIT(RX_WRAP); > @@ -1040,6 +1045,10 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev) > netdev_vdbg(bp->dev, "Allocated ring entry %u\n", entry); > mapping = dma_map_single(&bp->pdev->dev, skb->data, > len, DMA_TO_DEVICE); > + if (dma_mapping_error(&bp->pdev->dev, mapping)) { > + dev_kfree_skb(skb); You need to unlock bp->lock before returning. Also, I think this should be kfree_skb(), as that can be observed through dropwatch whereas dev_kfree_skb() is completely silent. Ben. > + return NETDEV_TX_OK; > + } > > tx_skb = &bp->tx_skb[entry]; > tx_skb->skb = skb; -- Ben Hutchings All the simple programs have been written, and all the good names taken. [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 811 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RFC 1/2] net: macb: Check DMA mappings for error 2014-02-23 0:13 ` Ben Hutchings @ 2014-02-26 18:36 ` Sören Brinkmann 0 siblings, 0 replies; 5+ messages in thread From: Sören Brinkmann @ 2014-02-26 18:36 UTC (permalink / raw) To: Ben Hutchings Cc: Nicolas Ferre, netdev, linux-kernel, Vinod Koul, Dan Williams, David S. Miller, Michal Simek On Sun, 2014-02-23 at 12:13AM +0000, Ben Hutchings wrote: > On Fri, 2014-02-21 at 11:30 -0800, Soren Brinkmann wrote: > [...] > > --- a/drivers/net/ethernet/cadence/macb.c > > +++ b/drivers/net/ethernet/cadence/macb.c > > @@ -632,11 +632,16 @@ static void gem_rx_refill(struct macb *bp) > > "Unable to allocate sk_buff\n"); > > break; > > } > > - bp->rx_skbuff[entry] = skb; > > > > /* now fill corresponding descriptor entry */ > > paddr = dma_map_single(&bp->pdev->dev, skb->data, > > bp->rx_buffer_size, DMA_FROM_DEVICE); > > + if (dma_mapping_error(&bp->pdev->dev, paddr)) { > > + dev_kfree_skb(skb); > > + break; > > + } > > + > > + bp->rx_skbuff[entry] = skb; > > This bit looks good. > > > if (entry == RX_RING_SIZE - 1) > > paddr |= MACB_BIT(RX_WRAP); > > @@ -1040,6 +1045,10 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev) > > netdev_vdbg(bp->dev, "Allocated ring entry %u\n", entry); > > mapping = dma_map_single(&bp->pdev->dev, skb->data, > > len, DMA_TO_DEVICE); > > + if (dma_mapping_error(&bp->pdev->dev, mapping)) { > > + dev_kfree_skb(skb); > > You need to unlock bp->lock before returning. Also, I think this should > be kfree_skb(), as that can be observed through dropwatch whereas > dev_kfree_skb() is completely silent. I interpret Ben's comments as: This series is not utterly wrong and wanted. I'll send a v2 with the changes Ben suggested. Thanks for reviewing. Sören ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH RFC 2/2] net: macb: DMA-unmap full rx-buffer 2014-02-21 19:30 [PATCH RFC 0/2] net: macb: Fixing DMA usage Soren Brinkmann 2014-02-21 19:30 ` [PATCH RFC 1/2] net: macb: Check DMA mappings for error Soren Brinkmann @ 2014-02-21 19:30 ` Soren Brinkmann 1 sibling, 0 replies; 5+ messages in thread From: Soren Brinkmann @ 2014-02-21 19:30 UTC (permalink / raw) To: Nicolas Ferre Cc: netdev, linux-kernel, Vinod Koul, Dan Williams, David S. Miller, Michal Simek, Soren Brinkmann When allocating RX buffers a fixed size is used, while freeing is based on actually received bytes, resulting in the following kernel warning when CONFIG_DMA_API_DEBUG is enabled: WARNING: CPU: 0 PID: 0 at lib/dma-debug.c:1051 check_unmap+0x258/0x894() macb e000b000.ethernet: DMA-API: device driver frees DMA memory with different size [device address=0x000000002d170040] [map size=1536 bytes] [unmap size=60 bytes] Modules linked in: CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.14.0-rc3-xilinx-00220-g49f84081ce4f #65 [<c001516c>] (unwind_backtrace) from [<c0011df8>] (show_stack+0x10/0x14) [<c0011df8>] (show_stack) from [<c03c775c>] (dump_stack+0x7c/0xc8) [<c03c775c>] (dump_stack) from [<c00245cc>] (warn_slowpath_common+0x60/0x84) [<c00245cc>] (warn_slowpath_common) from [<c0024670>] (warn_slowpath_fmt+0x2c/0x3c) [<c0024670>] (warn_slowpath_fmt) from [<c0227d44>] (check_unmap+0x258/0x894) [<c0227d44>] (check_unmap) from [<c0228588>] (debug_dma_unmap_page+0x64/0x70) [<c0228588>] (debug_dma_unmap_page) from [<c02ab78c>] (gem_rx+0x118/0x170) [<c02ab78c>] (gem_rx) from [<c02ac4d4>] (macb_poll+0x24/0x94) [<c02ac4d4>] (macb_poll) from [<c031222c>] (net_rx_action+0x6c/0x188) [<c031222c>] (net_rx_action) from [<c0028a28>] (__do_softirq+0x108/0x280) [<c0028a28>] (__do_softirq) from [<c0028e8c>] (irq_exit+0x84/0xf8) [<c0028e8c>] (irq_exit) from [<c000f360>] (handle_IRQ+0x68/0x8c) [<c000f360>] (handle_IRQ) from [<c0008528>] (gic_handle_irq+0x3c/0x60) [<c0008528>] (gic_handle_irq) from [<c0012904>] (__irq_svc+0x44/0x78) Exception stack(0xc056df20 to 0xc056df68) df20: 00000001 c0577430 00000000 c0577430 04ce8e0d 00000002 edfce238 00000000 df40: 04e20f78 00000002 c05981f4 00000000 00000008 c056df68 c0064008 c02d7658 df60: 20000013 ffffffff [<c0012904>] (__irq_svc) from [<c02d7658>] (cpuidle_enter_state+0x54/0xf8) [<c02d7658>] (cpuidle_enter_state) from [<c02d77dc>] (cpuidle_idle_call+0xe0/0x138) [<c02d77dc>] (cpuidle_idle_call) from [<c000f660>] (arch_cpu_idle+0x8/0x3c) [<c000f660>] (arch_cpu_idle) from [<c006bec4>] (cpu_startup_entry+0xbc/0x124) [<c006bec4>] (cpu_startup_entry) from [<c053daec>] (start_kernel+0x350/0x3b0) ---[ end trace d5fdc38641bd3a11 ]--- Mapped at: [<c0227184>] debug_dma_map_page+0x48/0x11c [<c02ab32c>] gem_rx_refill+0x154/0x1f8 [<c02ac7b4>] macb_open+0x270/0x3e0 [<c03152e0>] __dev_open+0x7c/0xfc [<c031554c>] __dev_change_flags+0x8c/0x140 Fixing this by passing the same size which is passed during mapping the memory to the unmap function as well. Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> --- drivers/net/ethernet/cadence/macb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c index 64eeab6f9971..f325bc5c779f 100644 --- a/drivers/net/ethernet/cadence/macb.c +++ b/drivers/net/ethernet/cadence/macb.c @@ -730,7 +730,7 @@ static int gem_rx(struct macb *bp, int budget) skb_put(skb, len); addr = MACB_BF(RX_WADDR, MACB_BFEXT(RX_WADDR, addr)); dma_unmap_single(&bp->pdev->dev, addr, - len, DMA_FROM_DEVICE); + bp->rx_buffer_size, DMA_FROM_DEVICE); skb->protocol = eth_type_trans(skb, bp->dev); skb_checksum_none_assert(skb); -- 1.9.0.1.g4196000 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-02-26 18:36 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-02-21 19:30 [PATCH RFC 0/2] net: macb: Fixing DMA usage Soren Brinkmann 2014-02-21 19:30 ` [PATCH RFC 1/2] net: macb: Check DMA mappings for error Soren Brinkmann 2014-02-23 0:13 ` Ben Hutchings 2014-02-26 18:36 ` Sören Brinkmann 2014-02-21 19:30 ` [PATCH RFC 2/2] net: macb: DMA-unmap full rx-buffer Soren Brinkmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).