[RFC PATCH net-next 0/7] net: phy: introduce phy numbering

[RFC PATCH net-next 0/7] net: phy: introduce phy numbering

[Maxime Chevallier]

Hello everyone,

This is the first RFC series introducing ethernet PHY numbering, in an
effort to better represent the link components and allow userspace to
configure these.

As of today, PHY devices are hidden behind the struct net_device from
userspace, but there exists commands such as PLCA configuration,
cable-testing, soon timestamping, that actually target the phy_device.

These commands rely on the ndev->phydev pointer to find the phy_device.

However, there exists use-cases where we have multiple PHY devices
between the MAC and the front-facing port. The most common case right
now is when a PHY acts as a media-converter, and is wired to an SFP
port :

[MAC] - [PHY] - [SFP][PHY]


Modules plugged in that port may contain a PHY too, and this is
where discrepencies start to happen.

In this case, ndev->phydev will point to the innermost PHY. Users
willing to use the SFP phy for cable-testing for example would get
unexpected results, as the middle PHY will be reached.

This is worsen by the fact that in a scenario like this :

[MAC] - [SFP][PHY]

the ndev->phydev pointer do point to the SFP PHY.

This is only the tip of the iceberg, such scenarios can happen with
other designs that include a mii mux, which isn't supported yet but
would require PHY enumeration to work.

This series therefore tries to add the ability to enumerate the PHYs
sitting behind a MAC, and assign them a unique number.

I've used the term of "phy namespace" to emphasize the fact that the PHY
numbering really is specific to an interface, each interface maintaining
its numbering scheme, starting from 0, and wrapping after all u32 values
have been exhausted.

The PHY namespace is for now contained within struct net_device, meaning
that PHYs that aren't related at all to any net_device wouldn't be
numbered as of right now. The only case I identified is when a PHY sits
between 2 DSA switches, but I don't know how relevant this is.

The phy_ns is its own struct, for now owned by net_device, but it could
be shared with struct dsa_port for example to make a MAC and the DSA CPU
port share the same phy ns.

This is early work, and it has its shortcomings :

 - I didn't include netlink notifications on PHY insersion/removal, but
   I think this could definitely be useful

 - the netlink API would need polishing, I struggle a bit with finding
   the correct netlink design pattern to return variale-length list of u32.

 - I would like to port netlink commands such as cable-test and plca to
   this new model, by adding an optional PHYINDEX field in the request.
   The idea would be that if the PHYINDEX is passed in the netlink
   request, we lookup the corresponding phy_device, and if not, we
   fallback to ndev->phydev.

 - Naming is hard, feel free to suggest any correction

Let me know what you think of this approach,

Best regards,

Maxime

Maxime Chevallier (7):
  net: phy: introduce phy numbering and phy namespaces
  net: sfp: pass the phy_device when disconnecting an sfp module's PHY
  net: phy: add helpers to handle sfp phy connect/disconnect
  net: ethtool: add a netlink command to list PHYs
  netlink: specs: add phy_list command
  net: ethtool: add a netlink command to get PHY information
  netlink: specs: add command to show individual phy information

 Documentation/netlink/specs/ethtool.yaml |  65 ++++++++++++
 drivers/net/phy/Makefile                 |   2 +-
 drivers/net/phy/at803x.c                 |   2 +
 drivers/net/phy/marvell-88x2222.c        |   2 +
 drivers/net/phy/marvell.c                |   2 +
 drivers/net/phy/marvell10g.c             |   2 +
 drivers/net/phy/phy_device.c             |  53 ++++++++++
 drivers/net/phy/phy_ns.c                 |  65 ++++++++++++
 drivers/net/phy/phylink.c                |   3 +-
 drivers/net/phy/sfp-bus.c                |   4 +-
 include/linux/netdevice.h                |   2 +
 include/linux/phy.h                      |   6 ++
 include/linux/phy_ns.h                   |  30 ++++++
 include/linux/sfp.h                      |   2 +-
 include/uapi/linux/ethtool.h             |   7 ++
 include/uapi/linux/ethtool_netlink.h     |  27 +++++
 net/core/dev.c                           |   3 +
 net/ethtool/Makefile                     |   2 +-
 net/ethtool/netlink.c                    |  20 ++++
 net/ethtool/netlink.h                    |   4 +
 net/ethtool/phy.c                        | 124 +++++++++++++++++++++++
 net/ethtool/phy_list.c                   |  99 ++++++++++++++++++
 22 files changed, 520 insertions(+), 6 deletions(-)
 create mode 100644 drivers/net/phy/phy_ns.c
 create mode 100644 include/linux/phy_ns.h
 create mode 100644 net/ethtool/phy.c
 create mode 100644 net/ethtool/phy_list.c

-- 
2.41.0

[RFC PATCH net-next 1/7] net: phy: introduce phy numbering and phy namespaces

[Maxime Chevallier]

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, hence the notion of PHY
namespaces. 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>
---
 drivers/net/phy/Makefile     |  2 +-
 drivers/net/phy/phy_device.c | 13 ++++++++
 drivers/net/phy/phy_ns.c     | 65 ++++++++++++++++++++++++++++++++++++
 include/linux/netdevice.h    |  2 ++
 include/linux/phy.h          |  4 +++
 include/linux/phy_ns.h       | 30 +++++++++++++++++
 net/core/dev.c               |  3 ++
 7 files changed, 118 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/phy/phy_ns.c
 create mode 100644 include/linux/phy_ns.h

diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index c945ed9bd14b..baa95d9f24e4 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_ns.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
index 2ce74593d6e4..0c029ae5130a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -29,6 +29,7 @@
 #include <linux/phy.h>
 #include <linux/phylib_stubs.h>
 #include <linux/phy_led_triggers.h>
+#include <linux/phy_ns.h>
 #include <linux/pse-pd/pse.h>
 #include <linux/property.h>
 #include <linux/rtnetlink.h>
@@ -265,6 +266,14 @@ static void phy_mdio_device_remove(struct mdio_device *mdiodev)
 
 static struct phy_driver genphy_driver;
 
+static struct phy_namespace *phy_get_ns(struct phy_device *phydev)
+{
+	if (phydev->attached_dev)
+		return &phydev->attached_dev->phy_ns;
+
+	return NULL;
+}
+
 static LIST_HEAD(phy_fixup_list);
 static DEFINE_MUTEX(phy_fixup_lock);
 
@@ -677,6 +686,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 
 	dev->state = PHY_DOWN;
 	INIT_LIST_HEAD(&dev->leds);
+	INIT_LIST_HEAD(&dev->node);
 
 	mutex_init(&dev->lock);
 	INIT_DELAYED_WORK(&dev->state_queue, phy_state_machine);
@@ -1489,6 +1499,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 		if (phydev->sfp_bus_attached)
 			dev->sfp_bus = phydev->sfp_bus;
+
+		phy_ns_add_phy(&dev->phy_ns, phydev);
 	}
 
 	/* Some Ethernet drivers try to connect to a PHY device before
@@ -1814,6 +1826,7 @@ void phy_detach(struct phy_device *phydev)
 	if (dev) {
 		phydev->attached_dev->phydev = NULL;
 		phydev->attached_dev = NULL;
+		phy_ns_del_phy(&dev->phy_ns, phydev);
 	}
 	phydev->phylink = NULL;
 
diff --git a/drivers/net/phy/phy_ns.c b/drivers/net/phy/phy_ns.c
new file mode 100644
index 000000000000..d7865028ab20
--- /dev/null
+++ b/drivers/net/phy/phy_ns.c
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Infrastructure to handle all PHY devices connected to a given netdev,
+ * either directly or indirectly attached.
+ *
+ * Copyright (c) 2023 Maxime Chevallier<maxime.chevallier@bootlin.com>
+ */
+
+#include <linux/phy.h>
+#include <linux/phy_ns.h>
+
+static int phy_ns_next_phyindex(struct phy_namespace *phy_ns)
+{
+	int phyindex = phy_ns->last_attributed_index;
+
+	for (;;) {
+		if (++phyindex <= 0)
+			phyindex = 1;
+		if (!phy_ns_get_by_index(phy_ns, phyindex))
+			return phy_ns->last_attributed_index = phyindex;
+	}
+}
+
+struct phy_device *phy_ns_get_by_index(struct phy_namespace *phy_ns,
+				       int phyindex)
+{
+	struct phy_device *phy;
+
+	mutex_lock(&phy_ns->ns_lock);
+	list_for_each_entry(phy, &phy_ns->phys, node)
+		if (phy->phyindex == phyindex)
+			goto unlock;
+
+	phy = NULL;
+unlock:
+	mutex_unlock(&phy_ns->ns_lock);
+	return phy;
+}
+EXPORT_SYMBOL_GPL(phy_ns_get_by_index);
+
+void phy_ns_add_phy(struct phy_namespace *phy_ns, struct phy_device *phy)
+{
+	/* PHYs can be attached and detached, they will keep their id */
+	if (!phy->phyindex)
+		phy->phyindex = phy_ns_next_phyindex(phy_ns);
+
+	mutex_lock(&phy_ns->ns_lock);
+	list_add(&phy->node, &phy_ns->phys);
+	mutex_unlock(&phy_ns->ns_lock);
+}
+EXPORT_SYMBOL_GPL(phy_ns_add_phy);
+
+void phy_ns_del_phy(struct phy_namespace *phy_ns, struct phy_device *phy)
+{
+	mutex_lock(&phy_ns->ns_lock);
+	list_del(&phy->node);
+	mutex_unlock(&phy_ns->ns_lock);
+}
+EXPORT_SYMBOL_GPL(phy_ns_del_phy);
+
+void phy_ns_init(struct phy_namespace *phy_ns)
+{
+	INIT_LIST_HEAD(&phy_ns->phys);
+	mutex_init(&phy_ns->ns_lock);
+}
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 0896aaa91dd7..ef86cb87a38a 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -43,6 +43,7 @@
 
 #include <linux/netdev_features.h>
 #include <linux/neighbour.h>
