[PATCH 0/3] sh_eth: Remove redundant alignment adjustment

[PATCH 0/3] sh_eth: Remove redundant alignment adjustment

[Yoshihiro Kaneko]

This series is based on net tree.

Mitsuhiro Kimura (3):
  sh_eth: Remove redundant alignment adjustment
  sh_eth: Fix skb alloc size and alignment adjust rule.
  sh_eth: Fix dma mapping issue

 drivers/net/ethernet/renesas/sh_eth.c | 65 +++++++++++++++++++++--------------
 drivers/net/ethernet/renesas/sh_eth.h |  2 ++
 2 files changed, 41 insertions(+), 26 deletions(-)

-- 
1.9.1

[PATCH 1/3] sh_eth: Remove redundant alignment adjustment

[Yoshihiro Kaneko]

From: Mitsuhiro Kimura <mitsuhiro.kimura.kc@renesas.com>

PTR_ALIGN macro after skb_reserve is redundant, because skb_reserve
function adjusts the alignment of skb->data.

Signed-off-by: Mitsuhiro Kimura <mitsuhiro.kimura.kc@renesas.com>
Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
---
 drivers/net/ethernet/renesas/sh_eth.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 60e9c2c..49e963e 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -1141,7 +1141,7 @@ static void sh_eth_ring_format(struct net_device *ndev)
 
 		/* RX descriptor */
 		rxdesc = &mdp->rx_ring[i];
-		rxdesc->addr = virt_to_phys(PTR_ALIGN(skb->data, 4));
+		rxdesc->addr = virt_to_phys(skb->data);
 		rxdesc->status = cpu_to_edmac(mdp, RD_RACT | RD_RFP);
 
 		/* The size of the buffer is 16 byte boundary. */
@@ -1477,7 +1477,7 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
 			sh_eth_set_receive_align(skb);
 
 			skb_checksum_none_assert(skb);
-			rxdesc->addr = virt_to_phys(PTR_ALIGN(skb->data, 4));
+			rxdesc->addr = virt_to_phys(skb->data);
 		}
 		if (entry >= mdp->num_rx_ring - 1)
 			rxdesc->status |-- 
1.9.1

[PATCH 2/3] sh_eth: Fix skb alloc size and alignment adjust rule.

[Yoshihiro Kaneko]

From: Mitsuhiro Kimura <mitsuhiro.kimura.kc@renesas.com>

In the current driver, allocation size of skb does not care the alignment
adjust after allocation.
And also, in the current implementation, buffer alignment method by
sh_eth_set_receive_align function has a bug that this function displace
buffer start address forcedly when the alignment is corrected.
In the result, tail of the skb will exceed allocated area and kernel panic
will be occurred.
This patch fix this issue.

Signed-off-by: Mitsuhiro Kimura <mitsuhiro.kimura.kc@renesas.com>
Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
---
 drivers/net/ethernet/renesas/sh_eth.c | 35 ++++++++++++++---------------------
 drivers/net/ethernet/renesas/sh_eth.h |  2 ++
 2 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 49e963e..0e4a407 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -917,21 +917,12 @@ static int sh_eth_reset(struct net_device *ndev)
 	return ret;
 }
 
-#if defined(CONFIG_CPU_SH4) || defined(CONFIG_ARCH_SHMOBILE)
 static void sh_eth_set_receive_align(struct sk_buff *skb)
 {
-	int reserve;
-
-	reserve = SH4_SKB_RX_ALIGN - ((u32)skb->data & (SH4_SKB_RX_ALIGN - 1));
+	u32 reserve = (u32)skb->data & (SH_ETH_RX_ALIGN - 1);
 	if (reserve)
-		skb_reserve(skb, reserve);
-}
-#else
-static void sh_eth_set_receive_align(struct sk_buff *skb)
-{
-	skb_reserve(skb, SH2_SH3_SKB_RX_ALIGN);
+		skb_reserve(skb, SH_ETH_RX_ALIGN - reserve);
 }
-#endif
 
 
 /* CPU <-> EDMAC endian convert */
@@ -1119,6 +1110,7 @@ static void sh_eth_ring_format(struct net_device *ndev)
 	struct sh_eth_txdesc *txdesc = NULL;
 	int rx_ringsize = sizeof(*rxdesc) * mdp->num_rx_ring;
 	int tx_ringsize = sizeof(*txdesc) * mdp->num_tx_ring;
