>-----Original Message-----
>From: Vladimir Oltean <[email protected]>
>Sent: Monday, March 1, 2021 1:18 PM
>To: David S . Miller <[email protected]>; Jakub Kicinski
><[email protected]>; [email protected]
>Cc: Michael Walle <[email protected]>; Claudiu Manoil
><[email protected]>; Alexandru Marginean
><[email protected]>; Andrew Lunn <[email protected]>;
>Vladimir Oltean <[email protected]>
>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 <[email protected]>
>---
>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
