diff mbox series

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

Message ID 20240429131008.439231-1-maxime.chevallier@bootlin.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net-next] net: phy: Don't conditionally compile the phy_link_topology creation | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 4843 this patch: 4843
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 1041 this patch: 1041
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 5120 this patch: 5120
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 84 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-04-29--15-00 (tests: 994)

Commit Message

Maxime Chevallier April 29, 2024, 1:10 p.m. UTC
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.

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/
---
 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(-)

Comments

Heiner Kallweit April 30, 2024, 8:17 a.m. UTC | #1
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.

> 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/
> ---
>  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 */
Maxime Chevallier April 30, 2024, 11:57 a.m. UTC | #2
Hello Heiner,

On Tue, 30 Apr 2024 10:17:31 +0200
Heiner Kallweit <hkallweit1@gmail.com> 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
?"

Thanks,

Maxime

> 
> > 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/
> > ---
> >  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 */  
>
Maxime Chevallier April 30, 2024, 1:36 p.m. UTC | #3
On Tue, 30 Apr 2024 13:57:34 +0200
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:

> Hello Heiner,
> 
> On Tue, 30 Apr 2024 10:17:31 +0200
> Heiner Kallweit <hkallweit1@gmail.com> 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@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/
> > > ---
> > >  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 */    
> >   
> 
>
Heiner Kallweit April 30, 2024, 9:29 p.m. UTC | #4
On 30.04.2024 13:57, Maxime Chevallier wrote:
> Hello Heiner,
> 
> On Tue, 30 Apr 2024 10:17:31 +0200
> Heiner Kallweit <hkallweit1@gmail.com> 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
> ?"
> 
> Thanks,
> 
> Maxime
> 
>>
>>> 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/
>>> ---
>>>  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 */  
>>
> 

To go a little bit more into detail:

phy_link_topo_create() and phy_link_topo_destroy() are used in net/core/dev.c
only. Do you expect them to be ever used by other callers?
If not, their functionality could be moved to net/core/dev.c.
Supposedly guarded by IS_ENABLED(CONFIG_PHYLIB), alternatively a new config
symbol for link_topo support.

To get rid of the dependency you could also lazy-inizialize
netdev->link_topo. For this phy_link_topo_add_phy() would have
to take the netdev as first argument, not the topo.
Then the first call to phy_link_topo_add_phy() would initialize
netdev->link_topo.

I think functions like phy_link_topo_get_phy() should also check for
topo != NULL first, maybe combined with a WARN_ON().
They are exported and you have no control over its use.
Maxime Chevallier May 2, 2024, 7:05 a.m. UTC | #5
Hi,

On Tue, 30 Apr 2024 23:29:37 +0200
Heiner Kallweit <hkallweit1@gmail.com> wrote:

> On 30.04.2024 13:57, Maxime Chevallier wrote:
> > Hello Heiner,
> > 
> > On Tue, 30 Apr 2024 10:17:31 +0200
> > Heiner Kallweit <hkallweit1@gmail.com> 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
> > ?"
> > 
> > Thanks,
> > 
> > Maxime
> >   
> >>  
> >>> 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/
> >>> ---
> >>>  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 */    
> >>  
> >   
> 
> To go a little bit more into detail:
> 
> phy_link_topo_create() and phy_link_topo_destroy() are used in net/core/dev.c
> only. Do you expect them to be ever used by other callers?
> If not, their functionality could be moved to net/core/dev.c.
> Supposedly guarded by IS_ENABLED(CONFIG_PHYLIB), alternatively a new config
> symbol for link_topo support.
> 
> To get rid of the dependency you could also lazy-inizialize
> netdev->link_topo. For this phy_link_topo_add_phy() would have
> to take the netdev as first argument, not the topo.
> Then the first call to phy_link_topo_add_phy() would initialize
> netdev->link_topo.
> 
> I think functions like phy_link_topo_get_phy() should also check for
> topo != NULL first, maybe combined with a WARN_ON().
> They are exported and you have no control over its use.

Thanks Heiner for the explanations. I'll rework based on that. The
original reason I didn't directly include the netdev as a parameter for
these function, or didn't put any helper in net/core/dev.c is because I
wanted to avoid too strong of a link between the topology and netdev.

There are some PHYs for which we can't assign any netdev (PHYs that
would sit in-between 2 chained DSA switches is the only example I have
in mind though), but TBH the code in its actual shape doesn't address
these either, so it's a useless design constraint.

So, let me indeed do that, it will probably make for a simpler and more
straightforward design.

Thanks for the input,

Maxime
diff mbox series

Patch

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 */