linux-kernel - RE: [PATCH 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:
 <PAXPR04MB8510D7A400FB0CEAC786F524882F2@PAXPR04MB8510.eurprd04.prod.outlook.com>
Date: Tue, 26 Nov 2024 06:43:43 +0000
From: Wei Fang <wei.fang@....com>
To: Frank Li <frank.li@....com>
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>, "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>, "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 net] net: phy: micrel: Dynamically control external clock
 of KSZ PHY



> On Mon, Nov 25, 2024 at 10:29:05AM +0800, Wei Fang wrote:
> > On the i.MX6ULL-14x14-EVK board, when using the 6.12 kernel, it is
> > found that after disabling the two network ports, the clk_enable_count
> > of the enet1_ref and enet2_ref clocks is not 0 (these two clocks are
> > used as the clock source of the RMII reference clock of the two
> > KSZ8081 PHYs), but there is no such problem in the 6.6 kernel.
> 
> skip your debug progress, just descript the problem itself.
> 
> >
> > After analysis, we found 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 resume() of the PHY
> > driver is called, and the clock is disabled when suspend() is called.
> > Since the PHY driver's resume() and suspend() interfaces are not
> > called in pairs, an additional clk_enable flag is added. When
> > suspend() is
> 
> Why  resume() and suspend() is not call paired?
> 

For the MAC drivers which use PHY Library, when the network interface
is up (net_device_ops:: ndo_open() is called). The phy_driver:: resume()
will be called twice:

net_device_ops:: ndo_open()
	--> of_phy_connect()
		--> phy_attach_direct()
			--> phy_resume()
				--> phy_driver:: resume() #1
	--> phy_start()
		--> __phy_resume()
			--> phy_driver:: resume() #2

But phy_driver:: suspend() is called only once when the network port
is down (net_device_ops:: ndo_stop ()).

net_device_ops:: ndo_stop ()
	--> phy_stop()
		--> phy_suspend()
			--> phy_driver::suspend() #1

For MAC drivers which use phylink also have the same situation.

net_device_ops:: ndo_open()
	--> phylink_of_phy_connect()
		--> phy_attach_direct()
			--> phy_resume()
				--> phy_driver:: resume() #1
	--> phylink_start()
		--> phy_start()
			--> __phy_resume()
				--> phy_driver:: resume() #2

net_device_ops:: ndo_stop ()
	--> phylink_stop()
		--> phy_stop()
			--> phy_suspend()
				--> phy_driver::suspend() #1

> 
> > called, the clock is disabled only if clk_enable is true. Conversely,
> > when resume() is called, the clock is enabled if clk_enable is false.
> >
> > Fixes: 985329462723 ("net: phy: micrel: use
> > devm_clk_get_optional_enabled for the rmii-ref clock")
> > Fixes: 99ac4cbcc2a5 ("net: phy: micrel: allow usage of generic
> > ethernet-phy clock")
> > Signed-off-by: Wei Fang <wei.fang@....com>
> > ---
> >  drivers/net/phy/micrel.c | 103
> > ++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 95 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index
> > 3ef508840674..44577b5d48d5 100644
> > --- a/drivers/net/phy/micrel.c
> > +++ b/drivers/net/phy/micrel.c
> > @@ -432,10 +432,12 @@ struct kszphy_ptp_priv {  struct kszphy_priv {
> >  	struct kszphy_ptp_priv ptp_priv;
> >  	const struct kszphy_type *type;
> > +	struct clk *clk;
> >  	int led_mode;
> >  	u16 vct_ctrl1000;
> >  	bool rmii_ref_clk_sel;
> >  	bool rmii_ref_clk_sel_val;
> > +	bool clk_enable;
> >  	u64 stats[ARRAY_SIZE(kszphy_hw_stats)];
> >  };
> >
> > @@ -2050,8 +2052,27 @@ static void kszphy_get_stats(struct phy_device
> *phydev,
> >  		data[i] = kszphy_get_stat(phydev, i);  }
> >
> > +static void kszphy_enable_clk(struct kszphy_priv *priv) {
> > +	if (!priv->clk_enable && priv->clk) {
> > +		clk_prepare_enable(priv->clk);
> > +		priv->clk_enable = true;
> > +	}
> > +}
> > +
> > +static void kszphy_disable_clk(struct kszphy_priv *priv) {
> > +	if (priv->clk_enable && priv->clk) {
> > +		clk_disable_unprepare(priv->clk);
> > +		priv->clk_enable = false;
> > +	}
> > +}
> 
> Generally, clock not check enable status, just call enable/disable pair.
>

Yes, but for PHY driver, the situation is different, resume() is called once
more than suspend(). So we need to ensure clk_prepare_enable() will
not be called repeatedly.