Messages in this thread | | | Date | Tue, 30 Apr 2024 23:29:37 +0200 | Subject | Re: [PATCH net-next] net: phy: Don't conditionally compile the phy_link_topology creation | From | Heiner Kallweit <> |
| |
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.
| |