linux-kernel - Re: [PATCH v4 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: <73d5ea2b-8cec-46e9-80a9-a7a96657c2d4@lunn.ch>
Date: Fri, 13 Dec 2024 11:44:34 +0100
From: Andrew Lunn <andrew@...n.ch>
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@....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.

> @@ -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.

> @@ -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.

> +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);

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.

	Andrew