netdev - Re: [PATCH 4/5] net: phy: smsc: add phy refclk in support

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: <2993e0ed-ebe9-fd85-4650-7e53c15cfe34@gmail.com>
Date:   Mon, 31 Aug 2020 09:32:24 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Marco Felsch <m.felsch@...gutronix.de>, davem@...emloft.net,
        kuba@...nel.org, robh+dt@...nel.org, andrew@...n.ch,
        hkallweit1@...il.com, linux@...linux.org.uk, zhengdejin5@...il.com,
        richard.leitner@...data.com
Cc:     netdev@...r.kernel.org, devicetree@...r.kernel.org,
        kernel@...gutronix.de
Subject: Re: [PATCH 4/5] net: phy: smsc: add phy refclk in support



On 8/31/2020 6:48 AM, Marco Felsch wrote:
> Add support to specify the clock provider for the phy refclk and don't
> rely on 'magic' host clock setup. [1] tried to address this by
> introducing a flag and fixing the corresponding host. But this commit
> breaks the IRQ support since the irq setup during .config_intr() is
> thrown away because the reset comes from the side without respecting the
> current phy-state within the phy-state-machine. Furthermore the commit
> fixed the problem only for FEC based hosts other hosts acting like the
> FEC are not covered.
> 
> This commit goes the other way around to address the bug fixed by [1].
> Instead of resetting the device from the side every time the refclk gets
> (re-)enabled it requests and enables the clock till the device gets
> removed. The phy is still rest but now within the phylib and  with
> respect to the phy-state-machine.
> 
> [1] commit 7f64e5b18ebb ("net: phy: smsc: LAN8710/20: add
>      PHY_RST_AFTER_CLK_EN flag")
> 
> Signed-off-by: Marco Felsch <m.felsch@...gutronix.de>
> ---
>   drivers/net/phy/smsc.c | 30 ++++++++++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> index 79574fcbd880..b98a7845681f 100644
> --- a/drivers/net/phy/smsc.c
> +++ b/drivers/net/phy/smsc.c
> @@ -12,6 +12,7 @@
>    *
>    */
>   
> +#include <linux/clk.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/mii.h>
> @@ -33,6 +34,7 @@ static struct smsc_hw_stat smsc_hw_stats[] = {
>   
>   struct smsc_phy_priv {
>   	bool energy_enable;
> +	struct clk *refclk;
>   };
>   
>   static int smsc_phy_config_intr(struct phy_device *phydev)
> @@ -194,11 +196,19 @@ static void smsc_get_stats(struct phy_device *phydev,
>   		data[i] = smsc_get_stat(phydev, i);
>   }
>   
> +static void smsc_clk_disable_action(void *data)
> +{
> +	struct smsc_phy_priv *priv = data;
> +
> +	clk_disable_unprepare(priv->refclk);
> +}
> +
>   static int smsc_phy_probe(struct phy_device *phydev)
>   {
>   	struct device *dev = &phydev->mdio.dev;
>   	struct device_node *of_node = dev->of_node;
>   	struct smsc_phy_priv *priv;
> +	int ret;
>   
>   	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>   	if (!priv)
> @@ -211,6 +221,26 @@ static int smsc_phy_probe(struct phy_device *phydev)
>   
>   	phydev->priv = priv;
>   
> +	priv->refclk = devm_clk_get_optional(dev, NULL);
> +	if (IS_ERR(priv->refclk)) {
> +		if (PTR_ERR(priv->refclk) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +
> +		/* Clocks are optional all errors should be ignored here */
> +		return 0;
> +	}
> +
> +	/* Starting from here errors should not be ignored anymore */
> +	ret = clk_set_rate(priv->refclk, 50 * 1000 * 1000);
> +	if (ret)
> +		return ret;

The clock should be enabled first before attempting a rate change, and 
this also causes a more fundamental question: what is the sate of the 
clock when the PHY driver is probed, and is the reference clock feeding 
into the MDIO logic of the PHY.

By that I mean that if the reference clock was disabled, would the PHY 
still respond to MDIO reads such that you would be able to probe and 
identify it?

If not, your demv_clk_get_optional() is either too late, or assuming a 
prior state, or you are working around this in Device Tree by using a 
compatible string with the form 
"^ethernet-phy-id[a-f0-9]{4}\\.[a-f0-9]{4}$" in which case, this is a 
making assumptions about how the OF MDIO layer works which is not ideal.

I am preparing some patches that aim at enabling a given MDIO device's 
clock prior to probing it and should be able to post them by today.

> +
> +	ret = clk_prepare_enable(priv->refclk);
> +	if (ret)
> +		return ret;
> +
> +	devm_add_action_or_reset(dev, smsc_clk_disable_action, priv);
> +
>   	return 0;
>   }
>   
> 

-- 
Florian