linux-kernel - RE: [PATCH v3 net] net: phy: micrel: Dynamically control external clock of KSZ PHY

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:
 <PAXPR04MB85104EC1BFE4075DF1A27B93883D2@PAXPR04MB8510.eurprd04.prod.outlook.com>
Date: Tue, 10 Dec 2024 02:45:57 +0000
From: Wei Fang <wei.fang@....com>
To: Jakub Kicinski <kuba@...nel.org>
CC: "andrew@...n.ch" <andrew@...n.ch>, "hkallweit1@...il.com"
	<hkallweit1@...il.com>, "linux@...linux.org.uk" <linux@...linux.org.uk>,
	"davem@...emloft.net" <davem@...emloft.net>, "edumazet@...gle.com"
	<edumazet@...gle.com>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"florian.fainelli@...adcom.com" <florian.fainelli@...adcom.com>,
	"heiko.stuebner@...rry.de" <heiko.stuebner@...rry.de>, "fank.li@....com"
	<fank.li@....com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"imx@...ts.linux.dev" <imx@...ts.linux.dev>
Subject: RE: [PATCH v3 net] net: phy: micrel: Dynamically control external
 clock of KSZ PHY

> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: 2024年12月10日 10:15
> To: Wei Fang <wei.fang@....com>
> Cc: andrew@...n.ch; hkallweit1@...il.com; linux@...linux.org.uk;
> davem@...emloft.net; edumazet@...gle.com; pabeni@...hat.com;
> florian.fainelli@...adcom.com; heiko.stuebner@...rry.de; fank.li@....com;
> netdev@...r.kernel.org; linux-kernel@...r.kernel.org; imx@...ts.linux.dev
> Subject: Re: [PATCH v3 net] net: phy: micrel: Dynamically control external
> clock of KSZ PHY
>
> On Fri,  6 Dec 2024 09:21:13 +0800 Wei Fang wrote:
> > On the i.MX6ULL-14x14-EVK board, enet1_ref and enet2_ref are used as
> > the clock sources for two external KSZ PHYs. However, after closing
> > the two FEC ports, the clk_enable_count of the enet1_ref and enet2_ref
> > clocks is not 0. The root cause is that since the commit 985329462723 ("net:
> phy:
> > micrel: use devm_clk_get_optional_enabled for the rmii-ref clock"),
> > the external clock of KSZ PHY has been enabled when the PHY driver
> > probes, and it can only be disabled when the PHY driver is removed.
> > This causes the clock to continue working when the system is suspended
> > or the network port is down.
> >
> > To solve this problem, the clock is enabled when phy_driver::resume()
> > is called, and the clock is disabled when phy_driver::suspend() is called.
> > Since phy_driver::resume() and phy_driver::suspend() are not called in
> > pairs, an additional clk_enable flag is added. When
> > phy_driver::suspend() is called, the clock is disabled only if
> > clk_enable is true. Conversely, when phy_driver::resume() is called,
> > the clock is enabled if clk_enable is false.
>
> Sorry that nobody replied to you but yes, I believe the simpler fix you proposed
> here:
> https://lore.ker/
> nel.org%2Fall%2FPAXPR04MB8510D36DDA1B9E98B2FB77B488362%40PAXPR
> 04MB8510.eurprd04.prod.outlook.com%2F&data=05%7C02%7Cwei.fang%40n
> xp.com%7Cda6de7c31d2a4f93267608dd18c06fa1%7C686ea1d3bc2b4c6fa92
> cd99c5c301635%7C0%7C0%7C638693936967952515%7CUnknown%7CTWF
> pbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4z
> MiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=YjR%2Fupk
> pye%2FmMXCH02HNEUiot1kGaD1xkeUEO9hY8P0%3D&reserved=0
> is better for net. In net-next we can try to keep the clock enabled and/or try to
> fix the imbalance in resume calls that forces you to track manually if the clock
> was enabled.

Hi Jakub,

The simple fix could only fix the commit 985329462723 ("net: phy: micrel: use
devm_clk_get_optional_enabled for the rmii-ref clock"), because as the commit
message said some clock suppliers need to be enabled so that the driver can get
the correct clock rate.

But the problem is that the simple fix cannot fix the 99ac4cbcc2a5 ("net: phy:
micrel: allow usage of generic ethernet-phy clock"). The change is as follows,
this change just enables the clock when the PHY driver probes. There are no
other operations on the clock, such as obtaining the clock rate. So you still think
a simple fix is good enough for net tree?

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index de116a0753a1..d2aa3d0695e3 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -2021,6 +2021,11 @@ static int kszphy_probe(struct phy_device *phydev)
                                   rate);
                        return -EINVAL;
                }
+       } else if (!clk) {
+               /* unnamed clock from the generic ethernet-phy binding */
+               clk = devm_clk_get_optional_enabled(&phydev->mdio.dev, NULL);
+               if (IS_ERR(clk))
+                       return PTR_ERR(clk);
        }