[PATCH] gianfar: synchronize DMA API usage by free_skb_rx_queue w/ gfar_new_page

[PATCH] gianfar: synchronize DMA API usage by free_skb_rx_queue w/ gfar_new_page

[Arseny Solokha]

From: Arseny Solokha <asolokha@kb.kras.ru>

In spite of switching to paged allocation of Rx buffers, the driver still
called dma_unmap_single() in the Rx queues tear-down path.

The DMA region unmapping code in free_skb_rx_queue() basically predates
the introduction of paged allocation to the driver. While being refactored,
it apparently hasn't reflected the change in the DMA API usage by its
counterpart gfar_new_page().

As a result, setting an interface to the DOWN state now yields the following:

  # ip link set eth2 down
  fsl-gianfar ffe24000.ethernet: DMA-API: device driver frees DMA memory with wrong function [device address=0x000000001ecd0000] [size=40]
  ------------[ cut here ]------------
  WARNING: CPU: 1 PID: 189 at lib/dma-debug.c:1123 check_unmap+0x8e0/0xa28
  CPU: 1 PID: 189 Comm: ip Tainted: G           O    4.9.5 #1
  task: dee73400 task.stack: dede2000
  NIP: c02101e8 LR: c02101e8 CTR: c0260d74
  REGS: dede3bb0 TRAP: 0700   Tainted: G           O     (4.9.5)
  MSR: 00021000 <CE,ME>  CR: 28002222  XER: 00000000

  GPR00: c02101e8 dede3c60 dee73400 000000b6 dfbd033c dfbd36c4 1f622000 dede2000
  GPR08: 00000007 c05b1634 1f622000 00000000 22002484 100a9904 00000000 00000000
  GPR16: 00000000 db4c849c 00000002 db4c8480 00000001 df142240 db4c84bc 00000000
  GPR24: c0706148 c0700000 00029000 c07552e8 c07323b4 dede3cb8 c07605e0 db535540
  NIP [c02101e8] check_unmap+0x8e0/0xa28
  LR [c02101e8] check_unmap+0x8e0/0xa28
  Call Trace:
  [dede3c60] [c02101e8] check_unmap+0x8e0/0xa28 (unreliable)
  [dede3cb0] [c02103b8] debug_dma_unmap_page+0x88/0x9c
  [dede3d30] [c02dffbc] free_skb_resources+0x2c4/0x404
  [dede3d80] [c02e39b4] gfar_close+0x24/0xc8
  [dede3da0] [c0361550] __dev_close_many+0xa0/0xf8
  [dede3dd0] [c03616f0] __dev_close+0x2c/0x4c
  [dede3df0] [c036b1b8] __dev_change_flags+0xa0/0x174
  [dede3e10] [c036b2ac] dev_change_flags+0x20/0x60
  [dede3e30] [c03e130c] devinet_ioctl+0x540/0x824
  [dede3e90] [c0347dcc] sock_ioctl+0x134/0x298
  [dede3eb0] [c0111814] do_vfs_ioctl+0xac/0x854
  [dede3f20] [c0111ffc] SyS_ioctl+0x40/0x74
  [dede3f40] [c000f290] ret_from_syscall+0x0/0x3c
  --- interrupt: c01 at 0xff45da0
      LR = 0xff45cd0
  Instruction dump:
  811d001c 7c66482e 813d0020 9061000c 807f000c 5463103a 7cc6182e 3c60c052
  386309ac 90c10008 4cc63182 4826b845 <0fe00000> 4bfffa60 3c80c052 388402c4
  ---[ end trace 695ae6d7ac1d0c47 ]---
  Mapped at:
   [<c02e22a8>] gfar_alloc_rx_buffs+0x178/0x248
   [<c02e3ef0>] startup_gfar+0x368/0x570
   [<c036aeb4>] __dev_open+0xdc/0x150
   [<c036b1b8>] __dev_change_flags+0xa0/0x174
   [<c036b2ac>] dev_change_flags+0x20/0x60

Even though the issue was discovered in 4.9 kernel, the code in question
is identical in the current net and net-next trees.

Fixes: 75354148ce69 ("gianfar: Add paged allocation and Rx S/G")
Signed-off-by: Arseny Solokha <asolokha@kb.kras.ru>
---
 drivers/net/ethernet/freescale/gianfar.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index c1b671667920..957bfc220978 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -2010,8 +2010,8 @@ static void free_skb_rx_queue(struct gfar_priv_rx_q *rx_queue)
 		if (!rxb->page)
 			continue;
 
-		dma_unmap_single(rx_queue->dev, rxb->dma,
-				 PAGE_SIZE, DMA_FROM_DEVICE);
+		dma_unmap_page(rx_queue->dev, rxb->dma,
+			       PAGE_SIZE, DMA_FROM_DEVICE);
 		__free_page(rxb->page);
 
 		rxb->page = NULL;
-- 
2.11.0

RE: [PATCH] gianfar: synchronize DMA API usage by free_skb_rx_queue w/ gfar_new_page

[Claudiu Manoil]

>-----Original Message-----
>From: Arseny Solokha [mailto:asolokha@kb.kras.ru]
>Sent: Sunday, January 29, 2017 2:52 PM
>To: Claudiu Manoil <claudiu.manoil@nxp.com>
>Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Arseny Solokha
><asolokha@kb.kras.ru>
>Subject: [PATCH] gianfar: synchronize DMA API usage by free_skb_rx_queue w/
>gfar_new_page
>
>From: Arseny Solokha <asolokha@kb.kras.ru>
>
>In spite of switching to paged allocation of Rx buffers, the driver still
>called dma_unmap_single() in the Rx queues tear-down path.
>

Except for the dma-debug part generating the warning, the implementation of 
dma_unmap_single() looks identical to dma_unmap_page(), both wrappers calling 
ops->unmap_page() with same params.
But for consistency, dma_unmap_page() seems the better choice indeed.
Thanks.

Acked-by: Claudiu Manoil <claudiu.manoil@nxp.com>

Re: [PATCH] gianfar: synchronize DMA API usage by free_skb_rx_queue w/ gfar_new_page

[David Miller]

From: Arseny Solokha <asolokha@kb.kras.ru>
Date: Sun, 29 Jan 2017 19:52:20 +0700

> From: Arseny Solokha <asolokha@kb.kras.ru>
> 
> In spite of switching to paged allocation of Rx buffers, the driver still
> called dma_unmap_single() in the Rx queues tear-down path.
> 
> The DMA region unmapping code in free_skb_rx_queue() basically predates
> the introduction of paged allocation to the driver. While being refactored,
> it apparently hasn't reflected the change in the DMA API usage by its
> counterpart gfar_new_page().
> 
> As a result, setting an interface to the DOWN state now yields the following:
 ...
> Even though the issue was discovered in 4.9 kernel, the code in question
> is identical in the current net and net-next trees.
> 
> Fixes: 75354148ce69 ("gianfar: Add paged allocation and Rx S/G")
> Signed-off-by: Arseny Solokha <asolokha@kb.kras.ru>

Applied, thanks.