Hi Romain On Tue, Apr 30, 2024 at 09:29:42AM +0200, Romain Gantois wrote: > From: Serge Semin <fancer.lancer@xxxxxxxxx> > > Currently the XPCS handler destruction is performed in the > stmmac_mdio_unregister() method. It doesn't look good because the handler > isn't originally created in the corresponding protagonist > stmmac_mdio_unregister(), but in the stmmac_xpcs_setup() function. In > order to have more coherent MDIO and XPCS setup/cleanup procedures, > let's move the DW XPCS destruction to the dedicated stmmac_pcs_clean() > method. > > This method will also be used to cleanup PCS hardware using the > pcs_exit() callback that will be introduced to stmmac in a subsequent > patch. > > Signed-off-by: Serge Semin <fancer.lancer@xxxxxxxxx> > Co-developed-by: Romain Gantois <romain.gantois@xxxxxxxxxxx> > Signed-off-by: Romain Gantois <romain.gantois@xxxxxxxxxxx> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac.h | 1 + > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 +++++- > drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 12 +++++++++--- > 3 files changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h > index dddcaa9220cc3..7e0d727ed795b 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h > @@ -361,6 +361,7 @@ int stmmac_mdio_unregister(struct net_device *ndev); > int stmmac_mdio_register(struct net_device *ndev); > int stmmac_mdio_reset(struct mii_bus *mii); > int stmmac_xpcs_setup(struct mii_bus *mii); > +void stmmac_pcs_clean(struct stmmac_priv *priv); > void stmmac_set_ethtool_ops(struct net_device *netdev); > > int stmmac_init_tstamp_counter(struct stmmac_priv *priv, u32 systime_flags); > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index 59bf83904b62d..2a55c5d07f6b8 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -7789,8 +7789,9 @@ int stmmac_dvr_probe(struct device *device, > > error_netdev_register: > phylink_destroy(priv->phylink); > -error_xpcs_setup: > error_phy_setup: > + stmmac_pcs_clean(priv); > +error_xpcs_setup: > if (priv->hw->pcs != STMMAC_PCS_TBI && > priv->hw->pcs != STMMAC_PCS_RTBI) > stmmac_mdio_unregister(ndev); > @@ -7832,6 +7833,9 @@ void stmmac_dvr_remove(struct device *dev) > if (priv->plat->stmmac_rst) > reset_control_assert(priv->plat->stmmac_rst); > reset_control_assert(priv->plat->stmmac_ahb_rst); > + > + stmmac_pcs_clean(priv); > + > if (priv->hw->pcs != STMMAC_PCS_TBI && > priv->hw->pcs != STMMAC_PCS_RTBI) > stmmac_mdio_unregister(ndev); > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c > index 0542cfd1817e6..508bd39cbe2b3 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c > @@ -523,6 +523,15 @@ int stmmac_xpcs_setup(struct mii_bus *bus) > return 0; > } > > +void stmmac_pcs_clean(struct stmmac_priv *priv) Ideally it would have been great to have the entire driver fixed to accept the stmmac_priv pointer as the functions argument. But this would be too tiresome. Anyway seeing the PCS-setup protagonist method has the net_device pointer passed I would implement the same prototype for the antagonist even though it would require an additional local variable. That will make the MDIO and PCS local interface-functions looking alike and as if unified. That is the reason of why I made stmmac_xpcs_clean() accepting the net_device pointer. Alternatively both stmmac_pcs_setup() and stmmac_pcs_clean() could be converted to just accepting a pointer to the stmmac_priv instance. -Serge(y) > +{ > + if (!priv->hw->xpcs) > + return; > + > + xpcs_destroy(priv->hw->xpcs); > + priv->hw->xpcs = NULL; > +} > + > /** > * stmmac_mdio_register > * @ndev: net device structure > @@ -679,9 +688,6 @@ int stmmac_mdio_unregister(struct net_device *ndev) > if (!priv->mii) > return 0; > > - if (priv->hw->xpcs) > - xpcs_destroy(priv->hw->xpcs); > - > mdiobus_unregister(priv->mii); > priv->mii->priv = NULL; > mdiobus_free(priv->mii); > > -- > 2.44.0 >