netdev - Re: [EXT] Re: [PATCH] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link

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]
Date: Wed, 26 Jul 2023 18:01:10 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Shenwei Wang <shenwei.wang@....com>
Cc: Vladimir Oltean <olteanv@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Maxime Coquelin <mcoquelin.stm32@...il.com>,
	Shawn Guo <shawnguo@...nel.org>, dl-linux-imx <linux-imx@....com>,
	Giuseppe Cavallaro <peppe.cavallaro@...com>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Jose Abreu <joabreu@...opsys.com>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-stm32@...md-mailman.stormreply.com" <linux-stm32@...md-mailman.stormreply.com>,
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>,
	"imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	Frank Li <frank.li@....com>
Subject: Re: [EXT] Re: [PATCH] net: stmmac: dwmac-imx: pause the TXC clock in
 fixed-link

On Wed, Jul 26, 2023 at 03:59:38PM +0000, Shenwei Wang wrote:
> > -----Original Message-----
> > From: Russell King <linux@...linux.org.uk>
> > Sent: Wednesday, July 26, 2023 10:29 AM
> > To: Shenwei Wang <shenwei.wang@....com>
> > Cc: Vladimir Oltean <olteanv@...il.com>; David S. Miller
> > <davem@...emloft.net>; Eric Dumazet <edumazet@...gle.com>; Jakub
> > Kicinski <kuba@...nel.org>; Paolo Abeni <pabeni@...hat.com>; Maxime
> > Coquelin <mcoquelin.stm32@...il.com>; Shawn Guo <shawnguo@...nel.org>;
> > dl-linux-imx <linux-imx@....com>; Giuseppe Cavallaro
> > <peppe.cavallaro@...com>; Alexandre Torgue <alexandre.torgue@...s.st.com>;
> > Jose Abreu <joabreu@...opsys.com>; Sascha Hauer <s.hauer@...gutronix.de>;
> > Pengutronix Kernel Team <kernel@...gutronix.de>; Fabio Estevam
> > <festevam@...il.com>; netdev@...r.kernel.org; linux-stm32@...md-
> > mailman.stormreply.com; linux-arm-kernel@...ts.infradead.org;
> > imx@...ts.linux.dev; Frank Li <frank.li@....com>
> > Subject: Re: [EXT] Re: [PATCH] net: stmmac: dwmac-imx: pause the TXC clock in
> > fixed-link
> >
> > Caution: This is an external email. Please take care when clicking links or
> > opening attachments. When in doubt, report the message using the 'Report this
> > email' button
> >
> >
> > On Wed, Jul 26, 2023 at 03:10:19PM +0000, Shenwei Wang wrote:
> > > > if (of_phy_is_fixed_link(dwmac->dev->of_node)) {
> > > >
> > >
> > > This does not help in this case. What I need to determine is if the PHY currently
> > in use is a fixed-link.
> > > The dwmac DTS node may have multiple PHY nodes defined, including both
> > fixed-link and real PHYs.
> >
> > ... and this makes me wonder what DT node structure you think would describe a
> > fixed-link.
> >
> > A valid ethernet device node would be:
> >
> >         dwmac-node {
> >                 phy-handle = <&phy1>;
> >         };
> >
> > In this case:
> >         dwmac->dev->of_node points at "dwmac-node"
> >         plat->phylink_node points at "dwmac-node"
> >         plat->phy_node points at "phy1"
> >         Your "dn" is NULL.
> >         Therefore, your imx_dwmac_is_fixed_link() returns false.
> >
> >         dwmac-node {
> >                 fixed-link {
> >                         speed = <...>;
> >                         full-duplex;
> >                 };
> >         };
> >
> > In this case:
> >         dwmac->dev->of_node points at "dwmac-node"
> >         plat->phylink_node points at "dwmac-node"
> >         plat->phy_node is NULL
> >         Your "dn" points at the "fixed-link" node.
> >         Therefore, your imx_dwmac_is_fixed_link() also returns false.
> >
> > Now, as far as your comment "What I need to determine is if the PHY currently
> > in use is a fixed-link." I'm just going "Eh? What?" at that, because it makes zero
> > sense to me.
> >
> > stmmac uses phylink. phylink doesn't use a PHY for fixed-links, unlike the old
> > phylib-based fixed-link implementation that software-emulated a clause-22 PHY.
> > With phylink, when fixed-link is specified, there is _no_ PHY.
> 
> So you mean the fixed-link node will always be the highest priority to
> be used in the phylink use case?

Yes, because that is how all network drivers have behaved. If you look
at the function that Vladimir pointed out, then you will notice that
the mere presence of a fixed-link node makes it a "fixed link".

> If so, I just need to check if there is a fixed-link node as Vladimir pointed out, right?

You could, but that is grossly inefficient, and I will NAK it because
by doing so, it makes this messy driver even worse.

> > There is no need to do any of this poking about to determine if the link that is
> > being brought up is a fixed-link or not, because phylink's callbacks into the MAC
> > driver already contain this information in the "mode" argument. However, that
> > is not passed to the driver's internal
> > priv->plat->fix_mac_speed() method - but this is the information you
> > need.
> >
> 
> Yes, you are right. The best way is to change the fix_mac_speed prototype
> but it will change several other platforms. That's why I didn't go that way.

Why is that a problem?

I really don't get this "I can't get at information I need without
changing a driver internal interface, so I'll write some really
inefficient code to work around the problem and make the driver
even more messy" attitude.

It's not like you're changing a publicly visible API - it's a
driver private API and all the users of it are in the kernel tree.

A standard part of open source development is not to bodge around
existing code, but to implement efficient solutions to problems.

As phylink *already* tells stmmac_mac_link_up() whether it is
operating with a PHY, fixed-link, or in-band mode, the stmmac
layer has the information you need, but doesn't pass this into
the fix_mac_speed() function.

The best solution to this is *not* to bodge around it by trying
to second-guess what's going on and thus creating messy code.

Given that we have the full source available which we can modify,
then changing things like this function pointer prototype is
absolutely acceptable, and in this case is the correct way to
address the issue you have.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!