[PATCH 0/2] net: sh_eth: DMA mapping API fixes

Hello,

Here are two patches that fix how the sh_eth driver is using the DMA
mapping API: a bogus struct device is used in some places, or a NULL
struct device is used.

Best regards,

Thomas

Thomas Petazzoni (2):
  net: sh_eth: use correct "struct device" when calling DMA mapping
    functions
  net: sh_eth: don't use NULL as "struct device" for the DMA mapping API

 drivers/net/ethernet/renesas/sh_eth.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

-- 
2.13.6

Using NULL as argument for the DMA mapping API is bogus, as the DMA
mapping API may use information from the "struct device" to perform
the DMA mapping operation. Therefore, pass the appropriate "struct
device".

Signed-off-by: Thomas Petazzoni <redacted>
---
 drivers/net/ethernet/renesas/sh_eth.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 91e918e654fe..db72d13cebb9 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c

@@ -1187,7 +1187,7 @@ static void sh_eth_ring_free(struct net_device *ndev)
 			}
 		}
 		ringsize = sizeof(struct sh_eth_rxdesc) * mdp->num_rx_ring;
-		dma_free_coherent(NULL, ringsize, mdp->rx_ring,
+		dma_free_coherent(&mdp->pdev->dev, ringsize, mdp->rx_ring,
 				  mdp->rx_desc_dma);
 		mdp->rx_ring = NULL;
 	}

@@ -1204,7 +1204,7 @@ static void sh_eth_ring_free(struct net_device *ndev)
 		sh_eth_tx_free(ndev, false);
 
 		ringsize = sizeof(struct sh_eth_txdesc) * mdp->num_tx_ring;
-		dma_free_coherent(NULL, ringsize, mdp->tx_ring,
+		dma_free_coherent(&mdp->pdev->dev, ringsize, mdp->tx_ring,
 				  mdp->tx_desc_dma);
 		mdp->tx_ring = NULL;
 	}

@@ -1324,8 +1324,8 @@ static int sh_eth_ring_init(struct net_device *ndev)
 
 	/* Allocate all Rx descriptors. */
 	rx_ringsize = sizeof(struct sh_eth_rxdesc) * mdp->num_rx_ring;
-	mdp->rx_ring = dma_alloc_coherent(NULL, rx_ringsize, &mdp->rx_desc_dma,
-					  GFP_KERNEL);
+	mdp->rx_ring = dma_alloc_coherent(&mdp->pdev->dev, rx_ringsize,
+					  &mdp->rx_desc_dma, GFP_KERNEL);
 	if (!mdp->rx_ring)
 		goto ring_free;
 

@@ -1333,8 +1333,8 @@ static int sh_eth_ring_init(struct net_device *ndev)
 
 	/* Allocate all Tx descriptors. */
 	tx_ringsize = sizeof(struct sh_eth_txdesc) * mdp->num_tx_ring;
-	mdp->tx_ring = dma_alloc_coherent(NULL, tx_ringsize, &mdp->tx_desc_dma,
-					  GFP_KERNEL);
+	mdp->tx_ring = dma_alloc_coherent(&mdp->pdev->dev, tx_ringsize,
+					  &mdp->tx_desc_dma, GFP_KERNEL);
 	if (!mdp->tx_ring)
 		goto ring_free;
 	return 0;

-- 
2.13.6

There are two types of "struct device": the one representing the
physical device on its physical bus (platform, SPI, PCI, etc.), and
the one representing the logical device in its device class (net,
etc.).

The DMA mapping API expects to receive as argument a "struct device"
representing the physical device, as the "struct device" contains
information about the bus that the DMA API needs.

However, the sh_eth driver mistakenly uses the "struct device"
representing the logical device (embedded in "struct net_device")
rather than the "struct device" representing the physical device on
its bus.

This commit fixes that by adjusting all calls to the DMA mapping API.

Signed-off-by: Thomas Petazzoni <redacted>
---
 drivers/net/ethernet/renesas/sh_eth.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 7e060aa9fbed..91e918e654fe 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c