+	int skbuff_size = mdp->rx_buf_sz + SH_ETH_RX_ALIGN - 1;
 
 	mdp->cur_rx = 0;
 	mdp->cur_tx = 0;
@@ -1131,21 +1123,21 @@ static void sh_eth_ring_format(struct net_device *ndev)
 	for (i = 0; i < mdp->num_rx_ring; i++) {
 		/* skb */
 		mdp->rx_skbuff[i] = NULL;
-		skb = netdev_alloc_skb(ndev, mdp->rx_buf_sz);
+		skb = netdev_alloc_skb(ndev, skbuff_size);
 		mdp->rx_skbuff[i] = skb;
 		if (skb = NULL)
 			break;
-		dma_map_single(&ndev->dev, skb->data, mdp->rx_buf_sz,
-			       DMA_FROM_DEVICE);
 		sh_eth_set_receive_align(skb);
 
 		/* RX descriptor */
 		rxdesc = &mdp->rx_ring[i];
+		/* The size of the buffer is 16 byte boundary. */
+		rxdesc->buffer_length = ALIGN(mdp->rx_buf_sz, 16);
+		dma_map_single(&ndev->dev, skb->data, rxdesc->buffer_length,
+			       DMA_FROM_DEVICE);
 		rxdesc->addr = virt_to_phys(skb->data);
 		rxdesc->status = cpu_to_edmac(mdp, RD_RACT | RD_RFP);
 
-		/* The size of the buffer is 16 byte boundary. */
-		rxdesc->buffer_length = ALIGN(mdp->rx_buf_sz, 16);
 		/* Rx descriptor address set */
 		if (i = 0) {
 			sh_eth_write(ndev, mdp->rx_desc_dma, RDLAR);
@@ -1397,6 +1389,7 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
 	struct sk_buff *skb;
 	u16 pkt_len = 0;
 	u32 desc_status;
+	int skbuff_size = mdp->rx_buf_sz + SH_ETH_RX_ALIGN - 1;
 
 	rxdesc = &mdp->rx_ring[entry];
 	while (!(rxdesc->status & cpu_to_edmac(mdp, RD_RACT))) {
@@ -1448,8 +1441,8 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
 			if (mdp->cd->rpadir)
 				skb_reserve(skb, NET_IP_ALIGN);
 			dma_sync_single_for_cpu(&ndev->dev, rxdesc->addr,
-						mdp->rx_buf_sz,
-						DMA_FROM_DEVICE);
+					ALIGN(mdp->rx_buf_sz, 16),
+					DMA_FROM_DEVICE);
 			skb_put(skb, pkt_len);
 			skb->protocol = eth_type_trans(skb, ndev);
 			netif_receive_skb(skb);
@@ -1468,13 +1461,13 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
 		rxdesc->buffer_length = ALIGN(mdp->rx_buf_sz, 16);
 
 		if (mdp->rx_skbuff[entry] = NULL) {
-			skb = netdev_alloc_skb(ndev, mdp->rx_buf_sz);
+			skb = netdev_alloc_skb(ndev, skbuff_size);
 			mdp->rx_skbuff[entry] = skb;
 			if (skb = NULL)
 				break;	/* Better luck next round. */
-			dma_map_single(&ndev->dev, skb->data, mdp->rx_buf_sz,
-				       DMA_FROM_DEVICE);
 			sh_eth_set_receive_align(skb);
+			dma_map_single(&ndev->dev, skb->data,
+				       rxdesc->buffer_length, DMA_FROM_DEVICE);
 
 			skb_checksum_none_assert(skb);
 			rxdesc->addr = virt_to_phys(skb->data);
diff --git a/drivers/net/ethernet/renesas/sh_eth.h b/drivers/net/ethernet/renesas/sh_eth.h
index b37c427..d138ebe 100644
--- a/drivers/net/ethernet/renesas/sh_eth.h
+++ b/drivers/net/ethernet/renesas/sh_eth.h
@@ -163,8 +163,10 @@ enum {
 /* Driver's parameters */
 #if defined(CONFIG_CPU_SH4) || defined(CONFIG_ARCH_SHMOBILE)
 #define SH4_SKB_RX_ALIGN	32
+#define SH_ETH_RX_ALIGN		(SH4_SKB_RX_ALIGN)
 #else
 #define SH2_SH3_SKB_RX_ALIGN	2
+#define SH_ETH_RX_ALIGN		(SH2_SH3_SKB_RX_ALIGN)
 #endif
 
 /* Register's bits
-- 
1.9.1

[PATCH 3/3] sh_eth: Fix dma mapping issue

[Yoshihiro Kaneko]

From: Mitsuhiro Kimura <mitsuhiro.kimura.kc@renesas.com>

When CONFIG_DMA_API_DEBUG=y, many DMA error messages reports.
In order to use DMA debug, This patch fix following issues.

Issue 1:
If dma_mapping_error function is not called appropriately after
DMA mapping, DMA debug will report error message when DMA unmap
function is called.

Issue 2:
If skb_reserve function is called after DMA mapping, the relationship
between mapping addr and mapping size will be broken.
In this case, DMA debug will report error messages when DMA sync
function and DMA unmap function are called.

Issue 3:
If the size of frame data is less than ETH_ZLEN, the size is resized
to ETH_ZLEN after DMA map function is called.
In the TX skb freeing function, dma unmap function is called with that
resized value. So, unmap size error will reported.

Issue 4:
In the rx function, DMA map function is called without DMA unmap function
is called for RX skb reallocating.
It will case the DMA debug error that number of debug entry is full and
DMA debug logic is stopped.

Signed-off-by: Mitsuhiro Kimura <mitsuhiro.kimura.kc@renesas.com>
Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
---
 drivers/net/ethernet/renesas/sh_eth.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 0e4a407..23318cf 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -1136,6 +1136,11 @@ static void sh_eth_ring_format(struct net_device *ndev)
 		dma_map_single(&ndev->dev, skb->data, rxdesc->buffer_length,
 			       DMA_FROM_DEVICE);
 		rxdesc->addr = virt_to_phys(skb->data);
+		if (dma_mapping_error(&ndev->dev, rxdesc->addr)) {
+			dev_kfree_skb(mdp->rx_skbuff[i]);
+			mdp->rx_skbuff[i] = NULL;
+			break;
+		}
 		rxdesc->status = cpu_to_edmac(mdp, RD_RACT | RD_RFP);
 
 		/* Rx descriptor address set */
@@ -1364,7 +1369,7 @@ static int sh_eth_txfree(struct net_device *ndev)
 		if (mdp->tx_skbuff[entry]) {
 			dma_unmap_single(&ndev->dev, txdesc->addr,
 					 txdesc->buffer_length, DMA_TO_DEVICE);
-			dev_kfree_skb_irq(mdp->tx_skbuff[entry]);
+			dev_kfree_skb_any(mdp->tx_skbuff[entry]);
 			mdp->tx_skbuff[entry] = NULL;
 			free_num++;
 		}
@@ -1466,11 +1471,19 @@ 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_unmap_single(&ndev->dev, rxdesc->addr,
+					 rxdesc->buffer_length,
+					 DMA_FROM_DEVICE);
 			dma_map_single(&ndev->dev, skb->data,
 				       rxdesc->buffer_length, DMA_FROM_DEVICE);
 
 			skb_checksum_none_assert(skb);
 			rxdesc->addr = virt_to_phys(skb->data);
+			if (dma_mapping_error(&ndev->dev, rxdesc->addr)) {
+				dev_kfree_skb_any(mdp->rx_skbuff[entry]);
+				mdp->rx_skbuff[entry] = NULL;
+				break;
+			}
 		}
 		if (entry >= mdp->num_rx_ring - 1)
 			rxdesc->status |@@ -2104,12 +2117,18 @@ static int sh_eth_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	if (!mdp->cd->hw_swap)
 		sh_eth_soft_swap(phys_to_virt(ALIGN(txdesc->addr, 4)),
 				 skb->len + 2);
-	txdesc->addr = dma_map_single(&ndev->dev, skb->data, skb->len,
-				      DMA_TO_DEVICE);
 	if (skb->len < ETH_ZLEN)
 		txdesc->buffer_length = ETH_ZLEN;
 	else
 		txdesc->buffer_length = skb->len;
+	txdesc->addr = dma_map_single(&ndev->dev, skb->data,
+				      txdesc->buffer_length,
+				      DMA_TO_DEVICE);
+	if (dma_mapping_error(&ndev->dev, txdesc->addr)) {
+		dev_kfree_skb_any(mdp->tx_skbuff[entry]);
+		mdp->tx_skbuff[entry] = NULL;
+		goto out;
+	}
 
 	if (entry >= mdp->num_tx_ring - 1)
 		txdesc->status |= cpu_to_edmac(mdp, TD_TACT | TD_TDLE);
@@ -2121,6 +2140,7 @@ static int sh_eth_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	if (!(sh_eth_read(ndev, EDTRR) & sh_eth_get_edtrr_trns(mdp)))
 		sh_eth_write(ndev, sh_eth_get_edtrr_trns(mdp), EDTRR);
 
+out:
 	return NETDEV_TX_OK;
 }
 
-- 
1.9.1

Re: [PATCH 0/3] sh_eth: Remove redundant alignment adjustment

[Geert Uytterhoeven]

Hi Kaneko-san,

On Thu, Nov 13, 2014 at 8:04 AM, Yoshihiro Kaneko <ykaneko0929@gmail.com> wrote:
> This series is based on net tree.
>
> Mitsuhiro Kimura (3):
>   sh_eth: Remove redundant alignment adjustment
>   sh_eth: Fix skb alloc size and alignment adjust rule.
>   sh_eth: Fix dma mapping issue

Thanks!

I've applied this series, and your series "[PATCH 0/2] Fix sleeping function
called from invalid context", and the warnings from dma-debug.c I was
seeing on r8a7791/koelsch are gone.

Tested-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/3] sh_eth: Remove redundant alignment adjustment

[Sergei Shtylyov]

On 11/13/2014 10:04 AM, Yoshihiro Kaneko wrote:

> From: Mitsuhiro Kimura <mitsuhiro.kimura.kc@renesas.com>

> PTR_ALIGN macro after skb_reserve is redundant, because skb_reserve
> function adjusts the alignment of skb->data.

    OK, but where is the bug? There must be one if you base this patch on the 
'net' tree...

> Signed-off-by: Mitsuhiro Kimura <mitsuhiro.kimura.kc@renesas.com>
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>

WBR, Sergei

Re: [PATCH 2/3] sh_eth: Fix skb alloc size and alignment adjust rule.

[Sergei Shtylyov]

On 11/13/2014 10:05 AM, Yoshihiro Kaneko wrote:

> From: Mitsuhiro Kimura <mitsuhiro.kimura.kc@renesas.com>

> In the current driver, allocation size of skb does not care the alignment
> adjust after allocation.
> And also, in the current implementation, buffer alignment method by
> sh_eth_set_receive_align function has a bug that this function displace
> buffer start address forcedly when the alignment is corrected.
> In the result, tail of the skb will exceed allocated area and kernel panic
> will be occurred.

    Oh, have never seen panic but Geert has reported WARNINGs from the DMA 
debug code...

> This patch fix this issue.

> Signed-off-by: Mitsuhiro Kimura <mitsuhiro.kimura.kc@renesas.com>
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> ---
>   drivers/net/ethernet/renesas/sh_eth.c | 35 ++++++++++++++---------------------
>   drivers/net/ethernet/renesas/sh_eth.h |  2 ++
>   2 files changed, 16 insertions(+), 21 deletions(-)

> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> index 49e963e..0e4a407 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -917,21 +917,12 @@ static int sh_eth_reset(struct net_device *ndev)
>   	return ret;
>   }
>
> -#if defined(CONFIG_CPU_SH4) || defined(CONFIG_ARCH_SHMOBILE)
>   static void sh_eth_set_receive_align(struct sk_buff *skb)
>   {
> -	int reserve;
> -
> -	reserve = SH4_SKB_RX_ALIGN - ((u32)skb->data & (SH4_SKB_RX_ALIGN - 1));
> +	u32 reserve = (u32)skb->data & (SH_ETH_RX_ALIGN - 1);

    Please keep an empty line after declaration, as it was before this patch.

>   	if (reserve)
> -		skb_reserve(skb, reserve);
> -}
> -#else
> -static void sh_eth_set_receive_align(struct sk_buff *skb)
> -{
> -	skb_reserve(skb, SH2_SH3_SKB_RX_ALIGN);
> +		skb_reserve(skb, SH_ETH_RX_ALIGN - reserve);
>   }
> -#endif
>
>
>   /* CPU <-> EDMAC endian convert */
> @@ -1119,6 +1110,7 @@ static void sh_eth_ring_format(struct net_device *ndev)
>   	struct sh_eth_txdesc *txdesc = NULL;
>   	int rx_ringsize = sizeof(*rxdesc) * mdp->num_rx_ring;
>   	int tx_ringsize = sizeof(*txdesc) * mdp->num_tx_ring;
> +	int skbuff_size = mdp->rx_buf_sz + SH_ETH_RX_ALIGN - 1;
>
>   	mdp->cur_rx = 0;
>   	mdp->cur_tx = 0;
> @@ -1131,21 +1123,21 @@ static void sh_eth_ring_format(struct net_device *ndev)
>   	for (i = 0; i < mdp->num_rx_ring; i++) {
>   		/* skb */
>   		mdp->rx_skbuff[i] = NULL;
> -		skb = netdev_alloc_skb(ndev, mdp->rx_buf_sz);
> +		skb = netdev_alloc_skb(ndev, skbuff_size);
>   		mdp->rx_skbuff[i] = skb;
>   		if (skb = NULL)
>   			break;
> -		dma_map_single(&ndev->dev, skb->data, mdp->rx_buf_sz,
> -			       DMA_FROM_DEVICE);
>   		sh_eth_set_receive_align(skb);
>
>   		/* RX descriptor */
>   		rxdesc = &mdp->rx_ring[i];
> +		/* The size of the buffer is 16 byte boundary. */

    Is *on* 16 byte boundary, you mean?

> +		rxdesc->buffer_length = ALIGN(mdp->rx_buf_sz, 16);
> +		dma_map_single(&ndev->dev, skb->data, rxdesc->buffer_length,
> +			       DMA_FROM_DEVICE);
>   		rxdesc->addr = virt_to_phys(skb->data);
>   		rxdesc->status = cpu_to_edmac(mdp, RD_RACT | RD_RFP);
>
> -		/* The size of the buffer is 16 byte boundary. */

    Ah, you're just copying an existent comment... well, seems a good time to 
fix it then. :-)

