Received-SPF: pass (google.com: domain of linux-kernel+bounces-162182-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 29 Apr 2024 13:55:42 +0200
From: Maxime Chevallier <maxime.chevallier@bootlin.com>
To: Heiner Kallweit <hkallweit1@gmail.com>
Cc: davem@davemloft.net, netdev@vger.kernel.org,
 linux-kernel@vger.kernel.org, thomas.petazzoni@bootlin.com, Andrew Lunn
 <andrew@lunn.ch>, Jakub Kicinski <kuba@kernel.org>, Eric Dumazet
 <edumazet@google.com>, Paolo Abeni <pabeni@redhat.com>, Russell King
 <linux@armlinux.org.uk>, linux-arm-kernel@lists.infradead.org, Christophe
 Leroy <christophe.leroy@csgroup.eu>, Herve Codina
 <herve.codina@bootlin.com>, Florian Fainelli <f.fainelli@gmail.com>,
 Vladimir Oltean <vladimir.oltean@nxp.com>, =?UTF-8?B?S8O2cnk=?= Maincent
 <kory.maincent@bootlin.com>, Jesse Brandeburg <jesse.brandeburg@intel.com>,
 Marek =?UTF-8?B?QmVow7pu?= <kabel@kernel.org>, Piergiorgio Beruto
 <piergiorgio.beruto@gmail.com>, Oleksij Rempel <o.rempel@pengutronix.de>,
 =?UTF-8?B?Tmljb2zDsg==?= Veronese <nicveronese@gmail.com>, Simon Horman
 <horms@kernel.org>, mwojtas@chromium.org, Nathan Chancellor
 <nathan@kernel.org>, Antoine Tenart <atenart@kernel.org>
Subject: Re: [PATCH net-next] net: phy: phy_link_topology: Handle NULL
 topologies
Message-ID: <20240429135542.73d039b2@device-28.home>
In-Reply-To: <d33888a9-3c26-47be-bead-5270b955307b@gmail.com>
References: <20240412104615.3779632-1-maxime.chevallier@bootlin.com>
	<c37482d9-f97b-4f9a-8a2d-efde1a654514@gmail.com>
	<20240412152335.751a8dbb@device-28.home>
	<d33888a9-3c26-47be-bead-5270b955307b@gmail.com>
Organization: Bootlin
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

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;    
> >>  
> >   
>