lkml.org 
[lkml]   [2024]   [Apr]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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;
> >>
> >
>


\
 
 \ /
  Last update: 2024-04-29 13:56    [W:0.068 / U:0.156 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site