[...]
> @@ -1448,8 +1441,8 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
>   			if (mdp->cd->rpadir)
>   				skb_reserve(skb, NET_IP_ALIGN);
>   			dma_sync_single_for_cpu(&ndev->dev, rxdesc->addr,
> -						mdp->rx_buf_sz,
> -						DMA_FROM_DEVICE);
> +					ALIGN(mdp->rx_buf_sz, 16),
> +					DMA_FROM_DEVICE);

    Please keep the original alignment of the continuation lines.

>   			skb_put(skb, pkt_len);
>   			skb->protocol = eth_type_trans(skb, ndev);
>   			netif_receive_skb(skb);
> @@ -1468,13 +1461,13 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
>   		rxdesc->buffer_length = ALIGN(mdp->rx_buf_sz, 16);
>
>   		if (mdp->rx_skbuff[entry] = NULL) {
> -			skb = netdev_alloc_skb(ndev, mdp->rx_buf_sz);
> +			skb = netdev_alloc_skb(ndev, skbuff_size);
>   			mdp->rx_skbuff[entry] = skb;
>   			if (skb = NULL)
>   				break;	/* Better luck next round. */
> -			dma_map_single(&ndev->dev, skb->data, mdp->rx_buf_sz,
> -				       DMA_FROM_DEVICE);
>   			sh_eth_set_receive_align(skb);
> +			dma_map_single(&ndev->dev, skb->data,
> +				       rxdesc->buffer_length, DMA_FROM_DEVICE);
>
>   			skb_checksum_none_assert(skb);
>   			rxdesc->addr = virt_to_phys(skb->data);
> diff --git a/drivers/net/ethernet/renesas/sh_eth.h b/drivers/net/ethernet/renesas/sh_eth.h
> index b37c427..d138ebe 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.h
> +++ b/drivers/net/ethernet/renesas/sh_eth.h
> @@ -163,8 +163,10 @@ enum {
>   /* Driver's parameters */
>   #if defined(CONFIG_CPU_SH4) || defined(CONFIG_ARCH_SHMOBILE)
>   #define SH4_SKB_RX_ALIGN	32
> +#define SH_ETH_RX_ALIGN		(SH4_SKB_RX_ALIGN)

    () not needed.

>   #else
>   #define SH2_SH3_SKB_RX_ALIGN	2
> +#define SH_ETH_RX_ALIGN		(SH2_SH3_SKB_RX_ALIGN)

    Likewise.
    And I don't think we still need {SH2_SH3|SH4}_SKB_RX_ALIGN after this patch.

[...]

WBR, Sergei

Re: [PATCH 3/3] sh_eth: Fix dma mapping issue

[Sergei Shtylyov]

On 11/13/2014 10:05 AM, Yoshihiro Kaneko wrote:

> From: Mitsuhiro Kimura <mitsuhiro.kimura.kc@renesas.com>

> When CONFIG_DMA_API_DEBUG=y, many DMA error messages reports.
> In order to use DMA debug, This patch fix following issues.

> Issue 1:
> If dma_mapping_error function is not called appropriately after
> DMA mapping, DMA debug will report error message when DMA unmap
> function is called.

> Issue 2:
> If skb_reserve function is called after DMA mapping, the relationship
> between mapping addr and mapping size will be broken.
> In this case, DMA debug will report error messages when DMA sync
> function and DMA unmap function are called.

> Issue 3:
> If the size of frame data is less than ETH_ZLEN, the size is resized
> to ETH_ZLEN after DMA map function is called.
> In the TX skb freeing function, dma unmap function is called with that
> resized value. So, unmap size error will reported.

> Issue 4:
> In the rx function, DMA map function is called without DMA unmap function
> is called for RX skb reallocating.
> It will case the DMA debug error that number of debug entry is full and
> DMA debug logic is stopped.

    The rule of thumb is "fix one issue per patch". Please split accordingly.

> Signed-off-by: Mitsuhiro Kimura <mitsuhiro.kimura.kc@renesas.com>
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>

    Thanks for beating me to it. Fixing these issues has been on my agenda for 
a long time... :-)

> ---
>   drivers/net/ethernet/renesas/sh_eth.c | 26 +++++++++++++++++++++++---
>   1 file changed, 23 insertions(+), 3 deletions(-)

> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> index 0e4a407..23318cf 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -1136,6 +1136,11 @@ static void sh_eth_ring_format(struct net_device *ndev)
>   		dma_map_single(&ndev->dev, skb->data, rxdesc->buffer_length,
>   			       DMA_FROM_DEVICE);
>   		rxdesc->addr = virt_to_phys(skb->data);

    Can't we get rid of these bogus virt_to_phys() calls, while at it? 
dma_map_single() returns a DMA address, no?

> +		if (dma_mapping_error(&ndev->dev, rxdesc->addr)) {
> +			dev_kfree_skb(mdp->rx_skbuff[i]);
> +			mdp->rx_skbuff[i] = NULL;
> +			break;
> +		}
>   		rxdesc->status = cpu_to_edmac(mdp, RD_RACT | RD_RFP);
>
>   		/* Rx descriptor address set */
> @@ -1364,7 +1369,7 @@ static int sh_eth_txfree(struct net_device *ndev)
>   		if (mdp->tx_skbuff[entry]) {
>   			dma_unmap_single(&ndev->dev, txdesc->addr,
>   					 txdesc->buffer_length, DMA_TO_DEVICE);
> -			dev_kfree_skb_irq(mdp->tx_skbuff[entry]);
> +			dev_kfree_skb_any(mdp->tx_skbuff[entry]);

    Hm, I'm not sure where is this described in the changelog...

>   			mdp->tx_skbuff[entry] = NULL;
>   			free_num++;
>   		}
> @@ -1466,11 +1471,19 @@ 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_unmap_single(&ndev->dev, rxdesc->addr,
> +					 rxdesc->buffer_length,
> +					 DMA_FROM_DEVICE);
>   			dma_map_single(&ndev->dev, skb->data,
>   				       rxdesc->buffer_length, DMA_FROM_DEVICE);
>
>   			skb_checksum_none_assert(skb);
>   			rxdesc->addr = virt_to_phys(skb->data);

    Likewise, can we get rid of this bogu?

> +			if (dma_mapping_error(&ndev->dev, rxdesc->addr)) {
> +				dev_kfree_skb_any(mdp->rx_skbuff[entry]);
> +				mdp->rx_skbuff[entry] = NULL;
> +				break;
> +			}
>   		}
>   		if (entry >= mdp->num_rx_ring - 1)
>   			rxdesc->status |> @@ -2104,12 +2117,18 @@ static int sh_eth_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>   	if (!mdp->cd->hw_swap)
>   		sh_eth_soft_swap(phys_to_virt(ALIGN(txdesc->addr, 4)),
>   				 skb->len + 2);
> -	txdesc->addr = dma_map_single(&ndev->dev, skb->data, skb->len,
> -				      DMA_TO_DEVICE);
>   	if (skb->len < ETH_ZLEN)
>   		txdesc->buffer_length = ETH_ZLEN;
>   	else
>   		txdesc->buffer_length = skb->len;
> +	txdesc->addr = dma_map_single(&ndev->dev, skb->data,
> +				      txdesc->buffer_length,
> +				      DMA_TO_DEVICE);
> +	if (dma_mapping_error(&ndev->dev, txdesc->addr)) {
> +		dev_kfree_skb_any(mdp->tx_skbuff[entry]);
> +		mdp->tx_skbuff[entry] = NULL;
> +		goto out;

    Why not just *return*?!

[...]

WBR, Sergei

Re: [PATCH 1/3] sh_eth: Remove redundant alignment adjustment

[Simon Horman]

On Fri, Nov 14, 2014 at 01:37:22AM +0300, Sergei Shtylyov wrote:
> On 11/13/2014 10:04 AM, Yoshihiro Kaneko wrote:
> 
> >From: Mitsuhiro Kimura <mitsuhiro.kimura.kc@renesas.com>
> 
> >PTR_ALIGN macro after skb_reserve is redundant, because skb_reserve
> >function adjusts the alignment of skb->data.
> 
>    OK, but where is the bug? There must be one if you base this patch on the
> 'net' tree...

I suppose this patch would be more appropriate for the net-next tree.

> >Signed-off-by: Mitsuhiro Kimura <mitsuhiro.kimura.kc@renesas.com>
> >Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> 
> WBR, Sergei
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 3/3] sh_eth: Fix dma mapping issue

