Messages in this thread | | | Date | Mon, 29 Apr 2024 13:55:42 +0200 | From | Maxime Chevallier <> | Subject | Re: [PATCH net-next] net: phy: phy_link_topology: Handle NULL topologies |
| |
Hello Heiner,
On Sat, 27 Apr 2024 21:34:21 +0200 Heiner Kallweit <hkallweit1@gmail.com> wrote:
> 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?
It still is indeed. I've been very busy last week unfortunately, I'll followup today though. Thanks for the patience,
Maxime
> > > 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; > >> > > >
| |