linux-kernel - Re: [PATCH net-next v2 3/3] net: stmmac: Add DWMAC glue layer for Renesas GBETH

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] [day] [month] [year] [list]

Message-ID: <CA+V-a8vGsWb6=30KuRp6huOHs0+K2pBdWPB0SuPPZjgWvZT3fA@mail.gmail.com>
Date: Sun, 9 Mar 2025 21:06:10 +0000
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Philipp Zabel <p.zabel@...gutronix.de>, Geert Uytterhoeven <geert+renesas@...der.be>, 
	Giuseppe Cavallaro <peppe.cavallaro@...com>, Jose Abreu <joabreu@...opsys.com>, 
	Alexandre Torgue <alexandre.torgue@...s.st.com>, netdev@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-renesas-soc@...r.kernel.org, Biju Das <biju.das.jz@...renesas.com>, 
	Fabrizio Castro <fabrizio.castro.jz@...esas.com>, 
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH net-next v2 3/3] net: stmmac: Add DWMAC glue layer for
 Renesas GBETH

Hi Russell,

On Sun, Mar 9, 2025 at 12:19 PM Russell King (Oracle)
<linux@...linux.org.uk> wrote:
>
> On Sun, Mar 09, 2025 at 11:24:57AM +0000, Lad, Prabhakar wrote:
> > Hi Russell,
> >
> > Thank you for the review.
> >
> > On Sun, Mar 9, 2025 at 8:50 AM Russell King (Oracle)
> > <linux@...linux.org.uk> wrote:
> > >
> > > On Sat, Mar 08, 2025 at 08:09:21PM +0000, Prabhakar wrote:
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> > > >
> > > > Add the DWMAC glue layer for the GBETH IP found in the Renesas RZ/V2H(P)
> > > > SoC.
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> > > > ---
> > > > v1->v2
> > > > - Dropped __initconst for renesas_gbeth_clks array
> > > > - Added clks_config callback
> > > > - Dropped STMMAC_FLAG_RX_CLK_RUNS_IN_LPI flag as this needs
> > > >   investigation.
> > >
> > > I thought you had got to the bottom of this, and it was a bug in your
> > > clock driver?
> > >
> > I have added a fix in the clock driver to ignore CLK_MON bits for
> > external clocks. The main reason for dropping this flag was despite
> > trying the below i.e. adding phy_eee_rx_clock_stop() just before
> > unregister_netdev() in stmmac_dvr_remove() still doesnt stop the Rx
> > clocks.
>
> That's not unexpected, because phy_eee_rx_clock_stop() does not control
> a clock gate.
>
> What phy_eee_rx_clock_stop() does is control the clock stop enable bit
> in the PHY. Please see IEEE 802.3 section 45.2.3.1.4 and other sections
> referred to from that section to gain the appropriate understanding.
>
> The point of adding the phy_eee_rx_clock_stop() call before
> stmmac_dvr_remove() was to _test_ (and *only as a test* - a point that
> I did stress) to see whether preventing the PHY from stopping it's
> receive clock solved the reset timeout on module reload. This test
> only makes sense if STMMAC_FLAG_RX_CLK_RUNS_IN_LPI has not been set.
>
Thank you for your clarification.

> As I understand it, you have found the real issue why that occurs, so
> it seems there is little need to continue with that test if, and only
> if, everything is now working reliably when removing and re-inserting
> the module.
>
> The key point here is "reliably". The receive side of the link *could*
> enter or exit LPI at *any* moment - it depends in the link partner. If
> the PHY has permission to stop it's receive clock, then this might lead
> to stmmac_reset() timing out because the PHY has stopped it's receive
> clock _if_ the receive-side LPI persists longer than the reset timeout.
>
> At this point, I am not certain what the current situation is. Are you
> now setting STMMAC_FLAG_RX_CLK_RUNS_IN_LPI because it solves a problem?
> If the answer is yes, then there is still a bug in the driver that needs
> to be solved and I've presented several solutions to that.
>
In this series I have dropped the STMMAC_FLAG_RX_CLK_RUNS_IN_LPI flag
with this unbind/bind works without any issues.

> I want to remove STMMAC_FLAG_RX_CLK_RUNS_IN_LPI from the stmmac driver
> so I'm going to NACK patches that add new uses. Sorry, but we need to
> solve the root problem, and stop hacking around it with flags to change
> behaviours.
>
I have retested the glue driver with and without
STMMAC_FLAG_RX_CLK_RUNS_IN_LPI flag and in both the cases I can see
the unbind/bind operation working without any timeouts (attached are
the logs).

Ive reworked your suggestions from v2, please let me know if I can
send out a v3 without STMMAC_FLAG_RX_CLK_RUNS_IN_LPI and with the new
STMMAC_FLAG_EN_TX_LPI_CLK_PHY_CAP flag added.

Cheers,
Prabhakar

View attachment "gbeth-rxlpi.txt" of type "text/plain" (89178 bytes)

View attachment "gbeth-no-rxlpi.txt" of type "text/plain" (89299 bytes)