[Simon Horman]

Hi Sergei,

On Fri, Nov 14, 2014 at 02:05:44AM +0300, Sergei Shtylyov wrote:
> On 11/13/2014 10:05 AM, Yoshihiro Kaneko wrote:
> 
> >From: Mitsuhiro Kimura <mitsuhiro.kimura.kc@renesas.com>
> 
> >When CONFIG_DMA_API_DEBUG=y, many DMA error messages reports.
> >In order to use DMA debug, This patch fix following issues.
> 
> >Issue 1:
> >If dma_mapping_error function is not called appropriately after
> >DMA mapping, DMA debug will report error message when DMA unmap
> >function is called.
> 
> >Issue 2:
> >If skb_reserve function is called after DMA mapping, the relationship
> >between mapping addr and mapping size will be broken.
> >In this case, DMA debug will report error messages when DMA sync
> >function and DMA unmap function are called.
> 
> >Issue 3:
> >If the size of frame data is less than ETH_ZLEN, the size is resized
> >to ETH_ZLEN after DMA map function is called.
> >In the TX skb freeing function, dma unmap function is called with that
> >resized value. So, unmap size error will reported.
> 
> >Issue 4:
> >In the rx function, DMA map function is called without DMA unmap function
> >is called for RX skb reallocating.
> >It will case the DMA debug error that number of debug entry is full and
> >DMA debug logic is stopped.
> 
>    The rule of thumb is "fix one issue per patch". Please split accordingly.
> 
> >Signed-off-by: Mitsuhiro Kimura <mitsuhiro.kimura.kc@renesas.com>
> >Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> 
>    Thanks for beating me to it. Fixing these issues has been on my agenda
> for a long time... :-)

