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

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

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

Ben.

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>
---
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 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>
---
I was able to trigger RDE by adding a udelay(10) to the loop in
sh_eth_rx() (limiting RX to <100,000 pps) and sending minimum size
frames with pktgen (~160,000 pps at 100M).  The RDE was recorded in the
netdev stats.  After the RDE and recover I could still pass traffic
successfully so no extra code appears to be needed for this chip.

Ben.

 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 2bc0be45c751..ed67951f5271 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -1540,7 +1540,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).

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
 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 ed67951f5271..317722e16043 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)
@@ -1459,8 +1458,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 317722e16043..07c537d900da 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -2178,7 +2178,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 3/4] Revert "sh_eth: Enable Rx descriptor word 0 shift for r8a7790"

[Ben Hutchings]

On Thu, 2015-02-26 at 14:21 +0000, Ben Hutchings wrote:
> 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).
[...]

This behaviour might of course be dependent on the revision of the chip.
I didn't find a chip ID register in the manual, but I found the recent
patch "ARM: shmobile: Add function to get SoCs revision data for R-Car
Gen2".  With that patch, /proc/cpuinfo on my test system shows:

Hardware        : Generic R8A7790 (Flattened Device Tree)
Revision        : 0020
Serial          : 0000000000000000

Ben.

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

[Sergei Shtylyov]

Hello.

On 02/26/2015 05:21 PM, Ben Hutchings wrote:

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

    Well, if your testing does match the manual, the reverted patch was most 
probably just wrong in the first place.

> Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> ---
>   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 ed67951f5271..317722e16043 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
[...]
> @@ -1459,8 +1458,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

    And that seems more logical to me, as we have the RFS bits starting with 
