| 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: <Z0S56m1YIFEPHA/Y@lizhi-Precision-Tower-5810>
Date: Mon, 25 Nov 2024 12:54:50 -0500
From: Frank Li <Frank.li@....com>
To: Wei Fang <wei.fang@....com>
Cc: andrew@...n.ch, 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, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, 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?
> 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.
Frank
> +
> static int kszphy_suspend(struct phy_device *phydev)
> {
> + struct kszphy_priv *priv = phydev->priv;
> + int ret;
> +
> /* Disable PHY Interrupts */
> if (phy_interrupt_is_valid(phydev)) {
> phydev->interrupts = PHY_INTERRUPT_DISABLED;
> @@ -2059,7 +2080,13 @@ static int kszphy_suspend(struct phy_device *phydev)
> phydev->drv->config_intr(phydev);
> }
>
> - return genphy_suspend(phydev);
> + ret = genphy_suspend(phydev);
> + if (ret)
> + return ret;
> +
> + kszphy_disable_clk(priv);
> +
> + return 0;
> }
>
> static void kszphy_parse_led_mode(struct phy_device *phydev)
> @@ -2088,8 +2115,11 @@ static void kszphy_parse_led_mode(struct phy_device *phydev)
>
> static int kszphy_resume(struct phy_device *phydev)
> {
> + struct kszphy_priv *priv = phydev->priv;
> int ret;
>
> + kszphy_enable_clk(priv);
> +
> genphy_resume(phydev);
>
> /* After switching from power-down to normal mode, an internal global
> @@ -2112,6 +2142,24 @@ static int kszphy_resume(struct phy_device *phydev)
> return 0;
> }
>
> +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;
> +}
> +
> static int ksz9477_resume(struct phy_device *phydev)
> {
> int ret;
> @@ -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)
> @@ -2194,7 +2245,7 @@ static int kszphy_probe(struct phy_device *phydev)
>
> kszphy_parse_led_mode(phydev);
>
> - clk = devm_clk_get_optional_enabled(&phydev->mdio.dev, "rmii-ref");
> + clk = devm_clk_get_optional(&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);
> @@ -2216,11 +2267,14 @@ static int kszphy_probe(struct phy_device *phydev)
> }
> } else if (!clk) {
> /* unnamed clock from the generic ethernet-phy binding */
> - clk = devm_clk_get_optional_enabled(&phydev->mdio.dev, NULL);
> + clk = devm_clk_get_optional(&phydev->mdio.dev, NULL);
> if (IS_ERR(clk))
> return PTR_ERR(clk);
> }
>
> + if (!IS_ERR_OR_NULL(clk))
> + priv->clk = clk;
> +
> if (ksz8041_fiber_mode(phydev))
> phydev->port = PORT_FIBRE;
>
> @@ -5290,15 +5344,45 @@ static int lan8841_probe(struct phy_device *phydev)
> return 0;
> }
>
> +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;
> }
>
> static struct phy_driver ksphy_driver[] = {
> @@ -5358,9 +5442,12 @@ static struct phy_driver ksphy_driver[] = {
> .get_sset_count = kszphy_get_sset_count,
> .get_strings = kszphy_get_strings,
> .get_stats = kszphy_get_stats,
> - /* 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.
> */
> + .suspend = ksz8041_suspend,
> + .resume = ksz8041_resume,
> }, {
> .phy_id = PHY_ID_KSZ8041RNLI,
> .phy_id_mask = MICREL_PHY_ID_MASK,
> @@ -5507,7 +5594,7 @@ static struct phy_driver ksphy_driver[] = {
> .get_sset_count = kszphy_get_sset_count,
> .get_strings = kszphy_get_strings,
> .get_stats = kszphy_get_stats,
> - .suspend = genphy_suspend,
> + .suspend = lan8804_suspend,
> .resume = kszphy_resume,
> .config_intr = lan8804_config_intr,
> .handle_interrupt = lan8804_handle_interrupt,
> @@ -5526,7 +5613,7 @@ static struct phy_driver ksphy_driver[] = {
> .get_strings = kszphy_get_strings,
> .get_stats = kszphy_get_stats,
> .suspend = lan8841_suspend,
> - .resume = genphy_resume,
> + .resume = lan8841_resume,
> .cable_test_start = lan8814_cable_test_start,
> .cable_test_get_status = ksz886x_cable_test_get_status,
> }, {
> --
> 2.34.1
>
Powered by blists - more mailing lists