Re: [PATCH net-next] net: phy: Don't conditionally compile the phy_link_topology creation

[Maxime Chevallier]

On Tue, 30 Apr 2024 13:57:34 +0200
Maxime Chevallier <maxime.chevallier@xxxxxxxxxxx> wrote:

> Hello Heiner,
> 
> On Tue, 30 Apr 2024 10:17:31 +0200
> Heiner Kallweit <hkallweit1@xxxxxxxxx> wrote:
> 
> > On 29.04.2024 15:10, Maxime Chevallier wrote:  
> > > The core of the phy_link_topology isn't directly tied to phylib, and at
> > > the moment it's initialized, phylib might not be loaded yet. Move the
> > > initialization of the topology to the phy_link_topology_core header,
> > > which contains the bare minimum so that we can initialize it at netdev
> > > creation.
> > >     
> > 
> > The change fixes the issue for me, but according to my personal taste
> > the code isn't intuitive and still error-prone. Also there's no good
> > reason to inline a function like phy_link_topo_create() and make it
> > publicly available. Do you expect it to be ever used outside net core?
> > In general it may make sense to add a config symbol for the topology
> > extension, there seem to be very few, specialized use cases for it.  
> 
> I think I'm missing the point here then. Do you mean adding a Kconfig
> option to explicitely turn phy_link_topology on ? or build it as a
> dedicated kernel module ?
> 
> Or do you see something such as "if phylib is M or Y, then build the
> topology stuff and make sure it's allocated when a netdev gets created
> ?"

I've prototyped something that's cleaner and should fit what you
described, which is to have a Kconfig option for phy_topology and
have it autoselected by CONFIG_SFP (for now, the only case where we can
have multiple PHYs on the link). When phy mux support is added (I'll
followup with that once the topology is settled), we can also make is
select the phy_topology config option. I'll send that patch when I'll
have properly tested it, especially with all the different bits
(phylib, sfp, drivers) being tested as modules or builtin.

Thanks for the tips,

Maxime

> 
> Thanks,
> 
> Maxime
> 
> >   
> > > 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/
> > > ---
> > >  drivers/net/phy/phy_link_topology.c    | 23 --------------------
> > >  include/linux/phy_link_topology.h      |  5 -----
> > >  include/linux/phy_link_topology_core.h | 30 +++++++++++++++++---------
> > >  3 files changed, 20 insertions(+), 38 deletions(-)
> > > 
> > > diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
> > > index 985941c5c558..960aedd73308 100644
> > > --- a/drivers/net/phy/phy_link_topology.c
> > > +++ b/drivers/net/phy/phy_link_topology.c
> > > @@ -12,29 +12,6 @@
> > >  #include <linux/rtnetlink.h>
> > >  #include <linux/xarray.h>
> > >  
> > > -struct phy_link_topology *phy_link_topo_create(struct net_device *dev)
> > > -{
> > > -	struct phy_link_topology *topo;
> > > -
> > > -	topo = kzalloc(sizeof(*topo), GFP_KERNEL);
> > > -	if (!topo)
> > > -		return ERR_PTR(-ENOMEM);
> > > -
> > > -	xa_init_flags(&topo->phys, XA_FLAGS_ALLOC1);
> > > -	topo->next_phy_index = 1;
> > > -
> > > -	return topo;
> > > -}
> > > -
> > > -void phy_link_topo_destroy(struct phy_link_topology *topo)
> > > -{
> > > -	if (!topo)
> > > -		return;
> > > -
> > > -	xa_destroy(&topo->phys);
> > > -	kfree(topo);
> > > -}
> > > -
> > >  int phy_link_topo_add_phy(struct phy_link_topology *topo,
> > >  			  struct phy_device *phy,
> > >  			  enum phy_upstream upt, void *upstream)
> > > diff --git a/include/linux/phy_link_topology.h b/include/linux/phy_link_topology.h
> > > index 6b79feb607e7..ad72d7881257 100644
> > > --- a/include/linux/phy_link_topology.h
> > > +++ b/include/linux/phy_link_topology.h
> > > @@ -32,11 +32,6 @@ struct phy_device_node {
> > >  	struct phy_device *phy;
> > >  };
> > >  
> > > -struct phy_link_topology {
> > > -	struct xarray phys;
> > > -	u32 next_phy_index;
> > > -};
> > > -
> > >  static inline struct phy_device *
> > >  phy_link_topo_get_phy(struct phy_link_topology *topo, u32 phyindex)
> > >  {
> > > diff --git a/include/linux/phy_link_topology_core.h b/include/linux/phy_link_topology_core.h
> > > index 0a6479055745..0116ec49cd1b 100644
> > > --- a/include/linux/phy_link_topology_core.h
> > > +++ b/include/linux/phy_link_topology_core.h
> > > @@ -2,24 +2,34 @@
> > >  #ifndef __PHY_LINK_TOPOLOGY_CORE_H
> > >  #define __PHY_LINK_TOPOLOGY_CORE_H
> > >  
> > > -struct phy_link_topology;
> > > +#include <linux/xarray.h>
> > >  
> > > -#if IS_REACHABLE(CONFIG_PHYLIB)
> > > -
> > > -struct phy_link_topology *phy_link_topo_create(struct net_device *dev);
> > > -void phy_link_topo_destroy(struct phy_link_topology *topo);
> > > -
> > > -#else
> > > +struct phy_link_topology {
> > > +	struct xarray phys;
> > > +	u32 next_phy_index;
> > > +};
> > >  
> > >  static inline struct phy_link_topology *phy_link_topo_create(struct net_device *dev)
> > >  {
> > > -	return NULL;
> > > +	struct phy_link_topology *topo;
> > > +
> > > +	topo = kzalloc(sizeof(*topo), GFP_KERNEL);
> > > +	if (!topo)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	xa_init_flags(&topo->phys, XA_FLAGS_ALLOC1);
> > > +	topo->next_phy_index = 1;
> > > +
> > > +	return topo;
> > >  }
> > >  
> > >  static inline void phy_link_topo_destroy(struct phy_link_topology *topo)
> > >  {
> > > -}
> > > +	if (!topo)
> > > +		return;
> > >  
> > > -#endif
> > > +	xa_destroy(&topo->phys);
> > > +	kfree(topo);
> > > +}
> > >  
> > >  #endif /* __PHY_LINK_TOPOLOGY_CORE_H */    
> >   
> 
>