netdev - Re: [RFC PATCH net-next v3 01/13] net: phy: Introduce ethernet link topology representation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20231209170241.GA5817@kernel.org>
Date: Sat, 9 Dec 2023 17:02:41 +0000
From: Simon Horman <horms@...nel.org>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc: davem@...emloft.net, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com,
	Andrew Lunn <andrew@...n.ch>, Jakub Kicinski <kuba@...nel.org>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Russell King <linux@...linux.org.uk>,
	linux-arm-kernel@...ts.infradead.org,
	Christophe Leroy <christophe.leroy@...roup.eu>,
	Herve Codina <herve.codina@...tlin.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Vladimir Oltean <vladimir.oltean@....com>,
	Köry Maincent <kory.maincent@...tlin.com>,
	Jesse Brandeburg <jesse.brandeburg@...el.com>,
	Jonathan Corbet <corbet@....net>,
	Marek Behún <kabel@...nel.org>,
	Piergiorgio Beruto <piergiorgio.beruto@...il.com>,
	Oleksij Rempel <o.rempel@...gutronix.de>,
	Nicolò Veronese <nicveronese@...il.com>
Subject: Re: [RFC PATCH net-next v3 01/13] net: phy: Introduce ethernet link
 topology representation

On Fri, Dec 01, 2023 at 05:36:51PM +0100, Maxime Chevallier wrote:
> Link topologies containing multiple network PHYs attached to the same
> net_device can be found when using a PHY as a media converter for use
> with an SFP connector, on which an SFP transceiver containing a PHY can
> be used.
> 
> With the current model, the transceiver's PHY can't be used for
> operations such as cable testing, timestamping, macsec offload, etc.
> 
> The reason being that most of the logic for these configuration, coming
> from either ethtool netlink or ioctls tend to use netdev->phydev, which
> in multi-phy systems will reference the PHY closest to the MAC.
> 
> Introduce a numbering scheme allowing to enumerate PHY devices that
> belong to any netdev, which can in turn allow userspace to take more
> precise decisions with regard to each PHY's configuration.
> 
> The numbering is maintained per-netdev, in a phy_device_list.
> The numbering works similarly to a netdevice's ifindex, with
> identifiers that are only recycled once INT_MAX has been reached.
> 
> This prevents races that could occur between PHY listing and SFP
> transceiver removal/insertion.
> 
> The identifiers are assigned at phy_attach time, as the numbering
> depends on the netdevice the phy is attached to.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@...tlin.com>

Hi Maxime,

some minor feedback from my side.

...

> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index f65e85c91fc1..3cf7774df57e 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -2,7 +2,7 @@
>  # Makefile for Linux PHY drivers
>  
>  libphy-y			:= phy.o phy-c45.o phy-core.o phy_device.o \
> -				   linkmode.o
> +				   linkmode.o phy_link_topology.o
>  mdio-bus-y			+= mdio_bus.o mdio_device.o
>  
>  ifdef CONFIG_MDIO_DEVICE
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c

...

> @@ -265,6 +266,14 @@ static void phy_mdio_device_remove(struct mdio_device *mdiodev)
>  
>  static struct phy_driver genphy_driver;
>  
> +static struct phy_link_topology *phy_get_link_topology(struct phy_device *phydev)
> +{
> +	if (phydev->attached_dev)
> +		return &phydev->attached_dev->link_topo;
> +
> +	return NULL;
> +}
> +

This function is declared static but is unused, which causes
allmodconfig W=1 builds to fail. Perhaps it could be introduced
in a latter patch where it is used?

...

> diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c

...

> +void phy_link_topo_init(struct phy_link_topology *topo)
> +{
> +	xa_init_flags(&topo->phys, XA_FLAGS_ALLOC1);
> +	topo->next_phy_index = 1;
> +}

...

> diff --git a/include/linux/phy_link_topology.h b/include/linux/phy_link_topology.h

...

> +#else
> +static struct phy_device *phy_link_topo_get_phy(struct phy_link_topology *topo,
> +						u32 phyindex)
> +{
> +	return NULL;
> +}
> +
> +static int phy_link_topo_add_phy(struct phy_link_topology *topo,
> +				 struct phy_device *phy,
> +				 enum phy_upstream upt, void *upstream)
> +{
> +	return 0;
> +}
> +
> +static void phy_link_topo_del_phy(struct phy_link_topology *topo,
> +				  struct phy_device *phy)
> +{
> +}
> +#endif

nit: functions in .h should be declared static inline

...

> diff --git a/net/core/dev.c b/net/core/dev.c

...

> @@ -10832,6 +10833,8 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
>  #ifdef CONFIG_NET_SCHED
>  	hash_init(dev->qdisc_hash);
>  #endif
> +	phy_link_topo_init(&dev->link_topo);
> +

I don't think this can work unless PHYLIB is compiled as a built-in.

>  	dev->priv_flags = IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM;
>  	setup(dev);
>  
> -- 
> 2.42.0
>