RE: broonie-regmap/for-next bisection: boot on ox820-cloudengines-pogoplug-series-3
From: Sriram Dash
Date: Tue Jan 07 2020 - 00:24:47 EST
> From: Florian Fainelli <f.fainelli@xxxxxxxxx>
> 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@xxxxxxxxxxxx>
> >> 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@xxxxxxxxxxxx> *
> >> * *
> >> * 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@xxxxxxxxxxx>
> >> 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@xxxxxxxxxxx>
> >> Signed-off-by: Sriram Dash <sriram.dash@xxxxxxxxxxx>
> >> Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@xxxxxxxxxxx>
> >> Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>
> >>
> >> 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