| 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
| ||
|
Message-ID:
<PAXPR04MB85106B8C8B32C92085642A68883B2@PAXPR04MB8510.eurprd04.prod.outlook.com>
Date: Mon, 16 Dec 2024 02:29:31 +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 v4 net] net: phy: micrel: Dynamically control external
clock of KSZ PHY
> -----Original Message-----
> From: Andrew Lunn <andrew@...n.ch>
> Sent: 2024年12月13日 18:45
> 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 v4 net] net: phy: micrel: Dynamically control external
> clock of KSZ PHY
>
> > 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.
>
> This is what i don't like too much, the fact suspend/resume is not
> called in pairs. That was probably a bad decision on my part, and
> maybe it should be revised. But that is out of scope for this patch,
> unless you want the challenge.
>
> > +static int ksz8041_resume(struct phy_device *phydev)
> > +{
> > + struct kszphy_priv *priv = phydev->priv;
> > +
> > + kszphy_enable_clk(priv);
> > +
> > + return 0;
> > +}
> > +
> > +static int ksz8041_suspend(struct phy_device *phydev)
> > +{
> > + struct kszphy_priv *priv = phydev->priv;
> > +
> > + kszphy_disable_clk(priv);
> > +
> > + return 0;
> > +}
>
> These two look odd. Why is there no call to
> genphy_suspend/genphy_resume? Probably a comment should be added
> here.
>
I have added the comment in ksphy_driver[], maybe it would be better to move
the comment here.
- /* No suspend/resume callbacks because of errata DS80000700A,
- * receiver error following software power down.
+ /* Because of errata DS80000700A, receiver error following software
+ * power down. Suspend and resume callbacks only disable and enable
+ * external rmii reference clock.
*/
> > @@ -2150,8 +2198,11 @@ static int ksz9477_resume(struct phy_device
> *phydev)
> >
> > static int ksz8061_resume(struct phy_device *phydev)
> > {
> > + struct kszphy_priv *priv = phydev->priv;
> > int ret;
> >
> > + kszphy_enable_clk(priv);
> > +
> > /* This function can be called twice when the Ethernet device is on. */
> > ret = phy_read(phydev, MII_BMCR);
> > if (ret < 0)
>
> What about 8061_suspend()? They normally are used in pairs.
Okay, I can add the ksz8061_suspend(), so that it is a pair with ksz8061_resume().
>
> > @@ -2221,6 +2272,11 @@ static int kszphy_probe(struct phy_device
> *phydev)
> > return PTR_ERR(clk);
> > }
> >
> > + if (!IS_ERR_OR_NULL(clk)) {
> > + clk_disable_unprepare(clk);
> > + priv->clk = clk;
> > + }
>
> I think you should improve the error handling. If
> devm_clk_get_optional_enabled() returns an error, you should fail the
> probe. If the clock does not exist, devm_clk_get_optional_enabled()
> will return a NULL pointer, which is a valid clock. You can
> enable/disable it etc. So you then do not need this check.
Okay, accept.
>
> > +static int lan8804_suspend(struct phy_device *phydev)
> > +{
> > + struct kszphy_priv *priv = phydev->priv;
> > + int ret;
> > +
> > + ret = genphy_suspend(phydev);
> > + if (ret)
> > + return ret;
> > +
> > + kszphy_disable_clk(priv);
> > +
> > + return 0;
> > +}
> > +
> > +static int lan8841_resume(struct phy_device *phydev)
> > +{
> > + struct kszphy_priv *priv = phydev->priv;
> > +
> > + kszphy_enable_clk(priv);
> > +
> > + return genphy_resume(phydev);
> > +}
> > +
> > static int lan8841_suspend(struct phy_device *phydev)
> > {
> > struct kszphy_priv *priv = phydev->priv;
> > struct kszphy_ptp_priv *ptp_priv = &priv->ptp_priv;
> > + int ret;
> >
> > if (ptp_priv->ptp_clock)
> > ptp_cancel_worker_sync(ptp_priv->ptp_clock);
> >
> > - return genphy_suspend(phydev);
> > + ret = genphy_suspend(phydev);
> > + if (ret)
> > + return ret;
> > +
> > + kszphy_disable_clk(priv);
> > +
> > + return 0;
>
> This can be simplified to
>
> return lan8804_suspend(phydev);
To be honest, I don't know the relationship between lan8804 and
lan8841, So I'm not sure if changes added later in lan8804_suspend()
will also work for lan8841. Otherwise, it will need to be reworked in
the future. So I think there is no need to make this simplification.
Unless you are very sure that the two platforms are compatible.
>
> In general, you should define a common low level function which does
> what all PHYs need, in this case, genphy_suspend() and disable the
> clock. Then add wrappers which do additional things.
>
Good suggestion, let me improve it, thanks
Powered by blists - more mailing lists