* [PATCH net-next] net: phy: Don't conditionally compile the phy_link_topology creation @ 2024-04-29 13:10 Maxime Chevallier 2024-04-30 8:17 ` Heiner Kallweit 0 siblings, 1 reply; 6+ messages in thread From: Maxime Chevallier @ 2024-04-29 13:10 UTC (permalink / raw) To: davem Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni, Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit, Vladimir Oltean, Köry Maincent, Jesse Brandeburg, Marek Behún, Piergiorgio Beruto, Oleksij Rempel, Nicolò Veronese, Simon Horman, mwojtas, Nathan Chancellor, Antoine Tenart 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(-) 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: phy: Don't conditionally compile the phy_link_topology creation 2024-04-29 13:10 [PATCH net-next] net: phy: Don't conditionally compile the phy_link_topology creation Maxime Chevallier @ 2024-04-30 8:17 ` Heiner Kallweit 2024-04-30 11:57 ` Maxime Chevallier 0 siblings, 1 reply; 6+ messages in thread From: Heiner Kallweit @ 2024-04-30 8:17 UTC (permalink / raw) To: Maxime Chevallier, davem Cc: netdev, linux-kernel, thomas.petazzoni, Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina, Florian Fainelli, Vladimir Oltean, Köry Maincent, Jesse Brandeburg, Marek Behún, Piergiorgio Beruto, Oleksij Rempel, Nicolò Veronese, Simon Horman, mwojtas, Nathan Chancellor, Antoine Tenart 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 */ _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: phy: Don't conditionally compile the phy_link_topology creation 2024-04-30 8:17 ` Heiner Kallweit @ 2024-04-30 11:57 ` Maxime Chevallier 2024-04-30 13:36 ` Maxime Chevallier 2024-04-30 21:29 ` Heiner Kallweit 0 siblings, 2 replies; 6+ messages in thread From: Maxime Chevallier @ 2024-04-30 11:57 UTC (permalink / raw) To: Heiner Kallweit Cc: davem, netdev, linux-kernel, thomas.petazzoni, Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina, Florian Fainelli, Vladimir Oltean, Köry Maincent, Jesse Brandeburg, Marek Behún, Piergiorgio Beruto, Oleksij Rempel, Nicolò Veronese, Simon Horman, mwojtas, Nathan Chancellor, Antoine Tenart 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 */ > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: phy: Don't conditionally compile the phy_link_topology creation 2024-04-30 11:57 ` Maxime Chevallier @ 2024-04-30 13:36 ` Maxime Chevallier 2024-04-30 21:29 ` Heiner Kallweit 1 sibling, 0 replies; 6+ messages in thread From: Maxime Chevallier @ 2024-04-30 13:36 UTC (permalink / raw) To: Heiner Kallweit Cc: davem, netdev, linux-kernel, thomas.petazzoni, Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina, Florian Fainelli, Vladimir Oltean, Köry Maincent, Jesse Brandeburg, Marek Behún, Piergiorgio Beruto, Oleksij Rempel, Nicolò Veronese, Simon Horman, mwojtas, Nathan Chancellor, Antoine Tenart 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 */ > > > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: phy: Don't conditionally compile the phy_link_topology creation 2024-04-30 11:57 ` Maxime Chevallier 2024-04-30 13:36 ` Maxime Chevallier @ 2024-04-30 21:29 ` Heiner Kallweit 2024-05-02 7:05 ` Maxime Chevallier 1 sibling, 1 reply; 6+ messages in thread From: Heiner Kallweit @ 2024-04-30 21:29 UTC (permalink / raw) To: Maxime Chevallier Cc: davem, netdev, linux-kernel, thomas.petazzoni, Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina, Florian Fainelli, Vladimir Oltean, Köry Maincent, Jesse Brandeburg, Marek Behún, Piergiorgio Beruto, Oleksij Rempel, Nicolò Veronese, Simon Horman, mwojtas, Nathan Chancellor, Antoine Tenart 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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: phy: Don't conditionally compile the phy_link_topology creation 2024-04-30 21:29 ` Heiner Kallweit @ 2024-05-02 7:05 ` Maxime Chevallier 0 siblings, 0 replies; 6+ messages in thread From: Maxime Chevallier @ 2024-05-02 7:05 UTC (permalink / raw) To: Heiner Kallweit Cc: davem, netdev, linux-kernel, thomas.petazzoni, Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina, Florian Fainelli, Vladimir Oltean, Köry Maincent, Jesse Brandeburg, Marek Behún, Piergiorgio Beruto, Oleksij Rempel, Nicolò Veronese, Simon Horman, mwojtas, Nathan Chancellor, Antoine Tenart 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-05-02 7:06 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-04-29 13:10 [PATCH net-next] net: phy: Don't conditionally compile the phy_link_topology creation Maxime Chevallier 2024-04-30 8:17 ` Heiner Kallweit 2024-04-30 11:57 ` Maxime Chevallier 2024-04-30 13:36 ` Maxime Chevallier 2024-04-30 21:29 ` Heiner Kallweit 2024-05-02 7:05 ` Maxime Chevallier
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox