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

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

[Thomas Petazzoni]

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

[PATCH 1/2] net: sh_eth: use correct "struct device" when calling DMA mapping functions

[Thomas Petazzoni]

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 <thomas.petazzoni@free-electrons.com>
---
 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

[PATCH 2/2] net: sh_eth: don't use NULL as "struct device" for the DMA mapping API

[Thomas Petazzoni]

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 <thomas.petazzoni@free-electrons.com>
---
 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

Re: [PATCH 1/2] net: sh_eth: use correct "struct device" when calling DMA mapping functions

[Sergei Shtylyov]

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 <thomas.petazzoni@free-electrons.com>

Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

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

[...]

MBR, Sergei

Re: [PATCH 2/2] net: sh_eth: don't use NULL as "struct device" for the DMA mapping API

[Sergei Shtylyov]

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 <thomas.petazzoni@free-electrons.com>

Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

[...]

MBR, Sergei

Re: [PATCH 1/2] net: sh_eth: use correct "struct device" when calling DMA mapping functions

[Sergei Shtylyov]

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 <thomas.petazzoni@free-electrons.com>
> ---
>   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 (although I'm seeing very strange things in the ravb driver built 
with AArch654 gcc 4.8.5).

[...]

MBR, Sergei

Re: [PATCH 1/2] net: sh_eth: use correct "struct device" when calling DMA mapping functions

[Geert Uytterhoeven]

On Mon, Dec 4, 2017 at 2:33 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> 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 <thomas.petazzoni@free-electrons.com>

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

Gr{oetje,eeting}s,

                        Geert

Re: [PATCH 1/2] net: sh_eth: use correct "struct device" when calling DMA mapping functions

[Geert Uytterhoeven]

On Mon, Dec 4, 2017 at 2:33 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> 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 <thomas.petazzoni@free-electrons.com>

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

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

Re: [PATCH 1/2] net: sh_eth: use correct "struct device" when calling DMA mapping functions

[Geert Uytterhoeven]

Hi Sergei,

On Mon, Dec 4, 2017 at 9:05 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> 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 <thomas.petazzoni@free-electrons.com>
>> ---
>>   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

Re: [PATCH 2/2] net: sh_eth: don't use NULL as "struct device" for the DMA mapping API

[Geert Uytterhoeven]

On Mon, Dec 4, 2017 at 2:33 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> 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 <thomas.petazzoni@free-electrons.com>

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

Gr{oetje,eeting}s,

                        Geert

Re: [PATCH 2/2] net: sh_eth: don't use NULL as "struct device" for the DMA mapping API

[Geert Uytterhoeven]

On Mon, Dec 4, 2017 at 2:33 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> 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 <thomas.petazzoni@free-electrons.com>

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

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

Re: [PATCH 1/2] net: sh_eth: use correct "struct device" when calling DMA mapping functions

[Thomas Petazzoni]

Hello,

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

> >    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

Re: [PATCH 1/2] net: sh_eth: use correct "struct device" when calling DMA mapping functions

[Geert Uytterhoeven]

Hi Thomas,

On Tue, Dec 5, 2017 at 10:38 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> On Tue, 5 Dec 2017 09:39:35 +0100, Geert Uytterhoeven wrote:
>> >    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

Re: [PATCH 1/2] net: sh_eth: use correct "struct device" when calling DMA mapping functions

[Geert Uytterhoeven]

Hi Thomas,

On Tue, Dec 5, 2017 at 10:38 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> On Tue, 5 Dec 2017 09:39:35 +0100, Geert Uytterhoeven wrote:
>> >    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

--
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

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

[David Miller]

From: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
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.