Messages in this thread | | | From | George McCollister <> | Date | Mon, 13 Sep 2021 08:38:05 -0500 | Subject | Re: [RFC PATCH net 5/5] net: dsa: xrs700x: be compatible with masters which unregister on shutdown |
| |
On Sun, Sep 12, 2021 at 7:09 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > Since commit 2f1e8ea726e9 ("net: dsa: link interfaces with the DSA > master to get rid of lockdep warnings"), DSA gained a requirement which > it did not fulfill, which is to unlink itself from the DSA master at > shutdown time. > > Since the Arrow SpeedChips XRS700x driver was introduced after the bad > commit, it has never worked with DSA masters which decide to unregister > their net_device on shutdown, effectively hanging the reboot process. > To fix that, we need to call dsa_switch_shutdown. > > These devices can be connected by I2C or by MDIO, and if I search for > I2C or MDIO bus drivers that implement their ->shutdown by redirecting > it to ->remove I don't see any, however this does not mean it would not > be possible. To be compatible with that pattern, it is necessary to > implement an "if this then not that" scheme, to avoid ->remove and > ->shutdown from being called both for the same struct device. > > Fixes: ee00b24f32eb ("net: dsa: add Arrow SpeedChips XRS700x driver") > Link: https://lore.kernel.org/netdev/20210909095324.12978-1-LinoSanfilippo@gmx.de/ > Reported-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > drivers/net/dsa/xrs700x/xrs700x.c | 6 ++++++ > drivers/net/dsa/xrs700x/xrs700x.h | 1 + > drivers/net/dsa/xrs700x/xrs700x_i2c.c | 18 ++++++++++++++++++ > drivers/net/dsa/xrs700x/xrs700x_mdio.c | 18 ++++++++++++++++++ > 4 files changed, 43 insertions(+) > > diff --git a/drivers/net/dsa/xrs700x/xrs700x.c b/drivers/net/dsa/xrs700x/xrs700x.c > index 130abb0f1438..469420941054 100644 > --- a/drivers/net/dsa/xrs700x/xrs700x.c > +++ b/drivers/net/dsa/xrs700x/xrs700x.c > @@ -822,6 +822,12 @@ void xrs700x_switch_remove(struct xrs700x *priv) > } > EXPORT_SYMBOL(xrs700x_switch_remove); > > +void xrs700x_switch_shutdown(struct xrs700x *priv) > +{ > + dsa_switch_shutdown(priv->ds); > +} > +EXPORT_SYMBOL(xrs700x_switch_shutdown); > + > MODULE_AUTHOR("George McCollister <george.mccollister@gmail.com>"); > MODULE_DESCRIPTION("Arrow SpeedChips XRS700x DSA driver"); > MODULE_LICENSE("GPL v2"); > diff --git a/drivers/net/dsa/xrs700x/xrs700x.h b/drivers/net/dsa/xrs700x/xrs700x.h > index ff62cf61b091..4d58257471d2 100644 > --- a/drivers/net/dsa/xrs700x/xrs700x.h > +++ b/drivers/net/dsa/xrs700x/xrs700x.h > @@ -40,3 +40,4 @@ struct xrs700x { > struct xrs700x *xrs700x_switch_alloc(struct device *base, void *devpriv); > int xrs700x_switch_register(struct xrs700x *priv); > void xrs700x_switch_remove(struct xrs700x *priv); > +void xrs700x_switch_shutdown(struct xrs700x *priv); > diff --git a/drivers/net/dsa/xrs700x/xrs700x_i2c.c b/drivers/net/dsa/xrs700x/xrs700x_i2c.c > index 489d9385b4f0..6deae388a0d6 100644 > --- a/drivers/net/dsa/xrs700x/xrs700x_i2c.c > +++ b/drivers/net/dsa/xrs700x/xrs700x_i2c.c > @@ -109,11 +109,28 @@ static int xrs700x_i2c_remove(struct i2c_client *i2c) > { > struct xrs700x *priv = i2c_get_clientdata(i2c); > > + if (!priv) > + return 0; > + > xrs700x_switch_remove(priv); > > + i2c_set_clientdata(i2c, NULL); > + > return 0; > } > > +static void xrs700x_i2c_shutdown(struct i2c_client *i2c) > +{ > + struct xrs700x *priv = i2c_get_clientdata(i2c); > + > + if (!priv) > + return; > + > + xrs700x_switch_shutdown(priv); > + > + i2c_set_clientdata(i2c, NULL); > +} > + > static const struct i2c_device_id xrs700x_i2c_id[] = { > { "xrs700x-switch", 0 }, > {}, > @@ -137,6 +154,7 @@ static struct i2c_driver xrs700x_i2c_driver = { > }, > .probe = xrs700x_i2c_probe, > .remove = xrs700x_i2c_remove, > + .shutdown = xrs700x_i2c_shutdown, > .id_table = xrs700x_i2c_id, > }; > > diff --git a/drivers/net/dsa/xrs700x/xrs700x_mdio.c b/drivers/net/dsa/xrs700x/xrs700x_mdio.c > index 44f58bee04a4..d01cf1073d49 100644 > --- a/drivers/net/dsa/xrs700x/xrs700x_mdio.c > +++ b/drivers/net/dsa/xrs700x/xrs700x_mdio.c > @@ -136,7 +136,24 @@ static void xrs700x_mdio_remove(struct mdio_device *mdiodev) > { > struct xrs700x *priv = dev_get_drvdata(&mdiodev->dev); > > + if (!priv) > + return; > + > xrs700x_switch_remove(priv); > + > + dev_set_drvdata(&mdiodev->dev, NULL); > +} > + > +static void xrs700x_mdio_shutdown(struct mdio_device *mdiodev) > +{ > + struct xrs700x *priv = dev_get_drvdata(&mdiodev->dev); > + > + if (!priv) > + return; > + > + xrs700x_switch_shutdown(priv); > + > + dev_set_drvdata(&mdiodev->dev, NULL); > } > > static const struct of_device_id __maybe_unused xrs700x_mdio_dt_ids[] = { > @@ -155,6 +172,7 @@ static struct mdio_driver xrs700x_mdio_driver = { > }, > .probe = xrs700x_mdio_probe, > .remove = xrs700x_mdio_remove, > + .shutdown = xrs700x_mdio_shutdown, > }; > > mdio_module_driver(xrs700x_mdio_driver); > -- > 2.25.1 >
Looks good to me. Assuming we do Reviewed-by for RFCs: Reviewed-by: George McCollister <george.mccollister@gmail.com>
| |