Messages in this thread | | | Date | Sat, 27 Apr 2024 21:34:21 +0200 | Subject | Re: [PATCH net-next] net: phy: phy_link_topology: Handle NULL topologies | From | Heiner Kallweit <> |
| |
On 12.04.2024 15:23, Maxime Chevallier wrote: > Hello Heiner, > > On Fri, 12 Apr 2024 15:07:46 +0200 > Heiner Kallweit <hkallweit1@gmail.com> wrote: > >> On 12.04.2024 12:46, Maxime Chevallier wrote: >>> In situations where phylib is a module, the topology can be NULL as it's >>> not initialized at netdev creation. >>> >> >> What we see here is a bigger drawback of IS_REACHABLE(). For phylib it's >> false from net core, but true from r8169 driver. So topo_create is a stub, >> but topo_add is not. IS_REACHABLE() hides dependencies. >> >> topo_create et al don't really use something from phylib. >> Therefore, could/should it be moved to net core? > > That's a valid point, and a better solution indeed. > >> At least for topo_create this would resolve the dependency. >> >> We could also add a config symbol and the PHY topology an optional >> extension of net core. > > That could be a thing indeed. It could be selected by phylib then, I > don't see it being a user-controlled option, as this would make it very > confusing for users to only be able to see when there are mutiple PHYs > on the link when the relevant option is enabled (but I might be wrong). > AFAIK the issue still exists on net-next. Are you going to submit an updated version?
> Maxime > >> >>> Allow passing a NULL topology pointer to phy_link_topo helpers. >>> >>> 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/ >>> --- >>> >>> Hi, >>> >>> This patch fixes a commit that is in net-next, hence the net-next tag and the >>> lack of "Fixes" tag. >>> >>> Nathan, Heiner, can you confirm this solves what you're seeing ? >>> >>> I think we can improve on this solution by moving the topology init at >>> the first PHY insertion and clearing it at netdev destruction. >>> >>> Maxime >>> >>> drivers/net/phy/phy_link_topology.c | 10 +++++++++- >>> include/linux/phy_link_topology.h | 7 ++++++- >>> 2 files changed, 15 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c >>> index 985941c5c558..0f3973f07fac 100644 >>> --- a/drivers/net/phy/phy_link_topology.c >>> +++ b/drivers/net/phy/phy_link_topology.c >>> @@ -42,6 +42,9 @@ int phy_link_topo_add_phy(struct phy_link_topology *topo, >>> struct phy_device_node *pdn; >>> int ret; >>> >>> + if (!topo) >>> + return 0; >>> + >>> pdn = kzalloc(sizeof(*pdn), GFP_KERNEL); >>> if (!pdn) >>> return -ENOMEM; >>> @@ -93,7 +96,12 @@ EXPORT_SYMBOL_GPL(phy_link_topo_add_phy); >>> void phy_link_topo_del_phy(struct phy_link_topology *topo, >>> struct phy_device *phy) >>> { >>> - struct phy_device_node *pdn = xa_erase(&topo->phys, phy->phyindex); >>> + struct phy_device_node *pdn; >>> + >>> + if (!topo) >>> + return; >>> + >>> + pdn = xa_erase(&topo->phys, phy->phyindex); >>> >>> /* We delete the PHY from the topology, however we don't re-set the >>> * phy->phyindex field. If the PHY isn't gone, we can re-assign it the >>> diff --git a/include/linux/phy_link_topology.h b/include/linux/phy_link_topology.h >>> index 6b79feb607e7..21ca78127d0f 100644 >>> --- a/include/linux/phy_link_topology.h >>> +++ b/include/linux/phy_link_topology.h >>> @@ -40,7 +40,12 @@ struct phy_link_topology { >>> static inline struct phy_device * >>> phy_link_topo_get_phy(struct phy_link_topology *topo, u32 phyindex) >>> { >>> - struct phy_device_node *pdn = xa_load(&topo->phys, phyindex); >>> + struct phy_device_node *pdn; >>> + >>> + if (!topo) >>> + return NULL; >>> + >>> + pdn = xa_load(&topo->phys, phyindex); >>> >>> if (pdn) >>> return pdn->phy; >> >
| |