[PATCH net 1/4] sh_eth: Ensure proper ordering of descriptor active bit write/read

[PATCH net 1/4] sh_eth: Ensure proper ordering of descriptor active bit write/read

[Ben Hutchings]

When submitting a DMA descriptor, the active bit must be written last.
When reading a completed DMA descriptor, the active bit must be read
first.

Add memory barriers to ensure that this ordering is maintained.

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
This is based on review only - I don't have a test case to show
problematic reordering.

Ben.

 drivers/net/ethernet/renesas/sh_eth.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 4da8bd263997..2bc0be45c751 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -1407,6 +1407,8 @@ static int sh_eth_txfree(struct net_device *ndev)
 		txdesc = &mdp->tx_ring[entry];
 		if (txdesc->status & cpu_to_edmac(mdp, TD_TACT))
 			break;
+		/* TACT bit must be checked before all the following reads */
+		rmb();
 		/* Free the original skb. */
 		if (mdp->tx_skbuff[entry]) {
 			dma_unmap_single(&ndev->dev, txdesc->addr,
@@ -1444,6 +1446,8 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
 	limit = boguscnt;
 	rxdesc = &mdp->rx_ring[entry];
 	while (!(rxdesc->status & cpu_to_edmac(mdp, RD_RACT))) {
+		/* RACT bit must be checked before all the following reads */
+		rmb();
 		desc_status = edmac_to_cpu(mdp, rxdesc->status);
 		pkt_len = rxdesc->frame_length;
 
@@ -1523,6 +1527,7 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
 			skb_checksum_none_assert(skb);
 			rxdesc->addr = dma_addr;
 		}
+		wmb(); /* RACT bit must be set after all the above writes */
 		if (entry >= mdp->num_rx_ring - 1)
 			rxdesc->status |=
 				cpu_to_edmac(mdp, RD_RACT | RD_RFP | RD_RDEL);
@@ -2192,6 +2197,7 @@ static int sh_eth_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	}
 	txdesc->buffer_length = skb->len;
 
+	wmb(); /* TACT bit must be set after all the above writes */
 	if (entry >= mdp->num_tx_ring - 1)
 		txdesc->status |= cpu_to_edmac(mdp, TD_TACT | TD_TDLE);
 	else
-- 
1.7.10.4

[PATCH net 0/4] Fixes for sh_eth #4 v2

[Ben Hutchings]

I'm continuing review and testing of Ethernet support on the R-Car H2
chip, with help from a colleague.  This series fixes a few more issues.

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

v2: Add note that the revert is not a pure revert.

Ben Hutchings (4):
  sh_eth: Ensure proper ordering of descriptor active bit write/read
  sh_eth: Fix RX recovery on R-Car in case of RX ring underrun
  Revert "sh_eth: Enable Rx descriptor word 0 shift for r8a7790"
  sh_eth: Really fix padding of short frames on TX

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

-- 
1.7.10.4

[PATCH net 1/4] sh_eth: Ensure proper ordering of descriptor active bit write/read

[Ben Hutchings]

When submitting a DMA descriptor, the active bit must be written last.
When reading a completed DMA descriptor, the active bit must be read
first.

Add memory barriers to ensure that this ordering is maintained.

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

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 654b48d1e61a..5c212a833bcf 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -1410,6 +1410,8 @@ static int sh_eth_txfree(struct net_device *ndev)
 		txdesc = &mdp->tx_ring[entry];
 		if (txdesc->status & cpu_to_edmac(mdp, TD_TACT))
 			break;
+		/* TACT bit must be checked before all the following reads */
+		rmb();
 		/* Free the original skb. */
 		if (mdp->tx_skbuff[entry]) {
 			dma_unmap_single(&ndev->dev, txdesc->addr,
@@ -1447,6 +1449,8 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
 	limit = boguscnt;
 	rxdesc = &mdp->rx_ring[entry];
 	while (!(rxdesc->status & cpu_to_edmac(mdp, RD_RACT))) {
+		/* RACT bit must be checked before all the following reads */
+		rmb();
 		desc_status = edmac_to_cpu(mdp, rxdesc->status);
 		pkt_len = rxdesc->frame_length;
 
@@ -1526,6 +1530,7 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
 			skb_checksum_none_assert(skb);
 			rxdesc->addr = dma_addr;
 		}
+		wmb(); /* RACT bit must be set after all the above writes */
 		if (entry >= mdp->num_rx_ring - 1)
 			rxdesc->status |=
 				cpu_to_edmac(mdp, RD_RACT | RD_RFP | RD_RDEL);
@@ -2195,6 +2200,7 @@ static int sh_eth_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	}
 	txdesc->buffer_length = skb->len;
 
+	wmb(); /* TACT bit must be set after all the above writes */
 	if (entry >= mdp->num_tx_ring - 1)
 		txdesc->status |= cpu_to_edmac(mdp, TD_TACT | TD_TDLE);
 	else
-- 
1.7.10.4

[PATCH net 2/4] sh_eth: Fix RX recovery on R-Car in case of RX ring underrun

[Ben Hutchings]

In case of RX ring underrun (RDE), we attempt to reset the software
descriptor pointers (dirty_rx and cur_rx) to match where the hardware
will read the next descriptor from, as that might not be the first
dirty descriptor.  This relies on reading RDFAR, but that register
doesn't exist on all supported chips - specifically, not on the R-Car
chips.  This will result in unpredictable behaviour on those chips
after an RDE.

Make this pointer reset conditional and assume that it isn't needed on
the R-Car chips.  This fix also assumes that RDFAR is never exposed at
offset 0 in the memory map - this is currently true, and a subsequent
commit will fix the ambiguity between offset 0 and no-offset in the
register offset maps.

Fixes: 79fba9f51755 ("net: sh_eth: fix the rxdesc pointer when rx ...")
Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
 drivers/net/ethernet/renesas/sh_eth.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 5c212a833bcf..3309494d12ad 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -1543,7 +1543,7 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
 	/* If we don't need to check status, don't. -KDU */
 	if (!(sh_eth_read(ndev, EDRRR) & EDRRR_R)) {
 		/* fix the values for the next receiving if RDE is set */
-		if (intr_status & EESR_RDE) {
+		if (intr_status & EESR_RDE && mdp->reg_offset[RDFAR] != 0) {
 			u32 count = (sh_eth_read(ndev, RDFAR) -
 				     sh_eth_read(ndev, RDLAR)) >> 4;
 
-- 
1.7.10.4

[PATCH net 3/4] Revert "sh_eth: Enable Rx descriptor word 0 shift for r8a7790"

[Ben Hutchings]

This reverts commit fd9af07c3404ac9ecbd0d859563360f51ce1ffde.

The hardware manual states that the frame error and multicast bits are
copied to bits 9:0 of RD0, not bits 25:16.  I've tested that this is
true for RFS1 (CRC error), RFS3 (frame too short), RFS4 (frame too
long) and RFS8 (multicast).

Also adjust a comment to agree with this.

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
v2: Add note that this is not a pure revert.

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

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 3309494d12ad..3406cda57a45 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -508,7 +508,6 @@ static struct sh_eth_cpu_data r8a779x_data = {
 	.tpauser	= 1,
 	.hw_swap	= 1,
 	.rmiimode	= 1,
-	.shift_rd0	= 1,
 };
 
 static void sh_eth_set_rate_sh7724(struct net_device *ndev)
@@ -1462,8 +1461,8 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
 
 		/* In case of almost all GETHER/ETHERs, the Receive Frame State
 		 * (RFS) bits in the Receive Descriptor 0 are from bit 9 to
-		 * bit 0. However, in case of the R8A7740, R8A779x, and
-		 * R7S72100 the RFS bits are from bit 25 to bit 16. So, the
+		 * bit 0. However, in case of the R8A7740 and R7S72100
+		 * the RFS bits are from bit 25 to bit 16. So, the
 		 * driver needs right shifting by 16.
 		 */
 		if (mdp->cd->shift_rd0)
-- 
1.7.10.4

[PATCH net 4/4] sh_eth: Really fix padding of short frames on TX

[Ben Hutchings]

My previous fix to clear padding of short frames used skb->len as the
DMA length, assuming that skb_padto() extended skb->len to include the
padding.  That isn't the case; we need to use skb_put_padto() instead.

(This wasn't immediately obvious because software padding isn't
actually needed on the R-Car H2.  We could make it conditional on
which chip is being driven, but it's probably not worth the effort.)

Reported-by: "Violeta Menéndez González" <violeta.menendez@codethink.co.uk>
Fixes: 612a17a54b50 ("sh_eth: Fix padding of short frames on TX")
Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
 drivers/net/ethernet/renesas/sh_eth.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 3406cda57a45..736d5d1624a1 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -2181,7 +2181,7 @@ static int sh_eth_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	}
 	spin_unlock_irqrestore(&mdp->lock, flags);
 
-	if (skb_padto(skb, ETH_ZLEN))
+	if (skb_put_padto(skb, ETH_ZLEN))
 		return NETDEV_TX_OK;
 
 	entry = mdp->cur_tx % mdp->num_tx_ring;
-- 
1.7.10.4

Re: [PATCH net 0/4] Fixes for sh_eth #4 v2

[David Miller]

From: Ben Hutchings <ben.hutchings@codethink.co.uk>
Date: Tue, 03 Mar 2015 00:51:08 +0000

> I'm continuing review and testing of Ethernet support on the R-Car H2
> chip, with help from a colleague.  This series fixes a few more issues.
> 
> These are not tested on any of the other supported chips.
> 
> v2: Add note that the revert is not a pure revert.

Series applied, thanks Ben.

Re: [PATCH net 1/4] sh_eth: Ensure proper ordering of descriptor active bit write/read

[Sergei Shtylyov]

Hello.

On 03/03/2015 03:52 AM, Ben Hutchings wrote:

> When submitting a DMA descriptor, the active bit must be written last.
> When reading a completed DMA descriptor, the active bit must be read
> first.

> Add memory barriers to ensure that this ordering is maintained.

    Looks like those should have been lighter dma_rmb() and dma_wmb() instead.
Correct, Alexander?

> Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> ---
>   drivers/net/ethernet/renesas/sh_eth.c |    6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> index 654b48d1e61a..5c212a833bcf 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -1410,6 +1410,8 @@ static int sh_eth_txfree(struct net_device *ndev)
>   		txdesc = &mdp->tx_ring[entry];
>   		if (txdesc->status & cpu_to_edmac(mdp, TD_TACT))
>   			break;
> +		/* TACT bit must be checked before all the following reads */
> +		rmb();
>   		/* Free the original skb. */
>   		if (mdp->tx_skbuff[entry]) {
>   			dma_unmap_single(&ndev->dev, txdesc->addr,
> @@ -1447,6 +1449,8 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
>   	limit = boguscnt;
>   	rxdesc = &mdp->rx_ring[entry];
>   	while (!(rxdesc->status & cpu_to_edmac(mdp, RD_RACT))) {
> +		/* RACT bit must be checked before all the following reads */
> +		rmb();
>   		desc_status = edmac_to_cpu(mdp, rxdesc->status);
>   		pkt_len = rxdesc->frame_length;
>
> @@ -1526,6 +1530,7 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
>   			skb_checksum_none_assert(skb);
>   			rxdesc->addr = dma_addr;
>   		}
> +		wmb(); /* RACT bit must be set after all the above writes */
>   		if (entry >= mdp->num_rx_ring - 1)
>   			rxdesc->status |=
>   				cpu_to_edmac(mdp, RD_RACT | RD_RFP | RD_RDEL);
> @@ -2195,6 +2200,7 @@ static int sh_eth_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>   	}
>   	txdesc->buffer_length = skb->len;
>
> +	wmb(); /* TACT bit must be set after all the above writes */
>   	if (entry >= mdp->num_tx_ring - 1)
>   		txdesc->status |= cpu_to_edmac(mdp, TD_TACT | TD_TDLE);
>   	else

WBR, Sergei

Re: [PATCH net 1/4] sh_eth: Ensure proper ordering of descriptor active bit write/read

[Ben Hutchings]

On Fri, 2015-04-10 at 01:21 +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 03/03/2015 03:52 AM, Ben Hutchings wrote:
> 
> > When submitting a DMA descriptor, the active bit must be written last.
> > When reading a completed DMA descriptor, the active bit must be read
> > first.
> 
> > Add memory barriers to ensure that this ordering is maintained.
> 
>     Looks like those should have been lighter dma_rmb() and dma_wmb() instead.

Yes, I think so.  I missed those additions to the DMA API.

Ben.

> Correct, Alexander?
> 
> > Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> > ---
> >   drivers/net/ethernet/renesas/sh_eth.c |    6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> > index 654b48d1e61a..5c212a833bcf 100644
> > --- a/drivers/net/ethernet/renesas/sh_eth.c
> > +++ b/drivers/net/ethernet/renesas/sh_eth.c
> > @@ -1410,6 +1410,8 @@ static int sh_eth_txfree(struct net_device *ndev)
> >   		txdesc = &mdp->tx_ring[entry];
> >   		if (txdesc->status & cpu_to_edmac(mdp, TD_TACT))
> >   			break;
> > +		/* TACT bit must be checked before all the following reads */
> > +		rmb();
> >   		/* Free the original skb. */
> >   		if (mdp->tx_skbuff[entry]) {
> >   			dma_unmap_single(&ndev->dev, txdesc->addr,
> > @@ -1447,6 +1449,8 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
> >   	limit = boguscnt;
> >   	rxdesc = &mdp->rx_ring[entry];
> >   	while (!(rxdesc->status & cpu_to_edmac(mdp, RD_RACT))) {
> > +		/* RACT bit must be checked before all the following reads */
> > +		rmb();
> >   		desc_status = edmac_to_cpu(mdp, rxdesc->status);
> >   		pkt_len = rxdesc->frame_length;
> >
> > @@ -1526,6 +1530,7 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
> >   			skb_checksum_none_assert(skb);
> >   			rxdesc->addr = dma_addr;
> >   		}
> > +		wmb(); /* RACT bit must be set after all the above writes */
> >   		if (entry >= mdp->num_rx_ring - 1)
> >   			rxdesc->status |=
> >   				cpu_to_edmac(mdp, RD_RACT | RD_RFP | RD_RDEL);
> > @@ -2195,6 +2200,7 @@ static int sh_eth_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> >   	}
> >   	txdesc->buffer_length = skb->len;
> >
> > +	wmb(); /* TACT bit must be set after all the above writes */
> >   	if (entry >= mdp->num_tx_ring - 1)
> >   		txdesc->status |= cpu_to_edmac(mdp, TD_TACT | TD_TDLE);
> >   	else
> 
> WBR, Sergei
>