+#include <linux/phy_ns.h>
 #include <uapi/linux/netdevice.h>
 #include <uapi/linux/if_bonding.h>
 #include <uapi/linux/pkt_cls.h>
@@ -2380,6 +2381,7 @@ struct net_device {
 	struct netprio_map __rcu *priomap;
 #endif
 	struct phy_device	*phydev;
+	struct phy_namespace	phy_ns;
 	struct sfp_bus		*sfp_bus;
 	struct lock_class_key	*qdisc_tx_busylock;
 	bool			proto_down;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 1351b802ffcf..b12fd33aa84a 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -543,6 +543,8 @@ struct macsec_ops;
  * @drv: Pointer to the driver for this PHY instance
  * @devlink: Create a link between phy dev and mac dev, if the external phy
  *           used by current mac interface is managed by another mac interface.
+ * @phyindex: Unique id across the phy's parent tree of phys to address the PHY
+ *	      from userspace, similar to ifindex. It's never recycled.
  * @phy_id: UID for this device found during discovery
  * @c45_ids: 802.3-c45 Device Identifiers if is_c45.
  * @is_c45:  Set to true if this PHY uses clause 45 addressing.
@@ -640,6 +642,7 @@ struct phy_device {
 
 	struct device_link *devlink;
 
+	int phyindex;
 	u32 phy_id;
 
 	struct phy_c45_device_ids c45_ids;
@@ -761,6 +764,7 @@ struct phy_device {
 	/* MACsec management functions */
 	const struct macsec_ops *macsec_ops;
 #endif
+	struct list_head node;
 };
 
 /* Generic phy_device::dev_flags */
diff --git a/include/linux/phy_ns.h b/include/linux/phy_ns.h
new file mode 100644
index 000000000000..ae173e637c62
--- /dev/null
+++ b/include/linux/phy_ns.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * PHY device namespaces allow maintaining a list of PHY devices that are
+ * part of a netdevice's link topology. PHYs can for example be chained,
+ * as is the case when using a PHY that exposes an SFP module, on which an
+ * SFP transceiver that embeds a PHY is connected.
+ *
+ * This list can then be used by userspace to leverage individual PHY
+ * capabilities.
+ */
+#ifndef __PHY_NS_H
+#define __PHY_NS_H
+
+struct mutex;
+
+struct phy_namespace {
+	struct list_head phys;
+	int last_attributed_index;
+
+	/* Protects the .phys list */
+	struct mutex ns_lock;
+};
+
+struct phy_device *phy_ns_get_by_index(struct phy_namespace *phy_ns,
+				       int phyindex);
+void phy_ns_add_phy(struct phy_namespace *phy_ns, struct phy_device *phy);
+void phy_ns_del_phy(struct phy_namespace *phy_ns, struct phy_device *phy);
+void phy_ns_init(struct phy_namespace *phy_ns);
+
+#endif /* __PHY_NS_H */
diff --git a/net/core/dev.c b/net/core/dev.c
index ccff2b6ef958..aa8b924269d7 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10729,6 +10729,9 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 	INIT_LIST_HEAD(&dev->net_notifier_list);
 #ifdef CONFIG_NET_SCHED
 	hash_init(dev->qdisc_hash);
+#endif
+#ifdef CONFIG_PHYLIB
+	phy_ns_init(&dev->phy_ns);
 #endif
 	dev->priv_flags = IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM;
 	setup(dev);
-- 
2.41.0

Re: [RFC PATCH net-next 1/7] net: phy: introduce phy numbering and phy namespaces

[Russell King (Oracle)]

On Thu, Sep 07, 2023 at 11:23:59AM +0200, Maxime Chevallier wrote:
> @@ -640,6 +642,7 @@ struct phy_device {
>  
>  	struct device_link *devlink;
>  
> +	int phyindex;
>  	u32 phy_id;
>  
>  	struct phy_c45_device_ids c45_ids;
> @@ -761,6 +764,7 @@ struct phy_device {
>  	/* MACsec management functions */
>  	const struct macsec_ops *macsec_ops;
>  #endif
> +	struct list_head node;

I haven't yet fully looked at this, but the one thing that did stand out
was this - please name it "phy_ns_node" so that the purpose of this node
is clear.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [RFC PATCH net-next 1/7] net: phy: introduce phy numbering and phy namespaces

[Russell King (Oracle)]

On Thu, Sep 07, 2023 at 11:23:59AM +0200, 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, hence the notion of PHY
> namespaces. 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.

I think you can simplify this code quite a bit by using idr.
idr_alloc_cyclic() looks like it will do the allocation you want,
plus the IDR subsystem will store the pointer to the object (in
this case the phy device) and allow you to look that up. That
probably gets rid of quite a bit of code.

You will need to handle the locking around IDR however.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [RFC PATCH net-next 1/7] net: phy: introduce phy numbering and phy namespaces

[Maxime Chevallier]

On Thu, 7 Sep 2023 11:14:08 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Thu, Sep 07, 2023 at 11:23:59AM +0200, 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, hence the notion of PHY
> > namespaces. 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.  
> 
> I think you can simplify this code quite a bit by using idr.
> idr_alloc_cyclic() looks like it will do the allocation you want,
> plus the IDR subsystem will store the pointer to the object (in
> this case the phy device) and allow you to look that up. That
> probably gets rid of quite a bit of code.
> 
> You will need to handle the locking around IDR however.

Oh thanks for pointing this out. I had considered idr but I didn't spot
the _cyclic() helper, and I had ruled that out thinking it would re-use
ids directly after freeing them. I'll be more than happy to use that.

Thanks,

Maxime

Re: [RFC PATCH net-next 1/7] net: phy: introduce phy numbering and phy namespaces

[Jakub Kicinski]

On Thu, 7 Sep 2023 14:19:04 +0200 Maxime Chevallier wrote:
> > I think you can simplify this code quite a bit by using idr.
> > idr_alloc_cyclic() looks like it will do the allocation you want,
> > plus the IDR subsystem will store the pointer to the object (in
> > this case the phy device) and allow you to look that up. That
> > probably gets rid of quite a bit of code.
> > 
> > You will need to handle the locking around IDR however.  
> 
> Oh thanks for pointing this out. I had considered idr but I didn't spot
> the _cyclic() helper, and I had ruled that out thinking it would re-use
> ids directly after freeing them. I'll be more than happy to use that.

Perhaps use xarray directly, I don't think we need the @base offset or
quick access to @next which AFAICT is the only reason one would prefer
IDR?

Re: [RFC PATCH net-next 1/7] net: phy: introduce phy numbering and phy namespaces

[Maxime Chevallier]

Hello Jakub,

On Fri, 8 Sep 2023 08:36:08 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> On Thu, 7 Sep 2023 14:19:04 +0200 Maxime Chevallier wrote:
> > > I think you can simplify this code quite a bit by using idr.
> > > idr_alloc_cyclic() looks like it will do the allocation you want,
> > > plus the IDR subsystem will store the pointer to the object (in
> > > this case the phy device) and allow you to look that up. That
> > > probably gets rid of quite a bit of code.
> > > 
> > > You will need to handle the locking around IDR however.    
> > 
> > Oh thanks for pointing this out. I had considered idr but I didn't spot
> > the _cyclic() helper, and I had ruled that out thinking it would re-use
> > ids directly after freeing them. I'll be more than happy to use that.  
> 
> Perhaps use xarray directly, I don't think we need the @base offset or
> quick access to @next which AFAICT is the only reason one would prefer
> IDR?

Oh indeed xa_alloc_cyclic looks to fit perfectly, thanks !

Maxime

Re: [RFC PATCH net-next 1/7] net: phy: introduce phy numbering and phy namespaces

[Andrew Lunn]

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

A minor point, and i know naming is hard, but i keep reading _ns_ and
think namespace, as in ip netns. Maybe we should think of something
other than ns.

      Andrew

Re: [RFC PATCH net-next 1/7] net: phy: introduce phy numbering and phy namespaces

[Maxime Chevallier]

Hello,

On Tue, 12 Sep 2023 17:41:31 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > 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.  
> 
> A minor point, and i know naming is hard, but i keep reading _ns_ and
> think namespace, as in ip netns. Maybe we should think of something
> other than ns.

Yeah that was the initial idea, to imply that the numering is
independent between netdevices... I thought about "phy_list", "phys",
"phy_devices" but none of that felt correct :(

Any idea here would be welcome :D

Maxime

>       Andrew

Re: [RFC PATCH net-next 1/7] net: phy: introduce phy numbering and phy namespaces

[Florian Fainelli]

On 9/12/23 09:10, Maxime Chevallier wrote:
> Hello,
> 
> On Tue, 12 Sep 2023 17:41:31 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
>>> 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.
>>
>> A minor point, and i know naming is hard, but i keep reading _ns_ and
>> think namespace, as in ip netns. Maybe we should think of something
>> other than ns.
> 
> Yeah that was the initial idea, to imply that the numering is
> independent between netdevices... I thought about "phy_list", "phys",
> "phy_devices" but none of that felt correct :(

How about phy_devices_list?
-- 
Florian

Re: [RFC PATCH net-next 1/7] net: phy: introduce phy numbering and phy namespaces

[Andrew Lunn]

On Thu, Sep 07, 2023 at 11:23:59AM +0200, 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.

I think we need more than a number. Topology needs to be a core
concept here, otherwise how is the user supposed to know which PHY to
use cable test on, etc.

However, it is not a simple problem. An SFP PHY should be the last in
a chain. So you can infer something from that. When we start adding
MII muxes, they will need to be part of the modal.

    Andrew

Re: [RFC PATCH net-next 1/7] net: phy: introduce phy numbering and phy namespaces

[Maxime Chevallier]

Hello Andrew,

On Tue, 12 Sep 2023 18:15:52 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Thu, Sep 07, 2023 at 11:23:59AM +0200, 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.  
> 
> I think we need more than a number. Topology needs to be a core
> concept here, otherwise how is the user supposed to know which PHY to
> use cable test on, etc.
> 
> However, it is not a simple problem. An SFP PHY should be the last in
> a chain. So you can infer something from that. When we start adding
> MII muxes, they will need to be part of the modal.

You raise a good point, we need to set a cursor on the level of detail
we want to have to describe the topology indeed.

I do have a patch that adds a notion of topology by keeping track of
the upstream device of each link component (either the ethernet
controller, another PHY, a mux, and SFP cage), but I got carried away
trying to find the correct granularity.

For example, say we have a PCS with a dedicated driver in the chain,
should it be part of the topology ? or do we stick to MAC, PHY, MUX,
SFP ?

To address the topology and more specifically cable-testing, I relied
on adding support for a phy_port, that would represent front-facing
ports, each PHY would have zero, one or more phy_ports, and from
userspace perspective, we would let user pick which port to use, then
have kernel-side logic to either deal with PHYs that have 2 ports, or
an actual mii mux with two single-port PHYs.

All in all for cable-testing, this solves the problem, as we could
include a way for users to know which PHY is attached to a port, and
therefore users could know which PHY is the outermost one.

However, it's not sufficient for things like timestamping. I think you
mentionned in another thread that there can be up to 7 devices that
could do the timestamping, and here it could be interesting to know
which is where, so that user can for example pick a PHY that has a
precise timestamping unit but that is also close-enough to the physical
port.

In that case, I will include what I have for topology description in
the next RFC.

Thanks for the insightful feedback,

Maxime

>     Andrew

[RFC PATCH net-next 2/7] net: sfp: pass the phy_device when disconnecting an sfp module's PHY

[Maxime Chevallier]

pass the phy_device as a parameter to the sfp upstream .disconnect_phy
operation. This is preparatory work to help track phy devices across
a net_device's link.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phylink.c | 3 ++-
 drivers/net/phy/sfp-bus.c | 4 ++--
 include/linux/sfp.h       | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 0d7354955d62..97e8019adad8 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -3325,7 +3325,8 @@ static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
 	return ret;
 }
 
-static void phylink_sfp_disconnect_phy(void *upstream)
+static void phylink_sfp_disconnect_phy(void *upstream,
+				       struct phy_device *phydev)
 {
 	phylink_disconnect_phy(upstream);
 }
diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index 208a9393c2df..f42e9a082935 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -486,7 +486,7 @@ static void sfp_unregister_bus(struct sfp_bus *bus)
 			bus->socket_ops->stop(bus->sfp);
 		bus->socket_ops->detach(bus->sfp);
 		if (bus->phydev && ops && ops->disconnect_phy)
-			ops->disconnect_phy(bus->upstream);
+			ops->disconnect_phy(bus->upstream, bus->phydev);
 	}
 	bus->registered = false;
 }
@@ -742,7 +742,7 @@ void sfp_remove_phy(struct sfp_bus *bus)
 	const struct sfp_upstream_ops *ops = sfp_get_upstream_ops(bus);
 
 	if (ops && ops->disconnect_phy)
-		ops->disconnect_phy(bus->upstream);
+		ops->disconnect_phy(bus->upstream, bus->phydev);
 	bus->phydev = NULL;
 }
 EXPORT_SYMBOL_GPL(sfp_remove_phy);
diff --git a/include/linux/sfp.h b/include/linux/sfp.h
index 9346cd44814d..0573e53b0c11 100644
--- a/include/linux/sfp.h
+++ b/include/linux/sfp.h
@@ -544,7 +544,7 @@ struct sfp_upstream_ops {
 	void (*link_down)(void *priv);
 	void (*link_up)(void *priv);
 	int (*connect_phy)(void *priv, struct phy_device *);
-	void (*disconnect_phy)(void *priv);
+	void (*disconnect_phy)(void *priv, struct phy_device *);
 };
 
 #if IS_ENABLED(CONFIG_SFP)
-- 
2.41.0

[RFC PATCH net-next 3/7] net: phy: add helpers to handle sfp phy connect/disconnect

[Maxime Chevallier]

There are a few PHY drivers that can handle SFP modules through their
sfp_upstream_ops. Introduce Phylib helpers to keep track of connected
SFP PHYs in a netdevice's namespace, by adding the SFP PHY to the
upstream PHY's netdev's namespace.

By doing so, these SFP PHYs can be enumerated and exposed to users,
which will be able to use their capabilities.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/at803x.c          |  2 ++
 drivers/net/phy/marvell-88x2222.c |  2 ++
 drivers/net/phy/marvell.c         |  2 ++
 drivers/net/phy/marvell10g.c      |  2 ++
 drivers/net/phy/phy_device.c      | 40 +++++++++++++++++++++++++++++++
 include/linux/phy.h               |  2 ++
 6 files changed, 50 insertions(+)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 37fb033e1c29..9b1659b03aa5 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -730,6 +730,8 @@ static const struct sfp_upstream_ops at803x_sfp_ops = {
 	.attach = phy_sfp_attach,
 	.detach = phy_sfp_detach,
 	.module_insert = at803x_sfp_insert,
+	.connect_phy = phy_sfp_connect_phy,
+	.disconnect_phy = phy_sfp_disconnect_phy,
 };
 
 static int at803x_parse_dt(struct phy_device *phydev)
diff --git a/drivers/net/phy/marvell-88x2222.c b/drivers/net/phy/marvell-88x2222.c
index e3aa30dad2e6..3f77bbc7e04f 100644
--- a/drivers/net/phy/marvell-88x2222.c
+++ b/drivers/net/phy/marvell-88x2222.c
@@ -555,6 +555,8 @@ static const struct sfp_upstream_ops sfp_phy_ops = {
 	.link_down = mv2222_sfp_link_down,
 	.attach = phy_sfp_attach,
 	.detach = phy_sfp_detach,
+	.connect_phy = phy_sfp_connect_phy,
+	.disconnect_phy = phy_sfp_disconnect_phy,
 };
 
 static int mv2222_probe(struct phy_device *phydev)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index eba652a4c1d8..674e29bce2cc 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -3254,6 +3254,8 @@ static const struct sfp_upstream_ops m88e1510_sfp_ops = {
 	.module_remove = m88e1510_sfp_remove,
 	.attach = phy_sfp_attach,
 	.detach = phy_sfp_detach,
+	.connect_phy = phy_sfp_connect_phy,
+	.disconnect_phy = phy_sfp_disconnect_phy,
 };
 
 static int m88e1510_probe(struct phy_device *phydev)
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index d4bb90d76881..a88ebc0a6be5 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -496,6 +496,8 @@ static int mv3310_sfp_insert(void *upstream, const struct sfp_eeprom_id *id)
 static const struct sfp_upstream_ops mv3310_sfp_ops = {
 	.attach = phy_sfp_attach,
 	.detach = phy_sfp_detach,
+	.connect_phy = phy_sfp_connect_phy,
+	.disconnect_phy = phy_sfp_disconnect_phy,
 	.module_insert = mv3310_sfp_insert,
 };
 
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 0c029ae5130a..090c5b14a7b8 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1362,6 +1362,46 @@ phy_standalone_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(phy_standalone);
 
+/**
+ * phy_sfp_connect_phy - Connect the SFP module's PHY to the upstream PHY
+ * @upstream: pointer to the upstream phy device
+ * @phy: pointer to the SFP module's phy device
+ *
+ * This helper allows keeping track of PHY devices on the link. It adds the
+ * SFP module's phy to the phy namespace of the upstream phy
+ */
+int phy_sfp_connect_phy(void *upstream, struct phy_device *phy)
+{
+	struct phy_device *phydev = upstream;
+	struct phy_namespace *phy_ns = phy_get_ns(phydev);
+
+	if (phy_ns)
+		phy_ns_add_phy(phy_ns, phy);
+
+	return 0;
+}
+EXPORT_SYMBOL(phy_sfp_connect_phy);
+
+/**
+ * phy_sfp_disconnect_phy - Disconnect the SFP module's PHY from the upstream PHY
+ * @upstream: pointer to the upstream phy device
+ * @phy: pointer to the SFP module's phy device
+ *
+ * This helper allows keeping track of PHY devices on the link. It removes the
+ * SFP module's phy to the phy namespace of the upstream phy. As the module phy
+ * will be destroyed, re-inserting the same module will add a new phy with a
+ * new index.
+ */
+void phy_sfp_disconnect_phy(void *upstream, struct phy_device *phy)
+{
+	struct phy_device *phydev = upstream;
+	struct phy_namespace *phy_ns = phy_get_ns(phydev);
+
+	if (phy_ns)
+		phy_ns_del_phy(phy_ns, phy);
+}
+EXPORT_SYMBOL(phy_sfp_disconnect_phy);
+
 /**
  * phy_sfp_attach - attach the SFP bus to the PHY upstream network device
  * @upstream: pointer to the phy device
diff --git a/include/linux/phy.h b/include/linux/phy.h
index b12fd33aa84a..02fdf5075f31 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1720,6 +1720,8 @@ int phy_suspend(struct phy_device *phydev);
 int phy_resume(struct phy_device *phydev);
 int __phy_resume(struct phy_device *phydev);
 int phy_loopback(struct phy_device *phydev, bool enable);
+int phy_sfp_connect_phy(void *upstream, struct phy_device *phy);
+void phy_sfp_disconnect_phy(void *upstream, struct phy_device *phy);
 void phy_sfp_attach(void *upstream, struct sfp_bus *bus);
 void phy_sfp_detach(void *upstream, struct sfp_bus *bus);
 int phy_sfp_probe(struct phy_device *phydev,
-- 
2.41.0

[RFC PATCH net-next 4/7] net: ethtool: add a netlink command to list PHYs

[Maxime Chevallier]

Introduce a new netlink message that lists all PHYs on a given interface
at a given time.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 include/uapi/linux/ethtool_netlink.h | 13 ++++
 net/ethtool/Makefile                 |  2 +-
 net/ethtool/netlink.c                | 10 +++
 net/ethtool/netlink.h                |  2 +
 net/ethtool/phy_list.c               | 99 ++++++++++++++++++++++++++++
 5 files changed, 125 insertions(+), 1 deletion(-)
 create mode 100644 net/ethtool/phy_list.c

diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 73e2c10dc2cc..b2a0d21fdd8e 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -57,6 +57,7 @@ enum {
 	ETHTOOL_MSG_PLCA_GET_STATUS,
 	ETHTOOL_MSG_MM_GET,
 	ETHTOOL_MSG_MM_SET,
+	ETHTOOL_MSG_PHY_LIST_GET,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_USER_CNT,
@@ -109,6 +110,7 @@ enum {
 	ETHTOOL_MSG_PLCA_NTF,
 	ETHTOOL_MSG_MM_GET_REPLY,
 	ETHTOOL_MSG_MM_NTF,
+	ETHTOOL_MSG_PHY_LIST_GET_REPLY,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_KERNEL_CNT,
@@ -975,6 +977,17 @@ enum {
 	ETHTOOL_A_MM_MAX = (__ETHTOOL_A_MM_CNT - 1)
 };
 
+enum {
+	ETHTOOL_A_PHY_LIST_UNSPEC,
+	ETHTOOL_A_PHY_LIST_HEADER,			/* nest - _A_HEADER_* */
+	ETHTOOL_A_PHY_LIST_COUNT,			/* u8 */
+	ETHTOOL_A_PHY_LIST_INDEX,			/* array, u32 */
+
+	/* add new constants above here */
+	__ETHTOOL_A_PHY_LIST_CNT,
+	ETHTOOL_A_PHY_LIST_MAX = (__ETHTOOL_A_PHY_LIST_CNT - 1)
+};
+
 /* generic netlink info */
 #define ETHTOOL_GENL_NAME "ethtool"
 #define ETHTOOL_GENL_VERSION 1
diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
index 504f954a1b28..a182c0dbbb9d 100644
--- a/net/ethtool/Makefile
+++ b/net/ethtool/Makefile
@@ -8,4 +8,4 @@ ethtool_nl-y	:= netlink.o bitset.o strset.o linkinfo.o linkmodes.o rss.o \
 		   linkstate.o debug.o wol.o features.o privflags.o rings.o \
 		   channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o \
 		   tunnels.o fec.o eeprom.o stats.o phc_vclocks.o mm.o \
-		   module.o pse-pd.o plca.o mm.o
+		   module.o pse-pd.o plca.o mm.o phy_list.o
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 3bbd5afb7b31..60b66b78055e 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -306,6 +306,7 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
 	[ETHTOOL_MSG_PLCA_GET_STATUS]	= &ethnl_plca_status_request_ops,
 	[ETHTOOL_MSG_MM_GET]		= &ethnl_mm_request_ops,
 	[ETHTOOL_MSG_MM_SET]		= &ethnl_mm_request_ops,
+	[ETHTOOL_MSG_PHY_LIST_GET]	= &ethnl_phy_list_request_ops,
 };
 
 static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
