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

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hello Heiner,

On Sat, 27 Apr 2024 21:34:21 +0200
Heiner Kallweit <hkallweit1@gmail.com> wrote:

> On 12.04.2024 15:23, Maxime Chevallier wrote:
> > Hello Heiner,
> > 
> > On Fri, 12 Apr 2024 15:07:46 +0200
> > Heiner Kallweit <hkallweit1@gmail.com> 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).
> >   
> AFAIK the issue still exists on net-next. Are you going to submit
> an updated version?

It still is indeed. I've been very busy last week unfortunately, I'll
followup today though. Thanks for the patience,

Maxime

> 
> > Maxime
> >   
> >>  
> >>> Allow passing a NULL topology pointer to phy_link_topo helpers.
> >>>
> >>> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> >>> Closes: https://lore.kernel.org/netdev/2e11b89d-100f-49e7-9c9a-834cc0b82f97@gmail.com/
> >>> 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;    
> >>  
> >   
>