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 <[email protected]>
Closes: https://lore.kernel.org/netdev/[email protected]/
Closes: https://lore.kernel.org/netdev/[email protected]/
---
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 */
--
2.44.0
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 <[email protected]>
> Closes: https://lore.kernel.org/netdev/[email protected]/
> Closes: https://lore.kernel.org/netdev/[email protected]/
> ---
> 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 */
Hello Heiner,
On Tue, 30 Apr 2024 10:17:31 +0200
Heiner Kallweit <[email protected]> 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 <[email protected]>
> > Closes: https://lore.kernel.org/netdev/[email protected]/
> > Closes: https://lore.kernel.org/netdev/[email protected]/
> > ---
> > 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 */
>
On Tue, 30 Apr 2024 13:57:34 +0200
Maxime Chevallier <[email protected]> wrote:
> Hello Heiner,
>
> On Tue, 30 Apr 2024 10:17:31 +0200
> Heiner Kallweit <[email protected]> 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 <[email protected]>
> > > Closes: https://lore.kernel.org/netdev/[email protected]/
> > > Closes: https://lore.kernel.org/netdev/[email protected]/
> > > ---
> > > 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 */
> >
>
>
On 30.04.2024 13:57, Maxime Chevallier wrote:
> Hello Heiner,
>
> On Tue, 30 Apr 2024 10:17:31 +0200
> Heiner Kallweit <[email protected]> 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 <[email protected]>
>>> Closes: https://lore.kernel.org/netdev/[email protected]/
>>> Closes: https://lore.kernel.org/netdev/[email protected]/
>>> ---
>>> 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.
Hi,
On Tue, 30 Apr 2024 23:29:37 +0200
Heiner Kallweit <[email protected]> wrote:
> On 30.04.2024 13:57, Maxime Chevallier wrote:
> > Hello Heiner,
> >
> > On Tue, 30 Apr 2024 10:17:31 +0200
> > Heiner Kallweit <[email protected]> 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 <[email protected]>
> >>> Closes: https://lore.kernel.org/netdev/[email protected]/
> >>> Closes: https://lore.kernel.org/netdev/[email protected]/
> >>> ---
> >>> 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