netdev - Re: [PATCH] net: fec: defer probe if PHY on external MDIO bus is not available

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: <YW7SWKiUy8LfvSkl@lunn.ch>
Date:   Tue, 19 Oct 2021 16:12:40 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Matthias Schiffer <matthias.schiffer@...tq-group.com>
Cc:     Joakim Zhang <qiangqing.zhang@....com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: fec: defer probe if PHY on external MDIO bus is not
 available

On Thu, Oct 14, 2021 at 01:30:43PM +0200, Matthias Schiffer wrote:
> On some SoCs like i.MX6UL it is common to use the same MDIO bus for PHYs
> on both Ethernet controllers. Currently device trees for such setups
> have to make assumptions regarding the probe order of the controllers:
> 
> For example in imx6ul-14x14-evk.dtsi, the MDIO bus of fec2 is used for
> the PHYs of both fec1 and fec2. The reason is that fec2 has a lower
> address than fec1 and is thus loaded first, so the bus is already
> available when fec1 is probed.
> 
> Besides being confusing, this limitation also makes it impossible to use
> the same device tree for variants of the i.MX6UL with one Ethernet
> controller (which have to use the MDIO of fec1, as fec2 does not exist)
> and variants with two controllers (which have to use fec2 because of the
> load order).
> 
> To fix this, defer the probe of the Ethernet controller when the PHY is
> not on our own MDIO bus and not available.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@...tq-group.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 47a6fc702ac7..dc070dd216e8 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -3820,7 +3820,28 @@ fec_probe(struct platform_device *pdev)
>  		goto failed_stop_mode;
>  
>  	phy_node = of_parse_phandle(np, "phy-handle", 0);
> -	if (!phy_node && of_phy_is_fixed_link(np)) {
> +	if (phy_node) {
> +		struct device_node *mdio_parent =
> +			of_get_next_parent(of_get_parent(phy_node));
> +
> +		ret = 0;
> +
> +		/* Skip PHY availability check for our own MDIO bus to avoid
> +		 * cyclic dependency
> +		 */
> +		if (mdio_parent != np) {
> +			struct phy_device *phy = of_phy_find_device(phy_node);
> +
> +			if (phy)
> +				put_device(&phy->mdio.dev);
> +			else
> +				ret = -EPROBE_DEFER;
> +		}

I've not looked at the details yet, just back from vacation. But this
seems wrong. I would of expected phylib to of returned -EPRODE_DEFER
at some point, when asked for a PHY which does not exist yet. All the
driver should need to do is make sure it returns the
-EPRODE_DEFER.

       Andrew