Messages in this thread Patch in this message |  | | From | "Sriram Dash" <> | Subject | RE: broonie-regmap/for-next bisection: boot on ox820-cloudengines-pogoplug-series-3 | Date | Tue, 7 Jan 2020 10:54:26 +0530 |
| |
> 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
|  |