@@ -1128,6 +1129,15 @@ static const struct genl_ops ethtool_genl_ops[] = {
 		.policy = ethnl_mm_set_policy,
 		.maxattr = ARRAY_SIZE(ethnl_mm_set_policy) - 1,
 	},
+	{
+		.cmd	= ETHTOOL_MSG_PHY_LIST_GET,
+		.doit	= ethnl_default_doit,
+		.start	= ethnl_default_start,
+		.dumpit	= ethnl_default_dumpit,
+		.done	= ethnl_default_done,
+		.policy = ethnl_phy_list_get_policy,
+		.maxattr = ARRAY_SIZE(ethnl_phy_list_get_policy) - 1,
+	},
 };
 
 static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 9a333a8d04c1..76859d8fbce0 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -395,6 +395,7 @@ extern const struct ethnl_request_ops ethnl_rss_request_ops;
 extern const struct ethnl_request_ops ethnl_plca_cfg_request_ops;
 extern const struct ethnl_request_ops ethnl_plca_status_request_ops;
 extern const struct ethnl_request_ops ethnl_mm_request_ops;
+extern const struct ethnl_request_ops ethnl_phy_list_request_ops;
 
 extern const struct nla_policy ethnl_header_policy[ETHTOOL_A_HEADER_FLAGS + 1];
 extern const struct nla_policy ethnl_header_policy_stats[ETHTOOL_A_HEADER_FLAGS + 1];
