RE: broonie-regmap/for-next bisection: boot on ox820-cloudengines-pogoplug-series-3

["Sriram Dash" <sriram.dash@samsung.com>]

> From: Florian Fainelli <f.fainelli@gmail.com>
> Subject: Re: broonie-regmap/for-next bisection: boot on ox820-cloudengines-
> pogoplug-series-3
> 
> On 1/3/20 5:30 AM, Sriram Dash wrote:
> >> From: kernelci.org bot <bot@kernelci.org>
> >> Subject: broonie-regmap/for-next bisection: boot on
> >> ox820-cloudengines-
> >> pogoplug-series-3
> >>
> >> * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
> >> * This automated bisection report was sent to you on the basis  *
> >> * that you may be involved with the breaking commit it has      *
> >> * found.  No manual investigation has been done to verify it,   *
> >> * and the root cause of the problem may be somewhere else.      *
> >> *                                                               *
> >> * If you do send a fix, please include this trailer:            *
> >> *   Reported-by: "kernelci.org bot" <bot@kernelci.org>          *
> >> *                                                               *
> >> * Hope this helps!                                              *
> >> * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
> >>
> >> broonie-regmap/for-next bisection: boot on
> >> ox820-cloudengines-pogoplug-
> >> series-3
> >>
> >> Summary:
> >>   Start:      46cf053efec6 Linux 5.5-rc3
> >>   Details:    https://protect2.fireeye.com/url?k=36fb52ed-6b2b5a21-
> 36fad9a2-
> >> 000babff3793-
> >> f64e7c227e0a8b34&u=https://protect2.fireeye.com/url?k=2379492a-7ee2b5
> >> 49-2378c265-0cc47a31cdbc-
> 914c67c9400b5bae&u=https://kernelci.org/boot
> >> /id/5e02ce65451524462f9731
> >> 4f
> >>   Plain log:
> >> https://protect2.fireeye.com/url?k=58f5fc3b-0525f4f7-58f47774-
> >> 000babff3793-f96a18481add0d7f&u=https://protect2.fireeye.com/url?k=3c
> >> 793260-61e2ce03-3c78b92f-0cc47a31cdbc-
> c77f49890593c376&u=https://stor
> >> age.kernelci.org//broonie-
> >> regmap/for-next/v5.5-rc3/arm/oxnas_v6_defconfig/gcc-8/lab-
> >> baylibre/boot-ox820-cloudengines-pogoplug-series-3.txt
> >>   HTML log:   https://protect2.fireeye.com/url?k=eaed2629-b73d2ee5-
> >> eaecad66-000babff3793-
> >> 84ba1e41025b4f73&u=https://protect2.fireeye.com/url?k=8e80051e-d31bf9
> >> 7d-8e818e51-0cc47a31cdbc-dd2d5f3d7e3c3cd2&u=https://storage.kernelci.
> >> org//broonie-regmap/for-
> >> next/v5.5-rc3/arm/oxnas_v6_defconfig/gcc-8/lab-baylibre/boot-ox820-
> >> cloudengines-pogoplug-series-3.html
> >>   Result:     d3e014ec7d5e net: stmmac: platform: Fix MDIO init for platforms
> >> without PHY
> >>
> >> Checks:
> >>   revert:     PASS
> >>   verify:     PASS
> >>
> >> Parameters:
> >>   Tree:       broonie-regmap
> >>   URL:
> >> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git
> >>   Branch:     for-next
> >>   Target:     ox820-cloudengines-pogoplug-series-3
> >>   CPU arch:   arm
> >>   Lab:        lab-baylibre
> >>   Compiler:   gcc-8
> >>   Config:     oxnas_v6_defconfig
> >>   Test suite: boot
> >>
> >> Breaking commit found:
> >>
> >> ---------------------------------------------------------------------
> >> ---------- commit d3e014ec7d5ebe9644b5486bc530b91e62bbf624
> >> Author: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> >> Date:   Thu Dec 19 15:47:01 2019 +0530
> >>
> >>     net: stmmac: platform: Fix MDIO init for platforms without PHY
> >>
> >>     The current implementation of "stmmac_dt_phy" function initializes
> >>     the MDIO platform bus data, even in the absence of PHY. This fix
> >>     will skip MDIO initialization if there is no PHY present.
> >>
> >>     Fixes: 7437127 ("net: stmmac: Convert to phylink and remove phylib logic")
> >>     Acked-by: Jayati Sahu <jayati.sahu@samsung.com>
> >>     Signed-off-by: Sriram Dash <sriram.dash@samsung.com>
> >>     Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> >>     Signed-off-by: David S. Miller <davem@davemloft.net>
> >>
> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> >> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> >> index bedaff0c13bd..cc8d7e7bf9ac 100644
> >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> >> @@ -320,7 +320,7 @@ static int stmmac_mtl_setup(struct
> >> platform_device *pdev,  static int stmmac_dt_phy(struct
> plat_stmmacenet_data *plat,
> >>  			 struct device_node *np, struct device *dev)  {
> >> -	bool mdio = true;
> >> +	bool mdio = false;
> >>  	static const struct of_device_id need_mdio_ids[] = {
> >>  		{ .compatible = "snps,dwc-qos-ethernet-4.10" },
> >>  		{},
> >> ---------------------------------------------------------------------
> >> ----------
> >>
> >>
> >> Git bisection log:
> >>
> >> ---------------------------------------------------------------------
> >> ----------
> >> git bisect start
> >> # good: [e42617b825f8073569da76dc4510bfa019b1c35a] Linux 5.5-rc1 git
> >> bisect good e42617b825f8073569da76dc4510bfa019b1c35a
> >> # bad: [46cf053efec6a3a5f343fead837777efe8252a46] Linux 5.5-rc3 git
> >> bisect bad 46cf053efec6a3a5f343fead837777efe8252a46
> >> # good: [2187f215ebaac73ddbd814696d7c7fa34f0c3de0] Merge tag
> >> 'for-5.5- rc2-tag' of
> >> git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux
> >> git bisect good 2187f215ebaac73ddbd814696d7c7fa34f0c3de0
> >> # good: [0dd1e3773ae8afc4bfdce782bdeffc10f9cae6ec] pipe: fix empty
> >> pipe check in pipe_write() git bisect good
> >> 0dd1e3773ae8afc4bfdce782bdeffc10f9cae6ec
> >> # good: [040cda8a15210f19da7e29232c897ca6ca6cc950] Merge tag
> >> 'wireless- drivers-2019-12-17' of
> >> git://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers
> >> git bisect good 040cda8a15210f19da7e29232c897ca6ca6cc950
> >> # bad: [4bfeadfc0712bbc8a6556eef6d47cbae1099dea3] Merge branch 'sfc-
> >> fix-bugs-introduced-by-XDP-patches'
> >> git bisect bad 4bfeadfc0712bbc8a6556eef6d47cbae1099dea3
> >> # good: [0fd260056ef84ede8f444c66a3820811691fe884] Merge
> >> git://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf
> >> git bisect good 0fd260056ef84ede8f444c66a3820811691fe884
> >> # good: [90b3b339364c76baa2436445401ea9ade040c216] net: hisilicon:
> >> Fix a BUG trigered by wrong bytes_compl git bisect good
> >> 90b3b339364c76baa2436445401ea9ade040c216
> >> # bad: [4c8dc00503db24deaf0b89dddfa84b7cba7cd4ce] qede: Disable
> >> hardware gro when xdp prog is installed git bisect bad
> >> 4c8dc00503db24deaf0b89dddfa84b7cba7cd4ce
> >> # bad: [28a3b8408f70b646e78880a7eb0a97c22ace98d1] net/smc: unregister
> >> ib devices in reboot_event git bisect bad
> >> 28a3b8408f70b646e78880a7eb0a97c22ace98d1
> >> # bad: [d3e014ec7d5ebe9644b5486bc530b91e62bbf624] net: stmmac:
> >> platform: Fix MDIO init for platforms without PHY git bisect bad
> >> d3e014ec7d5ebe9644b5486bc530b91e62bbf624
> >> # good: [af1c0e4e00f3cc76cb136ebf2e2c04e8b6446285] llc2: Fix return
> >> statement of llc_stat_ev_rx_null_dsap_xid_c (and _test_c) git bisect
> >> good
> >> af1c0e4e00f3cc76cb136ebf2e2c04e8b6446285
> >> # first bad commit: [d3e014ec7d5ebe9644b5486bc530b91e62bbf624] net:
> >> stmmac: platform: Fix MDIO init for platforms without PHY
> >> ---------------------------------------------------------------------
> >> ----------
> >
> >
> > The mdio bus will be allocated in case of a phy transceiver is on
> > board, but if fixed-link is configured, it will be NULL and
> > of_mdiobus_register will not take effect.
> 
> There appears to be another possible flaw in the code here:
> 
>                 for_each_child_of_node(np, plat->mdio_node) {
>                         if (of_device_is_compatible(plat->mdio_node,
>                                                     "snps,dwmac-mdio"))
>                                 break;
>                 }
> 
> the loop should use for_each_available_child_of_node() such that if a platform
> has a Device Tree definition where the MDIO bus node was provided but it was
> not disabled by default (a mistake, it should be disabled by default), and a "fixed-
> link" property ended up being used at the board level, we should not end-up with
> an invalid plat->mdio_node reference. Then the code could possibly eliminate
> the use of 'mdio' as a boolean and rely exclusively on plat->mdio_node. What do
> you think?
> 

Hello Florian,

Thanks for the review. We definitely see a problem here. For the platforms which have the snps,dwmac-mdio and they have made it disabled, it will fail.
Also, We can completely remove the mdio variable from the function stmmac_dt_phy as what we essentially do is to check the plat->mdio_node.

Something like this will help:

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 1f230bd..15c342e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -320,7 +320,6 @@ static int stmmac_mtl_setup(struct platform_device *pdev,
 static int stmmac_dt_phy(struct plat_stmmacenet_data *plat,
                         struct device_node *np, struct device *dev)
 {
-       bool mdio = false;
        static const struct of_device_id need_mdio_ids[] = {
                { .compatible = "snps,dwc-qos-ethernet-4.10" },
                {},
@@ -334,18 +333,13 @@ static int stmmac_dt_phy(struct plat_stmmacenet_data *plat,
                 * the MDIO
                 */
                for_each_child_of_node(np, plat->mdio_node) {
-                       if (of_device_is_compatible(plat->mdio_node,
+                       if (for_each_available_child_of_node(plat->mdio_node,
                                                    "snps,dwmac-mdio"))
                                break;
                }
        }

        if (plat->mdio_node) {
-               dev_dbg(dev, "Found MDIO subnode\n");
-               mdio = true;
-       }
-
-       if (mdio) {
                plat->mdio_bus_data =
                        devm_kzalloc(dev, sizeof(struct stmmac_mdio_bus_data),
                                     GFP_KERNEL);


Are you preparing a patch to address this, or we shall take it up?

> And an alternative to your fix would be to scan even further the MDIO bus node
> for available child nodes, if there are none, do not perform the MDIO
> initialization at all since we have no MDIO devices beneath.
> 
> 
> > The commit d3e014ec7d5e fixes the code for fixed-link configuration.
> > However, some platforms like oxnas820 which have phy transceivers
> > (rgmii), fail. This is because the platforms expect the allocation of
> > mdio_bus_data during stmmac_dt_phy.
> >
> > Proper solution to this is adding the mdio node in the device tree of
> > the platform which can be fetched by stmmac_dt_phy.
> 
> That sounds reasonable, but we should also not break existing platforms with
> existing Device Trees out there, as much as possible.

I understand your point. Changing DT should be the last thing we should do.
But, the code is broken for some platforms. Without the patch, the platforms with fixed-link will not work.
For example, stih418-b2199.dts, will fail without the commit d3e014ec7d5e.
With the patch, platforms with mdio and not declaring the dt parameters will fail.
For that , we have some proposal:
For the newer platforms , Make it mandatory to have the mdio or snps,dwmac-mdio property. 
There is no point of checking the device tree for mdio or snps,dwmac-mdio property and populating the plat->mdio_node, if the platforms are not having them in the device tree and expect mdio bus memory allocation. 

For the existing platforms, which do not have the mdio or snps,dwmac-mdio property and still have the phy, if they can, they must modify the dt and include the mdio or snps,dwmac-mdio property in their dts.
For those platforms, which cannot modify the dt due to some reason or other, the platform should have a quirk in the platform glue layers, and use it in the stmmac_platform driver  stmmac_dt_phy  function to enable the mdio.


> --
> Florian


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel