[PATCH kernel 0/4] Fixes for sh_eth #3

[PATCH kernel 0/4] Fixes for sh_eth #3

[Ben Hutchings]

I'm continuing review and testing of Ethernet support on the R-Car H2
chip.  This series fixes the last of the more serious issues I've found.

These are not tested on any of the other supported chips.

Ben.

Ben Hutchings (4):
  sh_eth: Remove RX overflow log messages
  sh_eth: Ensure DMA engines are stopped before freeing buffers
  sh_eth: Check for DMA mapping errors on transmit
  sh_eth: Fix DMA-API usage for RX buffers

 drivers/net/ethernet/renesas/sh_eth.c |   80 ++++++++++++++++++++++++---------
 1 file changed, 59 insertions(+), 21 deletions(-)

-- 
1.7.10.4

[PATCH kernel 1/4] sh_eth: Remove RX overflow log messages

[Ben Hutchings]

If RX traffic is overflowing the FIFO or DMA ring, logging every time
this happens just makes things worse.  These errors are visible in the
statistics anyway.

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
 drivers/net/ethernet/renesas/sh_eth.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 2c4820a..3b49375 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -1575,7 +1575,6 @@ ignore_link:
 		if (intr_status & EESR_RFRMER) {
 			/* Receive Frame Overflow int */
 			ndev->stats.rx_frame_errors++;
-			netif_err(mdp, rx_err, ndev, "Receive Abort\n");
 		}
 	}
 
