Re: [RESEND PATCH net-next v5 1/3] net: stmmac: split xPCS setup from mdio register

[Vladimir Oltean]

On Fri, Jun 04, 2021 at 02:51:53PM +0300, Vladimir Oltean wrote:
> On Fri, Jun 04, 2021 at 06:57:31PM +0800, Michael Sit Wei Hong wrote:
> > From: Voon Weifeng <weifeng.voon@xxxxxxxxx>
> > 
> > This patch is a preparation patch for the enabling of Intel mGbE 2.5Gbps
> > link speed. The Intel mGbR link speed configuration (1G/2.5G) is depends on
> > a mdio ADHOC register which can be configured in the bios menu.
> > As PHY interface might be different for 1G and 2.5G, the mdio bus need be
> > ready to check the link speed and select the PHY interface before probing
> > the xPCS.
> > 
> > Signed-off-by: Voon Weifeng <weifeng.voon@xxxxxxxxx>
> > Signed-off-by: Michael Sit Wei Hong <michael.wei.hong.sit@xxxxxxxxx>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/stmmac.h  |  1 +
> >  .../net/ethernet/stmicro/stmmac/stmmac_main.c |  7 ++
> >  .../net/ethernet/stmicro/stmmac/stmmac_mdio.c | 64 ++++++++++---------
> >  3 files changed, 43 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> > index b6cd43eda7ac..fd7212afc543 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> > @@ -311,6 +311,7 @@ enum stmmac_state {
> >  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_set_ethtool_ops(struct net_device *netdev);
> >  
> >  void stmmac_ptp_register(struct stmmac_priv *priv);
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 6d41dd6f9f7a..c1331c07623d 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -6991,6 +6991,12 @@ int stmmac_dvr_probe(struct device *device,
> >  		}
> >  	}
> >  
> > +	if (priv->plat->mdio_bus_data->has_xpcs) {
> 
> stmmac_mdio_register has:
> 
> 	if (!mdio_bus_data)
> 		return 0;
> 
> which suggests that some platforms might not populate priv->plat->mdio_bus_data.
> 
> Are you sure it is safe to go straight to dereferencing mdio_bus_data->has_xpcs
> in the common driver probe function?

This patch seems to agree with me:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=593f555fbc6091bbaec8dd2a38b47ee643412e61