Re: [PATCH net-next] net: phy: phy_link_topology: Handle NULL topologies

[Maxime Chevallier]

Hello Heiner,

On Fri, 12 Apr 2024 15:07:46 +0200
Heiner Kallweit <hkallweit1@xxxxxxxxx> wrote:

> On 12.04.2024 12:46, Maxime Chevallier wrote:
> > In situations where phylib is a module, the topology can be NULL as it's
> > not initialized at netdev creation.
> >   
> 
> What we see here is a bigger drawback of IS_REACHABLE(). For phylib it's
> false from net core, but true from r8169 driver. So topo_create is a stub,
> but topo_add is not. IS_REACHABLE() hides dependencies.
> 
> topo_create et al don't really use something from phylib.
> Therefore, could/should it be moved to net core?

That's a valid point, and a better solution indeed.

> At least for topo_create this would resolve the dependency.
> 
> We could also add a config symbol and the PHY topology an optional
> extension of net core.

That could be a thing indeed. It could be selected by phylib then, I
don't see it being a user-controlled option, as this would make it very
confusing for users to only be able to see when there are mutiple PHYs
on the link when the relevant option is enabled (but I might be wrong).

Maxime

> 
> > Allow passing a NULL topology pointer to phy_link_topo helpers.
> > 
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@xxxxxxxxxxx>
> > Closes: https://lore.kernel.org/netdev/2e11b89d-100f-49e7-9c9a-834cc0b82f97@xxxxxxxxx/
> > Closes: https://lore.kernel.org/netdev/20240409201553.GA4124869@dev-arch.thelio-3990X/
> > ---
> > 
> > Hi,
> > 
> > This patch fixes a commit that is in net-next, hence the net-next tag and the
> > lack of "Fixes" tag.
> > 
> > Nathan, Heiner, can you confirm this solves what you're seeing ?
> > 
> > I think we can improve on this solution by moving the topology init at
> > the first PHY insertion and clearing it at netdev destruction.
> > 
> > Maxime
> > 
> >  drivers/net/phy/phy_link_topology.c | 10 +++++++++-
> >  include/linux/phy_link_topology.h   |  7 ++++++-
> >  2 files changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
> > index 985941c5c558..0f3973f07fac 100644
> > --- a/drivers/net/phy/phy_link_topology.c
> > +++ b/drivers/net/phy/phy_link_topology.c
> > @@ -42,6 +42,9 @@ int phy_link_topo_add_phy(struct phy_link_topology *topo,
> >  	struct phy_device_node *pdn;
> >  	int ret;
> >  
> > +	if (!topo)
> > +		return 0;
> > +
> >  	pdn = kzalloc(sizeof(*pdn), GFP_KERNEL);
> >  	if (!pdn)
> >  		return -ENOMEM;
> > @@ -93,7 +96,12 @@ EXPORT_SYMBOL_GPL(phy_link_topo_add_phy);
> >  void phy_link_topo_del_phy(struct phy_link_topology *topo,
> >  			   struct phy_device *phy)
> >  {
> > -	struct phy_device_node *pdn = xa_erase(&topo->phys, phy->phyindex);
> > +	struct phy_device_node *pdn;
> > +
> > +	if (!topo)
> > +		return;
> > +
> > +	pdn = xa_erase(&topo->phys, phy->phyindex);
> >  
> >  	/* We delete the PHY from the topology, however we don't re-set the
> >  	 * phy->phyindex field. If the PHY isn't gone, we can re-assign it the
> > diff --git a/include/linux/phy_link_topology.h b/include/linux/phy_link_topology.h
> > index 6b79feb607e7..21ca78127d0f 100644
> > --- a/include/linux/phy_link_topology.h
> > +++ b/include/linux/phy_link_topology.h
> > @@ -40,7 +40,12 @@ struct phy_link_topology {
> >  static inline struct phy_device *
> >  phy_link_topo_get_phy(struct phy_link_topology *topo, u32 phyindex)
> >  {
> > -	struct phy_device_node *pdn = xa_load(&topo->phys, phyindex);
> > +	struct phy_device_node *pdn;
> > +
> > +	if (!topo)
> > +		return NULL;
> > +
> > +	pdn = xa_load(&topo->phys, phyindex);
> >  
> >  	if (pdn)
> >  		return pdn->phy;  
>