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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AM0PR04MB6754887223A3BE7FD4A227DD969A9@AM0PR04MB6754.eurprd04.prod.outlook.com>
Date:   Mon, 1 Mar 2021 13:34:16 +0000
From:   Claudiu Manoil <claudiu.manoil@....com>
To:     Vladimir Oltean <olteanv@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     Michael Walle <michael@...le.cc>,
        Alexandru Marginean <alexandru.marginean@....com>,
        Andrew Lunn <andrew@...n.ch>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: RE: [PATCH v3 net 8/8] net: enetc: keep RX ring consumer index in
 sync with hardware

>-----Original Message-----
>From: Vladimir Oltean <olteanv@...il.com>
>Sent: Monday, March 1, 2021 1:18 PM
>To: David S . Miller <davem@...emloft.net>; Jakub Kicinski
><kuba@...nel.org>; netdev@...r.kernel.org
>Cc: Michael Walle <michael@...le.cc>; Claudiu Manoil
><claudiu.manoil@....com>; Alexandru Marginean
><alexandru.marginean@....com>; Andrew Lunn <andrew@...n.ch>;
>Vladimir Oltean <vladimir.oltean@....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@....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