@@ -1149,7 +1149,8 @@ static int sh_eth_tx_free(struct net_device *ndev, bool sent_only)
 			   entry, le32_to_cpu(txdesc->status));
 		/* Free the original skb. */
 		if (mdp->tx_skbuff[entry]) {
-			dma_unmap_single(&ndev->dev, le32_to_cpu(txdesc->addr),
+			dma_unmap_single(&mdp->pdev->dev,
+					 le32_to_cpu(txdesc->addr),
 					 le32_to_cpu(txdesc->len) >> 16,
 					 DMA_TO_DEVICE);
 			dev_kfree_skb_irq(mdp->tx_skbuff[entry]);

@@ -1179,7 +1180,7 @@ static void sh_eth_ring_free(struct net_device *ndev)
 			if (mdp->rx_skbuff[i]) {
 				struct sh_eth_rxdesc *rxdesc = &mdp->rx_ring[i];
 
-				dma_unmap_single(&ndev->dev,
+				dma_unmap_single(&mdp->pdev->dev,
 						 le32_to_cpu(rxdesc->addr),
 						 ALIGN(mdp->rx_buf_sz, 32),
 						 DMA_FROM_DEVICE);

@@ -1245,9 +1246,9 @@ static void sh_eth_ring_format(struct net_device *ndev)
 
 		/* The size of the buffer is a multiple of 32 bytes. */
 		buf_len = ALIGN(mdp->rx_buf_sz, 32);
-		dma_addr = dma_map_single(&ndev->dev, skb->data, buf_len,
+		dma_addr = dma_map_single(&mdp->pdev->dev, skb->data, buf_len,
 					  DMA_FROM_DEVICE);
-		if (dma_mapping_error(&ndev->dev, dma_addr)) {
+		if (dma_mapping_error(&mdp->pdev->dev, dma_addr)) {
 			kfree_skb(skb);
 			break;
 		}

@@ -1527,7 +1528,7 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
 			mdp->rx_skbuff[entry] = NULL;
 			if (mdp->cd->rpadir)
 				skb_reserve(skb, NET_IP_ALIGN);
-			dma_unmap_single(&ndev->dev, dma_addr,
+			dma_unmap_single(&mdp->pdev->dev, dma_addr,
 					 ALIGN(mdp->rx_buf_sz, 32),
 					 DMA_FROM_DEVICE);
 			skb_put(skb, pkt_len);

@@ -1555,9 +1556,9 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
 			if (skb == NULL)
 				break;	/* Better luck next round. */
 			sh_eth_set_receive_align(skb);
-			dma_addr = dma_map_single(&ndev->dev, skb->data,
+			dma_addr = dma_map_single(&mdp->pdev->dev, skb->data,
 						  buf_len, DMA_FROM_DEVICE);
-			if (dma_mapping_error(&ndev->dev, dma_addr)) {
+			if (dma_mapping_error(&mdp->pdev->dev, dma_addr)) {
 				kfree_skb(skb);
 				break;
 			}

@@ -2441,9 +2442,9 @@ static int sh_eth_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	/* soft swap. */
 	if (!mdp->cd->hw_swap)
 		sh_eth_soft_swap(PTR_ALIGN(skb->data, 4), skb->len + 2);
-	dma_addr = dma_map_single(&ndev->dev, skb->data, skb->len,
+	dma_addr = dma_map_single(&mdp->pdev->dev, skb->data, skb->len,
 				  DMA_TO_DEVICE);
-	if (dma_mapping_error(&ndev->dev, dma_addr)) {
+	if (dma_mapping_error(&mdp->pdev->dev, dma_addr)) {
 		kfree_skb(skb);
 		return NETDEV_TX_OK;
 	}

-- 
2.13.6

On 12/04/2017 04:33 PM, Thomas Petazzoni wrote:

There are two types of "struct device": the one representing the
physical device on its physical bus (platform, SPI, PCI, etc.), and
the one representing the logical device in its device class (net,
etc.).

The DMA mapping API expects to receive as argument a "struct device"
representing the physical device, as the "struct device" contains
information about the bus that the DMA API needs.

However, the sh_eth driver mistakenly uses the "struct device"
representing the logical device (embedded in "struct net_device")
rather than the "struct device" representing the physical device on
its bus.

This commit fixes that by adjusting all calls to the DMA mapping API.

Signed-off-by: Thomas Petazzoni <redacted>

Acked-by: Sergei Shtylyov <redacted>

  I'm still unsure the ravb driver doesn't have this issue...

[...]

MBR, Sergei

On 12/04/2017 04:33 PM, Thomas Petazzoni wrote:

Using NULL as argument for the DMA mapping API is bogus, as the DMA
mapping API may use information from the "struct device" to perform
the DMA mapping operation. Therefore, pass the appropriate "struct
device".

Signed-off-by: Thomas Petazzoni <redacted>

Acked-by: Sergei Shtylyov <redacted>

[...]

MBR, Sergei

On 12/04/2017 04:33 PM, Thomas Petazzoni wrote:

   Using 'ndev->dev.parent' (as in ravb) also should work... not sure which is better (although I'm seeing very strange things in the ravb driver built with AArch654 gcc 4.8.5).

[...]

MBR, Sergei

On Mon, Dec 4, 2017 at 2:33 PM, Thomas Petazzoni
[off-list ref] wrote:

There are two types of "struct device": the one representing the
physical device on its physical bus (platform, SPI, PCI, etc.), and
the one representing the logical device in its device class (net,
etc.).

The DMA mapping API expects to receive as argument a "struct device"
representing the physical device, as the "struct device" contains
information about the bus that the DMA API needs.

However, the sh_eth driver mistakenly uses the "struct device"
representing the logical device (embedded in "struct net_device")
rather than the "struct device" representing the physical device on
its bus.

This commit fixes that by adjusting all calls to the DMA mapping API.

Signed-off-by: Thomas Petazzoni <redacted>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

Hi Sergei,

On Mon, Dec 4, 2017 at 9:05 PM, Sergei Shtylyov
[off-list ref] wrote:

On 12/04/2017 04:33 PM, Thomas Petazzoni wrote:

quoted

There are two types of "struct device": the one representing the
physical device on its physical bus (platform, SPI, PCI, etc.), and
the one representing the logical device in its device class (net,
etc.).

The DMA mapping API expects to receive as argument a "struct device"
representing the physical device, as the "struct device" contains
information about the bus that the DMA API needs.

However, the sh_eth driver mistakenly uses the "struct device"
representing the logical device (embedded in "struct net_device")
rather than the "struct device" representing the physical device on
its bus.

This commit fixes that by adjusting all calls to the DMA mapping API.

Signed-off-by: Thomas Petazzoni <redacted>
---
  drivers/net/ethernet/renesas/sh_eth.c | 19 ++++++++++---------
  1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c

b/drivers/net/ethernet/renesas/sh_eth.c
index 7e060aa9fbed..91e918e654fe 100644

--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c

@@ -1149,7 +1149,8 @@ static int sh_eth_tx_free(struct net_device *ndev,

bool sent_only)
                           entry, le32_to_cpu(txdesc->status));
                /* Free the original skb. */
                if (mdp->tx_skbuff[entry]) {
-                       dma_unmap_single(&ndev->dev,
le32_to_cpu(txdesc->addr),
+                       dma_unmap_single(&mdp->pdev->dev,

   Using 'ndev->dev.parent' (as in ravb) also should work... not sure which
is better

That was going to be my comment, too. I also haven't checked which
generates the smallest code.

(although I'm seeing very strange things in the ravb driver built
with AArch654 gcc 4.8.5).

Don't worry, you've just been bitten by the %p pointer hashing :-(
Use %px instead.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

On Mon, Dec 4, 2017 at 2:33 PM, Thomas Petazzoni
[off-list ref] wrote:

Using NULL as argument for the DMA mapping API is bogus, as the DMA
mapping API may use information from the "struct device" to perform
the DMA mapping operation. Therefore, pass the appropriate "struct
device".

Signed-off-by: Thomas Petazzoni <redacted>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

Hello,

On Tue, 5 Dec 2017 09:39:35 +0100, Geert Uytterhoeven wrote:

quoted

   Using 'ndev->dev.parent' (as in ravb) also should work... not sure which
is better  

That was going to be my comment, too. I also haven't checked which
generates the smallest code.

Using ndev->dev.parent actually generates bigger code (44 bytes larger) :

$ size drivers/net/ethernet/renesas/sh_eth.o.*
   text	   data	    bss	    dec	    hex	filename
  27803	    696	      0	  28499	   6f53	drivers/net/ethernet/renesas/sh_eth.o.new
  27759	    696	      0	  28455	   6f27	drivers/net/ethernet/renesas/sh_eth.o.orig

.orig is my original proposal, .new is with ndev->dev.parent. I'm using
a gcc 6.4.0 compiler.

Note also that the driver is already using mdp->pdev->dev all over the place:

$ grep -- mdp-\>pdev-\>dev drivers/net/ethernet/renesas/sh_eth.c
		pm_wakeup_event(&mdp->pdev->dev, 0);
	pm_runtime_get_sync(&mdp->pdev->dev);
	pm_runtime_put_sync(&mdp->pdev->dev);
	device_set_wakeup_enable(&mdp->pdev->dev, mdp->wol_enabled);
	pm_runtime_get_sync(&mdp->pdev->dev);
	pm_runtime_put_sync(&mdp->pdev->dev);
	pm_runtime_put_sync(&mdp->pdev->dev);
	struct device *dev = &mdp->pdev->dev;

Best regards,

Thomas Petazzoni
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Hi Thomas,

On Tue, Dec 5, 2017 at 10:38 AM, Thomas Petazzoni
[off-list ref] wrote:

On Tue, 5 Dec 2017 09:39:35 +0100, Geert Uytterhoeven wrote:

quoted

quoted

   Using 'ndev->dev.parent' (as in ravb) also should work... not sure which
is better

That was going to be my comment, too. I also haven't checked which
generates the smallest code.

Using ndev->dev.parent actually generates bigger code (44 bytes larger) :

Thanks a lot for checking!

Gr{oetje,eeting}s,

                        Geert

From: Thomas Petazzoni <redacted>
Date: Mon,  4 Dec 2017 14:33:25 +0100

Here are two patches that fix how the sh_eth driver is using the DMA
mapping API: a bogus struct device is used in some places, or a NULL
struct device is used.

Series applied.