Hi Simon,

On Sat, 9 Dec 2023 17:02:41 +0000
Simon Horman [off-list ref] wrote:

On Fri, Dec 01, 2023 at 05:36:51PM +0100, Maxime Chevallier wrote:

quoted

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@bootlin.com>  

Hi Maxime,

some minor feedback from my side.

...

quoted

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  

...

quoted

@@ -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?

Ah true, thanks for spotting this !

...

quoted

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

...

quoted

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

...

quoted

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

...

quoted

+#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

Will do in next version

...

quoted

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

...

quoted

@@ -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.

Inded, I need to better clarify and document the dependency with
PHYLIB.

Thanks a lot for the review,

Maxime