as this patch is somewhat involved and as you have pointed out needs a bit
of work I'm wondering if you could take it over.

> >---
> >  drivers/net/ethernet/renesas/sh_eth.c | 26 +++++++++++++++++++++++---
> >  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> >diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> >index 0e4a407..23318cf 100644
> >--- a/drivers/net/ethernet/renesas/sh_eth.c
> >+++ b/drivers/net/ethernet/renesas/sh_eth.c
> >@@ -1136,6 +1136,11 @@ static void sh_eth_ring_format(struct net_device *ndev)
> >  		dma_map_single(&ndev->dev, skb->data, rxdesc->buffer_length,
> >  			       DMA_FROM_DEVICE);
> >  		rxdesc->addr = virt_to_phys(skb->data);
> 
>    Can't we get rid of these bogus virt_to_phys() calls, while at it?
> dma_map_single() returns a DMA address, no?
> 
> >+		if (dma_mapping_error(&ndev->dev, rxdesc->addr)) {
> >+			dev_kfree_skb(mdp->rx_skbuff[i]);
> >+			mdp->rx_skbuff[i] = NULL;
> >+			break;
> >+		}
> >  		rxdesc->status = cpu_to_edmac(mdp, RD_RACT | RD_RFP);
> >
> >  		/* Rx descriptor address set */
> >@@ -1364,7 +1369,7 @@ static int sh_eth_txfree(struct net_device *ndev)
> >  		if (mdp->tx_skbuff[entry]) {
> >  			dma_unmap_single(&ndev->dev, txdesc->addr,
> >  					 txdesc->buffer_length, DMA_TO_DEVICE);
> >-			dev_kfree_skb_irq(mdp->tx_skbuff[entry]);
> >+			dev_kfree_skb_any(mdp->tx_skbuff[entry]);
> 
>    Hm, I'm not sure where is this described in the changelog...
> 
> >  			mdp->tx_skbuff[entry] = NULL;
> >  			free_num++;
> >  		}
> >@@ -1466,11 +1471,19 @@ 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_unmap_single(&ndev->dev, rxdesc->addr,
> >+					 rxdesc->buffer_length,
> >+					 DMA_FROM_DEVICE);
> >  			dma_map_single(&ndev->dev, skb->data,
> >  				       rxdesc->buffer_length, DMA_FROM_DEVICE);
> >
> >  			skb_checksum_none_assert(skb);
> >  			rxdesc->addr = virt_to_phys(skb->data);
> 
>    Likewise, can we get rid of this bogu?
> 
> >+			if (dma_mapping_error(&ndev->dev, rxdesc->addr)) {
> >+				dev_kfree_skb_any(mdp->rx_skbuff[entry]);
> >+				mdp->rx_skbuff[entry] = NULL;
> >+				break;
> >+			}
> >  		}
> >  		if (entry >= mdp->num_rx_ring - 1)
> >  			rxdesc->status |> >@@ -2104,12 +2117,18 @@ static int sh_eth_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> >  	if (!mdp->cd->hw_swap)
> >  		sh_eth_soft_swap(phys_to_virt(ALIGN(txdesc->addr, 4)),
> >  				 skb->len + 2);
> >-	txdesc->addr = dma_map_single(&ndev->dev, skb->data, skb->len,
> >-				      DMA_TO_DEVICE);
> >  	if (skb->len < ETH_ZLEN)
> >  		txdesc->buffer_length = ETH_ZLEN;
> >  	else
> >  		txdesc->buffer_length = skb->len;
> >+	txdesc->addr = dma_map_single(&ndev->dev, skb->data,
> >+				      txdesc->buffer_length,
> >+				      DMA_TO_DEVICE);
> >+	if (dma_mapping_error(&ndev->dev, txdesc->addr)) {
> >+		dev_kfree_skb_any(mdp->tx_skbuff[entry]);
> >+		mdp->tx_skbuff[entry] = NULL;
> >+		goto out;
> 
>    Why not just *return*?!
> 
> [...]
> 
> WBR, Sergei
> 

Re: [PATCH 3/3] sh_eth: Fix dma mapping issue

[Sergei Shtylyov]

Hello.

On 11/17/2014 07:09 AM, Simon Horman wrote:

>>> From: Mitsuhiro Kimura <mitsuhiro.kimura.kc@renesas.com>

>>> When CONFIG_DMA_API_DEBUG=y, many DMA error messages reports.
>>> In order to use DMA debug, This patch fix following issues.

>>> Issue 1:
>>> If dma_mapping_error function is not called appropriately after
>>> DMA mapping, DMA debug will report error message when DMA unmap
>>> function is called.

>>> Issue 2:
>>> If skb_reserve function is called after DMA mapping, the relationship
>>> between mapping addr and mapping size will be broken.
>>> In this case, DMA debug will report error messages when DMA sync
>>> function and DMA unmap function are called.

>>> Issue 3:
>>> If the size of frame data is less than ETH_ZLEN, the size is resized
>>> to ETH_ZLEN after DMA map function is called.
>>> In the TX skb freeing function, dma unmap function is called with that
>>> resized value. So, unmap size error will reported.

>>> Issue 4:
>>> In the rx function, DMA map function is called without DMA unmap function
>>> is called for RX skb reallocating.
>>> It will case the DMA debug error that number of debug entry is full and
>>> DMA debug logic is stopped.

>>     The rule of thumb is "fix one issue per patch". Please split accordingly.

>>> Signed-off-by: Mitsuhiro Kimura <mitsuhiro.kimura.kc@renesas.com>
>>> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>

>>     Thanks for beating me to it. Fixing these issues has been on my agenda
>> for a long time... :-)

> as this patch is somewhat involved and as you have pointed out needs a bit
> of work I'm wondering if you could take it over.

    Perhaps I could... but I'm busy with other stuff... not sure when can I 
get to it.

[...]

WBR, Sergei