@@ -441,6 +442,7 @@ extern const struct nla_policy ethnl_plca_set_cfg_policy[ETHTOOL_A_PLCA_MAX + 1]
 extern const struct nla_policy ethnl_plca_get_status_policy[ETHTOOL_A_PLCA_HEADER + 1];
 extern const struct nla_policy ethnl_mm_get_policy[ETHTOOL_A_MM_HEADER + 1];
 extern const struct nla_policy ethnl_mm_set_policy[ETHTOOL_A_MM_MAX + 1];
+extern const struct nla_policy ethnl_phy_list_get_policy[ETHTOOL_A_PHY_LIST_HEADER + 1];
 
 int ethnl_set_features(struct sk_buff *skb, struct genl_info *info);
 int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info);
diff --git a/net/ethtool/phy_list.c b/net/ethtool/phy_list.c
new file mode 100644
index 000000000000..689d08637391
--- /dev/null
+++ b/net/ethtool/phy_list.c
@@ -0,0 +1,99 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2023 Bootlin
+ *
+ * Ethtool netlink operations for Ethernet PHY specific operations
+ */
+#include "common.h"
+#include "netlink.h"
+
+#include <linux/phy.h>
+#include <linux/phy_ns.h>
+
+struct phy_list_req_info {
+	struct ethnl_req_info		base;
+};
+
+#define PHY_MAX_ENTRIES	16
+
+struct phy_list_reply_data {
+	struct ethnl_reply_data		base;
+	u8 n_phys;
+	u32 phy_indices[PHY_MAX_ENTRIES];
+};
+
+#define PHY_LIST_REPDATA(__reply_base) \
+	container_of(__reply_base, struct phy_list_reply_data, base)
+
+const struct nla_policy ethnl_phy_list_get_policy[ETHTOOL_A_PHY_LIST_HEADER + 1] = {
+	[ETHTOOL_A_PHY_LIST_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy_stats),
+};
+
+static int phy_list_prepare_data(const struct ethnl_req_info *req_base,
+				 struct ethnl_reply_data *reply_base,
+				 struct genl_info *info)
+{
+	struct phy_list_reply_data *data = PHY_LIST_REPDATA(reply_base);
+	struct net_device *dev = reply_base->dev;
+	struct phy_namespace *phy_ns = &dev->phy_ns;
+	struct phy_device *phydev;
+	int ret;
+
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		return ret;
+
+	data->n_phys = 0;
+
+	mutex_lock(&phy_ns->ns_lock);
+	list_for_each_entry(phydev, &phy_ns->phys, node)
+		data->phy_indices[data->n_phys++] = phydev->phyindex;
+	mutex_unlock(&phy_ns->ns_lock);
+
+	ethnl_ops_complete(dev);
+
+	return ret;
+}
+
+static int phy_list_reply_size(const struct ethnl_req_info *req_base,
+			       const struct ethnl_reply_data *reply_base)
+{
+	const struct phy_list_reply_data *data = PHY_LIST_REPDATA(reply_base);
+	int len = 0;
+
+	len += nla_total_size(sizeof(u8)); /* _PHY_LIST_COUNT */
+	len += nla_total_size(data->n_phys * sizeof(u32)); /* Array of _PHY_LIST_INDEX */
+
+	return len;
+}
+
+static int phy_list_fill_reply(struct sk_buff *skb,
+			       const struct ethnl_req_info *req_base,
+			       const struct ethnl_reply_data *reply_base)
+{
+	const struct phy_list_reply_data *data = PHY_LIST_REPDATA(reply_base);
+
+	if (nla_put_u8(skb, ETHTOOL_A_PHY_LIST_COUNT, data->n_phys))
+		return -EMSGSIZE;
+
+	if (!data->n_phys)
+		return 0;
+
+	if (nla_put(skb, ETHTOOL_A_PHY_LIST_INDEX, sizeof(u32) * data->n_phys,
+		    data->phy_indices))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+const struct ethnl_request_ops ethnl_phy_list_request_ops = {
+	.request_cmd		= ETHTOOL_MSG_PHY_LIST_GET,
+	.reply_cmd		= ETHTOOL_MSG_PHY_LIST_GET_REPLY,
+	.hdr_attr		= ETHTOOL_A_PHY_LIST_HEADER,
+	.req_info_size		= sizeof(struct phy_list_req_info),
+	.reply_data_size	= sizeof(struct phy_list_reply_data),
+
+	.prepare_data		= phy_list_prepare_data,
+	.reply_size		= phy_list_reply_size,
+	.fill_reply		= phy_list_fill_reply,
+};
-- 
2.41.0

Re: [RFC PATCH net-next 4/7] net: ethtool: add a netlink command to list PHYs

[Russell King (Oracle)]

On Thu, Sep 07, 2023 at 11:24:02AM +0200, Maxime Chevallier wrote:
> +#define PHY_MAX_ENTRIES	16
> +
> +struct phy_list_reply_data {
> +	struct ethnl_reply_data		base;
> +	u8 n_phys;
> +	u32 phy_indices[PHY_MAX_ENTRIES];

Please could you detail the decision making behind 16 entries - is this
arbitary or based on something?

Also, please consider what we should do if we happen to have more than
16 entries.

Finally, using u8 before an array of u32 can leave 3 bytes of padding.
It would be better to use u32 for n_phys to avoid that padding.

> +	mutex_lock(&phy_ns->ns_lock);
> +	list_for_each_entry(phydev, &phy_ns->phys, node)
> +		data->phy_indices[data->n_phys++] = phydev->phyindex;

I think this loop should limit its iterations to ensure that the
array can't overflow.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [RFC PATCH net-next 4/7] net: ethtool: add a netlink command to list PHYs

[Maxime Chevallier]

Hello Russell,

On Thu, 7 Sep 2023 11:00:24 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Thu, Sep 07, 2023 at 11:24:02AM +0200, Maxime Chevallier wrote:
> > +#define PHY_MAX_ENTRIES	16
> > +
> > +struct phy_list_reply_data {
> > +	struct ethnl_reply_data		base;
> > +	u8 n_phys;
> > +	u32 phy_indices[PHY_MAX_ENTRIES];  
> 
> Please could you detail the decision making behind 16 entries - is this
> arbitary or based on something?
> 
> Also, please consider what we should do if we happen to have more than
> 16 entries.

Ah indeed it was totally arbitrary, the idea was to have a fixed-size
reply struct, so that we can populate the
ethnl_request_ops.reply_data_size field and not do any manual memory
management. But I can store a pointer to the array of phy devices,
dynamically allocated and we won't have to deal with this fixed,
arbitrary-sized array anymore.

Sorry for not documenting this.

> Finally, using u8 before an array of u32 can leave 3 bytes of padding.
> It would be better to use u32 for n_phys to avoid that padding.

Sure thing, I'll change this

> > +	mutex_lock(&phy_ns->ns_lock);
> > +	list_for_each_entry(phydev, &phy_ns->phys, node)
> > +		data->phy_indices[data->n_phys++] = phydev->phyindex;  
> 
> I think this loop should limit its iterations to ensure that the
> array can't overflow.

Thanks,

Maxime

Re: [RFC PATCH net-next 4/7] net: ethtool: add a netlink command to list PHYs

[Andrew Lunn]

On Thu, Sep 07, 2023 at 02:16:35PM +0200, Maxime Chevallier wrote:
> Hello Russell,
> 
> On Thu, 7 Sep 2023 11:00:24 +0100
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> 
> > On Thu, Sep 07, 2023 at 11:24:02AM +0200, Maxime Chevallier wrote:
> > > +#define PHY_MAX_ENTRIES	16
> > > +
> > > +struct phy_list_reply_data {
> > > +	struct ethnl_reply_data		base;
> > > +	u8 n_phys;
> > > +	u32 phy_indices[PHY_MAX_ENTRIES];  
> > 
> > Please could you detail the decision making behind 16 entries - is this
> > arbitary or based on something?
> > 
> > Also, please consider what we should do if we happen to have more than
> > 16 entries.
> 
> Ah indeed it was totally arbitrary, the idea was to have a fixed-size
> reply struct, so that we can populate the
> ethnl_request_ops.reply_data_size field and not do any manual memory
> management. But I can store a pointer to the array of phy devices,
> dynamically allocated and we won't have to deal with this fixed,
> arbitrary-sized array anymore.

I think Jakub already commented on this somewhere, but netlink should
allow for arbitrary long lists.

      Andrew

Re: [RFC PATCH net-next 4/7] net: ethtool: add a netlink command to list PHYs

[Andrew Lunn]

> +static int phy_list_fill_reply(struct sk_buff *skb,
> +			       const struct ethnl_req_info *req_base,
> +			       const struct ethnl_reply_data *reply_base)
> +{
> +	const struct phy_list_reply_data *data = PHY_LIST_REPDATA(reply_base);
> +
> +	if (nla_put_u8(skb, ETHTOOL_A_PHY_LIST_COUNT, data->n_phys))
> +		return -EMSGSIZE;
> +
> +	if (!data->n_phys)
> +		return 0;
> +
> +	if (nla_put(skb, ETHTOOL_A_PHY_LIST_INDEX, sizeof(u32) * data->n_phys,
> +		    data->phy_indices))
> +		return -EMSGSIZE;
> +

Can we add additional information here to allow mapping to what is
under /sys ? A PHY has an struct device, so has a name. So maybe that
can be included?

	Andrew

[RFC PATCH net-next 5/7] netlink: specs: add phy_list command

[Maxime Chevallier]

Add a new command allowing to list PHYs that are present on a
netdevice's link. The list is an array of phyindex, per-netdevice unique
numbers that describe a PHY device.

Example messages :

 - No PHY on the link (Pure SFP interface) :

./cli.py --spec specs/ethtool.yaml --schema genetlink-legacy.yaml \
	 --do phy-list-get --json '{"header" : {"dev-name" : "eth3"}}'
{'header': {'dev-index': 5, 'dev-name': 'eth3'}, 'phy-count': 0}

 - One PHY on the link :

./cli.py --spec specs/ethtool.yaml --schema genetlink-legacy.yaml \
         --do phy-list-get --json '{"header" : {"dev-name" : "eth2"}}'
{'header': {'dev-index': 4, 'dev-name': 'eth2'},
 'phy-count': 1,
 'phy-indices': b'\x01\x00\x00\x00'}

 - 2 PHYs on the link (MAC - PHY - SFP[PHY]) :

./cli.py --spec specs/ethtool.yaml --schema genetlink-legacy.yaml \
         --do phy-list-get --json '{"header" : {"dev-name" : "eth0"}}'
{'header': {'dev-index': 2, 'dev-name': 'eth0'},
 'phy-count': 2,
 'phy-indices': b'\x02\x00\x00\x00\x01\x00\x00\x00'}

This PHY inddices can then be used by other netlink commands that would
target PHYs.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 Documentation/netlink/specs/ethtool.yaml | 28 ++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 837b565577ca..1139c88ed65c 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -942,6 +942,19 @@ attribute-sets:
       -
         name: burst-tmr
         type: u32
+  -
+    name: phy-list
+    attributes:
+      -
+        name: header
+        type: nest
+        nested-attributes: header
+      -
+        name: phy-count
+        type: u8
+      -
+        name: phy-indices
+        type: binary
 
 operations:
   enum-model: directional
@@ -1692,3 +1705,18 @@ operations:
       name: mm-ntf
       doc: Notification for change in MAC Merge configuration.
       notify: mm-get
+    -
+      name: phy-list-get
+      doc: Get list of PHY devices attached to an interface
+
+      attribute-set: phy-list
+
+      do: &phy-list-get-op
+        request:
+          attributes:
+            - header
+        reply:
+          attributes:
+            - header
+            - phy-count
+            - phy-indices
-- 
2.41.0

[RFC PATCH net-next 6/7] net: ethtool: add a netlink command to get PHY information

[Maxime Chevallier]

Now that we can list PHYs belonging to a netdevice, add a command to get
PHY-specific information, that we can extend as needed to get more data
such as link info, offloading support, etc.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 include/uapi/linux/ethtool.h         |   7 ++
 include/uapi/linux/ethtool_netlink.h |  14 +++
 net/ethtool/Makefile                 |   2 +-
 net/ethtool/netlink.c                |  10 +++
 net/ethtool/netlink.h                |   2 +
 net/ethtool/phy.c                    | 124 +++++++++++++++++++++++++++
 6 files changed, 158 insertions(+), 1 deletion(-)
 create mode 100644 net/ethtool/phy.c

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index f7fba0dc87e5..d74f839ad32c 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -2205,4 +2205,11 @@ struct ethtool_link_settings {
 	 * __u32 map_lp_advertising[link_mode_masks_nwords];
 	 */
 };
+
+enum phy_upstream_type {
+	PHY_UPSTREAM_MAC,
+	PHY_UPSTREAM_SFP,
+	PHY_UPSTREAM_PHY,
+};
+
 #endif /* _UAPI_LINUX_ETHTOOL_H */
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index b2a0d21fdd8e..ec96e816d564 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -58,6 +58,7 @@ enum {
 	ETHTOOL_MSG_MM_GET,
 	ETHTOOL_MSG_MM_SET,
 	ETHTOOL_MSG_PHY_LIST_GET,
+	ETHTOOL_MSG_PHY_GET,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_USER_CNT,
@@ -111,6 +112,7 @@ enum {
 	ETHTOOL_MSG_MM_GET_REPLY,
 	ETHTOOL_MSG_MM_NTF,
 	ETHTOOL_MSG_PHY_LIST_GET_REPLY,
+	ETHTOOL_MSG_PHY_GET_REPLY,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_KERNEL_CNT,
@@ -988,6 +990,18 @@ enum {
 	ETHTOOL_A_PHY_LIST_MAX = (__ETHTOOL_A_PHY_LIST_CNT - 1)
 };
 
+enum {
+	ETHTOOL_A_PHY_UNSPEC,
+	ETHTOOL_A_PHY_HEADER,			/* nest - _A_HEADER_* */
+	ETHTOOL_A_PHY_INDEX,			/* u32 */
+	ETHTOOL_A_PHY_DRVNAME,			/* string */
+	ETHTOOL_A_PHY_UPSTREAM_TYPE,		/* u8 */
+	ETHTOOL_A_PHY_ID,			/* u32 */
+
+	__ETHTOOL_A_PHY_CNT,
+	ETHTOOL_A_PHY_MAX = (__ETHTOOL_A_PHY_CNT - 1)
+};
+
 /* generic netlink info */
 #define ETHTOOL_GENL_NAME "ethtool"
 #define ETHTOOL_GENL_VERSION 1
diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
index a182c0dbbb9d..e6ef280431d6 100644
--- a/net/ethtool/Makefile
+++ b/net/ethtool/Makefile
@@ -8,4 +8,4 @@ ethtool_nl-y	:= netlink.o bitset.o strset.o linkinfo.o linkmodes.o rss.o \
 		   linkstate.o debug.o wol.o features.o privflags.o rings.o \
 		   channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o \
 		   tunnels.o fec.o eeprom.o stats.o phc_vclocks.o mm.o \
-		   module.o pse-pd.o plca.o mm.o phy_list.o
+		   module.o pse-pd.o plca.o mm.o phy_list.o phy.o
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 60b66b78055e..88a60fbb8806 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -307,6 +307,7 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
 	[ETHTOOL_MSG_MM_GET]		= &ethnl_mm_request_ops,
 	[ETHTOOL_MSG_MM_SET]		= &ethnl_mm_request_ops,
 	[ETHTOOL_MSG_PHY_LIST_GET]	= &ethnl_phy_list_request_ops,
+	[ETHTOOL_MSG_PHY_GET]		= &ethnl_phy_request_ops,
 };
 
 static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
@@ -1138,6 +1139,15 @@ static const struct genl_ops ethtool_genl_ops[] = {
 		.policy = ethnl_phy_list_get_policy,
 		.maxattr = ARRAY_SIZE(ethnl_phy_list_get_policy) - 1,
 	},
+	{
+		.cmd	= ETHTOOL_MSG_PHY_GET,
+		.doit	= ethnl_default_doit,
+		.start	= ethnl_default_start,
+		.dumpit	= ethnl_default_dumpit,
+		.done	= ethnl_default_done,
+		.policy = ethnl_phy_get_policy,
+		.maxattr = ARRAY_SIZE(ethnl_phy_get_policy) - 1,
+	},
 };
 
 static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 76859d8fbce0..15f75fd211fc 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -396,6 +396,7 @@ extern const struct ethnl_request_ops ethnl_plca_cfg_request_ops;
 extern const struct ethnl_request_ops ethnl_plca_status_request_ops;
 extern const struct ethnl_request_ops ethnl_mm_request_ops;
 extern const struct ethnl_request_ops ethnl_phy_list_request_ops;
+extern const struct ethnl_request_ops ethnl_phy_request_ops;
 
 extern const struct nla_policy ethnl_header_policy[ETHTOOL_A_HEADER_FLAGS + 1];
 extern const struct nla_policy ethnl_header_policy_stats[ETHTOOL_A_HEADER_FLAGS + 1];
@@ -443,6 +444,7 @@ extern const struct nla_policy ethnl_plca_get_status_policy[ETHTOOL_A_PLCA_HEADE
 extern const struct nla_policy ethnl_mm_get_policy[ETHTOOL_A_MM_HEADER + 1];
 extern const struct nla_policy ethnl_mm_set_policy[ETHTOOL_A_MM_MAX + 1];
 extern const struct nla_policy ethnl_phy_list_get_policy[ETHTOOL_A_PHY_LIST_HEADER + 1];
+extern const struct nla_policy ethnl_phy_get_policy[ETHTOOL_A_PHY_INDEX + 1];
 
 int ethnl_set_features(struct sk_buff *skb, struct genl_info *info);
 int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info);
diff --git a/net/ethtool/phy.c b/net/ethtool/phy.c
new file mode 100644
index 000000000000..0f646bec946b
--- /dev/null
+++ b/net/ethtool/phy.c
@@ -0,0 +1,124 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2023 Bootlin
+ *
+ */
+#include "common.h"
+#include "netlink.h"
+
+#include <linux/phy.h>
+#include <linux/phy_ns.h>
+
+struct phy_req_info {
+	struct ethnl_req_info		base;
+	u32				phyindex;
+};
+
+struct phy_reply_data {
+	struct ethnl_reply_data		base;
+	u32				phyindex;
+	const char			*drvname;
+	enum phy_upstream_type		upstream;
+	u32				id;
+};
+
+#define PHY_REQINFO(__req_base) \
+	container_of(__req_base, struct phy_req_info, base)
+#define PHY_REPDATA(__reply_base) \
+	container_of(__reply_base, struct phy_reply_data, base)
+
+const struct nla_policy ethnl_phy_get_policy[ETHTOOL_A_PHY_INDEX + 1] = {
+	[ETHTOOL_A_PHY_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
+	[ETHTOOL_A_PHY_INDEX] = NLA_POLICY_MAX(NLA_U32, 255),
+};
+
+static int phy_parse_request(struct ethnl_req_info *req_base,
+			     struct nlattr **tb,
+			     struct netlink_ext_ack *extack)
+{
+	struct phy_req_info *req_info = PHY_REQINFO(req_base);
+
+	if (!tb[ETHTOOL_A_PHY_INDEX])
+		return -EINVAL;
+
+	req_info->phyindex = nla_get_u32(tb[ETHTOOL_A_PHY_INDEX]);
+
+	return 0;
+}
+
+static int phy_prepare_data(const struct ethnl_req_info *req_base,
+			    struct ethnl_reply_data *reply_base,
+			    struct genl_info *info)
+{
+	struct phy_req_info *req_info = PHY_REQINFO(req_base);
+	struct phy_reply_data *data = PHY_REPDATA(reply_base);
+	struct net_device *dev = reply_base->dev;
+	struct phy_namespace *phy_ns = &dev->phy_ns;
+	struct phy_device *phydev;
+	int ret;
+
+	phydev = phy_ns_get_by_index(phy_ns, req_info->phyindex);
+	if (!phydev)
+		return -ENODEV;
+
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		return ret;
+
+	data->phyindex = req_info->phyindex;
+	data->drvname = phydev->drv->name;
+	if (phydev->is_on_sfp_module)
+		data->upstream = PHY_UPSTREAM_SFP;
+	else if (phydev->attached_dev)
+		data->upstream = PHY_UPSTREAM_MAC;
+	else
+		data->upstream = PHY_UPSTREAM_PHY;
+
+	data->id = phydev->phy_id;
+
+	ethnl_ops_complete(dev);
+
+	return ret;
+}
+
+static int phy_reply_size(const struct ethnl_req_info *req_base,
+			  const struct ethnl_reply_data *reply_base)
+{
+	const struct phy_reply_data *data = PHY_REPDATA(reply_base);
+	int len = 0;
+
+	len += nla_total_size(sizeof(u32));	/* ETHTOOL_A_PHY_INDEX */
+	len += ethnl_strz_size(data->drvname);	/* ETHTOOL_A_DRVNAME */
+	len += nla_total_size(sizeof(u8));	/* ETHTOOL_A_PHY_UPSTREAM_TYPE */
+	len += nla_total_size(sizeof(u32));	/* ETHTOOL_A_PHY_ID */
+
+	return len;
+}
+
+static int phy_fill_reply(struct sk_buff *skb,
+			  const struct ethnl_req_info *req_base,
+			  const struct ethnl_reply_data *reply_base)
+{
+	const struct phy_reply_data *data = PHY_REPDATA(reply_base);
+
+	if (nla_put_u32(skb, ETHTOOL_A_PHY_INDEX, data->phyindex) ||
+	    ethnl_put_strz(skb, ETHTOOL_A_PHY_DRVNAME, data->drvname) ||
+	    nla_put_u8(skb, ETHTOOL_A_PHY_UPSTREAM_TYPE, data->upstream) ||
+	    nla_put_u32(skb, ETHTOOL_A_PHY_ID, data->id))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+const struct ethnl_request_ops ethnl_phy_request_ops = {
+	.request_cmd		= ETHTOOL_MSG_PHY_GET,
+	.reply_cmd		= ETHTOOL_MSG_PHY_GET_REPLY,
+	.hdr_attr		= ETHTOOL_A_PHY_HEADER,
+	.req_info_size		= sizeof(struct phy_req_info),
+	.reply_data_size	= sizeof(struct phy_reply_data),
+
+	.parse_request		= phy_parse_request,
+	.prepare_data		= phy_prepare_data,
+	.reply_size		= phy_reply_size,
+	.fill_reply		= phy_fill_reply,
+};
-- 
2.41.0

Re: [RFC PATCH net-next 6/7] net: ethtool: add a netlink command to get PHY information

[Russell King (Oracle)]

On Thu, Sep 07, 2023 at 11:24:04AM +0200, Maxime Chevallier wrote:
> +	data->phyindex = req_info->phyindex;
> +	data->drvname = phydev->drv->name;
> +	if (phydev->is_on_sfp_module)

Please use the accessor provided:

	if (phy_on_sfp(phydev))

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [RFC PATCH net-next 6/7] net: ethtool: add a netlink command to get PHY information

[Maxime Chevallier]

Hello Russell,

On Thu, 7 Sep 2023 11:04:56 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Thu, Sep 07, 2023 at 11:24:04AM +0200, Maxime Chevallier wrote:
> > +	data->phyindex = req_info->phyindex;
> > +	data->drvname = phydev->drv->name;
> > +	if (phydev->is_on_sfp_module)  
> 
> Please use the accessor provided:
> 
> 	if (phy_on_sfp(phydev))

Ack, I'll switch to that then.

Thanks,

Maxime

Re: [RFC PATCH net-next 6/7] net: ethtool: add a netlink command to get PHY information

[Jakub Kicinski]

On Thu,  7 Sep 2023 11:24:04 +0200 Maxime Chevallier wrote:
> +enum phy_upstream_type {
> +	PHY_UPSTREAM_MAC,
> +	PHY_UPSTREAM_SFP,
> +	PHY_UPSTREAM_PHY,
> +};

Would be good to define the enum in the YAML spec, too.
That way YNL users can auto-magically see strings for the values.

Re: [RFC PATCH net-next 6/7] net: ethtool: add a netlink command to get PHY information

[Jakub Kicinski]

On Thu,  7 Sep 2023 11:24:04 +0200 Maxime Chevallier wrote:
>  	ETHTOOL_MSG_PHY_LIST_GET,
> +	ETHTOOL_MSG_PHY_GET,

The distinction between LIST_GET and GET is a bit odd for netlink.
GET has a do and a dump. The dump is effectively LIST_GET.

The dump can accept filtering arguments, like ifindex, if you want 
to narrow down the results, that's perfectly fine (you may need to
give up some of the built-in ethtool scaffolding, but it shouldn't 
be all that bad).

Re: [RFC PATCH net-next 6/7] net: ethtool: add a netlink command to get PHY information

[Maxime Chevallier]

Hello Jakub,

On Fri, 8 Sep 2023 08:46:06 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> On Thu,  7 Sep 2023 11:24:04 +0200 Maxime Chevallier wrote:
> >  	ETHTOOL_MSG_PHY_LIST_GET,
> > +	ETHTOOL_MSG_PHY_GET,  
> 
> The distinction between LIST_GET and GET is a bit odd for netlink.
> GET has a do and a dump. The dump is effectively LIST_GET.
> 
> The dump can accept filtering arguments, like ifindex, if you want 
> to narrow down the results, that's perfectly fine (you may need to
> give up some of the built-in ethtool scaffolding, but it shouldn't 
> be all that bad).

I'm currently implementing this, and I was wondering if it could be
worth it to include a pointer to struct phy_device directly in
ethnl_req_info.

This would share the logic for all netlink commands that target a
phy_device :

 - plca
 - pse-pd
 - cabletest
 - other future commands

Do you see this as acceptable ? we would grab the phy_device that
matches the passed phy_index in the request, and if none is specified,
we default to dev->phydev.

Thanks,

Maxime

Re: [RFC PATCH net-next 6/7] net: ethtool: add a netlink command to get PHY information

[Jakub Kicinski]

On Thu, 14 Sep 2023 11:36:13 +0200 Maxime Chevallier wrote:
> I'm currently implementing this, and I was wondering if it could be
> worth it to include a pointer to struct phy_device directly in
> ethnl_req_info.
> 
> This would share the logic for all netlink commands that target a
> phy_device :
> 
>  - plca
>  - pse-pd
>  - cabletest
>  - other future commands
> 
> Do you see this as acceptable ? we would grab the phy_device that
> matches the passed phy_index in the request, and if none is specified,
> we default to dev->phydev.

You may need to be careful with that. It could work in practice but 
the req_info is parsed without holding any locks, IIRC. And there
may also be some interplay between PHY state and ethnl_ops_begin().

From netlink perspective putting the PHY info in the header nest makes
perfect sense to me. Just not sure if you can actually get the object
when the parsing happens or you'd need to just store the index and
resolve it later? PHYLIB maintainers may be best at advising on the
lifetime expectations for phys..

Sorry for the delayed response, #vacation.

Re: [RFC PATCH net-next 6/7] net: ethtool: add a netlink command to get PHY information

[Andrew Lunn]

On Tue, Oct 03, 2023 at 06:55:35AM -0700, Jakub Kicinski wrote:
> On Thu, 14 Sep 2023 11:36:13 +0200 Maxime Chevallier wrote:
> > I'm currently implementing this, and I was wondering if it could be
> > worth it to include a pointer to struct phy_device directly in
> > ethnl_req_info.
> > 
> > This would share the logic for all netlink commands that target a
> > phy_device :
> > 
> >  - plca
> >  - pse-pd
> >  - cabletest
> >  - other future commands
> > 
> > Do you see this as acceptable ? we would grab the phy_device that
> > matches the passed phy_index in the request, and if none is specified,
> > we default to dev->phydev.
> 
> You may need to be careful with that. It could work in practice but 
> the req_info is parsed without holding any locks, IIRC. And there
> may also be some interplay between PHY state and ethnl_ops_begin().

We also need to ensure it is totally optional. There are MAC drivers
which reinvent the wheel in firmware. They can have multiple PHYs, or
PHY and SFP in parallel etc. All the typologies which you are
considering for phylink. Ideally we want the uAPI to work for
everybody, not just phylink. Its not our problem how said firmware
actually works, and what additional wheels they need to re-implement,
but we should try not to block them.

    Andrew

[RFC PATCH net-next 7/7] netlink: specs: add command to show individual phy information

[Maxime Chevallier]

With the ETHTOOL_CMD_PHY_LIST_GET command, we can obtain a list of PHYs
on a link, addressable through their phyindex. This index can be used to
issue PHY-specific commands. The phy_get command allows querying per-PHY
information. The information reported so-far is minimal (driver name,
phy id (for C22), upstream PHY type (real PHY, SFP phy), but we can
imagine extending this in the future to report PHY offloading
capabilities, status, and much more.

Example usage :

./cli.py --spec specs/ethtool.yaml --schema genetlink-legacy.yaml \
         --do phy-list-get --json '{"header" : {"dev-name" : "eth0"}}'
{'header': {'dev-index': 2, 'dev-name': 'eth0'},
 'phy-count': 2,
 'phy-indices': b'\x02\x00\x00\x00\x01\x00\x00\x00'}

./cli.py --spec specs/ethtool.yaml --schema genetlink-legacy.yaml \
         --do phy-get \
	 --json '{"header" : {"dev-name" : "eth0"}, "phy-index" : 1}'
{'drvname': 'mv88x3310',
 'header': {'dev-index': 2, 'dev-name': 'eth0'},
 'phy-id': 0,
 'phy-index': 1,
 'phy-upstream-type': 0}

./cli.py --spec specs/ethtool.yaml --schema genetlink-legacy.yaml \
	 --do phy-get \
	 --json '{"header" : {"dev-name" : "eth0"}, "phy-index" : 2}'
{'drvname': 'Marvell 88E1111',
 'header': {'dev-index': 2, 'dev-name': 'eth0'},
 'phy-id': 21040322,
 'phy-index': 2,
 'phy-upstream-type': 2}

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 Documentation/netlink/specs/ethtool.yaml | 37 ++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 1139c88ed65c..708a77423286 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -955,6 +955,25 @@ attribute-sets:
       -
         name: phy-indices
         type: binary
+  -
+    name: phy
+    attributes:
+      -
+        name: header
+        type: nest
+        nested-attributes: header
+      -
+        name: phy-index
+        type: u32
+      -
+        name: drvname
+        type: string
+      -
+        name: phy-upstream-type
+        type: u8
+      -
+        name: phy-id
+        type: u32
 
 operations:
   enum-model: directional
@@ -1720,3 +1739,21 @@ operations:
             - header
             - phy-count
             - phy-indices
+    -
+      name: phy-get
+      doc: Get a PHY's information
+
+      attribute-set: phy
+
+      do: &phy-get-op
+        request:
+          attributes:
+            - header
+            - phy-index
+        reply:
+          attributes:
+            - header
+            - phy-index
+            - drvname
+            - phy-upstream-type
+            - phy-id
-- 
2.41.0

Re: [RFC PATCH net-next 0/7] net: phy: introduce phy numbering

[Jakub Kicinski]

On Thu,  7 Sep 2023 11:23:58 +0200 Maxime Chevallier wrote:
>  - the netlink API would need polishing, I struggle a bit with finding
>    the correct netlink design pattern to return variale-length list of u32.

Think of them as a list, not an array.

Dump them one by one, don't try to wrap them in any way:
https://docs.kernel.org/next/userspace-api/netlink/specs.html#multi-attr-arrays
People have tried other things in the past:
https://docs.kernel.org/next/userspace-api/netlink/genetlink-legacy.html#attribute-type-nests
but in the end they add constraints and pain for little benefit.

Re: [RFC PATCH net-next 0/7] net: phy: introduce phy numbering

[Maxime Chevallier]

Hello Jakub

On Fri, 8 Sep 2023 08:41:08 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> On Thu,  7 Sep 2023 11:23:58 +0200 Maxime Chevallier wrote:
> >  - the netlink API would need polishing, I struggle a bit with finding
> >    the correct netlink design pattern to return variale-length list of u32.  
> 
> Think of them as a list, not an array.
> 
> Dump them one by one, don't try to wrap them in any way:
> https://docs.kernel.org/next/userspace-api/netlink/specs.html#multi-attr-arrays
> People have tried other things in the past:
> https://docs.kernel.org/next/userspace-api/netlink/genetlink-legacy.html#attribute-type-nests
> but in the end they add constraints and pain for little benefit.

Thanks for the pointers, this makes much more sense than my attempt at
creating an array.

This and your other comment on the .do vs .dump is exactly what I was
missing in my understanding of netlink.

Maxime

Re: [RFC PATCH net-next 0/7] net: phy: introduce phy numbering

[Andrew Lunn]

> The PHY namespace is for now contained within struct net_device, meaning
> that PHYs that aren't related at all to any net_device wouldn't be
> numbered as of right now. The only case I identified is when a PHY sits
> between 2 DSA switches, but I don't know how relevant this is.

It might be relevant for the CPU port of the switch. The SoC ethernet
with a PHY has its PHY associated to a netdev, and so it can be
managed. However, the CPU port does not have a netdev, so the PHY is a
bit homeless. Phylink gained the ability to manage PHYs which are not
associated to a netdev, so i think it can manage such a PHY. If not,
we assume the PHY is strapped to perform link up and autoneg on power
on, and otherwise leave it alone.

	Andrew

Re: [RFC PATCH net-next 0/7] net: phy: introduce phy numbering

[Maxime Chevallier]

Hello Andrew,

On Tue, 12 Sep 2023 17:36:56 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > The PHY namespace is for now contained within struct net_device, meaning
> > that PHYs that aren't related at all to any net_device wouldn't be
> > numbered as of right now. The only case I identified is when a PHY sits
> > between 2 DSA switches, but I don't know how relevant this is.  
> 
> It might be relevant for the CPU port of the switch. The SoC ethernet
> with a PHY has its PHY associated to a netdev, and so it can be
> managed. However, the CPU port does not have a netdev, so the PHY is a
> bit homeless. Phylink gained the ability to manage PHYs which are not
> associated to a netdev, so i think it can manage such a PHY. If not,
> we assume the PHY is strapped to perform link up and autoneg on power
> on, and otherwise leave it alone.

I agree and my plan, although still a bit hazy, is to share the phy_ns
between the netdev associated to the Ethernet MAC and the CPU dsa_port
of the switch, as they are on the same link. We could grab infos on the
PHYs connected to the port that way. Although the PHY isn't connected
to the same MAC, it's part of the same link, so I think it would be OK
to share the phy_ns.

We already do something in that direction, which is the stats gathering
on the CPU dsa port, which are reported alongside stats from the
ethernet MAC.

Would that be OK ? I haven't started the DSA part, I was waiting for
review on the overall idea, but I tried to keep this into consideration
hence the phy_ns notion :)

Thanks,

Maxime

Re: [RFC PATCH net-next 0/7] net: phy: introduce phy numbering

[Christophe Leroy]

Le 07/09/2023 à 11:23, Maxime Chevallier a écrit :
> [Vous ne recevez pas souvent de courriers de maxime.chevallier@bootlin.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
> 
> Hello everyone,
> 
> This is the first RFC series introducing ethernet PHY numbering, in an
> effort to better represent the link components and allow userspace to
> configure these.
> 
> As of today, PHY devices are hidden behind the struct net_device from
> userspace, but there exists commands such as PLCA configuration,
> cable-testing, soon timestamping, that actually target the phy_device.
> 
> These commands rely on the ndev->phydev pointer to find the phy_device.
> 
> However, there exists use-cases where we have multiple PHY devices
> between the MAC and the front-facing port. The most common case right
> now is when a PHY acts as a media-converter, and is wired to an SFP
> port :
> 
> [MAC] - [PHY] - [SFP][PHY]

FWIW when thinking about multiple PHY to a single MAC, what comes to my 
mind is the SIS 900 board, and its driver net/ethernet/sis/sis900.c

It has a function sis900_default_phy() that loops over all phys to find 
one with up-link then to put all but that one in ISOLATE mode. Then when 
the link goes down it loops again to find another up-link.

I guess your series would also help in that case, wouldn't it ?

Christophe

Re: [RFC PATCH net-next 0/7] net: phy: introduce phy numbering

[Andrew Lunn]

> FWIW when thinking about multiple PHY to a single MAC, what comes to my 
> mind is the SIS 900 board, and its driver net/ethernet/sis/sis900.c
> 
> It has a function sis900_default_phy() that loops over all phys to find 
> one with up-link then to put all but that one in ISOLATE mode. Then when 
> the link goes down it loops again to find another up-link.
> 
> I guess your series would also help in that case, wouldn't it ?

Yes, it would. However, that driver would need its PHY handling
re-written because it is using the old MII code, not phylib.

	Andrew