bit 16 only on the SoCs with the GEther compatible register layout (though the 
latter SoC doesn't support Gigabit speed).
    Having the RFS bits start at bit 16 is most probably connected to a SoC 
having support for hardware checksumming (bit 0-15 store the received frame 
checksum for at least R7S72100), so merging the 'shift_rd0' and 'hw_crc' flags 
seemed the reasonable next step to me (not taken due to the lack of 
documentation)...

>   		 * driver needs right shifting by 16.
>   		 */
>   		if (mdp->cd->shift_rd0)

    This hunk (inverted) was not a part of the commit you're reverting. 
Perhaps you shouldn't call this patch revert? Or make a remark about this hunk 
in the change-log?

WBR, Sergei

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

[Ben Hutchings]

On Thu, 2015-02-26 at 22:17 +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 02/26/2015 05:21 PM, Ben Hutchings wrote:
> 
> > 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).
> 
>     Well, if your testing does match the manual, the reverted patch was most 
> probably just wrong in the first place.
> 
> > Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> > ---
> >   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 ed67951f5271..317722e16043 100644
> > --- a/drivers/net/ethernet/renesas/sh_eth.c
> > +++ b/drivers/net/ethernet/renesas/sh_eth.c
> [...]
> > @@ -1459,8 +1458,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
> 
>     And that seems more logical to me, as we have the RFS bits starting with 
> bit 16 only on the SoCs with the GEther compatible register layout (though the 
> latter SoC doesn't support Gigabit speed).
>     Having the RFS bits start at bit 16 is most probably connected to a SoC 
> having support for hardware checksumming (bit 0-15 store the received frame 
> checksum for at least R7S72100), so merging the 'shift_rd0' and 'hw_crc' flags 
> seemed the reasonable next step to me (not taken due to the lack of 
> documentation)...

After this patch there will still be:

/* SH7757(GETHERC) */ .register_type = SH_ETH_REG_GIGABIT, .hw_crc = 0, .shift_rd0 = 0
/* SH7734 */          .register_type = SH_ETH_REG_GIGABIT, .hw_crc = 1, .shift_rd0 = 0
/* SH7763 */          .register_type = SH_ETH_REG_GIGABIT, .hw_crc = 0, .shift_rd0 = 0
/* R8A7740 */         .register_type = SH_ETH_REG_GIGABIT, .hw_crc = 0, .shift_rd0 = 1
/* R7S72100 */        .register_type = SH_ETH_REG_FAST_RZ, .hw_crc = 1, .shift_rd0 = 1

Do you really think R7S72100 is the only one of these with the flags set
correctly?

Also, the frame CRC is 32 bits and is surely checked by all versions of
the MAC.  Is the 16-bit 'CRC' actually a full-frame IP-style checksum?
Someone should make the driver actually use that where available.  (Not
me, I don't have one of those fancy checksumming chips.)

> >   		 * driver needs right shifting by 16.
> >   		 */
> >   		if (mdp->cd->shift_rd0)
> 
>     This hunk (inverted) was not a part of the commit you're reverting. 
> Perhaps you shouldn't call this patch revert? Or make a remark about this hunk 
> in the change-log?

Well, it's logically a revert.  I could mention that I'm also fixing a
comment to match.

Ben.

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

[Sergei Shtylyov]

On 02/26/2015 11:13 PM, Ben Hutchings wrote:

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

>>      Well, if your testing does match the manual, the reverted patch was most
>> probably just wrong in the first place.

>>> Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
>>> ---
>>>    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 ed67951f5271..317722e16043 100644
>>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
>> [...]
>>> @@ -1459,8 +1458,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

>>      And that seems more logical to me, as we have the RFS bits starting with
>> bit 16 only on the SoCs with the GEther compatible register layout (though the
>> latter SoC doesn't support Gigabit speed).
>>      Having the RFS bits start at bit 16 is most probably connected to a SoC
>> having support for hardware checksumming (bit 0-15 store the received frame
>> checksum for at least R7S72100), so merging the 'shift_rd0' and 'hw_crc' flags
>> seemed the reasonable next step to me (not taken due to the lack of
>> documentation)...

> After this patch there will still be:

> /* SH7757(GETHERC) */ .register_type = SH_ETH_REG_GIGABIT, .hw_crc = 0, .shift_rd0 = 0
> /* SH7734 */          .register_type = SH_ETH_REG_GIGABIT, .hw_crc = 1, .shift_rd0 = 0
> /* SH7763 */          .register_type = SH_ETH_REG_GIGABIT, .hw_crc = 0, .shift_rd0 = 0
> /* R8A7740 */         .register_type = SH_ETH_REG_GIGABIT, .hw_crc = 0, .shift_rd0 = 1
> /* R7S72100 */        .register_type = SH_ETH_REG_FAST_RZ, .hw_crc = 1, .shift_rd0 = 1

> Do you really think R7S72100 is the only one of these with the flags set
> correctly?

    I can't be certain since I only have R7S72100 manual but extrapolating it 
to other SoCs seemed reasonable enough. The driver itself doesn't support 
checksum offload, so the 'hw_crc' flag have little value currently, I think.

> Also, the frame CRC is 32 bits and is surely checked by all versions of
> the MAC.
> Is the 16-bit 'CRC' actually a full-frame IP-style checksum?

    I didn't mean frame CRC, I did mean (probably) IP packet checksum (which 
is 16-bit indeed). The flag name seems just wrong.

> Someone should make the driver actually use that where available.  (Not
> me, I don't have one of those fancy checksumming chips.)

    I don't (yet) have access to R7S72100 either, let alone the older SoCs...

>>>    		 * driver needs right shifting by 16.
>>>    		 */
>>>    		if (mdp->cd->shift_rd0)

>>      This hunk (inverted) was not a part of the commit you're reverting.
>> Perhaps you shouldn't call this patch revert? Or make a remark about this hunk
>> in the change-log?

> Well, it's logically a revert.  I could mention that I'm also fixing a
> comment to match.

    Yes, please.

> Ben.

WBR, Sergei

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

[Sergei Shtylyov]

Hello.

On 02/26/2015 05:19 PM, Ben Hutchings wrote:

> 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>
> ---
> I was able to trigger RDE by adding a udelay(10) to the loop in
> sh_eth_rx() (limiting RX to <100,000 pps) and sending minimum size
> frames with pktgen (~160,000 pps at 100M).  The RDE was recorded in the
> netdev stats.  After the RDE and recover I could still pass traffic
> successfully so no extra code appears to be needed for this chip.

    Thank you for at last tackling this; it was on my agenda for a long time...

WBR, Sergei

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

[David Miller]

From: Ben Hutchings <ben.hutchings@codethink.co.uk>
Date: Thu, 26 Feb 2015 14:12:17 +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.

Looks like there will be one more spin of these changes.

So I'll wait for #5

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

[David Miller]

From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Thu, 26 Feb 2015 23:35:12 +0300

> On 02/26/2015 11:13 PM, Ben Hutchings wrote:
>> Well, it's logically a revert.  I could mention that I'm also fixing a
>> comment to match.
> 
>    Yes, please.

Ok, then I'm waiting for a respin of this series.