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: <0dd0eb96ce6509b944d1a0b3cfa78e692409edc5.camel@ew.tq-group.com>
Date:   Mon, 18 Oct 2021 12:32:53 +0200
From:   Matthias Schiffer <matthias.schiffer@...tq-group.com>
To:     Joakim Zhang <qiangqing.zhang@....com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Subject: RE: [PATCH] net: fec: defer probe if PHY on external MDIO bus is
 not available

On Mon, 2021-10-18 at 10:20 +0000, Joakim Zhang wrote:
> 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.

Yes, not all SoC have FEC2 at the lower address, but this is the case
on i.MX6UL. As far as I can tell my patch should not hurt when the
order is already correct, as the added code will never retern
EPROBE_DEFER in these cases.

Obviously all existing Device Trees in the mainline kernel are defined
in a way that already works with the existing code, by using the FEC2
MDIO on i.MX6UL-based designs, or their Ethernet would not work
correctly.


> 
> > 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?

I only mentioned imx6ul-14x14-evk.dtsi as an example. The issue affects
the TQ-Systems MBa6ULx board, which I'm currently preparing for
mainline submission.

The bootloader disables the non-existing FEC2 node on MCIMX6G1, but
when the MDIO of FEC2 is used for both interfacse, this will also break
FEC1, so a separate Device Tree is currently needed for the MCIMX6G1.
We would prefer to use the same Device Tree for both variants, which is
possible by using the MDIO of FEC1 and applying this patch.


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