RE: [PATCH v3 net 8/8] net: enetc: keep RX ring consumer index in sync with hardware

[Claudiu Manoil <claudiu.manoil@nxp.com>]

>-----Original Message-----
>From: Vladimir Oltean <olteanv@gmail.com>
>Sent: Monday, March 1, 2021 1:18 PM
>To: David S . Miller <davem@davemloft.net>; Jakub Kicinski
><kuba@kernel.org>; netdev@vger.kernel.org
>Cc: Michael Walle <michael@walle.cc>; Claudiu Manoil
><claudiu.manoil@nxp.com>; Alexandru Marginean
><alexandru.marginean@nxp.com>; Andrew Lunn <andrew@lunn.ch>;
>Vladimir Oltean <vladimir.oltean@nxp.com>
>Subject: [PATCH v3 net 8/8] net: enetc: keep RX ring consumer index in sync
>with hardware
>

Hi Vladimir,

>
>Fixes: d4fd0404c1c9 ("enetc: Introduce basic PF and VF ENETC ethernet
>drivers")

I just realized I introduced this regression with the MDIO workaround patch.

I you look at the initial code from d4fd0404c1c9 ("enetc: Introduce basic PF and VF ENETC ethernet
drivers") , the consumer index is being updated with the value of next_to_use inside enetc_refill_rx_ring():
static int enetc_refill_rx_ring(struct enetc_bdr *rx_ring, const int buff_cnt)
{
[...]
       if (likely(j)) {
               rx_ring->next_to_alloc = i; /* keep track from page reuse */
               rx_ring->next_to_use = i;
               /* update ENETC's consumer index */
               enetc_wr_reg(rx_ring->rcir, i);
       }

       return j;
}
See:
https://elixir.bootlin.com/linux/v5.4.101/source/drivers/net/ethernet/freescale/enetc/enetc.c#L434

enetc_refill_rx_ring() being called on both data path and init path (enetc_setup_rxbdr).

With commit fd5736bf9f23 ("enetc: Workaround for MDIO register access issue") I messed this up:

I moved this update outside refill_rx_ring():

@@ -515,8 +533,6 @@ static int enetc_refill_rx_ring(struct enetc_bdr *rx_ring, const int buff_cnt)
        if (likely(j)) {
                rx_ring->next_to_alloc = i; /* keep track from page reuse */
                rx_ring->next_to_use = i;
-               /* update ENETC's consumer index */
-               enetc_wr_reg(rx_ring->rcir, i);
        }
[....]

Updated the data path side accordingly (changing update to the new accessor) :

@@ -684,23 +700,31 @@ static int enetc_clean_rx_ring(struct enetc_bdr *rx_ring,
                u32 bd_status;
                u16 size;

+               enetc_lock_mdio();
+
                if (cleaned_cnt >= ENETC_RXBD_BUNDLE) {
                        int count = enetc_refill_rx_ring(rx_ring, cleaned_cnt);

+                       /* update ENETC's consumer index */
+                       enetc_wr_reg_hot(rx_ring->rcir, rx_ring->next_to_use);
                        cleaned_cnt -= count;
                }
[...]

But on the init path I messed it up likely due to some merge conflict:

@@ -1225,6 +1252,7 @@ static void enetc_setup_rxbdr(struct enetc_hw *hw, struct enetc_bdr *rx_ring)
        rx_ring->idr = hw->reg + ENETC_SIRXIDR;

        enetc_refill_rx_ring(rx_ring, enetc_bd_unused(rx_ring));
+       enetc_wr(hw, ENETC_SIRXIDR, rx_ring->next_to_use);

Instead of:
	enetc_wr_reg(rx_ring->rcir, rx_ring->next_to_use);
	or  enetc_rxbdr_wr(hw, idx, ENETC_RBCIR, rx_ring->next_to_use); ... if you prefer.

Obviously writing to ENETC_SIRXIDR makes no sense, and just shows that something went wrong with that commit.

So the blamed commit for this is: fd5736bf9f23 ("enetc: Workaround for MDIO register access issue").

And you could merge patches 7/8 and 8/8, as they both deal with fixing the (merge conflict) regression
that I introduced with the MDIO w/a patch:

Fixes: fd5736bf9f23 ("enetc: Workaround for MDIO register access issue").

Sorry for all this trouble.

Thanks,
Claudiu

>Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
>---
>Changes in v3:
>Patch is new.
>
> drivers/net/ethernet/freescale/enetc/enetc.c | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c
>b/drivers/net/ethernet/freescale/enetc/enetc.c
>index abb29ee81463..30d7d4e83900 100644
>--- a/drivers/net/ethernet/freescale/enetc/enetc.c
>+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
>@@ -1212,6 +1212,8 @@ static void enetc_setup_rxbdr(struct enetc_hw
>*hw, struct enetc_bdr *rx_ring)
> 	rx_ring->idr = hw->reg + ENETC_SIRXIDR;
>
> 	enetc_refill_rx_ring(rx_ring, enetc_bd_unused(rx_ring));
>+	/* update ENETC's consumer index */
>+	enetc_rxbdr_wr(hw, idx, ENETC_RBCIR, rx_ring->next_to_use);
>
> 	/* enable ring */
> 	enetc_rxbdr_wr(hw, idx, ENETC_RBMR, rbmr);
>--
>2.25.1