Russell King pointed out that disabling the switch by clearing GSWIP_MDIO_GLOB_ENABLE before calling dsa_unregister_switch() is problematic, as it violates a Golden Rule of driver development to always first unpublish userspace interfaces and then disable the hardware. Fix this, and also simplify the probe() function, by introducing a dsa_switch_ops teardown() operation which takes care of clearing the GSWIP_MDIO_GLOB_ENABLE bit. Fixes: 14fceff4771e5 ("net: dsa: Add Lantiq / Intel DSA driver for vrx200") Suggested-by: "Russell King (Oracle)" Signed-off-by: Daniel Golle --- v4: initial submission, not present in previous versions drivers/net/dsa/lantiq/lantiq_gswip.c | 3 --- drivers/net/dsa/lantiq/lantiq_gswip_common.c | 13 ++++++++++--- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/drivers/net/dsa/lantiq/lantiq_gswip.c b/drivers/net/dsa/lantiq/lantiq_gswip.c index 57dd063c07403..b094001a7c805 100644 --- a/drivers/net/dsa/lantiq/lantiq_gswip.c +++ b/drivers/net/dsa/lantiq/lantiq_gswip.c @@ -444,9 +444,6 @@ static void gswip_remove(struct platform_device *pdev) if (!priv) return; - /* disable the switch */ - gswip_disable_switch(priv); - dsa_unregister_switch(priv->ds); for (i = 0; i < priv->num_gphy_fw; i++) diff --git a/drivers/net/dsa/lantiq/lantiq_gswip_common.c b/drivers/net/dsa/lantiq/lantiq_gswip_common.c index 9da39edf8f574..6b171d58e1862 100644 --- a/drivers/net/dsa/lantiq/lantiq_gswip_common.c +++ b/drivers/net/dsa/lantiq/lantiq_gswip_common.c @@ -752,6 +752,13 @@ static int gswip_setup(struct dsa_switch *ds) return 0; } +static void gswip_teardown(struct dsa_switch *ds) +{ + struct gswip_priv *priv = ds->priv; + + regmap_clear_bits(priv->mdio, GSWIP_MDIO_GLOB, GSWIP_MDIO_GLOB_ENABLE); +} + static enum dsa_tag_protocol gswip_get_tag_protocol(struct dsa_switch *ds, int port, enum dsa_tag_protocol mp) @@ -1629,6 +1636,7 @@ static const struct phylink_mac_ops gswip_phylink_mac_ops = { static const struct dsa_switch_ops gswip_switch_ops = { .get_tag_protocol = gswip_get_tag_protocol, .setup = gswip_setup, + .teardown = gswip_teardown, .port_setup = gswip_port_setup, .port_enable = gswip_port_enable, .port_disable = gswip_port_disable, @@ -1718,15 +1726,14 @@ int gswip_probe_common(struct gswip_priv *priv, u32 version) err = gswip_validate_cpu_port(priv->ds); if (err) - goto disable_switch; + goto unregister_switch; dev_info(priv->dev, "probed GSWIP version %lx mod %lx\n", GSWIP_VERSION_REV(version), GSWIP_VERSION_MOD(version)); return 0; -disable_switch: - gswip_disable_switch(priv); +unregister_switch: dsa_unregister_switch(priv->ds); return err; -- 2.52.0 The driver's .remove operation was calling gswip_disable_switch() which clears the GSWIP_MDIO_GLOB_ENABLE bit before calling dsa_unregister_switch() and thereby violating a Golden Rule of driver development to always unpublish userspace interfaces before disabling hardware, as pointed out by Russell King. Fix this by relying in GSWIP_MDIO_GLOB_ENABLE being cleared by the .teardown operation introduced by the previous commit ("net: dsa: lantiq_gswip: fix teardown order"). Fixes: 22335939ec907 ("net: dsa: add driver for MaxLinear GSW1xx switch family") Suggested-by: "Russell King (Oracle)" Signed-off-by: Daniel Golle --- v4: initial submission, not present in previous versions drivers/net/dsa/lantiq/mxl-gsw1xx.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/net/dsa/lantiq/mxl-gsw1xx.c b/drivers/net/dsa/lantiq/mxl-gsw1xx.c index cf33a16fd183b..cda966d71e889 100644 --- a/drivers/net/dsa/lantiq/mxl-gsw1xx.c +++ b/drivers/net/dsa/lantiq/mxl-gsw1xx.c @@ -652,8 +652,6 @@ static void gsw1xx_remove(struct mdio_device *mdiodev) if (!priv) return; - gswip_disable_switch(priv); - dsa_unregister_switch(priv->ds); } -- 2.52.0 The .shutdown operation should call dsa_switch_shutdown() just like it is done also by the sibling lantiq_gswip driver. Not doing that results in shutdown or reboot hanging and waiting for the CPU port becoming free, which introduces a longer delay and a WARNING before shutdown or reboot in case the driver is built-into the kernel. Fix this by calling dsa_switch_shutdown() in the driver's shutdown operation, harmonizing it with what is done in the lantiq_gswip driver. As a side-effect this now allows to remove the previously exported gswip_disable_switch() function which no longer got any users. Fixes: 22335939ec907 ("net: dsa: add driver for MaxLinear GSW1xx switch family") Signed-off-by: Daniel Golle --- v4: initial submission, not present in previous versions drivers/net/dsa/lantiq/lantiq_gswip.h | 2 -- drivers/net/dsa/lantiq/lantiq_gswip_common.c | 6 ------ drivers/net/dsa/lantiq/mxl-gsw1xx.c | 4 ++-- 3 files changed, 2 insertions(+), 10 deletions(-) diff --git a/drivers/net/dsa/lantiq/lantiq_gswip.h b/drivers/net/dsa/lantiq/lantiq_gswip.h index 9c38e51a75e80..2e0f2afbadbbc 100644 --- a/drivers/net/dsa/lantiq/lantiq_gswip.h +++ b/drivers/net/dsa/lantiq/lantiq_gswip.h @@ -294,8 +294,6 @@ struct gswip_priv { u16 version; }; -void gswip_disable_switch(struct gswip_priv *priv); - int gswip_probe_common(struct gswip_priv *priv, u32 version); #endif /* __LANTIQ_GSWIP_H */ diff --git a/drivers/net/dsa/lantiq/lantiq_gswip_common.c b/drivers/net/dsa/lantiq/lantiq_gswip_common.c index 6b171d58e1862..e790f2ef75884 100644 --- a/drivers/net/dsa/lantiq/lantiq_gswip_common.c +++ b/drivers/net/dsa/lantiq/lantiq_gswip_common.c @@ -1664,12 +1664,6 @@ static const struct dsa_switch_ops gswip_switch_ops = { .port_hsr_leave = dsa_port_simple_hsr_leave, }; -void gswip_disable_switch(struct gswip_priv *priv) -{ - regmap_clear_bits(priv->mdio, GSWIP_MDIO_GLOB, GSWIP_MDIO_GLOB_ENABLE); -} -EXPORT_SYMBOL_GPL(gswip_disable_switch); - static int gswip_validate_cpu_port(struct dsa_switch *ds) { struct gswip_priv *priv = ds->priv; diff --git a/drivers/net/dsa/lantiq/mxl-gsw1xx.c b/drivers/net/dsa/lantiq/mxl-gsw1xx.c index cda966d71e889..4dc287ad141e1 100644 --- a/drivers/net/dsa/lantiq/mxl-gsw1xx.c +++ b/drivers/net/dsa/lantiq/mxl-gsw1xx.c @@ -662,9 +662,9 @@ static void gsw1xx_shutdown(struct mdio_device *mdiodev) if (!priv) return; - dev_set_drvdata(&mdiodev->dev, NULL); + dsa_switch_shutdown(priv->ds); - gswip_disable_switch(priv); + dev_set_drvdata(&mdiodev->dev, NULL); } static const struct gswip_hw_info gsw12x_data = { -- 2.52.0 Despite being documented as self-clearing, the RANEG bit sometimes remains set, preventing auto-negotiation from happening. Manually clear the RANEG bit after 10ms as advised by MaxLinear. In order to not hold RTNL during the 10ms of waiting schedule delayed work to take care of clearing the bit asynchronously, which is similar to the self-clearing behavior. Fixes: 22335939ec90 ("net: dsa: add driver for MaxLinear GSW1xx switch family") Reported-by: Rasmus Villemoes Signed-off-by: Daniel Golle --- v4: * fix order of operations in remove and shutdown functions v3: * fix wrong parameter name in call of cancel_delayed_work_sync v2: * cancel pending work before setting RANEG bit * cancel pending work on remove and shutdown * document that GSW1XX_RST_REQ_SGMII_SHELL also clears RANEG bit * improve commit message drivers/net/dsa/lantiq/mxl-gsw1xx.c | 34 ++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/drivers/net/dsa/lantiq/mxl-gsw1xx.c b/drivers/net/dsa/lantiq/mxl-gsw1xx.c index 4dc287ad141e1..f8ff8a604bf53 100644 --- a/drivers/net/dsa/lantiq/mxl-gsw1xx.c +++ b/drivers/net/dsa/lantiq/mxl-gsw1xx.c @@ -11,10 +11,12 @@ #include #include +#include #include #include #include #include +#include #include #include "lantiq_gswip.h" @@ -29,6 +31,7 @@ struct gsw1xx_priv { struct regmap *clk; struct regmap *shell; struct phylink_pcs pcs; + struct delayed_work clear_raneg; phy_interface_t tbi_interface; struct gswip_priv gswip; }; @@ -145,7 +148,9 @@ static void gsw1xx_pcs_disable(struct phylink_pcs *pcs) { struct gsw1xx_priv *priv = pcs_to_gsw1xx(pcs); - /* Assert SGMII shell reset */ + cancel_delayed_work_sync(&priv->clear_raneg); + + /* Assert SGMII shell reset (will also clear RANEG bit) */ regmap_set_bits(priv->shell, GSW1XX_SHELL_RST_REQ, GSW1XX_RST_REQ_SGMII_SHELL); @@ -428,12 +433,29 @@ static int gsw1xx_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode, return 0; } +static void gsw1xx_pcs_clear_raneg(struct work_struct *work) +{ + struct gsw1xx_priv *priv = + container_of(work, struct gsw1xx_priv, clear_raneg.work); + + regmap_clear_bits(priv->sgmii, GSW1XX_SGMII_TBI_ANEGCTL, + GSW1XX_SGMII_TBI_ANEGCTL_RANEG); +} + static void gsw1xx_pcs_an_restart(struct phylink_pcs *pcs) { struct gsw1xx_priv *priv = pcs_to_gsw1xx(pcs); + cancel_delayed_work_sync(&priv->clear_raneg); + regmap_set_bits(priv->sgmii, GSW1XX_SGMII_TBI_ANEGCTL, GSW1XX_SGMII_TBI_ANEGCTL_RANEG); + + /* despite being documented as self-clearing, the RANEG bit + * sometimes remains set, preventing auto-negotiation from happening. + * MaxLinear advises to manually clear the bit after 10ms. + */ + schedule_delayed_work(&priv->clear_raneg, msecs_to_jiffies(10)); } static void gsw1xx_pcs_link_up(struct phylink_pcs *pcs, @@ -636,6 +658,8 @@ static int gsw1xx_probe(struct mdio_device *mdiodev) if (ret) return ret; + INIT_DELAYED_WORK(&priv->clear_raneg, gsw1xx_pcs_clear_raneg); + ret = gswip_probe_common(&priv->gswip, version); if (ret) return ret; @@ -648,16 +672,21 @@ static int gsw1xx_probe(struct mdio_device *mdiodev) static void gsw1xx_remove(struct mdio_device *mdiodev) { struct gswip_priv *priv = dev_get_drvdata(&mdiodev->dev); + struct gsw1xx_priv *gsw1xx_priv; if (!priv) return; dsa_unregister_switch(priv->ds); + + gsw1xx_priv = container_of(priv, struct gsw1xx_priv, gswip); + cancel_delayed_work_sync(&gsw1xx_priv->clear_raneg); } static void gsw1xx_shutdown(struct mdio_device *mdiodev) { struct gswip_priv *priv = dev_get_drvdata(&mdiodev->dev); + struct gsw1xx_priv *gsw1xx_priv; if (!priv) return; @@ -665,6 +694,9 @@ static void gsw1xx_shutdown(struct mdio_device *mdiodev) dsa_switch_shutdown(priv->ds); dev_set_drvdata(&mdiodev->dev, NULL); + + gsw1xx_priv = container_of(priv, struct gsw1xx_priv, gswip); + cancel_delayed_work_sync(&gsw1xx_priv->clear_raneg); } static const struct gswip_hw_info gsw12x_data = { -- 2.52.0