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]

Date:   Mon, 18 Oct 2021 10:20:57 +0000
From:   Joakim Zhang <qiangqing.zhang@....com>
To:     Matthias Schiffer <matthias.schiffer@...tq-group.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] net: fec: defer probe if PHY on external MDIO bus is not
 available


Hi Matthias,

> -----Original Message-----
> From: Matthias Schiffer <matthias.schiffer@...tq-group.com>
> Sent: 2021年10月14日 19:31
> To: Joakim Zhang <qiangqing.zhang@....com>; David S. Miller
> <davem@...emloft.net>; Jakub Kicinski <kuba@...nel.org>
> Cc: netdev@...r.kernel.org; linux-kernel@...r.kernel.org; Matthias Schiffer
> <matthias.schiffer@...tq-group.com>
> Subject: [PATCH] net: fec: defer probe if PHY on external MDIO bus is not
> available
> 
> 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.

It's not correct, I think, we have board designed to use fec1(which is lower address) to controller MDIO interface, such as,
https://source.codeaurora.org/external/imx/linux-imx/tree/arch/arm64/boot/dts/freescale/imx8qm-mek.dts?h=lf-5.10.y#n948
that means our driver can handle these cases, not related to the order.

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

Generally speaking, you should only include imx6ul.dtsi for your board design to cover SoC definition,
and imx6ul-14x14-evk.dtsi/ imx6ul-14x14-evk.dts is for our 14x14 EVK board. So do we really need this
defer probe?

> 
> 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;
> +		}
> +
> +		of_node_put(mdio_parent);
> +		if (ret)
> +			goto failed_phy;
> +	} else if (of_phy_is_fixed_link(np)) {
>  		ret = of_phy_register_fixed_link(np);
>  		if (ret < 0) {
>  			dev_err(&pdev->dev,
> --
> 2.17.1

Best Regards,
Joakim Zhang