@@ -1594,13 +1593,11 @@ ignore_link:
 	if (intr_status & EESR_RDE) {
 		/* Receive Descriptor Empty int */
 		ndev->stats.rx_over_errors++;
-		netif_err(mdp, rx_err, ndev, "Receive Descriptor Empty\n");
 	}
 
 	if (intr_status & EESR_RFE) {
 		/* Receive FIFO Overflow int */
 		ndev->stats.rx_fifo_errors++;
-		netif_err(mdp, rx_err, ndev, "Receive FIFO Overflow\n");
 	}
 
 	if (!mdp->cd->no_ade && (intr_status & EESR_ADE)) {
-- 
1.7.10.4

[PATCH kernel 2/4] sh_eth: Ensure DMA engines are stopped before freeing buffers

[Ben Hutchings]

Currently we try to clear EDRRR and EDTRR and immediately continue to
free buffers.  This is unsafe because:

- In general, register writes are not serialised with DMA, so we still
  have to wait for DMA to complete somehow
- The R8A7790 (R-Car H2) manual states that the TX running flag cannot
  be cleared by writing to EDTRR
- The same manual states that clearing the RX running flag only stops
  RX DMA at the next packet boundary

I applied this patch to the driver to detect DMA writes to freed
buffers:

> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -1098,7 +1098,14 @@ static void sh_eth_ring_free(struct net_device *ndev)
>  	/* Free Rx skb ringbuffer */
>  	if (mdp->rx_skbuff) {
>  		for (i = 0; i < mdp->num_rx_ring; i++)
> +			memcpy(mdp->rx_skbuff[i]->data,
> +			       "Hello, world", 12);
> +		msleep(100);
> +		for (i = 0; i < mdp->num_rx_ring; i++) {
> +			WARN_ON(memcmp(mdp->rx_skbuff[i]->data,
> +				       "Hello, world", 12));
>  			dev_kfree_skb(mdp->rx_skbuff[i]);
> +		}
>  	}
>  	kfree(mdp->rx_skbuff);
>  	mdp->rx_skbuff = NULL;

then ran the loop:

    while ethtool -G eth0 rx 128 ; ethtool -G eth0 rx 64; do echo -n .; done

and 'ping -f' toward the sh_eth port from another machine.  The
warning fired several times a minute.

To fix these issues:

- Deactivate all TX descriptors rather than writing to EDTRR
- As there seems to be no way of telling when RX DMA is stopped,
  perform a soft reset to ensure that both DMA enginess are stopped
- To reduce the possibility of the reset truncating a transmitted
  frame, disable egress and wait a reasonable time to reach a
  packet boundary before resetting
- Update statistics before resetting

(The 'reasonable time' does not allow for CS/CD in half-duplex
mode, but half-duplex no longer seems reasonable!)

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
I would prefer to find a way to wait for the DMA engines to stop, so
that it's only necessary to reset if they fail to do so within some time
limit.  But I just don't see it.

Ben.

 drivers/net/ethernet/renesas/sh_eth.c |   39 +++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 3b49375..e43c9ce 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -396,6 +396,9 @@ static const u16 sh_eth_offset_fast_sh3_sh2[SH_ETH_MAX_REGISTER_OFFSET] = {
 	[TSU_ADRL31]	= 0x01fc,
 };
 
+static void sh_eth_rcv_snd_disable(struct net_device *ndev);
+static struct net_device_stats *sh_eth_get_stats(struct net_device *ndev);
+
 static bool sh_eth_is_gether(struct sh_eth_private *mdp)
 {
 	return mdp->reg_offset == sh_eth_offset_gigabit;
@@ -1358,6 +1361,33 @@ static int sh_eth_dev_init(struct net_device *ndev, bool start)
 	return ret;
 }
 
+static void sh_eth_dev_exit(struct net_device *ndev)
+{
+	struct sh_eth_private *mdp = netdev_priv(ndev);
+	int i;
+
+	/* Deactivate all TX descriptors, so DMA should stop at next
+	 * packet boundary if it's currently running
+	 */
+	for (i = 0; i < mdp->num_tx_ring; i++)
+		mdp->tx_ring[i].status &= ~cpu_to_edmac(mdp, TD_TACT);
+
+	/* Disable TX FIFO egress to MAC */
+	sh_eth_rcv_snd_disable(ndev);
+
+	/* Stop RX DMA at next packet boundary */
+	sh_eth_write(ndev, 0, EDRRR);
+
+	/* Aside from TX DMA, we can't tell when the hardware is
+	 * really stopped, so we need to reset to make sure.
+	 * Before doing that, wait for long enough to *probably*
+	 * finish transmitting the last packet and poll stats.
+	 */
+	msleep(2); /* max frame time at 10 Mbps < 1250 us */
+	sh_eth_get_stats(ndev);
+	sh_eth_reset(ndev);
+}
+
 /* free Tx skb function */
 static int sh_eth_txfree(struct net_device *ndev)
 {
@@ -1986,9 +2016,7 @@ static int sh_eth_set_ringparam(struct net_device *ndev,
 		napi_synchronize(&mdp->napi);
 		sh_eth_write(ndev, 0x0000, EESIPR);
 
-		/* Stop the chip's Tx and Rx processes. */
-		sh_eth_write(ndev, 0, EDTRR);
-		sh_eth_write(ndev, 0, EDRRR);
+		sh_eth_dev_exit(ndev);
 
 		/* Free all the skbuffs in the Rx queue. */
 		sh_eth_ring_free(ndev);
@@ -2207,11 +2235,8 @@ static int sh_eth_close(struct net_device *ndev)
 	napi_disable(&mdp->napi);
 	sh_eth_write(ndev, 0x0000, EESIPR);
 
-	/* Stop the chip's Tx and Rx processes. */
-	sh_eth_write(ndev, 0, EDTRR);
-	sh_eth_write(ndev, 0, EDRRR);
+	sh_eth_dev_exit(ndev);
 
-	sh_eth_get_stats(ndev);
 	/* PHY Disconnect */
 	if (mdp->phydev) {
 		phy_stop(mdp->phydev);
-- 
1.7.10.4

[PATCH kernel 3/4] sh_eth: Check for DMA mapping errors on transmit

[Ben Hutchings]

dma_map_single() may fail if an IOMMU or swiotlb is in use, so
we need to check for this.

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
 drivers/net/ethernet/renesas/sh_eth.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index e43c9ce..48610ed 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -2174,6 +2174,10 @@ static int sh_eth_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 				 skb->len + 2);
 	txdesc->addr = dma_map_single(&ndev->dev, skb->data, skb->len,
 				      DMA_TO_DEVICE);
+	if (dma_mapping_error(&ndev->dev, txdesc->addr)) {
+		kfree_skb(skb);
+		return NETDEV_TX_OK;
+	}
 	txdesc->buffer_length = skb->len;
 
 	if (entry >= mdp->num_tx_ring - 1)
-- 
1.7.10.4

[PATCH kernel 4/4] sh_eth: Fix DMA-API usage for RX buffers

[Ben Hutchings]

- Use the return value of dma_map_single(), rather than calling
  virt_to_page() separately
- Check for mapping failue
- Call dma_unmap_single() rather than dma_sync_single_for_cpu()

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
 drivers/net/ethernet/renesas/sh_eth.c |   34 ++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 48610ed..4da8bd2 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -1123,6 +1123,7 @@ static void sh_eth_ring_format(struct net_device *ndev)
 	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;
+	dma_addr_t dma_addr;
 
 	mdp->cur_rx = 0;
 	mdp->cur_tx = 0;
@@ -1136,7 +1137,6 @@ static void sh_eth_ring_format(struct net_device *ndev)
 		/* skb */
 		mdp->rx_skbuff[i] = NULL;
 		skb = netdev_alloc_skb(ndev, skbuff_size);
-		mdp->rx_skbuff[i] = skb;
 		if (skb == NULL)
 			break;
 		sh_eth_set_receive_align(skb);
@@ -1145,9 +1145,15 @@ static void sh_eth_ring_format(struct net_device *ndev)
 		rxdesc = &mdp->rx_ring[i];
 		/* The size of the buffer is a multiple of 16 bytes. */
 		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);
+		dma_addr = dma_map_single(&ndev->dev, skb->data,
+					  rxdesc->buffer_length,
+					  DMA_FROM_DEVICE);
+		if (dma_mapping_error(&ndev->dev, dma_addr)) {
+			kfree_skb(skb);
+			break;
+		}
+		mdp->rx_skbuff[i] = skb;
+		rxdesc->addr = dma_addr;
 		rxdesc->status = cpu_to_edmac(mdp, RD_RACT | RD_RFP);
 
 		/* Rx descriptor address set */
@@ -1432,6 +1438,7 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
 	u16 pkt_len = 0;
 	u32 desc_status;
 	int skbuff_size = mdp->rx_buf_sz + SH_ETH_RX_ALIGN - 1;
+	dma_addr_t dma_addr;
 
 	boguscnt = min(boguscnt, *quota);
 	limit = boguscnt;
@@ -1479,9 +1486,9 @@ 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_sync_single_for_cpu(&ndev->dev, rxdesc->addr,
-						ALIGN(mdp->rx_buf_sz, 16),
-						DMA_FROM_DEVICE);
+			dma_unmap_single(&ndev->dev, rxdesc->addr,
+					 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);
@@ -1501,15 +1508,20 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
 
 		if (mdp->rx_skbuff[entry] == NULL) {
 			skb = netdev_alloc_skb(ndev, skbuff_size);
-			mdp->rx_skbuff[entry] = skb;
 			if (skb == NULL)
 				break;	/* Better luck next round. */
 			sh_eth_set_receive_align(skb);
-			dma_map_single(&ndev->dev, skb->data,
-				       rxdesc->buffer_length, DMA_FROM_DEVICE);
+			dma_addr = dma_map_single(&ndev->dev, skb->data,
+						  rxdesc->buffer_length,
+						  DMA_FROM_DEVICE);
+			if (dma_mapping_error(&ndev->dev, dma_addr)) {
+				kfree_skb(skb);
+				break;
+			}
+			mdp->rx_skbuff[entry] = skb;
 
 			skb_checksum_none_assert(skb);
-			rxdesc->addr = virt_to_phys(skb->data);
+			rxdesc->addr = dma_addr;
 		}
 		if (entry >= mdp->num_rx_ring - 1)
 			rxdesc->status |=
-- 
1.7.10.4

Re: [PATCH kernel 4/4] sh_eth: Fix DMA-API usage for RX buffers

[Sergei Shtylyov]

Hello.

On 01/27/2015 03:50 AM, Ben Hutchings wrote:

> - Use the return value of dma_map_single(), rather than calling
>    virt_to_page() separately
> - Check for mapping failue
> - Call dma_unmap_single() rather than dma_sync_single_for_cpu()

> Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>

    I wonder whether you're aware at the previous Renesas' attemept at fixing 
DMA issues:

http://marc.info/?t=141586251000003

    It was turned down because they tried to deal with several issues in one 
patch, and they have failed to follow up. Could you look at the rest of the 
DMA issues reported (and not reported) in that patch?

[...]

WBR, Sergei

Re: [PATCH kernel 4/4] sh_eth: Fix DMA-API usage for RX buffers

[Ben Hutchings]

On Fri, 2015-02-27 at 23:36 +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 01/27/2015 03:50 AM, Ben Hutchings wrote:
> 
> > - Use the return value of dma_map_single(), rather than calling
> >    virt_to_page() separately
> > - Check for mapping failue
> > - Call dma_unmap_single() rather than dma_sync_single_for_cpu()
> 
> > Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> 
>     I wonder whether you're aware at the previous Renesas' attemept at fixing 
> DMA issues:
> 
> http://marc.info/?t=141586251000003

No, I only started looking at R-Car stuff in December.

>     It was turned down because they tried to deal with several issues in one 
> patch, and they have failed to follow up. Could you look at the rest of the 
> DMA issues reported (and not reported) in that patch?

Issue 1 should be fixed by "sh_eth: Check for DMA mapping errors on
transmit" and this one.

I don't think that issue 2 still applies but I also don't see how it was
fixed, so maybe I'm missing something.

Issue 3 should have been fixed by "sh_eth: Fix padding of short frames
on TX" except that I used the wrong padding function, so it also needs
"sh_eth: Really fix padding of short frames on TX".

Issue 4 should be fixed by this one.  Except that I forgot to add
unmapping in sh_eth_ring_free(), so RX DMA mappings are still leaked
when the device is taken down.

Ben.

Re: [Linux-kernel] [PATCH kernel 0/4] Fixes for sh_eth #3

[Ben Hutchings]

On Tue, 2015-01-27 at 00:40 +0000, Ben Hutchings wrote:
> I'm continuing review and testing of Ethernet support on the R-Car H2
> chip.  This series fixes the last of the more serious issues I've found.
> 
> These are not tested on any of the other supported chips.

Excuse me, that subject prefix should be 'PATCH net'.

Ben.

Re: [PATCH kernel 0/4] Fixes for sh_eth #3

[David Miller]

From: Ben Hutchings <ben.hutchings@codethink.co.uk>
Date: Tue, 27 Jan 2015 00:40:32 +0000

> I'm continuing review and testing of Ethernet support on the R-Car H2
> chip.  This series fixes the last of the more serious issues I've found.
> 
> These are not tested on any of the other supported chips.

Series applied, thanks Ben.