linux-kernel - RE: [PATCH v2 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] [day] [month] [year] [list]
Message-ID:
 <PAXPR04MB8510D36DDA1B9E98B2FB77B488362@PAXPR04MB8510.eurprd04.prod.outlook.com>
Date: Tue, 3 Dec 2024 01:49:10 +0000
From: Wei Fang <wei.fang@....com>
To: Andrew Lunn <andrew@...n.ch>
CC: "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>, "kuba@...nel.org"
	<kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"florian.fainelli@...adcom.com" <florian.fainelli@...adcom.com>,
	"heiko.stuebner@...rry.de" <heiko.stuebner@...rry.de>, Frank Li
	<frank.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 v2 net] net: phy: micrel: Dynamically control external
 clock of KSZ PHY

> -----Original Message-----
> From: Andrew Lunn <andrew@...n.ch>
> Sent: 2024年12月2日 22:49
> To: Wei Fang <wei.fang@....com>
> Cc: hkallweit1@...il.com; linux@...linux.org.uk; davem@...emloft.net;
> edumazet@...gle.com; kuba@...nel.org; pabeni@...hat.com;
> florian.fainelli@...adcom.com; heiko.stuebner@...rry.de; Frank Li
> <frank.li@....com>; netdev@...r.kernel.org; linux-kernel@...r.kernel.org;
> imx@...ts.linux.dev
> Subject: Re: [PATCH v2 net] net: phy: micrel: Dynamically control external clock
> of KSZ PHY
> 
> On Mon, Dec 02, 2024 at 04:45:35PM +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.
> 
> The referenced commit is a one liner:
> 
> @@ -2001,7 +2001,7 @@ static int kszphy_probe(struct phy_device *phydev)
> 
>         kszphy_parse_led_mode(phydev);
> 
> -       clk = devm_clk_get(&phydev->mdio.dev, "rmii-ref");
> +       clk = devm_clk_get_optional_enabled(&phydev->mdio.dev,
> + "rmii-ref");
>         /* NOTE: clk may be NULL if building without CONFIG_HAVE_CLK */
> 
> and here you are adding 103 lines as a fix. This seems out of proportion. Did this
> truly work before this change?

For i.MX6ULL-14X14-EVK, the RMII reference clock is provided by the SoC and
enabled by FEC driver. Before the commit 985329462723, we can see that the
clk_enable_count of enet1_ref and enet2_ref is 1 when the FEC ports are up,
and the count is 0 when the ports are down. After the commit 985329462723,
the clk_enable_count is 2 when then ports are up, because the KSZ PHY driver
enables the clock as well, so the count is 2. But the PHY driver does not disable
clock except the PHY driver is removed. That is to say, the RMII reference clock
is always working no matter the port is down or the system enters suspend state.

> 
> The commit message says:
> 
>     While the external clock input will most likely be enabled, it's not
>     guaranteed and clk_get_rate in some suppliers will even just return
>     valid results when the clock is running.
> 
> So it seems like a much simpler fix is to put a clock_enable/clock_disable around
> clk_get_rate.
> 

Sorry, I was not aware of that some suppliers need to be enabled so that
clk_get_rate() can get the correct clk rate. So we can keep
devm_clk_get_optional_enabled() to get the clk rate and then disable
the clock. In addition, there is another situation that the clock is only enabled
by the PHY driver. In this case, my current modification is necessary. Of course,
this is a very corner case, after all, the clock was enabled by other drivers before
commit 985329462723. But after this commit, this possibility exists.

So what do you think? Should I simply disable the clock after getting the clk rate?
Or should I add the following modifications based on my current patch?

@@ -2245,7 +2245,7 @@ static int kszphy_probe(struct phy_device *phydev)

        kszphy_parse_led_mode(phydev);

-       clk = devm_clk_get_optional(&phydev->mdio.dev, "rmii-ref");
+       clk = devm_clk_get_optional_enabled(&phydev->mdio.dev, "rmii-ref");
        /* NOTE: clk may be NULL if building without CONFIG_HAVE_CLK */
        if (!IS_ERR_OR_NULL(clk)) {
                unsigned long rate = clk_get_rate(clk);
@@ -2267,13 +2267,15 @@ static int kszphy_probe(struct phy_device *phydev)
                }
        } else if (!clk) {
                /* unnamed clock from the generic ethernet-phy binding */
-               clk = devm_clk_get_optional(&phydev->mdio.dev, NULL);
+               clk = devm_clk_get_optional_enabled(&phydev->mdio.dev, NULL);
                if (IS_ERR(clk))
                        return PTR_ERR(clk);
        }

-       if (!IS_ERR_OR_NULL(clk))
+       if (!IS_ERR_OR_NULL(clk)) {
                priv->clk = clk;
+               clk_disable_unprepare(priv->clk);
+       }