All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH net-next v2 00/10] Introduce PHY listing and link_topology tracking
@ 2023-11-17 16:23 ` Maxime Chevallier
  0 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-17 16:23 UTC (permalink / raw
  To: davem
  Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
	Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni,
	Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina,
	Florian Fainelli, Heiner Kallweit, Vladimir Oltean,
	Köry Maincent, Jesse Brandeburg

Hello everyone,

As part of the ongoing effort to better describe the ethernet link
topology, this series introduces the first step by allowing to maintain
a list of all the ethernet PHYs that are connected to a given netdevice.

For now, this can happen when using a PHY as a media converter :

   MAC - PHYa - SFP - PHYb (in SFP module)

The issue with the above is that from userspace, we can only interact
with PHYa, as it's referenced by netdev->phydev, used in PHY-specific
netlink commands. This series therefore proposes to keep track of all
PHYs, through the struct link_topology, owned by struct
net_device. Phylib is therefore registering all PHYs and information on
their parent device, through direct attachment to a MAC or as part of an
SFP bus. This is done by patches 1 to 3.

Patches 4 to 6 introduce a new netlink command to get/dump the PHYs on a
given interface, with enough information to reconstruct the whole
topology, especially if we have chained PHYs.

Patches 7 to 10 are a proposition to extend the PLCA, PSE-PD, Cabletest
and stats reporting commands to take the PHY index into account, however
I only did minimal testing on these, and I'd like feedback on the idea.

Although more complete, this is still RFC, but I have some followup
series for a better port representation that depends on it, which I can
include for next revisions, but I don't want to make the series too big.

The first RFC was much less compete, but can be found here [1].

The overall topic was presented at Netdev 0x17 [2]

Best regards,

Maxime

[1] : https://lore.kernel.org/netdev/20230907092407.647139-1-maxime.chevallier@bootlin.com/
[2] : https://bootlin.com/pub/conferences/2023/netdev/multi-port-multi-phy-interfaces.pdf

Maxime Chevallier (10):
  net: phy: Introduce ethernet link topology representation
  net: sfp: pass the phy_device when disconnecting an sfp module's PHY
  net: phy: add helpers to handle sfp phy connect/disconnect
  net: sfp: Add helper to return the SFP bus name
  net: ethtool: Allow passing a phy index for some commands
  net: ethtool: Introduce a command to list PHYs on an interface
  net: ethtool: plca: Target the command to the requested PHY
  net: ethtool: pse-pd: Target the command to the requested PHY
  net: ethtool: cable-test: Target the command to the requested PHY
  net: ethtool: strset: Allow querying phy stats by index

 Documentation/netlink/specs/ethtool.yaml     |  69 ++++-
 Documentation/networking/ethtool-netlink.rst |  51 ++++
 MAINTAINERS                                  |   1 +
 drivers/net/phy/Makefile                     |   2 +-
 drivers/net/phy/at803x.c                     |   2 +
 drivers/net/phy/link_topology.c              |  78 ++++++
 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                 |  54 ++++
 drivers/net/phy/phylink.c                    |   3 +-
 drivers/net/phy/sfp-bus.c                    |  13 +-
 include/linux/ethtool_netlink.h              |   5 +
 include/linux/link_topology.h                |  59 ++++
 include/linux/link_topology_core.h           |  17 ++
 include/linux/netdevice.h                    |   3 +-
 include/linux/phy.h                          |   5 +
 include/linux/sfp.h                          |   8 +-
 include/uapi/linux/ethtool.h                 |   7 +
 include/uapi/linux/ethtool_netlink.h         |  30 ++
 net/core/dev.c                               |   4 +
 net/ethtool/Makefile                         |   2 +-
 net/ethtool/cabletest.c                      |  12 +-
 net/ethtool/netlink.c                        |  30 ++
 net/ethtool/netlink.h                        |  11 +-
 net/ethtool/phy.c                            | 279 +++++++++++++++++++
 net/ethtool/plca.c                           |  13 +-
 net/ethtool/pse-pd.c                         |  14 +-
 net/ethtool/strset.c                         |  15 +-
 29 files changed, 752 insertions(+), 41 deletions(-)
 create mode 100644 drivers/net/phy/link_topology.c
 create mode 100644 include/linux/link_topology.h
 create mode 100644 include/linux/link_topology_core.h
 create mode 100644 net/ethtool/phy.c

-- 
2.41.0


^ permalink raw reply	[flat|nested] 53+ messages in thread

* [RFC PATCH net-next v2 00/10] Introduce PHY listing and link_topology tracking
@ 2023-11-17 16:23 ` Maxime Chevallier
  0 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-17 16:23 UTC (permalink / raw
  To: davem
  Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
	Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni,
	Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina,
	Florian Fainelli, Heiner Kallweit, Vladimir Oltean,
	Köry Maincent, Jesse Brandeburg

Hello everyone,

As part of the ongoing effort to better describe the ethernet link
topology, this series introduces the first step by allowing to maintain
a list of all the ethernet PHYs that are connected to a given netdevice.

For now, this can happen when using a PHY as a media converter :

   MAC - PHYa - SFP - PHYb (in SFP module)

The issue with the above is that from userspace, we can only interact
with PHYa, as it's referenced by netdev->phydev, used in PHY-specific
netlink commands. This series therefore proposes to keep track of all
PHYs, through the struct link_topology, owned by struct
net_device. Phylib is therefore registering all PHYs and information on
their parent device, through direct attachment to a MAC or as part of an
SFP bus. This is done by patches 1 to 3.

Patches 4 to 6 introduce a new netlink command to get/dump the PHYs on a
given interface, with enough information to reconstruct the whole
topology, especially if we have chained PHYs.

Patches 7 to 10 are a proposition to extend the PLCA, PSE-PD, Cabletest
and stats reporting commands to take the PHY index into account, however
I only did minimal testing on these, and I'd like feedback on the idea.

Although more complete, this is still RFC, but I have some followup
series for a better port representation that depends on it, which I can
include for next revisions, but I don't want to make the series too big.

The first RFC was much less compete, but can be found here [1].

The overall topic was presented at Netdev 0x17 [2]

Best regards,

Maxime

[1] : https://lore.kernel.org/netdev/20230907092407.647139-1-maxime.chevallier@bootlin.com/
[2] : https://bootlin.com/pub/conferences/2023/netdev/multi-port-multi-phy-interfaces.pdf

Maxime Chevallier (10):
  net: phy: Introduce ethernet link topology representation
  net: sfp: pass the phy_device when disconnecting an sfp module's PHY
  net: phy: add helpers to handle sfp phy connect/disconnect
  net: sfp: Add helper to return the SFP bus name
  net: ethtool: Allow passing a phy index for some commands
  net: ethtool: Introduce a command to list PHYs on an interface
  net: ethtool: plca: Target the command to the requested PHY
  net: ethtool: pse-pd: Target the command to the requested PHY
  net: ethtool: cable-test: Target the command to the requested PHY
  net: ethtool: strset: Allow querying phy stats by index

 Documentation/netlink/specs/ethtool.yaml     |  69 ++++-
 Documentation/networking/ethtool-netlink.rst |  51 ++++
 MAINTAINERS                                  |   1 +
 drivers/net/phy/Makefile                     |   2 +-
 drivers/net/phy/at803x.c                     |   2 +
 drivers/net/phy/link_topology.c              |  78 ++++++
 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                 |  54 ++++
 drivers/net/phy/phylink.c                    |   3 +-
 drivers/net/phy/sfp-bus.c                    |  13 +-
 include/linux/ethtool_netlink.h              |   5 +
 include/linux/link_topology.h                |  59 ++++
 include/linux/link_topology_core.h           |  17 ++
 include/linux/netdevice.h                    |   3 +-
 include/linux/phy.h                          |   5 +
 include/linux/sfp.h                          |   8 +-
 include/uapi/linux/ethtool.h                 |   7 +
 include/uapi/linux/ethtool_netlink.h         |  30 ++
 net/core/dev.c                               |   4 +
 net/ethtool/Makefile                         |   2 +-
 net/ethtool/cabletest.c                      |  12 +-
 net/ethtool/netlink.c                        |  30 ++
 net/ethtool/netlink.h                        |  11 +-
 net/ethtool/phy.c                            | 279 +++++++++++++++++++
 net/ethtool/plca.c                           |  13 +-
 net/ethtool/pse-pd.c                         |  14 +-
 net/ethtool/strset.c                         |  15 +-
 29 files changed, 752 insertions(+), 41 deletions(-)
 create mode 100644 drivers/net/phy/link_topology.c
 create mode 100644 include/linux/link_topology.h
 create mode 100644 include/linux/link_topology_core.h
 create mode 100644 net/ethtool/phy.c

-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 53+ messages in thread

* [RFC PATCH net-next v2 01/10] net: phy: Introduce ethernet link topology representation
  2023-11-17 16:23 ` Maxime Chevallier
@ 2023-11-17 16:23   ` Maxime Chevallier
  -1 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-17 16:23 UTC (permalink / raw
  To: davem
  Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
	Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni,
	Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina,
	Florian Fainelli, Heiner Kallweit, Vladimir Oltean,
	Köry Maincent, Jesse Brandeburg

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>
---
 MAINTAINERS                        |  1 +
 drivers/net/phy/Makefile           |  2 +-
 drivers/net/phy/link_topology.c    | 78 ++++++++++++++++++++++++++++++
 drivers/net/phy/phy_device.c       | 14 ++++++
 include/linux/link_topology.h      | 59 ++++++++++++++++++++++
 include/linux/link_topology_core.h | 17 +++++++
 include/linux/netdevice.h          |  3 +-
 include/linux/phy.h                |  3 ++
 include/uapi/linux/ethtool.h       |  7 +++
 net/core/dev.c                     |  4 ++
 10 files changed, 186 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/phy/link_topology.c
 create mode 100644 include/linux/link_topology.h
 create mode 100644 include/linux/link_topology_core.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 76dea3f2e391..08b5892cf349 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7820,6 +7820,7 @@ F:	include/linux/mii.h
 F:	include/linux/of_net.h
 F:	include/linux/phy.h
 F:	include/linux/phy_fixed.h
+F:	include/linux/phy_list.h
 F:	include/linux/phylib_stubs.h
 F:	include/linux/platform_data/mdio-bcm-unimac.h
 F:	include/linux/platform_data/mdio-gpio.h
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index c945ed9bd14b..41d528e0e532 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 link_topology.o
 mdio-bus-y			+= mdio_bus.o mdio_device.o
 
 ifdef CONFIG_MDIO_DEVICE
diff --git a/drivers/net/phy/link_topology.c b/drivers/net/phy/link_topology.c
new file mode 100644
index 000000000000..0a08ca729e84
--- /dev/null
+++ b/drivers/net/phy/link_topology.c
@@ -0,0 +1,78 @@
+// 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/link_topology.h>
+#include <linux/netdevice.h>
+#include <linux/phy.h>
+#include <linux/xarray.h>
+
+struct phy_device *link_topo_get_phy(struct link_topology *lt, int phyindex)
+{
+	struct phy_device_node *pdn = xa_load(&lt->phys, phyindex);
+
+	if (pdn)
+		return pdn->phy;
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(link_topo_get_phy);
+
+int link_topo_add_phy(struct link_topology *lt, struct phy_device *phy,
+		      enum phy_upstream upt, void *upstream)
+{
+	struct phy_device_node *pdn;
+	int ret;
+
+	pdn = kzalloc(sizeof(*pdn), GFP_KERNEL);
+	if (!pdn)
+		return -ENOMEM;
+
+	pdn->phy = phy;
+	switch (upt) {
+	case PHY_UPSTREAM_MAC:
+		pdn->upstream.netdev = (struct net_device *)upstream;
+		if (phy_on_sfp(phy))
+			pdn->parent_sfp_bus = pdn->upstream.netdev->sfp_bus;
+		break;
+	case PHY_UPSTREAM_PHY:
+		pdn->upstream.phydev = (struct phy_device *)upstream;
+		if (phy_on_sfp(phy))
+			pdn->parent_sfp_bus = pdn->upstream.phydev->sfp_bus;
+		break;
+	default:
+		ret = -EINVAL;
+		goto err;
+	}
+	pdn->upstream_type = upt;
+
+	ret = xa_alloc_cyclic(&lt->phys, &phy->phyindex, pdn, xa_limit_32b,
+			      &lt->next_phy_index, GFP_KERNEL);
+	if (ret)
+		goto err;
+
+	return 0;
+
+err:
+	kfree(pdn);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(link_topo_add_phy);
+
+void link_topo_del_phy(struct link_topology *lt, struct phy_device *phy)
+{
+	struct phy_device_node *pdn = xa_erase(&lt->phys, phy->phyindex);
+
+	kfree(pdn);
+}
+EXPORT_SYMBOL_GPL(link_topo_del_phy);
+
+void link_topo_init(struct link_topology *lt)
+{
+	xa_init_flags(&lt->phys, XA_FLAGS_ALLOC1);
+	lt->next_phy_index = 1;
+}
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 2ce74593d6e4..b38b0dc00ef7 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -19,6 +19,7 @@
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
+#include <linux/link_topology.h>
 #include <linux/list.h>
 #include <linux/mdio.h>
 #include <linux/mii.h>
@@ -265,6 +266,14 @@ static void phy_mdio_device_remove(struct mdio_device *mdiodev)
 
 static struct phy_driver genphy_driver;
 
+static struct link_topology *phy_get_link_topology(struct phy_device *phydev)
+{
+	if (phydev->attached_dev)
+		return &phydev->attached_dev->link_topo;
+
+	return NULL;
+}
+
 static LIST_HEAD(phy_fixup_list);
 static DEFINE_MUTEX(phy_fixup_lock);
 
@@ -1489,6 +1498,10 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 		if (phydev->sfp_bus_attached)
 			dev->sfp_bus = phydev->sfp_bus;
+
+		err = link_topo_add_phy(&dev->link_topo, phydev, PHY_UPSTREAM_MAC, dev);
+		if (err)
+			goto error;
 	}
 
 	/* Some Ethernet drivers try to connect to a PHY device before
@@ -1814,6 +1827,7 @@ void phy_detach(struct phy_device *phydev)
 	if (dev) {
 		phydev->attached_dev->phydev = NULL;
 		phydev->attached_dev = NULL;
+		link_topo_del_phy(&dev->link_topo, phydev);
 	}
 	phydev->phylink = NULL;
 
diff --git a/include/linux/link_topology.h b/include/linux/link_topology.h
new file mode 100644
index 000000000000..7054fcb7ebc5
--- /dev/null
+++ b/include/linux/link_topology.h
@@ -0,0 +1,59 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * PHY device list 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 __LINK_TOPOLOGY_H
+#define __LINK_TOPOLOGY_H
+
+#include <linux/ethtool.h>
+#include <linux/link_topology_core.h>
+
+struct xarray;
+struct phy_device;
+struct net_device;
+struct sfp_bus;
+
+struct phy_device_node {
+	enum phy_upstream upstream_type;
+
+	union {
+		struct net_device	*netdev;
+		struct phy_device	*phydev;
+	} upstream;
+
+	struct sfp_bus *parent_sfp_bus;
+
+	struct phy_device *phy;
+};
+
+#if IS_ENABLED(CONFIG_PHYLIB)
+struct phy_device *link_topo_get_phy(struct link_topology *lt, int phyindex);
+int link_topo_add_phy(struct link_topology *lt, struct phy_device *phy,
+		      enum phy_upstream upt, void *upstream);
+
+void link_topo_del_phy(struct link_topology *lt, struct phy_device *phy);
+
+#else
+static struct phy_device *link_topo_get_phy(struct link_topology *lt, int phyindex)
+{
+	return NULL;
+}
+
+static int link_topo_add_phy(struct link_topology *lt, struct phy_device *phy,
+			     enum phy_upstream upt, void *upstream)
+{
+	return 0;
+}
+
+static void link_topo_del_phy(struct link_topology *lt, struct phy_device *phy)
+{
+}
+#endif
+
+#endif /* __LINK_TOPOLOGY_H */
diff --git a/include/linux/link_topology_core.h b/include/linux/link_topology_core.h
new file mode 100644
index 000000000000..e8f214a22e1f
--- /dev/null
+++ b/include/linux/link_topology_core.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __LINK_TOPOLOGY_CORE_H
+#define __LINK_TOPOLOGY_CORE_H
+
+struct xarray;
+
+struct link_topology {
+	struct xarray phys;
+	struct xarray ports;
+
+	u32 next_phy_index;
+	u32 next_port_index;
+};
+
+void link_topo_init(struct link_topology *lt);
+
+#endif /* __LINK_TOPOLOGY_CORE_H */
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index a16c9cc063fe..7021a0d3d982 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -40,7 +40,6 @@
 #include <net/dcbnl.h>
 #endif
 #include <net/netprio_cgroup.h>
-
 #include <linux/netdev_features.h>
 #include <linux/neighbour.h>
 #include <uapi/linux/netdevice.h>
@@ -52,6 +51,7 @@
 #include <net/net_trackers.h>
 #include <net/net_debug.h>
 #include <net/dropreason-core.h>
+#include <linux/link_topology_core.h>
 
 struct netpoll_info;
 struct device;
@@ -2405,6 +2405,7 @@ struct net_device {
 #if IS_ENABLED(CONFIG_CGROUP_NET_PRIO)
 	struct netprio_map __rcu *priomap;
 #endif
+	struct link_topology	link_topo;
 	struct phy_device	*phydev;
 	struct sfp_bus		*sfp_bus;
 	struct lock_class_key	*qdisc_tx_busylock;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 3cc52826f18e..d698180b1df0 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;
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index f7fba0dc87e5..5794a2a55399 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 {
+	PHY_UPSTREAM_MAC,
+	PHY_UPSTREAM_SFP,
+	PHY_UPSTREAM_PHY,
+};
+
 #endif /* _UAPI_LINUX_ETHTOOL_H */
diff --git a/net/core/dev.c b/net/core/dev.c
index 0d548431f3fa..a78087240480 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -153,6 +153,7 @@
 #include <linux/prandom.h>
 #include <linux/once_lite.h>
 #include <net/netdev_rx_queue.h>
+#include <linux/link_topology_core.h>
 
 #include "dev.h"
 #include "net-sysfs.h"
@@ -10774,6 +10775,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
+	link_topo_init(&dev->link_topo);
 #endif
 	dev->priv_flags = IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM;
 	setup(dev);
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [RFC PATCH net-next v2 01/10] net: phy: Introduce ethernet link topology representation
@ 2023-11-17 16:23   ` Maxime Chevallier
  0 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-17 16:23 UTC (permalink / raw
  To: davem
  Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
	Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni,
	Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina,
	Florian Fainelli, Heiner Kallweit, Vladimir Oltean,
	Köry Maincent, Jesse Brandeburg

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>
---
 MAINTAINERS                        |  1 +
 drivers/net/phy/Makefile           |  2 +-
 drivers/net/phy/link_topology.c    | 78 ++++++++++++++++++++++++++++++
 drivers/net/phy/phy_device.c       | 14 ++++++
 include/linux/link_topology.h      | 59 ++++++++++++++++++++++
 include/linux/link_topology_core.h | 17 +++++++
 include/linux/netdevice.h          |  3 +-
 include/linux/phy.h                |  3 ++
 include/uapi/linux/ethtool.h       |  7 +++
 net/core/dev.c                     |  4 ++
 10 files changed, 186 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/phy/link_topology.c
 create mode 100644 include/linux/link_topology.h
 create mode 100644 include/linux/link_topology_core.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 76dea3f2e391..08b5892cf349 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7820,6 +7820,7 @@ F:	include/linux/mii.h
 F:	include/linux/of_net.h
 F:	include/linux/phy.h
 F:	include/linux/phy_fixed.h
+F:	include/linux/phy_list.h
 F:	include/linux/phylib_stubs.h
 F:	include/linux/platform_data/mdio-bcm-unimac.h
 F:	include/linux/platform_data/mdio-gpio.h
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index c945ed9bd14b..41d528e0e532 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 link_topology.o
 mdio-bus-y			+= mdio_bus.o mdio_device.o
 
 ifdef CONFIG_MDIO_DEVICE
diff --git a/drivers/net/phy/link_topology.c b/drivers/net/phy/link_topology.c
new file mode 100644
index 000000000000..0a08ca729e84
--- /dev/null
+++ b/drivers/net/phy/link_topology.c
@@ -0,0 +1,78 @@
+// 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/link_topology.h>
+#include <linux/netdevice.h>
+#include <linux/phy.h>
+#include <linux/xarray.h>
+
+struct phy_device *link_topo_get_phy(struct link_topology *lt, int phyindex)
+{
+	struct phy_device_node *pdn = xa_load(&lt->phys, phyindex);
+
+	if (pdn)
+		return pdn->phy;
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(link_topo_get_phy);
+
+int link_topo_add_phy(struct link_topology *lt, struct phy_device *phy,
+		      enum phy_upstream upt, void *upstream)
+{
+	struct phy_device_node *pdn;
+	int ret;
+
+	pdn = kzalloc(sizeof(*pdn), GFP_KERNEL);
+	if (!pdn)
+		return -ENOMEM;
+
+	pdn->phy = phy;
+	switch (upt) {
+	case PHY_UPSTREAM_MAC:
+		pdn->upstream.netdev = (struct net_device *)upstream;
+		if (phy_on_sfp(phy))
+			pdn->parent_sfp_bus = pdn->upstream.netdev->sfp_bus;
+		break;
+	case PHY_UPSTREAM_PHY:
+		pdn->upstream.phydev = (struct phy_device *)upstream;
+		if (phy_on_sfp(phy))
+			pdn->parent_sfp_bus = pdn->upstream.phydev->sfp_bus;
+		break;
+	default:
+		ret = -EINVAL;
+		goto err;
+	}
+	pdn->upstream_type = upt;
+
+	ret = xa_alloc_cyclic(&lt->phys, &phy->phyindex, pdn, xa_limit_32b,
+			      &lt->next_phy_index, GFP_KERNEL);
+	if (ret)
+		goto err;
+
+	return 0;
+
+err:
+	kfree(pdn);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(link_topo_add_phy);
+
+void link_topo_del_phy(struct link_topology *lt, struct phy_device *phy)
+{
+	struct phy_device_node *pdn = xa_erase(&lt->phys, phy->phyindex);
+
+	kfree(pdn);
+}
+EXPORT_SYMBOL_GPL(link_topo_del_phy);
+
+void link_topo_init(struct link_topology *lt)
+{
+	xa_init_flags(&lt->phys, XA_FLAGS_ALLOC1);
+	lt->next_phy_index = 1;
+}
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 2ce74593d6e4..b38b0dc00ef7 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -19,6 +19,7 @@
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
+#include <linux/link_topology.h>
 #include <linux/list.h>
 #include <linux/mdio.h>
 #include <linux/mii.h>
@@ -265,6 +266,14 @@ static void phy_mdio_device_remove(struct mdio_device *mdiodev)
 
 static struct phy_driver genphy_driver;
 
+static struct link_topology *phy_get_link_topology(struct phy_device *phydev)
+{
+	if (phydev->attached_dev)
+		return &phydev->attached_dev->link_topo;
+
+	return NULL;
+}
+
 static LIST_HEAD(phy_fixup_list);
 static DEFINE_MUTEX(phy_fixup_lock);
 
@@ -1489,6 +1498,10 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 		if (phydev->sfp_bus_attached)
 			dev->sfp_bus = phydev->sfp_bus;
+
+		err = link_topo_add_phy(&dev->link_topo, phydev, PHY_UPSTREAM_MAC, dev);
+		if (err)
+			goto error;
 	}
 
 	/* Some Ethernet drivers try to connect to a PHY device before
@@ -1814,6 +1827,7 @@ void phy_detach(struct phy_device *phydev)
 	if (dev) {
 		phydev->attached_dev->phydev = NULL;
 		phydev->attached_dev = NULL;
+		link_topo_del_phy(&dev->link_topo, phydev);
 	}
 	phydev->phylink = NULL;
 
diff --git a/include/linux/link_topology.h b/include/linux/link_topology.h
new file mode 100644
index 000000000000..7054fcb7ebc5
--- /dev/null
+++ b/include/linux/link_topology.h
@@ -0,0 +1,59 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * PHY device list 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 __LINK_TOPOLOGY_H
+#define __LINK_TOPOLOGY_H
+
+#include <linux/ethtool.h>
+#include <linux/link_topology_core.h>
+
+struct xarray;
+struct phy_device;
+struct net_device;
+struct sfp_bus;
+
+struct phy_device_node {
+	enum phy_upstream upstream_type;
+
+	union {
+		struct net_device	*netdev;
+		struct phy_device	*phydev;
+	} upstream;
+
+	struct sfp_bus *parent_sfp_bus;
+
+	struct phy_device *phy;
+};
+
+#if IS_ENABLED(CONFIG_PHYLIB)
+struct phy_device *link_topo_get_phy(struct link_topology *lt, int phyindex);
+int link_topo_add_phy(struct link_topology *lt, struct phy_device *phy,
+		      enum phy_upstream upt, void *upstream);
+
+void link_topo_del_phy(struct link_topology *lt, struct phy_device *phy);
+
+#else
+static struct phy_device *link_topo_get_phy(struct link_topology *lt, int phyindex)
+{
+	return NULL;
+}
+
+static int link_topo_add_phy(struct link_topology *lt, struct phy_device *phy,
+			     enum phy_upstream upt, void *upstream)
+{
+	return 0;
+}
+
+static void link_topo_del_phy(struct link_topology *lt, struct phy_device *phy)
+{
+}
+#endif
+
+#endif /* __LINK_TOPOLOGY_H */
diff --git a/include/linux/link_topology_core.h b/include/linux/link_topology_core.h
new file mode 100644
index 000000000000..e8f214a22e1f
--- /dev/null
+++ b/include/linux/link_topology_core.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __LINK_TOPOLOGY_CORE_H
+#define __LINK_TOPOLOGY_CORE_H
+
+struct xarray;
+
+struct link_topology {
+	struct xarray phys;
+	struct xarray ports;
+
+	u32 next_phy_index;
+	u32 next_port_index;
+};
+
+void link_topo_init(struct link_topology *lt);
+
+#endif /* __LINK_TOPOLOGY_CORE_H */
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index a16c9cc063fe..7021a0d3d982 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -40,7 +40,6 @@
 #include <net/dcbnl.h>
 #endif
 #include <net/netprio_cgroup.h>
-
 #include <linux/netdev_features.h>
 #include <linux/neighbour.h>
 #include <uapi/linux/netdevice.h>
@@ -52,6 +51,7 @@
 #include <net/net_trackers.h>
 #include <net/net_debug.h>
 #include <net/dropreason-core.h>
+#include <linux/link_topology_core.h>
 
 struct netpoll_info;
 struct device;
@@ -2405,6 +2405,7 @@ struct net_device {
 #if IS_ENABLED(CONFIG_CGROUP_NET_PRIO)
 	struct netprio_map __rcu *priomap;
 #endif
+	struct link_topology	link_topo;
 	struct phy_device	*phydev;
 	struct sfp_bus		*sfp_bus;
 	struct lock_class_key	*qdisc_tx_busylock;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 3cc52826f18e..d698180b1df0 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;
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index f7fba0dc87e5..5794a2a55399 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 {
+	PHY_UPSTREAM_MAC,
+	PHY_UPSTREAM_SFP,
+	PHY_UPSTREAM_PHY,
+};
+
 #endif /* _UAPI_LINUX_ETHTOOL_H */
diff --git a/net/core/dev.c b/net/core/dev.c
index 0d548431f3fa..a78087240480 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -153,6 +153,7 @@
 #include <linux/prandom.h>
 #include <linux/once_lite.h>
 #include <net/netdev_rx_queue.h>
+#include <linux/link_topology_core.h>
 
 #include "dev.h"
 #include "net-sysfs.h"
@@ -10774,6 +10775,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
+	link_topo_init(&dev->link_topo);
 #endif
 	dev->priv_flags = IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM;
 	setup(dev);
-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [RFC PATCH net-next v2 02/10] net: sfp: pass the phy_device when disconnecting an sfp module's PHY
  2023-11-17 16:23 ` Maxime Chevallier
@ 2023-11-17 16:23   ` Maxime Chevallier
  -1 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-17 16:23 UTC (permalink / raw
  To: davem
  Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
	Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni,
	Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina,
	Florian Fainelli, Heiner Kallweit, Vladimir Oltean,
	Köry Maincent, Jesse Brandeburg

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 6712883498bb..2abb56c3bff0 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -3310,7 +3310,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


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [RFC PATCH net-next v2 02/10] net: sfp: pass the phy_device when disconnecting an sfp module's PHY
@ 2023-11-17 16:23   ` Maxime Chevallier
  0 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-17 16:23 UTC (permalink / raw
  To: davem
  Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
	Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni,
	Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina,
	Florian Fainelli, Heiner Kallweit, Vladimir Oltean,
	Köry Maincent, Jesse Brandeburg

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 6712883498bb..2abb56c3bff0 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -3310,7 +3310,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [RFC PATCH net-next v2 03/10] net: phy: add helpers to handle sfp phy connect/disconnect
  2023-11-17 16:23 ` Maxime Chevallier
@ 2023-11-17 16:23   ` Maxime Chevallier
  -1 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-17 16:23 UTC (permalink / raw
  To: davem
  Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
	Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni,
	Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina,
	Florian Fainelli, Heiner Kallweit, Vladimir Oltean,
	Köry Maincent, Jesse Brandeburg

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 b38b0dc00ef7..3d68e1f8d785 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1361,6 +1361,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 link_topology *lt = phy_get_link_topology(phydev);
+
+	if (lt)
+		return link_topo_add_phy(lt, phy, PHY_UPSTREAM_PHY, phydev);
+
+	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 link_topology *lt = phy_get_link_topology(phydev);
+
+	if (lt)
+		link_topo_del_phy(lt, 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 d698180b1df0..2d3ae93250e6 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1719,6 +1719,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


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [RFC PATCH net-next v2 03/10] net: phy: add helpers to handle sfp phy connect/disconnect
@ 2023-11-17 16:23   ` Maxime Chevallier
  0 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-17 16:23 UTC (permalink / raw
  To: davem
  Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
	Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni,
	Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina,
	Florian Fainelli, Heiner Kallweit, Vladimir Oltean,
	Köry Maincent, Jesse Brandeburg

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 b38b0dc00ef7..3d68e1f8d785 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1361,6 +1361,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 link_topology *lt = phy_get_link_topology(phydev);
+
+	if (lt)
+		return link_topo_add_phy(lt, phy, PHY_UPSTREAM_PHY, phydev);
+
+	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 link_topology *lt = phy_get_link_topology(phydev);
+
+	if (lt)
+		link_topo_del_phy(lt, 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 d698180b1df0..2d3ae93250e6 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1719,6 +1719,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [RFC PATCH net-next v2 04/10] net: sfp: Add helper to return the SFP bus name
  2023-11-17 16:23 ` Maxime Chevallier
@ 2023-11-17 16:23   ` Maxime Chevallier
  -1 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-17 16:23 UTC (permalink / raw
  To: davem
  Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
	Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni,
	Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina,
	Florian Fainelli, Heiner Kallweit, Vladimir Oltean,
	Köry Maincent, Jesse Brandeburg

Knowing the bus name is helpful when we want to expose the link topology
to userspace, add a helper to return the SFP bus name.

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

diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index f42e9a082935..835fb2271b12 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -859,3 +859,12 @@ void sfp_unregister_socket(struct sfp_bus *bus)
 	sfp_bus_put(bus);
 }
 EXPORT_SYMBOL_GPL(sfp_unregister_socket);
+
+const char *sfp_get_name(struct sfp_bus *bus)
+{
+	if (bus->sfp_dev)
+		return dev_name(bus->sfp_dev);
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(sfp_get_name);
diff --git a/include/linux/sfp.h b/include/linux/sfp.h
index 0573e53b0c11..96f7644908bd 100644
--- a/include/linux/sfp.h
+++ b/include/linux/sfp.h
@@ -570,6 +570,7 @@ struct sfp_bus *sfp_bus_find_fwnode(const struct fwnode_handle *fwnode);
 int sfp_bus_add_upstream(struct sfp_bus *bus, void *upstream,
 			 const struct sfp_upstream_ops *ops);
 void sfp_bus_del_upstream(struct sfp_bus *bus);
+const char *sfp_get_name(struct sfp_bus *bus);
 #else
 static inline int sfp_parse_port(struct sfp_bus *bus,
 				 const struct sfp_eeprom_id *id,
@@ -648,6 +649,11 @@ static inline int sfp_bus_add_upstream(struct sfp_bus *bus, void *upstream,
 static inline void sfp_bus_del_upstream(struct sfp_bus *bus)
 {
 }
+
+static const char *sfp_get_name(struct sfp_bus *bus)
+{
+	return NULL;
+}
 #endif
 
 #endif
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [RFC PATCH net-next v2 04/10] net: sfp: Add helper to return the SFP bus name
@ 2023-11-17 16:23   ` Maxime Chevallier
  0 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-17 16:23 UTC (permalink / raw
  To: davem
  Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
	Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni,
	Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina,
	Florian Fainelli, Heiner Kallweit, Vladimir Oltean,
	Köry Maincent, Jesse Brandeburg

Knowing the bus name is helpful when we want to expose the link topology
to userspace, add a helper to return the SFP bus name.

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

diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index f42e9a082935..835fb2271b12 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -859,3 +859,12 @@ void sfp_unregister_socket(struct sfp_bus *bus)
 	sfp_bus_put(bus);
 }
 EXPORT_SYMBOL_GPL(sfp_unregister_socket);
+
+const char *sfp_get_name(struct sfp_bus *bus)
+{
+	if (bus->sfp_dev)
+		return dev_name(bus->sfp_dev);
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(sfp_get_name);
diff --git a/include/linux/sfp.h b/include/linux/sfp.h
index 0573e53b0c11..96f7644908bd 100644
--- a/include/linux/sfp.h
+++ b/include/linux/sfp.h
@@ -570,6 +570,7 @@ struct sfp_bus *sfp_bus_find_fwnode(const struct fwnode_handle *fwnode);
 int sfp_bus_add_upstream(struct sfp_bus *bus, void *upstream,
 			 const struct sfp_upstream_ops *ops);
 void sfp_bus_del_upstream(struct sfp_bus *bus);
+const char *sfp_get_name(struct sfp_bus *bus);
 #else
 static inline int sfp_parse_port(struct sfp_bus *bus,
 				 const struct sfp_eeprom_id *id,
@@ -648,6 +649,11 @@ static inline int sfp_bus_add_upstream(struct sfp_bus *bus, void *upstream,
 static inline void sfp_bus_del_upstream(struct sfp_bus *bus)
 {
 }
+
+static const char *sfp_get_name(struct sfp_bus *bus)
+{
+	return NULL;
+}
 #endif
 
 #endif
-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [RFC PATCH net-next v2 05/10] net: ethtool: Allow passing a phy index for some commands
  2023-11-17 16:23 ` Maxime Chevallier
@ 2023-11-17 16:23   ` Maxime Chevallier
  -1 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-17 16:23 UTC (permalink / raw
  To: davem
  Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
	Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni,
	Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina,
	Florian Fainelli, Heiner Kallweit, Vladimir Oltean,
	Köry Maincent, Jesse Brandeburg

Some netlink commands are target towards ethernet PHYs, to control some
of their features. As there's several such commands, add the ability to
pass a PHY index in the ethnl request, which will populate the generic
ethnl_req_info with the relevant phydev when the command targets a PHY.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 include/uapi/linux/ethtool_netlink.h |  1 +
 net/ethtool/netlink.c                | 22 ++++++++++++++++++++++
 net/ethtool/netlink.h                |  8 ++++++--
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 73e2c10dc2cc..e557cf35250e 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -133,6 +133,7 @@ enum {
 	ETHTOOL_A_HEADER_DEV_INDEX,		/* u32 */
 	ETHTOOL_A_HEADER_DEV_NAME,		/* string */
 	ETHTOOL_A_HEADER_FLAGS,			/* u32 - ETHTOOL_FLAG_* */
+	ETHTOOL_A_HEADER_PHY_INDEX,		/* u32 */
 
 	/* add new constants above here */
 	__ETHTOOL_A_HEADER_CNT,
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 3bbd5afb7b31..e83ee844b60f 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -4,6 +4,7 @@
 #include <linux/ethtool_netlink.h>
 #include <linux/pm_runtime.h>
 #include "netlink.h"
+#include <linux/link_topology.h>
 
 static struct genl_family ethtool_genl_family;
 
@@ -20,6 +21,7 @@ const struct nla_policy ethnl_header_policy[] = {
 					    .len = ALTIFNAMSIZ - 1 },
 	[ETHTOOL_A_HEADER_FLAGS]	= NLA_POLICY_MASK(NLA_U32,
 							  ETHTOOL_FLAGS_BASIC),
+	[ETHTOOL_A_HEADER_PHY_INDEX]		= NLA_POLICY_MIN(NLA_U32, 1),
 };
 
 const struct nla_policy ethnl_header_policy_stats[] = {
@@ -28,6 +30,7 @@ const struct nla_policy ethnl_header_policy_stats[] = {
 					    .len = ALTIFNAMSIZ - 1 },
 	[ETHTOOL_A_HEADER_FLAGS]	= NLA_POLICY_MASK(NLA_U32,
 							  ETHTOOL_FLAGS_STATS),
+	[ETHTOOL_A_HEADER_PHY_INDEX]		= NLA_POLICY_MIN(NLA_U32, 1),
 };
 
 int ethnl_ops_begin(struct net_device *dev)
@@ -91,6 +94,7 @@ int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info,
 {
 	struct nlattr *tb[ARRAY_SIZE(ethnl_header_policy)];
 	const struct nlattr *devname_attr;
+	struct phy_device *phydev = NULL;
 	struct net_device *dev = NULL;
 	u32 flags = 0;
 	int ret;
@@ -145,6 +149,24 @@ int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info,
 		return -EINVAL;
 	}
 
+	if (dev) {
+		if (tb[ETHTOOL_A_HEADER_PHY_INDEX]) {
+			u32 phy_index = nla_get_u32(tb[ETHTOOL_A_HEADER_PHY_INDEX]);
+
+			phydev = link_topo_get_phy(&dev->link_topo, phy_index);
+			if (!phydev) {
+				NL_SET_ERR_MSG_ATTR(extack, header, "no phy matches phy index");
+				return -EINVAL;
+			}
+		} else {
+			/* If we need a PHY but no phy index is specified, fallback
+			 * to dev->phydev
+			 */
+			phydev = dev->phydev;
+		}
+	}
+
+	req_info->phydev = phydev;
 	req_info->dev = dev;
 	req_info->flags = flags;
 	return 0;
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 9a333a8d04c1..3f5eb60bdf0b 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -250,6 +250,7 @@ static inline unsigned int ethnl_reply_header_size(void)
  * @dev:   network device the request is for (may be null)
  * @dev_tracker: refcount tracker for @dev reference
  * @flags: request flags common for all request types
+ * @phydev: phy_device connected to @dev this request is for (may be null)
  *
  * This is a common base for request specific structures holding data from
  * parsed userspace request. These always embed struct ethnl_req_info at
@@ -259,6 +260,7 @@ struct ethnl_req_info {
 	struct net_device	*dev;
 	netdevice_tracker	dev_tracker;
 	u32			flags;
+	struct phy_device	*phydev;
 };
 
 static inline void ethnl_parse_header_dev_put(struct ethnl_req_info *req_info)
@@ -395,9 +397,10 @@ 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_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];
+extern const struct nla_policy ethnl_header_policy[ETHTOOL_A_HEADER_PHY_INDEX + 1];
+extern const struct nla_policy ethnl_header_policy_stats[ETHTOOL_A_HEADER_PHY_INDEX + 1];
 extern const struct nla_policy ethnl_strset_get_policy[ETHTOOL_A_STRSET_COUNTS_ONLY + 1];
 extern const struct nla_policy ethnl_linkinfo_get_policy[ETHTOOL_A_LINKINFO_HEADER + 1];
 extern const struct nla_policy ethnl_linkinfo_set_policy[ETHTOOL_A_LINKINFO_TP_MDIX_CTRL + 1];
@@ -441,6 +444,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_get_policy[ETHTOOL_A_PHY_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);
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [RFC PATCH net-next v2 05/10] net: ethtool: Allow passing a phy index for some commands
@ 2023-11-17 16:23   ` Maxime Chevallier
  0 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-17 16:23 UTC (permalink / raw
  To: davem
  Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
	Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni,
	Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina,
	Florian Fainelli, Heiner Kallweit, Vladimir Oltean,
	Köry Maincent, Jesse Brandeburg

Some netlink commands are target towards ethernet PHYs, to control some
of their features. As there's several such commands, add the ability to
pass a PHY index in the ethnl request, which will populate the generic
ethnl_req_info with the relevant phydev when the command targets a PHY.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 include/uapi/linux/ethtool_netlink.h |  1 +
 net/ethtool/netlink.c                | 22 ++++++++++++++++++++++
 net/ethtool/netlink.h                |  8 ++++++--
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 73e2c10dc2cc..e557cf35250e 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -133,6 +133,7 @@ enum {
 	ETHTOOL_A_HEADER_DEV_INDEX,		/* u32 */
 	ETHTOOL_A_HEADER_DEV_NAME,		/* string */
 	ETHTOOL_A_HEADER_FLAGS,			/* u32 - ETHTOOL_FLAG_* */
+	ETHTOOL_A_HEADER_PHY_INDEX,		/* u32 */
 
 	/* add new constants above here */
 	__ETHTOOL_A_HEADER_CNT,
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 3bbd5afb7b31..e83ee844b60f 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -4,6 +4,7 @@
 #include <linux/ethtool_netlink.h>
 #include <linux/pm_runtime.h>
 #include "netlink.h"
+#include <linux/link_topology.h>
 
 static struct genl_family ethtool_genl_family;
 
@@ -20,6 +21,7 @@ const struct nla_policy ethnl_header_policy[] = {
 					    .len = ALTIFNAMSIZ - 1 },
 	[ETHTOOL_A_HEADER_FLAGS]	= NLA_POLICY_MASK(NLA_U32,
 							  ETHTOOL_FLAGS_BASIC),
+	[ETHTOOL_A_HEADER_PHY_INDEX]		= NLA_POLICY_MIN(NLA_U32, 1),
 };
 
 const struct nla_policy ethnl_header_policy_stats[] = {
@@ -28,6 +30,7 @@ const struct nla_policy ethnl_header_policy_stats[] = {
 					    .len = ALTIFNAMSIZ - 1 },
 	[ETHTOOL_A_HEADER_FLAGS]	= NLA_POLICY_MASK(NLA_U32,
 							  ETHTOOL_FLAGS_STATS),
+	[ETHTOOL_A_HEADER_PHY_INDEX]		= NLA_POLICY_MIN(NLA_U32, 1),
 };
 
 int ethnl_ops_begin(struct net_device *dev)
@@ -91,6 +94,7 @@ int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info,
 {
 	struct nlattr *tb[ARRAY_SIZE(ethnl_header_policy)];
 	const struct nlattr *devname_attr;
+	struct phy_device *phydev = NULL;
 	struct net_device *dev = NULL;
 	u32 flags = 0;
 	int ret;
@@ -145,6 +149,24 @@ int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info,
 		return -EINVAL;
 	}
 
+	if (dev) {
+		if (tb[ETHTOOL_A_HEADER_PHY_INDEX]) {
+			u32 phy_index = nla_get_u32(tb[ETHTOOL_A_HEADER_PHY_INDEX]);
+
+			phydev = link_topo_get_phy(&dev->link_topo, phy_index);
+			if (!phydev) {
+				NL_SET_ERR_MSG_ATTR(extack, header, "no phy matches phy index");
+				return -EINVAL;
+			}
+		} else {
+			/* If we need a PHY but no phy index is specified, fallback
+			 * to dev->phydev
+			 */
+			phydev = dev->phydev;
+		}
+	}
+
+	req_info->phydev = phydev;
 	req_info->dev = dev;
 	req_info->flags = flags;
 	return 0;
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 9a333a8d04c1..3f5eb60bdf0b 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -250,6 +250,7 @@ static inline unsigned int ethnl_reply_header_size(void)
  * @dev:   network device the request is for (may be null)
  * @dev_tracker: refcount tracker for @dev reference
  * @flags: request flags common for all request types
+ * @phydev: phy_device connected to @dev this request is for (may be null)
  *
  * This is a common base for request specific structures holding data from
  * parsed userspace request. These always embed struct ethnl_req_info at
@@ -259,6 +260,7 @@ struct ethnl_req_info {
 	struct net_device	*dev;
 	netdevice_tracker	dev_tracker;
 	u32			flags;
+	struct phy_device	*phydev;
 };
 
 static inline void ethnl_parse_header_dev_put(struct ethnl_req_info *req_info)
@@ -395,9 +397,10 @@ 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_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];
+extern const struct nla_policy ethnl_header_policy[ETHTOOL_A_HEADER_PHY_INDEX + 1];
+extern const struct nla_policy ethnl_header_policy_stats[ETHTOOL_A_HEADER_PHY_INDEX + 1];
 extern const struct nla_policy ethnl_strset_get_policy[ETHTOOL_A_STRSET_COUNTS_ONLY + 1];
 extern const struct nla_policy ethnl_linkinfo_get_policy[ETHTOOL_A_LINKINFO_HEADER + 1];
 extern const struct nla_policy ethnl_linkinfo_set_policy[ETHTOOL_A_LINKINFO_TP_MDIX_CTRL + 1];
@@ -441,6 +444,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_get_policy[ETHTOOL_A_PHY_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);
-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [RFC PATCH net-next v2 06/10] net: ethtool: Introduce a command to list PHYs on an interface
  2023-11-17 16:23 ` Maxime Chevallier
@ 2023-11-17 16:23   ` Maxime Chevallier
  -1 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-17 16:23 UTC (permalink / raw
  To: davem
  Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
	Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni,
	Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina,
	Florian Fainelli, Heiner Kallweit, Vladimir Oltean,
	Köry Maincent, Jesse Brandeburg

As we have the ability to track the PHYs connected to a net_device
through the link_topology, we can expose this list to userspace. This
allows userspace to use these identifiers for phy-specific commands and
take the decision of which PHY to target by knowing the link topology.

Add PHY_GET and PHY_DUMP, which can be a filtered DUMP operation to list
devices on only one interface.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 Documentation/netlink/specs/ethtool.yaml     |  69 ++++-
 Documentation/networking/ethtool-netlink.rst |  51 ++++
 include/linux/ethtool_netlink.h              |   5 +
 include/uapi/linux/ethtool_netlink.h         |  29 ++
 net/ethtool/Makefile                         |   2 +-
 net/ethtool/netlink.c                        |   8 +
 net/ethtool/netlink.h                        |   3 +
 net/ethtool/phy.c                            | 279 +++++++++++++++++++
 8 files changed, 444 insertions(+), 2 deletions(-)
 create mode 100644 net/ethtool/phy.c

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 5c7a65b009b4..ac9352d8ef57 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -16,6 +16,11 @@ definitions:
     name: stringset
     type: enum
     entries: []
+  -
+    name: phy-upstream-type
+    enum-name:
+    type: enum
+    entries: [ mac, sfp, phy ]
 
 attribute-sets:
   -
@@ -30,7 +35,9 @@ attribute-sets:
       -
         name: flags
         type: u32
-
+      -
+        name: phy-index
+        type: u32
   -
     name: bitset-bit
     attributes:
@@ -939,6 +946,45 @@ attribute-sets:
       -
         name: burst-tmr
         type: u32
+  -
+    name: phy-upstream
+    attributes:
+      -
+        name: index
+        type: u32
+      -
+        name: sfp-name
+        type: string
+  -
+    name: phy
+    attributes:
+      -
+        name: header
+        type: nest
+        nested-attributes: header
+      -
+        name: phy-index
+        type: u32
+      -
+        name: drvname
+        type: string
+      -
+        name: name
+        type: string
+      -
+        name: upstream-type
+        type: u8
+        enum: phy-upstream-type
+      -
+        name: upstream
+        type: nest
+        nested-attributes: phy-upstream
+      -
+        name: downstream-sfp-name
+        type: string
+      -
+        name: id
+        type: u32
 
 operations:
   enum-model: directional
@@ -1689,3 +1735,24 @@ operations:
       name: mm-ntf
       doc: Notification for change in MAC Merge configuration.
       notify: mm-get
+    -
+      name: phy-get
+      doc: Get PHY devices attached to an interface
+
+      attribute-set: phy
+
+      do: &phy-get-op
+        request:
+          attributes:
+            - header
+        reply:
+          attributes:
+            - header
+            - index
+            - drvname
+            - name
+            - upstream-type
+            - upstream
+            - downstream-sfp-name
+            - id
+      dump: *phy-get-op
diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 2540c70952ff..29ef675f45c0 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -57,6 +57,7 @@ Structure of this header is
   ``ETHTOOL_A_HEADER_DEV_INDEX``  u32     device ifindex
   ``ETHTOOL_A_HEADER_DEV_NAME``   string  device name
   ``ETHTOOL_A_HEADER_FLAGS``      u32     flags common for all requests
+  ``ETHTOOL_A_HEADER_PHY_INDEX``  u32     phy device index
   ==============================  ======  =============================
 
 ``ETHTOOL_A_HEADER_DEV_INDEX`` and ``ETHTOOL_A_HEADER_DEV_NAME`` identify the
@@ -81,6 +82,12 @@ the behaviour is backward compatible, i.e. requests from old clients not aware
 of the flag should be interpreted the way the client expects. A client must
 not set flags it does not understand.
 
+``ETHTOOL_A_HEADER_PHY_INDEX`` identify the ethernet PHY the message relates to.
+As there are numerous commands that are related to PHY configuration, and because
+we can have more than one PHY on the link, the PHY index can be passed in the
+request for the commands that needs it. It is however not mandatory, and if it
+is not passed for commands that target a PHY, the net_device.phydev pointer
+is used, as a fallback that keeps the legacy behaviour.
 
 Bit sets
 ========
@@ -1994,6 +2001,49 @@ The attributes are propagated to the driver through the following structure:
 .. kernel-doc:: include/linux/ethtool.h
     :identifiers: ethtool_mm_cfg
 
+PHY_GET
+======
+
+Retrieve information about a given Ethernet PHY sitting on the link. As there
+can be more than one PHY, the DUMP operation can be used to list the PHYs
+present on a given interface, by passing an interface index or name in
+the dump request
+
+Request contents:
+
+  ====================================  ======  ==========================
+  ``ETHTOOL_A_PHY_HEADER``              nested  request header
+  ====================================  ======  ==========================
+
+Kernel response contents:
+
+  =================================     ======  ==========================
+  ``ETHTOOL_A_PHY_HEADER``              nested  request header
+  ``ETHTOOL_A_PHY_INDEX``               u32     the phy's unique index, that can
+                                                be used for phy-specific requests
+  ``ETHTOOL_A_PHY_DRVNAME``             string  the phy driver name
+  ``ETHTOOL_A_PHY_NAME``                string  the phy device name
+  ``ETHTOOL_A_PHY_UPSTREAM_TYPE``       u32     the type of device this phy is
+                                                connected to
+  ``ETHTOOL_A_PHY_UPSTREAM_PHY``        nested  if the phy is connected to another
+                                                phy, this nest contains info on
+                                                that connection
+  ``ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME`` string  if the phy controls an sfp bus,
+                                                the name of the sfp bus
+  ``ETHTOOL_A_PHY_ID``                  u32     the phy id if the phy is C22
+  =================================     ======  ==========================
+
+When ``ETHTOOL_A_PHY_UPSTREAM_TYPE`` is PHY_UPSTREAM_PHY, the PHY's parent is
+another PHY. Information on the parent PHY will be set in the
+``ETHTOOL_A_PHY_UPSTREAM_PHY`` nest, which has the following structure :
+
+  =================================     ======  ==========================
+  ``ETHTOOL_A_PHY_UPSTREAM_INDEX``      u32     the PHY index of the upstream PHY
+  ``ETHTOOL_A_PHY_UPSTREAM_SFP_NAME``   string  if this PHY is connected to it's
+                                                parent PHY through an SFP bus, the
+                                                name of this sfp bus
+  =================================     ======  ==========================
+
 Request translation
 ===================
 
@@ -2100,4 +2150,5 @@ are netlink only.
   n/a                                 ``ETHTOOL_MSG_PLCA_GET_STATUS``
   n/a                                 ``ETHTOOL_MSG_MM_GET``
   n/a                                 ``ETHTOOL_MSG_MM_SET``
+  n/a                                 ``ETHTOOL_MSG_PHY_GET``
   =================================== =====================================
diff --git a/include/linux/ethtool_netlink.h b/include/linux/ethtool_netlink.h
index fae0dfb9a9c8..b4c40f1cc854 100644
--- a/include/linux/ethtool_netlink.h
+++ b/include/linux/ethtool_netlink.h
@@ -118,5 +118,10 @@ static inline bool ethtool_dev_mm_supported(struct net_device *dev)
 	return false;
 }
 
+int ethnl_phy_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
+{
+	return -EOPNOTSUPP;
+}
+
 #endif /* IS_ENABLED(CONFIG_ETHTOOL_NETLINK) */
 #endif /* _LINUX_ETHTOOL_NETLINK_H_ */
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index e557cf35250e..7d621963698a 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_GET,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_USER_CNT,
@@ -109,6 +110,8 @@ enum {
 	ETHTOOL_MSG_PLCA_NTF,
 	ETHTOOL_MSG_MM_GET_REPLY,
 	ETHTOOL_MSG_MM_NTF,
+	ETHTOOL_MSG_PHY_GET_REPLY,
+	ETHTOOL_MSG_PHY_NTF,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_KERNEL_CNT,
@@ -976,6 +979,32 @@ enum {
 	ETHTOOL_A_MM_MAX = (__ETHTOOL_A_MM_CNT - 1)
 };
 
+enum {
+	ETHTOOL_A_PHY_UPSTREAM_UNSPEC,
+	ETHTOOL_A_PHY_UPSTREAM_INDEX,			/* u32 */
+	ETHTOOL_A_PHY_UPSTREAM_SFP_NAME,		/* string */
+
+	/* add new constants above here */
+	__ETHTOOL_A_PHY_UPSTREAM_CNT,
+	ETHTOOL_A_PHY_UPSTREAM_MAX = (__ETHTOOL_A_PHY_UPSTREAM_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_NAME,			/* string */
+	ETHTOOL_A_PHY_UPSTREAM_TYPE,		/* u8 */
+	ETHTOOL_A_PHY_UPSTREAM,			/* nest - _A_PHY_UPSTREAM_* */
+	ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME,	/* string */
+	ETHTOOL_A_PHY_ID,			/* u32 */
+
+	/* add new constants above here */
+	__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 504f954a1b28..0ccd0e9afd3f 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.o
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index e83ee844b60f..80c8c312a584 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -1150,6 +1150,14 @@ 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_GET,
+		.doit	= ethnl_phy_doit,
+		.start	= ethnl_phy_start,
+		.dumpit	= ethnl_phy_dumpit,
+		.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 3f5eb60bdf0b..844c83c2ec7f 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -452,6 +452,9 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info);
 int ethnl_tunnel_info_doit(struct sk_buff *skb, struct genl_info *info);
 int ethnl_tunnel_info_start(struct netlink_callback *cb);
 int ethnl_tunnel_info_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
+int ethnl_phy_start(struct netlink_callback *cb);
+int ethnl_phy_doit(struct sk_buff *skb, struct genl_info *info);
+int ethnl_phy_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
 
 extern const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN];
 extern const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN];
diff --git a/net/ethtool/phy.c b/net/ethtool/phy.c
new file mode 100644
index 000000000000..ac6b4612301c
--- /dev/null
+++ b/net/ethtool/phy.c
@@ -0,0 +1,279 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2023 Bootlin
+ *
+ */
+#include "common.h"
+#include "netlink.h"
+
+#include <linux/link_topology.h>
+#include <linux/phy.h>
+#include <linux/sfp.h>
+
+struct phy_req_info {
+	struct ethnl_req_info		base;
+	struct phy_device_node		pdn;
+};
+
+#define PHY_REQINFO(__req_base) \
+	container_of(__req_base, struct phy_req_info, base)
+
+const struct nla_policy ethnl_phy_get_policy[ETHTOOL_A_PHY_HEADER + 1] = {
+	[ETHTOOL_A_PHY_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
+};
+
+/* Caller holds rtnl */
+static ssize_t
+ethnl_phy_reply_size(const struct ethnl_req_info *req_base,
+		     struct netlink_ext_ack *extack)
+{
+	struct phy_device_node *pdn;
+	struct phy_device *phydev;
+	struct link_topology *lt;
+	unsigned long index;
+	size_t size;
+
+	lt = &req_base->dev->link_topo;
+
+	size = nla_total_size(0);
+
+	xa_for_each(&lt->phys, index, pdn) {
+		phydev = pdn->phy;
+
+		/* ETHTOOL_A_PHY_INDEX */
+		size += nla_total_size(sizeof(u32));
+
+		/* ETHTOOL_A_DRVNAME */
+		size += nla_total_size(strlen(phydev->drv->name));
+
+		/* ETHTOOL_A_NAME */
+		size += nla_total_size(strlen(dev_name(&phydev->mdio.dev)));
+
+		/* ETHTOOL_A_PHY_UPSTREAM_TYPE */
+		size += nla_total_size(sizeof(u8));
+
+		/* ETHTOOL_A_PHY_ID */
+		size += nla_total_size(sizeof(u32));
+
+		if (phy_on_sfp(phydev)) {
+			/* ETHTOOL_A_PHY_UPSTREAM_SFP_NAME */
+			if (sfp_get_name(pdn->parent_sfp_bus))
+				size += nla_total_size(strlen(sfp_get_name(pdn->parent_sfp_bus)));
+
+			/* ETHTOOL_A_PHY_UPSTREAM_INDEX */
+			size += nla_total_size(sizeof(u32));
+		}
+
+		/* ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME */
+		if (phydev->sfp_bus)
+			size += nla_total_size(strlen(sfp_get_name(phydev->sfp_bus)));
+	}
+
+	return size;
+}
+
+static int
+ethnl_phy_fill_reply(const struct ethnl_req_info *req_base, struct sk_buff *skb)
+{
+	struct phy_req_info *req_info = PHY_REQINFO(req_base);
+	struct phy_device_node *pdn = &req_info->pdn;
+	struct phy_device *phydev = pdn->phy;
+	enum phy_upstream ptype;
+	struct nlattr *nest;
+
+	ptype = pdn->upstream_type;
+
+	if (nla_put_u32(skb, ETHTOOL_A_PHY_INDEX, phydev->phyindex) ||
+	    nla_put_string(skb, ETHTOOL_A_PHY_DRVNAME, phydev->drv->name) ||
+	    nla_put_string(skb, ETHTOOL_A_PHY_NAME, dev_name(&phydev->mdio.dev)) ||
+	    nla_put_u8(skb, ETHTOOL_A_PHY_UPSTREAM_TYPE, ptype) ||
+	    nla_put_u32(skb, ETHTOOL_A_PHY_ID, phydev->phy_id))
+		return -EMSGSIZE;
+
+	if (ptype == PHY_UPSTREAM_PHY) {
+		struct phy_device *upstream = pdn->upstream.phydev;
+
+		nest = nla_nest_start(skb, ETHTOOL_A_PHY_UPSTREAM);
+		if (!nest)
+			return -EMSGSIZE;
+
+		/* Parent index */
+		if (nla_put_u32(skb, ETHTOOL_A_PHY_UPSTREAM_INDEX, upstream->phyindex))
+			return -EMSGSIZE;
+
+		if (pdn->parent_sfp_bus && sfp_get_name(pdn->parent_sfp_bus) &&
+		    nla_put_string(skb, ETHTOOL_A_PHY_UPSTREAM_SFP_NAME,
+				   sfp_get_name(pdn->parent_sfp_bus)))
+			return -EMSGSIZE;
+
+		nla_nest_end(skb, nest);
+	}
+
+	if (phydev->sfp_bus &&
+	    nla_put_string(skb, ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME,
+			   sfp_get_name(phydev->sfp_bus)))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int ethnl_phy_parse_request(struct ethnl_req_info *req_base,
+				   struct nlattr **tb)
+{
+	struct phy_req_info *req_info = PHY_REQINFO(req_base);
+	struct link_topology *lt = &req_base->dev->link_topo;
+	struct phy_device_node *pdn;
+
+	if (!req_base->phydev)
+		return 0;
+
+	pdn = xa_load(&lt->phys, req_base->phydev->phyindex);
+	memcpy(&req_info->pdn, pdn, sizeof(*pdn));
+
+	return 0;
+}
+
+int ethnl_phy_doit(struct sk_buff *skb, struct genl_info *info)
+{
+	struct phy_req_info req_info = {};
+	struct nlattr **tb = info->attrs;
+	struct sk_buff *rskb;
+	void *reply_payload;
+	int reply_len;
+	int ret;
+
+	ret = ethnl_parse_header_dev_get(&req_info.base,
+					 tb[ETHTOOL_A_PHY_HEADER],
+					 genl_info_net(info), info->extack,
+					 true);
+	if (ret < 0)
+		return ret;
+
+	rtnl_lock();
+
+	ret = ethnl_phy_parse_request(&req_info.base, tb);
+	if (ret < 0)
+		goto err_unlock_rtnl;
+
+	/* No PHY, return early */
+	if (!req_info.pdn.phy)
+		goto err_unlock_rtnl;
+
+	ret = ethnl_phy_reply_size(&req_info.base, info->extack);
+	if (ret < 0)
+		goto err_unlock_rtnl;
+	reply_len = ret + ethnl_reply_header_size();
+
+	rskb = ethnl_reply_init(reply_len, req_info.base.dev,
+				ETHTOOL_MSG_PHY_GET_REPLY,
+				ETHTOOL_A_PHY_HEADER,
+				info, &reply_payload);
+	if (!rskb) {
+		ret = -ENOMEM;
+		goto err_unlock_rtnl;
+	}
+
+	ret = ethnl_phy_fill_reply(&req_info.base, rskb);
+	if (ret)
+		goto err_free_msg;
+
+	rtnl_unlock();
+	ethnl_parse_header_dev_put(&req_info.base);
+	genlmsg_end(rskb, reply_payload);
+
+	return genlmsg_reply(rskb, info);
+
+err_free_msg:
+	nlmsg_free(rskb);
+err_unlock_rtnl:
+	rtnl_unlock();
+	ethnl_parse_header_dev_put(&req_info.base);
+	return ret;
+}
+
+struct ethnl_phy_dump_ctx {
+	struct ethnl_req_info	req_info;
+};
+
+int ethnl_phy_start(struct netlink_callback *cb)
+{
+	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
+	struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
+	struct nlattr **tb = info->info.attrs;
+	int ret;
+
+	BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->ctx));
+
+	memset(ctx, 0, sizeof(*ctx));
+
+	ret = ethnl_parse_header_dev_get(&ctx->req_info,
+					 tb[ETHTOOL_A_PHY_HEADER],
+					 sock_net(cb->skb->sk), cb->extack,
+					 false);
+	return ret;
+}
+
+int ethnl_phy_dump_one_dev(struct sk_buff *skb, struct net_device *dev,
+			   struct netlink_callback *cb)
+{
+	struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
+	struct phy_req_info *pri = PHY_REQINFO(&ctx->req_info);
+	struct phy_device_node *pdn;
+	unsigned long index = 1;
+	void *ehdr;
+	int ret;
+
+	ctx->req_info.dev = dev;
+
+	xa_for_each(&dev->link_topo.phys, index, pdn) {
+		ehdr = ethnl_dump_put(skb, cb,
+				      ETHTOOL_MSG_PHY_GET_REPLY);
+		if (!ehdr) {
+			ret = -EMSGSIZE;
+			break;
+		}
+
+		ret = ethnl_fill_reply_header(skb, dev,
+					      ETHTOOL_A_PHY_HEADER);
+		if (ret < 0) {
+			genlmsg_cancel(skb, ehdr);
+			break;
+		}
+
+		memcpy(&pri->pdn, pdn, sizeof(*pdn));
+		ret = ethnl_phy_fill_reply(&ctx->req_info, skb);
+
+		genlmsg_end(skb, ehdr);
+	}
+
+	ctx->req_info.dev = NULL;
+
+	return ret;
+}
+
+int ethnl_phy_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
+{
+	struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
+	struct net *net = sock_net(skb->sk);
+	unsigned long ifindex = 1;
+	struct net_device *dev;
+	int ret = 0;
+
+	rtnl_lock();
+
+	if (ctx->req_info.dev) {
+		ret = ethnl_phy_dump_one_dev(skb, ctx->req_info.dev, cb);
+	} else {
+		for_each_netdev_dump(net, dev, ifindex) {
+			ret = ethnl_phy_dump_one_dev(skb, dev, cb);
+			if (ret)
+				break;
+		}
+	}
+	rtnl_unlock();
+
+	if (ret == -EMSGSIZE && skb->len)
+		return skb->len;
+	return ret;
+}
+
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [RFC PATCH net-next v2 06/10] net: ethtool: Introduce a command to list PHYs on an interface
@ 2023-11-17 16:23   ` Maxime Chevallier
  0 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-17 16:23 UTC (permalink / raw
  To: davem
  Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
	Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni,
	Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina,
	Florian Fainelli, Heiner Kallweit, Vladimir Oltean,
	Köry Maincent, Jesse Brandeburg

As we have the ability to track the PHYs connected to a net_device
through the link_topology, we can expose this list to userspace. This
allows userspace to use these identifiers for phy-specific commands and
take the decision of which PHY to target by knowing the link topology.

Add PHY_GET and PHY_DUMP, which can be a filtered DUMP operation to list
devices on only one interface.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 Documentation/netlink/specs/ethtool.yaml     |  69 ++++-
 Documentation/networking/ethtool-netlink.rst |  51 ++++
 include/linux/ethtool_netlink.h              |   5 +
 include/uapi/linux/ethtool_netlink.h         |  29 ++
 net/ethtool/Makefile                         |   2 +-
 net/ethtool/netlink.c                        |   8 +
 net/ethtool/netlink.h                        |   3 +
 net/ethtool/phy.c                            | 279 +++++++++++++++++++
 8 files changed, 444 insertions(+), 2 deletions(-)
 create mode 100644 net/ethtool/phy.c

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 5c7a65b009b4..ac9352d8ef57 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -16,6 +16,11 @@ definitions:
     name: stringset
     type: enum
     entries: []
+  -
+    name: phy-upstream-type
+    enum-name:
+    type: enum
+    entries: [ mac, sfp, phy ]
 
 attribute-sets:
   -
@@ -30,7 +35,9 @@ attribute-sets:
       -
         name: flags
         type: u32
-
+      -
+        name: phy-index
+        type: u32
   -
     name: bitset-bit
     attributes:
@@ -939,6 +946,45 @@ attribute-sets:
       -
         name: burst-tmr
         type: u32
+  -
+    name: phy-upstream
+    attributes:
+      -
+        name: index
+        type: u32
+      -
+        name: sfp-name
+        type: string
+  -
+    name: phy
+    attributes:
+      -
+        name: header
+        type: nest
+        nested-attributes: header
+      -
+        name: phy-index
+        type: u32
+      -
+        name: drvname
+        type: string
+      -
+        name: name
+        type: string
+      -
+        name: upstream-type
+        type: u8
+        enum: phy-upstream-type
+      -
+        name: upstream
+        type: nest
+        nested-attributes: phy-upstream
+      -
+        name: downstream-sfp-name
+        type: string
+      -
+        name: id
+        type: u32
 
 operations:
   enum-model: directional
@@ -1689,3 +1735,24 @@ operations:
       name: mm-ntf
       doc: Notification for change in MAC Merge configuration.
       notify: mm-get
+    -
+      name: phy-get
+      doc: Get PHY devices attached to an interface
+
+      attribute-set: phy
+
+      do: &phy-get-op
+        request:
+          attributes:
+            - header
+        reply:
+          attributes:
+            - header
+            - index
+            - drvname
+            - name
+            - upstream-type
+            - upstream
+            - downstream-sfp-name
+            - id
+      dump: *phy-get-op
diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 2540c70952ff..29ef675f45c0 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -57,6 +57,7 @@ Structure of this header is
   ``ETHTOOL_A_HEADER_DEV_INDEX``  u32     device ifindex
   ``ETHTOOL_A_HEADER_DEV_NAME``   string  device name
   ``ETHTOOL_A_HEADER_FLAGS``      u32     flags common for all requests
+  ``ETHTOOL_A_HEADER_PHY_INDEX``  u32     phy device index
   ==============================  ======  =============================
 
 ``ETHTOOL_A_HEADER_DEV_INDEX`` and ``ETHTOOL_A_HEADER_DEV_NAME`` identify the
@@ -81,6 +82,12 @@ the behaviour is backward compatible, i.e. requests from old clients not aware
 of the flag should be interpreted the way the client expects. A client must
 not set flags it does not understand.
 
+``ETHTOOL_A_HEADER_PHY_INDEX`` identify the ethernet PHY the message relates to.
+As there are numerous commands that are related to PHY configuration, and because
+we can have more than one PHY on the link, the PHY index can be passed in the
+request for the commands that needs it. It is however not mandatory, and if it
+is not passed for commands that target a PHY, the net_device.phydev pointer
+is used, as a fallback that keeps the legacy behaviour.
 
 Bit sets
 ========
@@ -1994,6 +2001,49 @@ The attributes are propagated to the driver through the following structure:
 .. kernel-doc:: include/linux/ethtool.h
     :identifiers: ethtool_mm_cfg
 
+PHY_GET
+======
+
+Retrieve information about a given Ethernet PHY sitting on the link. As there
+can be more than one PHY, the DUMP operation can be used to list the PHYs
+present on a given interface, by passing an interface index or name in
+the dump request
+
+Request contents:
+
+  ====================================  ======  ==========================
+  ``ETHTOOL_A_PHY_HEADER``              nested  request header
+  ====================================  ======  ==========================
+
+Kernel response contents:
+
+  =================================     ======  ==========================
+  ``ETHTOOL_A_PHY_HEADER``              nested  request header
+  ``ETHTOOL_A_PHY_INDEX``               u32     the phy's unique index, that can
+                                                be used for phy-specific requests
+  ``ETHTOOL_A_PHY_DRVNAME``             string  the phy driver name
+  ``ETHTOOL_A_PHY_NAME``                string  the phy device name
+  ``ETHTOOL_A_PHY_UPSTREAM_TYPE``       u32     the type of device this phy is
+                                                connected to
+  ``ETHTOOL_A_PHY_UPSTREAM_PHY``        nested  if the phy is connected to another
+                                                phy, this nest contains info on
+                                                that connection
+  ``ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME`` string  if the phy controls an sfp bus,
+                                                the name of the sfp bus
+  ``ETHTOOL_A_PHY_ID``                  u32     the phy id if the phy is C22
+  =================================     ======  ==========================
+
+When ``ETHTOOL_A_PHY_UPSTREAM_TYPE`` is PHY_UPSTREAM_PHY, the PHY's parent is
+another PHY. Information on the parent PHY will be set in the
+``ETHTOOL_A_PHY_UPSTREAM_PHY`` nest, which has the following structure :
+
+  =================================     ======  ==========================
+  ``ETHTOOL_A_PHY_UPSTREAM_INDEX``      u32     the PHY index of the upstream PHY
+  ``ETHTOOL_A_PHY_UPSTREAM_SFP_NAME``   string  if this PHY is connected to it's
+                                                parent PHY through an SFP bus, the
+                                                name of this sfp bus
+  =================================     ======  ==========================
+
 Request translation
 ===================
 
@@ -2100,4 +2150,5 @@ are netlink only.
   n/a                                 ``ETHTOOL_MSG_PLCA_GET_STATUS``
   n/a                                 ``ETHTOOL_MSG_MM_GET``
   n/a                                 ``ETHTOOL_MSG_MM_SET``
+  n/a                                 ``ETHTOOL_MSG_PHY_GET``
   =================================== =====================================
diff --git a/include/linux/ethtool_netlink.h b/include/linux/ethtool_netlink.h
index fae0dfb9a9c8..b4c40f1cc854 100644
--- a/include/linux/ethtool_netlink.h
+++ b/include/linux/ethtool_netlink.h
@@ -118,5 +118,10 @@ static inline bool ethtool_dev_mm_supported(struct net_device *dev)
 	return false;
 }
 
+int ethnl_phy_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
+{
+	return -EOPNOTSUPP;
+}
+
 #endif /* IS_ENABLED(CONFIG_ETHTOOL_NETLINK) */
 #endif /* _LINUX_ETHTOOL_NETLINK_H_ */
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index e557cf35250e..7d621963698a 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_GET,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_USER_CNT,
@@ -109,6 +110,8 @@ enum {
 	ETHTOOL_MSG_PLCA_NTF,
 	ETHTOOL_MSG_MM_GET_REPLY,
 	ETHTOOL_MSG_MM_NTF,
+	ETHTOOL_MSG_PHY_GET_REPLY,
+	ETHTOOL_MSG_PHY_NTF,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_KERNEL_CNT,
@@ -976,6 +979,32 @@ enum {
 	ETHTOOL_A_MM_MAX = (__ETHTOOL_A_MM_CNT - 1)
 };
 
+enum {
+	ETHTOOL_A_PHY_UPSTREAM_UNSPEC,
+	ETHTOOL_A_PHY_UPSTREAM_INDEX,			/* u32 */
+	ETHTOOL_A_PHY_UPSTREAM_SFP_NAME,		/* string */
+
+	/* add new constants above here */
+	__ETHTOOL_A_PHY_UPSTREAM_CNT,
+	ETHTOOL_A_PHY_UPSTREAM_MAX = (__ETHTOOL_A_PHY_UPSTREAM_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_NAME,			/* string */
+	ETHTOOL_A_PHY_UPSTREAM_TYPE,		/* u8 */
+	ETHTOOL_A_PHY_UPSTREAM,			/* nest - _A_PHY_UPSTREAM_* */
+	ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME,	/* string */
+	ETHTOOL_A_PHY_ID,			/* u32 */
+
+	/* add new constants above here */
+	__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 504f954a1b28..0ccd0e9afd3f 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.o
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index e83ee844b60f..80c8c312a584 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -1150,6 +1150,14 @@ 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_GET,
+		.doit	= ethnl_phy_doit,
+		.start	= ethnl_phy_start,
+		.dumpit	= ethnl_phy_dumpit,
+		.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 3f5eb60bdf0b..844c83c2ec7f 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -452,6 +452,9 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info);
 int ethnl_tunnel_info_doit(struct sk_buff *skb, struct genl_info *info);
 int ethnl_tunnel_info_start(struct netlink_callback *cb);
 int ethnl_tunnel_info_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
+int ethnl_phy_start(struct netlink_callback *cb);
+int ethnl_phy_doit(struct sk_buff *skb, struct genl_info *info);
+int ethnl_phy_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
 
 extern const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN];
 extern const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN];
diff --git a/net/ethtool/phy.c b/net/ethtool/phy.c
new file mode 100644
index 000000000000..ac6b4612301c
--- /dev/null
+++ b/net/ethtool/phy.c
@@ -0,0 +1,279 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2023 Bootlin
+ *
+ */
+#include "common.h"
+#include "netlink.h"
+
+#include <linux/link_topology.h>
+#include <linux/phy.h>
+#include <linux/sfp.h>
+
+struct phy_req_info {
+	struct ethnl_req_info		base;
+	struct phy_device_node		pdn;
+};
+
+#define PHY_REQINFO(__req_base) \
+	container_of(__req_base, struct phy_req_info, base)
+
+const struct nla_policy ethnl_phy_get_policy[ETHTOOL_A_PHY_HEADER + 1] = {
+	[ETHTOOL_A_PHY_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
+};
+
+/* Caller holds rtnl */
+static ssize_t
+ethnl_phy_reply_size(const struct ethnl_req_info *req_base,
+		     struct netlink_ext_ack *extack)
+{
+	struct phy_device_node *pdn;
+	struct phy_device *phydev;
+	struct link_topology *lt;
+	unsigned long index;
+	size_t size;
+
+	lt = &req_base->dev->link_topo;
+
+	size = nla_total_size(0);
+
+	xa_for_each(&lt->phys, index, pdn) {
+		phydev = pdn->phy;
+
+		/* ETHTOOL_A_PHY_INDEX */
+		size += nla_total_size(sizeof(u32));
+
+		/* ETHTOOL_A_DRVNAME */
+		size += nla_total_size(strlen(phydev->drv->name));
+
+		/* ETHTOOL_A_NAME */
+		size += nla_total_size(strlen(dev_name(&phydev->mdio.dev)));
+
+		/* ETHTOOL_A_PHY_UPSTREAM_TYPE */
+		size += nla_total_size(sizeof(u8));
+
+		/* ETHTOOL_A_PHY_ID */
+		size += nla_total_size(sizeof(u32));
+
+		if (phy_on_sfp(phydev)) {
+			/* ETHTOOL_A_PHY_UPSTREAM_SFP_NAME */
+			if (sfp_get_name(pdn->parent_sfp_bus))
+				size += nla_total_size(strlen(sfp_get_name(pdn->parent_sfp_bus)));
+
+			/* ETHTOOL_A_PHY_UPSTREAM_INDEX */
+			size += nla_total_size(sizeof(u32));
+		}
+
+		/* ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME */
+		if (phydev->sfp_bus)
+			size += nla_total_size(strlen(sfp_get_name(phydev->sfp_bus)));
+	}
+
+	return size;
+}
+
+static int
+ethnl_phy_fill_reply(const struct ethnl_req_info *req_base, struct sk_buff *skb)
+{
+	struct phy_req_info *req_info = PHY_REQINFO(req_base);
+	struct phy_device_node *pdn = &req_info->pdn;
+	struct phy_device *phydev = pdn->phy;
+	enum phy_upstream ptype;
+	struct nlattr *nest;
+
+	ptype = pdn->upstream_type;
+
+	if (nla_put_u32(skb, ETHTOOL_A_PHY_INDEX, phydev->phyindex) ||
+	    nla_put_string(skb, ETHTOOL_A_PHY_DRVNAME, phydev->drv->name) ||
+	    nla_put_string(skb, ETHTOOL_A_PHY_NAME, dev_name(&phydev->mdio.dev)) ||
+	    nla_put_u8(skb, ETHTOOL_A_PHY_UPSTREAM_TYPE, ptype) ||
+	    nla_put_u32(skb, ETHTOOL_A_PHY_ID, phydev->phy_id))
+		return -EMSGSIZE;
+
+	if (ptype == PHY_UPSTREAM_PHY) {
+		struct phy_device *upstream = pdn->upstream.phydev;
+
+		nest = nla_nest_start(skb, ETHTOOL_A_PHY_UPSTREAM);
+		if (!nest)
+			return -EMSGSIZE;
+
+		/* Parent index */
+		if (nla_put_u32(skb, ETHTOOL_A_PHY_UPSTREAM_INDEX, upstream->phyindex))
+			return -EMSGSIZE;
+
+		if (pdn->parent_sfp_bus && sfp_get_name(pdn->parent_sfp_bus) &&
+		    nla_put_string(skb, ETHTOOL_A_PHY_UPSTREAM_SFP_NAME,
+				   sfp_get_name(pdn->parent_sfp_bus)))
+			return -EMSGSIZE;
+
+		nla_nest_end(skb, nest);
+	}
+
+	if (phydev->sfp_bus &&
+	    nla_put_string(skb, ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME,
+			   sfp_get_name(phydev->sfp_bus)))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int ethnl_phy_parse_request(struct ethnl_req_info *req_base,
+				   struct nlattr **tb)
+{
+	struct phy_req_info *req_info = PHY_REQINFO(req_base);
+	struct link_topology *lt = &req_base->dev->link_topo;
+	struct phy_device_node *pdn;
+
+	if (!req_base->phydev)
+		return 0;
+
+	pdn = xa_load(&lt->phys, req_base->phydev->phyindex);
+	memcpy(&req_info->pdn, pdn, sizeof(*pdn));
+
+	return 0;
+}
+
+int ethnl_phy_doit(struct sk_buff *skb, struct genl_info *info)
+{
+	struct phy_req_info req_info = {};
+	struct nlattr **tb = info->attrs;
+	struct sk_buff *rskb;
+	void *reply_payload;
+	int reply_len;
+	int ret;
+
+	ret = ethnl_parse_header_dev_get(&req_info.base,
+					 tb[ETHTOOL_A_PHY_HEADER],
+					 genl_info_net(info), info->extack,
+					 true);
+	if (ret < 0)
+		return ret;
+
+	rtnl_lock();
+
+	ret = ethnl_phy_parse_request(&req_info.base, tb);
+	if (ret < 0)
+		goto err_unlock_rtnl;
+
+	/* No PHY, return early */
+	if (!req_info.pdn.phy)
+		goto err_unlock_rtnl;
+
+	ret = ethnl_phy_reply_size(&req_info.base, info->extack);
+	if (ret < 0)
+		goto err_unlock_rtnl;
+	reply_len = ret + ethnl_reply_header_size();
+
+	rskb = ethnl_reply_init(reply_len, req_info.base.dev,
+				ETHTOOL_MSG_PHY_GET_REPLY,
+				ETHTOOL_A_PHY_HEADER,
+				info, &reply_payload);
+	if (!rskb) {
+		ret = -ENOMEM;
+		goto err_unlock_rtnl;
+	}
+
+	ret = ethnl_phy_fill_reply(&req_info.base, rskb);
+	if (ret)
+		goto err_free_msg;
+
+	rtnl_unlock();
+	ethnl_parse_header_dev_put(&req_info.base);
+	genlmsg_end(rskb, reply_payload);
+
+	return genlmsg_reply(rskb, info);
+
+err_free_msg:
+	nlmsg_free(rskb);
+err_unlock_rtnl:
+	rtnl_unlock();
+	ethnl_parse_header_dev_put(&req_info.base);
+	return ret;
+}
+
+struct ethnl_phy_dump_ctx {
+	struct ethnl_req_info	req_info;
+};
+
+int ethnl_phy_start(struct netlink_callback *cb)
+{
+	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
+	struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
+	struct nlattr **tb = info->info.attrs;
+	int ret;
+
+	BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->ctx));
+
+	memset(ctx, 0, sizeof(*ctx));
+
+	ret = ethnl_parse_header_dev_get(&ctx->req_info,
+					 tb[ETHTOOL_A_PHY_HEADER],
+					 sock_net(cb->skb->sk), cb->extack,
+					 false);
+	return ret;
+}
+
+int ethnl_phy_dump_one_dev(struct sk_buff *skb, struct net_device *dev,
+			   struct netlink_callback *cb)
+{
+	struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
+	struct phy_req_info *pri = PHY_REQINFO(&ctx->req_info);
+	struct phy_device_node *pdn;
+	unsigned long index = 1;
+	void *ehdr;
+	int ret;
+
+	ctx->req_info.dev = dev;
+
+	xa_for_each(&dev->link_topo.phys, index, pdn) {
+		ehdr = ethnl_dump_put(skb, cb,
+				      ETHTOOL_MSG_PHY_GET_REPLY);
+		if (!ehdr) {
+			ret = -EMSGSIZE;
+			break;
+		}
+
+		ret = ethnl_fill_reply_header(skb, dev,
+					      ETHTOOL_A_PHY_HEADER);
+		if (ret < 0) {
+			genlmsg_cancel(skb, ehdr);
+			break;
+		}
+
+		memcpy(&pri->pdn, pdn, sizeof(*pdn));
+		ret = ethnl_phy_fill_reply(&ctx->req_info, skb);
+
+		genlmsg_end(skb, ehdr);
+	}
+
+	ctx->req_info.dev = NULL;
+
+	return ret;
+}
+
+int ethnl_phy_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
+{
+	struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
+	struct net *net = sock_net(skb->sk);
+	unsigned long ifindex = 1;
+	struct net_device *dev;
+	int ret = 0;
+
+	rtnl_lock();
+
+	if (ctx->req_info.dev) {
+		ret = ethnl_phy_dump_one_dev(skb, ctx->req_info.dev, cb);
+	} else {
+		for_each_netdev_dump(net, dev, ifindex) {
+			ret = ethnl_phy_dump_one_dev(skb, dev, cb);
+			if (ret)
+				break;
+		}
+	}
+	rtnl_unlock();
+
+	if (ret == -EMSGSIZE && skb->len)
+		return skb->len;
+	return ret;
+}
+
-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [RFC PATCH net-next v2 07/10] net: ethtool: plca: Target the command to the requested PHY
  2023-11-17 16:23 ` Maxime Chevallier
@ 2023-11-17 16:23   ` Maxime Chevallier
  -1 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-17 16:23 UTC (permalink / raw
  To: davem
  Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
	Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni,
	Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina,
	Florian Fainelli, Heiner Kallweit, Vladimir Oltean,
	Köry Maincent, Jesse Brandeburg

PLCA is a PHY-specific command. Instead of targeting the command
towards dev->phydev, use the request to pick the targeted PHY.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 net/ethtool/plca.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/net/ethtool/plca.c b/net/ethtool/plca.c
index b1e2e3b5027f..2b3e419f4dc2 100644
--- a/net/ethtool/plca.c
+++ b/net/ethtool/plca.c
@@ -61,7 +61,7 @@ static int plca_get_cfg_prepare_data(const struct ethnl_req_info *req_base,
 	int ret;
 
 	// check that the PHY device is available and connected
-	if (!dev->phydev) {
+	if (!req_base->phydev) {
 		ret = -EOPNOTSUPP;
 		goto out;
 	}
@@ -80,7 +80,7 @@ static int plca_get_cfg_prepare_data(const struct ethnl_req_info *req_base,
 	memset(&data->plca_cfg, 0xff,
 	       sizeof_field(struct plca_reply_data, plca_cfg));
 
-	ret = ops->get_plca_cfg(dev->phydev, &data->plca_cfg);
+	ret = ops->get_plca_cfg(req_base->phydev, &data->plca_cfg);
 	ethnl_ops_complete(dev);
 
 out:
@@ -141,7 +141,6 @@ const struct nla_policy ethnl_plca_set_cfg_policy[] = {
 static int
 ethnl_set_plca(struct ethnl_req_info *req_info, struct genl_info *info)
 {
-	struct net_device *dev = req_info->dev;
 	const struct ethtool_phy_ops *ops;
 	struct nlattr **tb = info->attrs;
 	struct phy_plca_cfg plca_cfg;
@@ -149,7 +148,7 @@ ethnl_set_plca(struct ethnl_req_info *req_info, struct genl_info *info)
 	int ret;
 
 	// check that the PHY device is available and connected
-	if (!dev->phydev)
+	if (!req_info->phydev)
 		return -EOPNOTSUPP;
 
 	ops = ethtool_phy_ops;
@@ -168,7 +167,7 @@ ethnl_set_plca(struct ethnl_req_info *req_info, struct genl_info *info)
 	if (!mod)
 		return 0;
 
-	ret = ops->set_plca_cfg(dev->phydev, &plca_cfg, info->extack);
+	ret = ops->set_plca_cfg(req_info->phydev, &plca_cfg, info->extack);
 	return ret < 0 ? ret : 1;
 }
 
@@ -204,7 +203,7 @@ static int plca_get_status_prepare_data(const struct ethnl_req_info *req_base,
 	int ret;
 
 	// check that the PHY device is available and connected
-	if (!dev->phydev) {
+	if (!req_base->phydev) {
 		ret = -EOPNOTSUPP;
 		goto out;
 	}
@@ -223,7 +222,7 @@ static int plca_get_status_prepare_data(const struct ethnl_req_info *req_base,
 	memset(&data->plca_st, 0xff,
 	       sizeof_field(struct plca_reply_data, plca_st));
 
-	ret = ops->get_plca_status(dev->phydev, &data->plca_st);
+	ret = ops->get_plca_status(req_base->phydev, &data->plca_st);
 	ethnl_ops_complete(dev);
 out:
 	return ret;
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [RFC PATCH net-next v2 07/10] net: ethtool: plca: Target the command to the requested PHY
@ 2023-11-17 16:23   ` Maxime Chevallier
  0 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-17 16:23 UTC (permalink / raw
  To: davem
  Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
	Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni,
	Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina,
	Florian Fainelli, Heiner Kallweit, Vladimir Oltean,
	Köry Maincent, Jesse Brandeburg

PLCA is a PHY-specific command. Instead of targeting the command
towards dev->phydev, use the request to pick the targeted PHY.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 net/ethtool/plca.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/net/ethtool/plca.c b/net/ethtool/plca.c
index b1e2e3b5027f..2b3e419f4dc2 100644
--- a/net/ethtool/plca.c
+++ b/net/ethtool/plca.c
@@ -61,7 +61,7 @@ static int plca_get_cfg_prepare_data(const struct ethnl_req_info *req_base,
 	int ret;
 
 	// check that the PHY device is available and connected
-	if (!dev->phydev) {
+	if (!req_base->phydev) {
 		ret = -EOPNOTSUPP;
 		goto out;
 	}
@@ -80,7 +80,7 @@ static int plca_get_cfg_prepare_data(const struct ethnl_req_info *req_base,
 	memset(&data->plca_cfg, 0xff,
 	       sizeof_field(struct plca_reply_data, plca_cfg));
 
-	ret = ops->get_plca_cfg(dev->phydev, &data->plca_cfg);
+	ret = ops->get_plca_cfg(req_base->phydev, &data->plca_cfg);
 	ethnl_ops_complete(dev);
 
 out:
@@ -141,7 +141,6 @@ const struct nla_policy ethnl_plca_set_cfg_policy[] = {
 static int
 ethnl_set_plca(struct ethnl_req_info *req_info, struct genl_info *info)
 {
-	struct net_device *dev = req_info->dev;
 	const struct ethtool_phy_ops *ops;
 	struct nlattr **tb = info->attrs;
 	struct phy_plca_cfg plca_cfg;
@@ -149,7 +148,7 @@ ethnl_set_plca(struct ethnl_req_info *req_info, struct genl_info *info)
 	int ret;
 
 	// check that the PHY device is available and connected
-	if (!dev->phydev)
+	if (!req_info->phydev)
 		return -EOPNOTSUPP;
 
 	ops = ethtool_phy_ops;
@@ -168,7 +167,7 @@ ethnl_set_plca(struct ethnl_req_info *req_info, struct genl_info *info)
 	if (!mod)
 		return 0;
 
-	ret = ops->set_plca_cfg(dev->phydev, &plca_cfg, info->extack);
+	ret = ops->set_plca_cfg(req_info->phydev, &plca_cfg, info->extack);
 	return ret < 0 ? ret : 1;
 }
 
@@ -204,7 +203,7 @@ static int plca_get_status_prepare_data(const struct ethnl_req_info *req_base,
 	int ret;
 
 	// check that the PHY device is available and connected
-	if (!dev->phydev) {
+	if (!req_base->phydev) {
 		ret = -EOPNOTSUPP;
 		goto out;
 	}
@@ -223,7 +222,7 @@ static int plca_get_status_prepare_data(const struct ethnl_req_info *req_base,
 	memset(&data->plca_st, 0xff,
 	       sizeof_field(struct plca_reply_data, plca_st));
 
-	ret = ops->get_plca_status(dev->phydev, &data->plca_st);
+	ret = ops->get_plca_status(req_base->phydev, &data->plca_st);
 	ethnl_ops_complete(dev);
 out:
 	return ret;
-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [RFC PATCH net-next v2 08/10] net: ethtool: pse-pd: Target the command to the requested PHY
  2023-11-17 16:23 ` Maxime Chevallier
@ 2023-11-17 16:23   ` Maxime Chevallier
  -1 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-17 16:23 UTC (permalink / raw
  To: davem
  Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
	Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni,
	Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina,
	Florian Fainelli, Heiner Kallweit, Vladimir Oltean,
	Köry Maincent, Jesse Brandeburg

PSE and PD configuration is a PHY-specific command. Instead of targeting
the command towards dev->phydev, use the request to pick the targeted
PHY device.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 net/ethtool/pse-pd.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/net/ethtool/pse-pd.c b/net/ethtool/pse-pd.c
index cc478af77111..0d9cd9c87104 100644
--- a/net/ethtool/pse-pd.c
+++ b/net/ethtool/pse-pd.c
@@ -31,17 +31,10 @@ const struct nla_policy ethnl_pse_get_policy[ETHTOOL_A_PSE_HEADER + 1] = {
 	[ETHTOOL_A_PSE_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
 };
 
-static int pse_get_pse_attributes(struct net_device *dev,
+static int pse_get_pse_attributes(struct phy_device *phydev,
 				  struct netlink_ext_ack *extack,
 				  struct pse_reply_data *data)
 {
-	struct phy_device *phydev = dev->phydev;
-
-	if (!phydev) {
-		NL_SET_ERR_MSG(extack, "No PHY is attached");
-		return -EOPNOTSUPP;
-	}
-
 	if (!phydev->psec) {
 		NL_SET_ERR_MSG(extack, "No PSE is attached");
 		return -EOPNOTSUPP;
@@ -64,7 +57,7 @@ static int pse_prepare_data(const struct ethnl_req_info *req_base,
 	if (ret < 0)
 		return ret;
 
-	ret = pse_get_pse_attributes(dev, info->extack, data);
+	ret = pse_get_pse_attributes(req_base->phydev, info->extack, data);
 
 	ethnl_ops_complete(dev);
 
@@ -124,7 +117,6 @@ ethnl_set_pse_validate(struct ethnl_req_info *req_info, struct genl_info *info)
 static int
 ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 {
-	struct net_device *dev = req_info->dev;
 	struct pse_control_config config = {};
 	struct nlattr **tb = info->attrs;
 	struct phy_device *phydev;
@@ -132,7 +124,7 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 	/* this values are already validated by the ethnl_pse_set_policy */
 	config.admin_cotrol = nla_get_u32(tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL]);
 
-	phydev = dev->phydev;
+	phydev = req_info->phydev;
 	if (!phydev) {
 		NL_SET_ERR_MSG(info->extack, "No PHY is attached");
 		return -EOPNOTSUPP;
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [RFC PATCH net-next v2 08/10] net: ethtool: pse-pd: Target the command to the requested PHY
@ 2023-11-17 16:23   ` Maxime Chevallier
  0 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-17 16:23 UTC (permalink / raw
  To: davem
  Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
	Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni,
	Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina,
	Florian Fainelli, Heiner Kallweit, Vladimir Oltean,
	Köry Maincent, Jesse Brandeburg

PSE and PD configuration is a PHY-specific command. Instead of targeting
the command towards dev->phydev, use the request to pick the targeted
PHY device.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 net/ethtool/pse-pd.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/net/ethtool/pse-pd.c b/net/ethtool/pse-pd.c
index cc478af77111..0d9cd9c87104 100644
--- a/net/ethtool/pse-pd.c
+++ b/net/ethtool/pse-pd.c
@@ -31,17 +31,10 @@ const struct nla_policy ethnl_pse_get_policy[ETHTOOL_A_PSE_HEADER + 1] = {
 	[ETHTOOL_A_PSE_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
 };
 
-static int pse_get_pse_attributes(struct net_device *dev,
+static int pse_get_pse_attributes(struct phy_device *phydev,
 				  struct netlink_ext_ack *extack,
 				  struct pse_reply_data *data)
 {
-	struct phy_device *phydev = dev->phydev;
-
-	if (!phydev) {
-		NL_SET_ERR_MSG(extack, "No PHY is attached");
-		return -EOPNOTSUPP;
-	}
-
 	if (!phydev->psec) {
 		NL_SET_ERR_MSG(extack, "No PSE is attached");
 		return -EOPNOTSUPP;
@@ -64,7 +57,7 @@ static int pse_prepare_data(const struct ethnl_req_info *req_base,
 	if (ret < 0)
 		return ret;
 
-	ret = pse_get_pse_attributes(dev, info->extack, data);
+	ret = pse_get_pse_attributes(req_base->phydev, info->extack, data);
 
 	ethnl_ops_complete(dev);
 
@@ -124,7 +117,6 @@ ethnl_set_pse_validate(struct ethnl_req_info *req_info, struct genl_info *info)
 static int
 ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 {
-	struct net_device *dev = req_info->dev;
 	struct pse_control_config config = {};
 	struct nlattr **tb = info->attrs;
 	struct phy_device *phydev;
@@ -132,7 +124,7 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 	/* this values are already validated by the ethnl_pse_set_policy */
 	config.admin_cotrol = nla_get_u32(tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL]);
 
-	phydev = dev->phydev;
+	phydev = req_info->phydev;
 	if (!phydev) {
 		NL_SET_ERR_MSG(info->extack, "No PHY is attached");
 		return -EOPNOTSUPP;
-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [RFC PATCH net-next v2 09/10] net: ethtool: cable-test: Target the command to the requested PHY
  2023-11-17 16:23 ` Maxime Chevallier
@ 2023-11-17 16:23   ` Maxime Chevallier
  -1 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-17 16:23 UTC (permalink / raw
  To: davem
  Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
	Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni,
	Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina,
	Florian Fainelli, Heiner Kallweit, Vladimir Oltean,
	Köry Maincent, Jesse Brandeburg

Cable testing is a PHY-specific command. Instead of targeting the command
towards dev->phydev, use the request to pick the targeted PHY.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 net/ethtool/cabletest.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c
index 06a151165c31..6b00d0800f23 100644
--- a/net/ethtool/cabletest.c
+++ b/net/ethtool/cabletest.c
@@ -69,7 +69,7 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
 		return ret;
 
 	dev = req_info.dev;
-	if (!dev->phydev) {
+	if (!req_info.phydev) {
 		ret = -EOPNOTSUPP;
 		goto out_dev_put;
 	}
@@ -85,12 +85,12 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
 	if (ret < 0)
 		goto out_rtnl;
 
-	ret = ops->start_cable_test(dev->phydev, info->extack);
+	ret = ops->start_cable_test(req_info.phydev, info->extack);
 
 	ethnl_ops_complete(dev);
 
 	if (!ret)
-		ethnl_cable_test_started(dev->phydev,
+		ethnl_cable_test_started(req_info.phydev,
 					 ETHTOOL_MSG_CABLE_TEST_NTF);
 
 out_rtnl:
@@ -321,7 +321,7 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
 		return ret;
 
 	dev = req_info.dev;
-	if (!dev->phydev) {
+	if (!req_info.phydev) {
 		ret = -EOPNOTSUPP;
 		goto out_dev_put;
 	}
@@ -342,12 +342,12 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
 	if (ret < 0)
 		goto out_rtnl;
 
-	ret = ops->start_cable_test_tdr(dev->phydev, info->extack, &cfg);
+	ret = ops->start_cable_test_tdr(req_info.phydev, info->extack, &cfg);
 
 	ethnl_ops_complete(dev);
 
 	if (!ret)
-		ethnl_cable_test_started(dev->phydev,
+		ethnl_cable_test_started(req_info.phydev,
 					 ETHTOOL_MSG_CABLE_TEST_TDR_NTF);
 
 out_rtnl:
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [RFC PATCH net-next v2 09/10] net: ethtool: cable-test: Target the command to the requested PHY
@ 2023-11-17 16:23   ` Maxime Chevallier
  0 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-17 16:23 UTC (permalink / raw
  To: davem
  Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
	Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni,
	Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina,
	Florian Fainelli, Heiner Kallweit, Vladimir Oltean,
	Köry Maincent, Jesse Brandeburg

Cable testing is a PHY-specific command. Instead of targeting the command
towards dev->phydev, use the request to pick the targeted PHY.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 net/ethtool/cabletest.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c
index 06a151165c31..6b00d0800f23 100644
--- a/net/ethtool/cabletest.c
+++ b/net/ethtool/cabletest.c
@@ -69,7 +69,7 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
 		return ret;
 
 	dev = req_info.dev;
-	if (!dev->phydev) {
+	if (!req_info.phydev) {
 		ret = -EOPNOTSUPP;
 		goto out_dev_put;
 	}
@@ -85,12 +85,12 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
 	if (ret < 0)
 		goto out_rtnl;
 
-	ret = ops->start_cable_test(dev->phydev, info->extack);
+	ret = ops->start_cable_test(req_info.phydev, info->extack);
 
 	ethnl_ops_complete(dev);
 
 	if (!ret)
-		ethnl_cable_test_started(dev->phydev,
+		ethnl_cable_test_started(req_info.phydev,
 					 ETHTOOL_MSG_CABLE_TEST_NTF);
 
 out_rtnl:
@@ -321,7 +321,7 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
 		return ret;
 
 	dev = req_info.dev;
-	if (!dev->phydev) {
+	if (!req_info.phydev) {
 		ret = -EOPNOTSUPP;
 		goto out_dev_put;
 	}
@@ -342,12 +342,12 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
 	if (ret < 0)
 		goto out_rtnl;
 
-	ret = ops->start_cable_test_tdr(dev->phydev, info->extack, &cfg);
+	ret = ops->start_cable_test_tdr(req_info.phydev, info->extack, &cfg);
 
 	ethnl_ops_complete(dev);
 
 	if (!ret)
-		ethnl_cable_test_started(dev->phydev,
+		ethnl_cable_test_started(req_info.phydev,
 					 ETHTOOL_MSG_CABLE_TEST_TDR_NTF);
 
 out_rtnl:
-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [RFC PATCH net-next v2 10/10] net: ethtool: strset: Allow querying phy stats by index
  2023-11-17 16:23 ` Maxime Chevallier
@ 2023-11-17 16:23   ` Maxime Chevallier
  -1 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-17 16:23 UTC (permalink / raw
  To: davem
  Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
	Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni,
	Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina,
	Florian Fainelli, Heiner Kallweit, Vladimir Oltean,
	Köry Maincent, Jesse Brandeburg

The ETH_SS_PHY_STATS command gets PHY statistics. Use the phydev pointer
from the ethnl request to allow query phy stats from each PHY on the
link.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 net/ethtool/strset.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
index c678b484a079..70c00631c51f 100644
--- a/net/ethtool/strset.c
+++ b/net/ethtool/strset.c
@@ -233,17 +233,18 @@ static void strset_cleanup_data(struct ethnl_reply_data *reply_base)
 }
 
 static int strset_prepare_set(struct strset_info *info, struct net_device *dev,
-			      unsigned int id, bool counts_only)
+			      struct phy_device *phydev, unsigned int id,
+			      bool counts_only)
 {
 	const struct ethtool_phy_ops *phy_ops = ethtool_phy_ops;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
 	void *strings;
 	int count, ret;
 
-	if (id == ETH_SS_PHY_STATS && dev->phydev &&
+	if (id == ETH_SS_PHY_STATS && phydev &&
 	    !ops->get_ethtool_phy_stats && phy_ops &&
 	    phy_ops->get_sset_count)
-		ret = phy_ops->get_sset_count(dev->phydev);
+		ret = phy_ops->get_sset_count(phydev);
 	else if (ops->get_sset_count && ops->get_strings)
 		ret = ops->get_sset_count(dev, id);
 	else
@@ -258,10 +259,10 @@ static int strset_prepare_set(struct strset_info *info, struct net_device *dev,
 		strings = kcalloc(count, ETH_GSTRING_LEN, GFP_KERNEL);
 		if (!strings)
 			return -ENOMEM;
-		if (id == ETH_SS_PHY_STATS && dev->phydev &&
+		if (id == ETH_SS_PHY_STATS && phydev &&
 		    !ops->get_ethtool_phy_stats && phy_ops &&
 		    phy_ops->get_strings)
-			phy_ops->get_strings(dev->phydev, strings);
+			phy_ops->get_strings(phydev, strings);
 		else
 			ops->get_strings(dev, id, strings);
 		info->strings = strings;
@@ -305,8 +306,8 @@ static int strset_prepare_data(const struct ethnl_req_info *req_base,
 		    !data->sets[i].per_dev)
 			continue;
 
-		ret = strset_prepare_set(&data->sets[i], dev, i,
-					 req_info->counts_only);
+		ret = strset_prepare_set(&data->sets[i], dev, req_base->phydev,
+					 i, req_info->counts_only);
 		if (ret < 0)
 			goto err_ops;
 	}
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [RFC PATCH net-next v2 10/10] net: ethtool: strset: Allow querying phy stats by index
@ 2023-11-17 16:23   ` Maxime Chevallier
  0 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-17 16:23 UTC (permalink / raw
  To: davem
  Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
	Andrew Lunn, Jakub Kicinski, Eric Dumazet, Paolo Abeni,
	Russell King, linux-arm-kernel, Christophe Leroy, Herve Codina,
	Florian Fainelli, Heiner Kallweit, Vladimir Oltean,
	Köry Maincent, Jesse Brandeburg

The ETH_SS_PHY_STATS command gets PHY statistics. Use the phydev pointer
from the ethnl request to allow query phy stats from each PHY on the
link.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 net/ethtool/strset.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
index c678b484a079..70c00631c51f 100644
--- a/net/ethtool/strset.c
+++ b/net/ethtool/strset.c
@@ -233,17 +233,18 @@ static void strset_cleanup_data(struct ethnl_reply_data *reply_base)
 }
 
 static int strset_prepare_set(struct strset_info *info, struct net_device *dev,
-			      unsigned int id, bool counts_only)
+			      struct phy_device *phydev, unsigned int id,
+			      bool counts_only)
 {
 	const struct ethtool_phy_ops *phy_ops = ethtool_phy_ops;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
 	void *strings;
 	int count, ret;
 
-	if (id == ETH_SS_PHY_STATS && dev->phydev &&
+	if (id == ETH_SS_PHY_STATS && phydev &&
 	    !ops->get_ethtool_phy_stats && phy_ops &&
 	    phy_ops->get_sset_count)
-		ret = phy_ops->get_sset_count(dev->phydev);
+		ret = phy_ops->get_sset_count(phydev);
 	else if (ops->get_sset_count && ops->get_strings)
 		ret = ops->get_sset_count(dev, id);
 	else
@@ -258,10 +259,10 @@ static int strset_prepare_set(struct strset_info *info, struct net_device *dev,
 		strings = kcalloc(count, ETH_GSTRING_LEN, GFP_KERNEL);
 		if (!strings)
 			return -ENOMEM;
-		if (id == ETH_SS_PHY_STATS && dev->phydev &&
+		if (id == ETH_SS_PHY_STATS && phydev &&
 		    !ops->get_ethtool_phy_stats && phy_ops &&
 		    phy_ops->get_strings)
-			phy_ops->get_strings(dev->phydev, strings);
+			phy_ops->get_strings(phydev, strings);
 		else
 			ops->get_strings(dev, id, strings);
 		info->strings = strings;
@@ -305,8 +306,8 @@ static int strset_prepare_data(const struct ethnl_req_info *req_base,
 		    !data->sets[i].per_dev)
 			continue;
 
-		ret = strset_prepare_set(&data->sets[i], dev, i,
-					 req_info->counts_only);
+		ret = strset_prepare_set(&data->sets[i], dev, req_base->phydev,
+					 i, req_info->counts_only);
 		if (ret < 0)
 			goto err_ops;
 	}
-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 06/10] net: ethtool: Introduce a command to list PHYs on an interface
  2023-11-17 16:23   ` Maxime Chevallier
  (?)
@ 2023-11-18  4:44   ` kernel test robot
  -1 siblings, 0 replies; 53+ messages in thread
From: kernel test robot @ 2023-11-18  4:44 UTC (permalink / raw
  To: Maxime Chevallier; +Cc: llvm, oe-kbuild-all

Hi Maxime,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Maxime-Chevallier/net-phy-Introduce-ethernet-link-topology-representation/20231117-234210
base:   net-next/main
patch link:    https://lore.kernel.org/r/20231117162323.626979-7-maxime.chevallier%40bootlin.com
patch subject: [RFC PATCH net-next v2 06/10] net: ethtool: Introduce a command to list PHYs on an interface
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20231118/202311181249.7bUVFYez-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231118/202311181249.7bUVFYez-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311181249.7bUVFYez-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> net/ethtool/phy.c:216:5: warning: no previous prototype for function 'ethnl_phy_dump_one_dev' [-Wmissing-prototypes]
   int ethnl_phy_dump_one_dev(struct sk_buff *skb, struct net_device *dev,
       ^
   net/ethtool/phy.c:216:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int ethnl_phy_dump_one_dev(struct sk_buff *skb, struct net_device *dev,
   ^
   static 
   1 warning generated.


vim +/ethnl_phy_dump_one_dev +216 net/ethtool/phy.c

   215	
 > 216	int ethnl_phy_dump_one_dev(struct sk_buff *skb, struct net_device *dev,
   217				   struct netlink_callback *cb)
   218	{
   219		struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
   220		struct phy_req_info *pri = PHY_REQINFO(&ctx->req_info);
   221		struct phy_device_node *pdn;
   222		unsigned long index = 1;
   223		void *ehdr;
   224		int ret;
   225	
   226		ctx->req_info.dev = dev;
   227	
   228		xa_for_each(&dev->link_topo.phys, index, pdn) {
   229			ehdr = ethnl_dump_put(skb, cb,
   230					      ETHTOOL_MSG_PHY_GET_REPLY);
   231			if (!ehdr) {
   232				ret = -EMSGSIZE;
   233				break;
   234			}
   235	
   236			ret = ethnl_fill_reply_header(skb, dev,
   237						      ETHTOOL_A_PHY_HEADER);
   238			if (ret < 0) {
   239				genlmsg_cancel(skb, ehdr);
   240				break;
   241			}
   242	
   243			memcpy(&pri->pdn, pdn, sizeof(*pdn));
   244			ret = ethnl_phy_fill_reply(&ctx->req_info, skb);
   245	
   246			genlmsg_end(skb, ehdr);
   247		}
   248	
   249		ctx->req_info.dev = NULL;
   250	
   251		return ret;
   252	}
   253	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 04/10] net: sfp: Add helper to return the SFP bus name
  2023-11-17 16:23   ` Maxime Chevallier
  (?)
@ 2023-11-18  4:44   ` kernel test robot
  -1 siblings, 0 replies; 53+ messages in thread
From: kernel test robot @ 2023-11-18  4:44 UTC (permalink / raw
  To: Maxime Chevallier; +Cc: oe-kbuild-all

Hi Maxime,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Maxime-Chevallier/net-phy-Introduce-ethernet-link-topology-representation/20231117-234210
base:   net-next/main
patch link:    https://lore.kernel.org/r/20231117162323.626979-5-maxime.chevallier%40bootlin.com
patch subject: [RFC PATCH net-next v2 04/10] net: sfp: Add helper to return the SFP bus name
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20231118/202311181214.uTtjF2ku-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231118/202311181214.uTtjF2ku-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311181214.uTtjF2ku-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from net/ethtool/ioctl.c:23:
>> include/linux/sfp.h:653:20: warning: 'sfp_get_name' defined but not used [-Wunused-function]
     653 | static const char *sfp_get_name(struct sfp_bus *bus)
         |                    ^~~~~~~~~~~~


vim +/sfp_get_name +653 include/linux/sfp.h

   652	
 > 653	static const char *sfp_get_name(struct sfp_bus *bus)
   654	{
   655		return NULL;
   656	}
   657	#endif
   658	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 06/10] net: ethtool: Introduce a command to list PHYs on an interface
  2023-11-17 16:23   ` Maxime Chevallier
  (?)
  (?)
@ 2023-11-18  5:34   ` kernel test robot
  -1 siblings, 0 replies; 53+ messages in thread
From: kernel test robot @ 2023-11-18  5:34 UTC (permalink / raw
  To: Maxime Chevallier; +Cc: oe-kbuild-all

Hi Maxime,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Maxime-Chevallier/net-phy-Introduce-ethernet-link-topology-representation/20231117-234210
base:   net-next/main
patch link:    https://lore.kernel.org/r/20231117162323.626979-7-maxime.chevallier%40bootlin.com
patch subject: [RFC PATCH net-next v2 06/10] net: ethtool: Introduce a command to list PHYs on an interface
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20231118/202311181328.C3AKGhlh-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231118/202311181328.C3AKGhlh-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311181328.C3AKGhlh-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> net/ethtool/phy.c:216:5: warning: no previous prototype for 'ethnl_phy_dump_one_dev' [-Wmissing-prototypes]
     216 | int ethnl_phy_dump_one_dev(struct sk_buff *skb, struct net_device *dev,
         |     ^~~~~~~~~~~~~~~~~~~~~~
   In file included from include/net/genetlink.h:6,
                    from net/ethtool/netlink.h:8,
                    from net/ethtool/phy.c:7:
   In function 'nla_put_string',
       inlined from 'ethnl_phy_fill_reply' at net/ethtool/phy.c:113:6:
>> include/net/netlink.h:1566:39: warning: argument 1 null where non-null expected [-Wnonnull]
    1566 |         return nla_put(skb, attrtype, strlen(str) + 1, str);
         |                                       ^~~~~~~~~~~
   In file included from include/linux/bitmap.h:12,
                    from include/linux/cpumask.h:12,
                    from arch/x86/include/asm/cpumask.h:5,
                    from arch/x86/include/asm/msr.h:11,
                    from arch/x86/include/asm/processor.h:23,
                    from arch/x86/include/asm/timex.h:5,
                    from include/linux/timex.h:67,
                    from include/linux/time32.h:13,
                    from include/linux/time.h:60,
                    from include/linux/ktime.h:24,
                    from include/linux/timer.h:6,
                    from include/linux/netdevice.h:24,
                    from net/ethtool/common.h:6,
                    from net/ethtool/phy.c:6:
   net/ethtool/phy.c: In function 'ethnl_phy_fill_reply':
   include/linux/string.h:129:24: note: in a call to function 'strlen' declared 'nonnull'
     129 | extern __kernel_size_t strlen(const char *);
         |                        ^~~~~~
   In function 'ethnl_phy_reply_size',
       inlined from 'ethnl_phy_doit' at net/ethtool/phy.c:162:8:
>> net/ethtool/phy.c:69:48: warning: argument 1 null where non-null expected [-Wnonnull]
      69 |                         size += nla_total_size(strlen(sfp_get_name(phydev->sfp_bus)));
         |                                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from include/linux/bitmap.h:12,
                    from include/linux/cpumask.h:12,
                    from arch/x86/include/asm/cpumask.h:5,
                    from arch/x86/include/asm/msr.h:11,
                    from arch/x86/include/asm/processor.h:23,
                    from arch/x86/include/asm/timex.h:5,
                    from include/linux/timex.h:67,
                    from include/linux/time32.h:13,
                    from include/linux/time.h:60,
                    from include/linux/ktime.h:24,
                    from include/linux/timer.h:6,
                    from include/linux/netdevice.h:24,
                    from net/ethtool/common.h:6,
                    from net/ethtool/phy.c:6:
   net/ethtool/phy.c: In function 'ethnl_phy_doit':
   include/linux/string.h:129:24: note: in a call to function 'strlen' declared 'nonnull'
     129 | extern __kernel_size_t strlen(const char *);
         |                        ^~~~~~


vim +/ethnl_phy_dump_one_dev +216 net/ethtool/phy.c

    24	
    25	/* Caller holds rtnl */
    26	static ssize_t
    27	ethnl_phy_reply_size(const struct ethnl_req_info *req_base,
    28			     struct netlink_ext_ack *extack)
    29	{
    30		struct phy_device_node *pdn;
    31		struct phy_device *phydev;
    32		struct link_topology *lt;
    33		unsigned long index;
    34		size_t size;
    35	
    36		lt = &req_base->dev->link_topo;
    37	
    38		size = nla_total_size(0);
    39	
    40		xa_for_each(&lt->phys, index, pdn) {
    41			phydev = pdn->phy;
    42	
    43			/* ETHTOOL_A_PHY_INDEX */
    44			size += nla_total_size(sizeof(u32));
    45	
    46			/* ETHTOOL_A_DRVNAME */
    47			size += nla_total_size(strlen(phydev->drv->name));
    48	
    49			/* ETHTOOL_A_NAME */
    50			size += nla_total_size(strlen(dev_name(&phydev->mdio.dev)));
    51	
    52			/* ETHTOOL_A_PHY_UPSTREAM_TYPE */
    53			size += nla_total_size(sizeof(u8));
    54	
    55			/* ETHTOOL_A_PHY_ID */
    56			size += nla_total_size(sizeof(u32));
    57	
    58			if (phy_on_sfp(phydev)) {
    59				/* ETHTOOL_A_PHY_UPSTREAM_SFP_NAME */
    60				if (sfp_get_name(pdn->parent_sfp_bus))
    61					size += nla_total_size(strlen(sfp_get_name(pdn->parent_sfp_bus)));
    62	
    63				/* ETHTOOL_A_PHY_UPSTREAM_INDEX */
    64				size += nla_total_size(sizeof(u32));
    65			}
    66	
    67			/* ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME */
    68			if (phydev->sfp_bus)
  > 69				size += nla_total_size(strlen(sfp_get_name(phydev->sfp_bus)));
    70		}
    71	
    72		return size;
    73	}
    74	
    75	static int
    76	ethnl_phy_fill_reply(const struct ethnl_req_info *req_base, struct sk_buff *skb)
    77	{
    78		struct phy_req_info *req_info = PHY_REQINFO(req_base);
    79		struct phy_device_node *pdn = &req_info->pdn;
    80		struct phy_device *phydev = pdn->phy;
    81		enum phy_upstream ptype;
    82		struct nlattr *nest;
    83	
    84		ptype = pdn->upstream_type;
    85	
    86		if (nla_put_u32(skb, ETHTOOL_A_PHY_INDEX, phydev->phyindex) ||
    87		    nla_put_string(skb, ETHTOOL_A_PHY_DRVNAME, phydev->drv->name) ||
    88		    nla_put_string(skb, ETHTOOL_A_PHY_NAME, dev_name(&phydev->mdio.dev)) ||
    89		    nla_put_u8(skb, ETHTOOL_A_PHY_UPSTREAM_TYPE, ptype) ||
    90		    nla_put_u32(skb, ETHTOOL_A_PHY_ID, phydev->phy_id))
    91			return -EMSGSIZE;
    92	
    93		if (ptype == PHY_UPSTREAM_PHY) {
    94			struct phy_device *upstream = pdn->upstream.phydev;
    95	
    96			nest = nla_nest_start(skb, ETHTOOL_A_PHY_UPSTREAM);
    97			if (!nest)
    98				return -EMSGSIZE;
    99	
   100			/* Parent index */
   101			if (nla_put_u32(skb, ETHTOOL_A_PHY_UPSTREAM_INDEX, upstream->phyindex))
   102				return -EMSGSIZE;
   103	
   104			if (pdn->parent_sfp_bus && sfp_get_name(pdn->parent_sfp_bus) &&
   105			    nla_put_string(skb, ETHTOOL_A_PHY_UPSTREAM_SFP_NAME,
   106					   sfp_get_name(pdn->parent_sfp_bus)))
   107				return -EMSGSIZE;
   108	
   109			nla_nest_end(skb, nest);
   110		}
   111	
   112		if (phydev->sfp_bus &&
   113		    nla_put_string(skb, ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME,
   114				   sfp_get_name(phydev->sfp_bus)))
   115			return -EMSGSIZE;
   116	
   117		return 0;
   118	}
   119	
   120	static int ethnl_phy_parse_request(struct ethnl_req_info *req_base,
   121					   struct nlattr **tb)
   122	{
   123		struct phy_req_info *req_info = PHY_REQINFO(req_base);
   124		struct link_topology *lt = &req_base->dev->link_topo;
   125		struct phy_device_node *pdn;
   126	
   127		if (!req_base->phydev)
   128			return 0;
   129	
   130		pdn = xa_load(&lt->phys, req_base->phydev->phyindex);
   131		memcpy(&req_info->pdn, pdn, sizeof(*pdn));
   132	
   133		return 0;
   134	}
   135	
   136	int ethnl_phy_doit(struct sk_buff *skb, struct genl_info *info)
   137	{
   138		struct phy_req_info req_info = {};
   139		struct nlattr **tb = info->attrs;
   140		struct sk_buff *rskb;
   141		void *reply_payload;
   142		int reply_len;
   143		int ret;
   144	
   145		ret = ethnl_parse_header_dev_get(&req_info.base,
   146						 tb[ETHTOOL_A_PHY_HEADER],
   147						 genl_info_net(info), info->extack,
   148						 true);
   149		if (ret < 0)
   150			return ret;
   151	
   152		rtnl_lock();
   153	
   154		ret = ethnl_phy_parse_request(&req_info.base, tb);
   155		if (ret < 0)
   156			goto err_unlock_rtnl;
   157	
   158		/* No PHY, return early */
   159		if (!req_info.pdn.phy)
   160			goto err_unlock_rtnl;
   161	
   162		ret = ethnl_phy_reply_size(&req_info.base, info->extack);
   163		if (ret < 0)
   164			goto err_unlock_rtnl;
   165		reply_len = ret + ethnl_reply_header_size();
   166	
   167		rskb = ethnl_reply_init(reply_len, req_info.base.dev,
   168					ETHTOOL_MSG_PHY_GET_REPLY,
   169					ETHTOOL_A_PHY_HEADER,
   170					info, &reply_payload);
   171		if (!rskb) {
   172			ret = -ENOMEM;
   173			goto err_unlock_rtnl;
   174		}
   175	
   176		ret = ethnl_phy_fill_reply(&req_info.base, rskb);
   177		if (ret)
   178			goto err_free_msg;
   179	
   180		rtnl_unlock();
   181		ethnl_parse_header_dev_put(&req_info.base);
   182		genlmsg_end(rskb, reply_payload);
   183	
   184		return genlmsg_reply(rskb, info);
   185	
   186	err_free_msg:
   187		nlmsg_free(rskb);
   188	err_unlock_rtnl:
   189		rtnl_unlock();
   190		ethnl_parse_header_dev_put(&req_info.base);
   191		return ret;
   192	}
   193	
   194	struct ethnl_phy_dump_ctx {
   195		struct ethnl_req_info	req_info;
   196	};
   197	
   198	int ethnl_phy_start(struct netlink_callback *cb)
   199	{
   200		const struct genl_dumpit_info *info = genl_dumpit_info(cb);
   201		struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
   202		struct nlattr **tb = info->info.attrs;
   203		int ret;
   204	
   205		BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->ctx));
   206	
   207		memset(ctx, 0, sizeof(*ctx));
   208	
   209		ret = ethnl_parse_header_dev_get(&ctx->req_info,
   210						 tb[ETHTOOL_A_PHY_HEADER],
   211						 sock_net(cb->skb->sk), cb->extack,
   212						 false);
   213		return ret;
   214	}
   215	
 > 216	int ethnl_phy_dump_one_dev(struct sk_buff *skb, struct net_device *dev,
   217				   struct netlink_callback *cb)
   218	{
   219		struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
   220		struct phy_req_info *pri = PHY_REQINFO(&ctx->req_info);
   221		struct phy_device_node *pdn;
   222		unsigned long index = 1;
   223		void *ehdr;
   224		int ret;
   225	
   226		ctx->req_info.dev = dev;
   227	
   228		xa_for_each(&dev->link_topo.phys, index, pdn) {
   229			ehdr = ethnl_dump_put(skb, cb,
   230					      ETHTOOL_MSG_PHY_GET_REPLY);
   231			if (!ehdr) {
   232				ret = -EMSGSIZE;
   233				break;
   234			}
   235	
   236			ret = ethnl_fill_reply_header(skb, dev,
   237						      ETHTOOL_A_PHY_HEADER);
   238			if (ret < 0) {
   239				genlmsg_cancel(skb, ehdr);
   240				break;
   241			}
   242	
   243			memcpy(&pri->pdn, pdn, sizeof(*pdn));
   244			ret = ethnl_phy_fill_reply(&ctx->req_info, skb);
   245	
   246			genlmsg_end(skb, ehdr);
   247		}
   248	
   249		ctx->req_info.dev = NULL;
   250	
   251		return ret;
   252	}
   253	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 01/10] net: phy: Introduce ethernet link topology representation
  2023-11-17 16:23   ` Maxime Chevallier
@ 2023-11-21  0:24     ` Andrew Lunn
  -1 siblings, 0 replies; 53+ messages in thread
From: Andrew Lunn @ 2023-11-21  0:24 UTC (permalink / raw
  To: Maxime Chevallier
  Cc: davem, netdev, linux-kernel, thomas.petazzoni, Jakub Kicinski,
	Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

> +int link_topo_add_phy(struct link_topology *lt, struct phy_device *phy,
> +		      enum phy_upstream upt, void *upstream)
> +{
> +	ret = xa_alloc_cyclic(&lt->phys, &phy->phyindex, pdn, xa_limit_32b,
> +			      &lt->next_phy_index, GFP_KERNEL);
> +	if (ret)
> +		goto err;
> +
> +	return 0;

It looks like that could be just return xa_alloc_cyclic(...);

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

I think this filename is too generic. Maybe phy_link_topology.h, or
move it into include/net.

> +struct phy_device *link_topo_get_phy(struct link_topology *lt, int phyindex);
> +int link_topo_add_phy(struct link_topology *lt, struct phy_device *phy,
> +		      enum phy_upstream upt, void *upstream);
> +
> +void link_topo_del_phy(struct link_topology *lt, struct phy_device *phy);

What is the locking for these functions? Are you assuming RTNL? Maybe
add ASSERT_RTNL(); into them to make this clear.

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

Again, i think this filename is too generic.

> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index a16c9cc063fe..7021a0d3d982 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -40,7 +40,6 @@
>  #include <net/dcbnl.h>
>  #endif
>  #include <net/netprio_cgroup.h>
> -
>  #include <linux/netdev_features.h>

Whitespace change.

>  #include <linux/neighbour.h>
>  #include <uapi/linux/netdevice.h>
> @@ -52,6 +51,7 @@
>  #include <net/net_trackers.h>
>  #include <net/net_debug.h>
>  #include <net/dropreason-core.h>
> +#include <linux/link_topology_core.h>
>  
>  struct netpoll_info;
>  struct device;
> @@ -2405,6 +2405,7 @@ struct net_device {
>  #if IS_ENABLED(CONFIG_CGROUP_NET_PRIO)
>  	struct netprio_map __rcu *priomap;
>  #endif
> +	struct link_topology	link_topo;
>  	struct phy_device	*phydev;
>  	struct sfp_bus		*sfp_bus;
>  	struct lock_class_key	*qdisc_tx_busylock;
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 3cc52826f18e..d698180b1df0 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;

Is this int, or unsigned int? Is a negative value possible and legal?

> +enum phy_upstream {
> +	PHY_UPSTREAM_MAC,
> +	PHY_UPSTREAM_SFP,
> +	PHY_UPSTREAM_PHY,
> +};

Please document what these actually mean.

       Andrew

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 01/10] net: phy: Introduce ethernet link topology representation
@ 2023-11-21  0:24     ` Andrew Lunn
  0 siblings, 0 replies; 53+ messages in thread
From: Andrew Lunn @ 2023-11-21  0:24 UTC (permalink / raw
  To: Maxime Chevallier
  Cc: davem, netdev, linux-kernel, thomas.petazzoni, Jakub Kicinski,
	Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

> +int link_topo_add_phy(struct link_topology *lt, struct phy_device *phy,
> +		      enum phy_upstream upt, void *upstream)
> +{
> +	ret = xa_alloc_cyclic(&lt->phys, &phy->phyindex, pdn, xa_limit_32b,
> +			      &lt->next_phy_index, GFP_KERNEL);
> +	if (ret)
> +		goto err;
> +
> +	return 0;

It looks like that could be just return xa_alloc_cyclic(...);

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

I think this filename is too generic. Maybe phy_link_topology.h, or
move it into include/net.

> +struct phy_device *link_topo_get_phy(struct link_topology *lt, int phyindex);
> +int link_topo_add_phy(struct link_topology *lt, struct phy_device *phy,
> +		      enum phy_upstream upt, void *upstream);
> +
> +void link_topo_del_phy(struct link_topology *lt, struct phy_device *phy);

What is the locking for these functions? Are you assuming RTNL? Maybe
add ASSERT_RTNL(); into them to make this clear.

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

Again, i think this filename is too generic.

> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index a16c9cc063fe..7021a0d3d982 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -40,7 +40,6 @@
>  #include <net/dcbnl.h>
>  #endif
>  #include <net/netprio_cgroup.h>
> -
>  #include <linux/netdev_features.h>

Whitespace change.

>  #include <linux/neighbour.h>
>  #include <uapi/linux/netdevice.h>
> @@ -52,6 +51,7 @@
>  #include <net/net_trackers.h>
>  #include <net/net_debug.h>
>  #include <net/dropreason-core.h>
> +#include <linux/link_topology_core.h>
>  
>  struct netpoll_info;
>  struct device;
> @@ -2405,6 +2405,7 @@ struct net_device {
>  #if IS_ENABLED(CONFIG_CGROUP_NET_PRIO)
>  	struct netprio_map __rcu *priomap;
>  #endif
> +	struct link_topology	link_topo;
>  	struct phy_device	*phydev;
>  	struct sfp_bus		*sfp_bus;
>  	struct lock_class_key	*qdisc_tx_busylock;
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 3cc52826f18e..d698180b1df0 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;

Is this int, or unsigned int? Is a negative value possible and legal?

> +enum phy_upstream {
> +	PHY_UPSTREAM_MAC,
> +	PHY_UPSTREAM_SFP,
> +	PHY_UPSTREAM_PHY,
> +};

Please document what these actually mean.

       Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 03/10] net: phy: add helpers to handle sfp phy connect/disconnect
  2023-11-17 16:23   ` Maxime Chevallier
@ 2023-11-21  0:57     ` Andrew Lunn
  -1 siblings, 0 replies; 53+ messages in thread
From: Andrew Lunn @ 2023-11-21  0:57 UTC (permalink / raw
  To: Maxime Chevallier
  Cc: davem, netdev, linux-kernel, thomas.petazzoni, Jakub Kicinski,
	Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

> +/**
> + * 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;

Will this function only ever be called from a PHY driver? If so, we
know upstream is PHY. So we can avoid using void * and make it a
struct phy_device *. 

       Andrew

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 03/10] net: phy: add helpers to handle sfp phy connect/disconnect
@ 2023-11-21  0:57     ` Andrew Lunn
  0 siblings, 0 replies; 53+ messages in thread
From: Andrew Lunn @ 2023-11-21  0:57 UTC (permalink / raw
  To: Maxime Chevallier
  Cc: davem, netdev, linux-kernel, thomas.petazzoni, Jakub Kicinski,
	Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

> +/**
> + * 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;

Will this function only ever be called from a PHY driver? If so, we
know upstream is PHY. So we can avoid using void * and make it a
struct phy_device *. 

       Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 04/10] net: sfp: Add helper to return the SFP bus name
  2023-11-17 16:23   ` Maxime Chevallier
@ 2023-11-21  1:00     ` Andrew Lunn
  -1 siblings, 0 replies; 53+ messages in thread
From: Andrew Lunn @ 2023-11-21  1:00 UTC (permalink / raw
  To: Maxime Chevallier
  Cc: davem, netdev, linux-kernel, thomas.petazzoni, Jakub Kicinski,
	Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

> +const char *sfp_get_name(struct sfp_bus *bus)
> +{
> +	if (bus->sfp_dev)
> +		return dev_name(bus->sfp_dev);
> +
> +	return NULL;
> +}

Locking? Do you assume rtnl? Does this function need to take rtnl?

	 Andrew

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 04/10] net: sfp: Add helper to return the SFP bus name
@ 2023-11-21  1:00     ` Andrew Lunn
  0 siblings, 0 replies; 53+ messages in thread
From: Andrew Lunn @ 2023-11-21  1:00 UTC (permalink / raw
  To: Maxime Chevallier
  Cc: davem, netdev, linux-kernel, thomas.petazzoni, Jakub Kicinski,
	Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

> +const char *sfp_get_name(struct sfp_bus *bus)
> +{
> +	if (bus->sfp_dev)
> +		return dev_name(bus->sfp_dev);
> +
> +	return NULL;
> +}

Locking? Do you assume rtnl? Does this function need to take rtnl?

	 Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 05/10] net: ethtool: Allow passing a phy index for some commands
  2023-11-17 16:23   ` Maxime Chevallier
@ 2023-11-21  1:08     ` Andrew Lunn
  -1 siblings, 0 replies; 53+ messages in thread
From: Andrew Lunn @ 2023-11-21  1:08 UTC (permalink / raw
  To: Maxime Chevallier
  Cc: davem, netdev, linux-kernel, thomas.petazzoni, Jakub Kicinski,
	Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

> +	if (dev) {
> +		if (tb[ETHTOOL_A_HEADER_PHY_INDEX]) {
> +			u32 phy_index = nla_get_u32(tb[ETHTOOL_A_HEADER_PHY_INDEX]);
> +
> +			phydev = link_topo_get_phy(&dev->link_topo, phy_index);

struct phy_device *link_topo_get_phy(struct link_topology *lt, int phyindex)

We have u32 vs int here for phyindex. It would be good to have the
same type everywhere.


> +			if (!phydev) {
> +				NL_SET_ERR_MSG_ATTR(extack, header, "no phy matches phy index");
> +				return -EINVAL;
> +			}
> +		} else {
> +			/* If we need a PHY but no phy index is specified, fallback
> +			 * to dev->phydev
> +			 */
> +			phydev = dev->phydev;
> +		}
> +	}
> +
> +	req_info->phydev = phydev;

Don't forget to update Documentation/networking/ethtool-netlink.rst.

      Andrew

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 05/10] net: ethtool: Allow passing a phy index for some commands
@ 2023-11-21  1:08     ` Andrew Lunn
  0 siblings, 0 replies; 53+ messages in thread
From: Andrew Lunn @ 2023-11-21  1:08 UTC (permalink / raw
  To: Maxime Chevallier
  Cc: davem, netdev, linux-kernel, thomas.petazzoni, Jakub Kicinski,
	Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

> +	if (dev) {
> +		if (tb[ETHTOOL_A_HEADER_PHY_INDEX]) {
> +			u32 phy_index = nla_get_u32(tb[ETHTOOL_A_HEADER_PHY_INDEX]);
> +
> +			phydev = link_topo_get_phy(&dev->link_topo, phy_index);

struct phy_device *link_topo_get_phy(struct link_topology *lt, int phyindex)

We have u32 vs int here for phyindex. It would be good to have the
same type everywhere.


> +			if (!phydev) {
> +				NL_SET_ERR_MSG_ATTR(extack, header, "no phy matches phy index");
> +				return -EINVAL;
> +			}
> +		} else {
> +			/* If we need a PHY but no phy index is specified, fallback
> +			 * to dev->phydev
> +			 */
> +			phydev = dev->phydev;
> +		}
> +	}
> +
> +	req_info->phydev = phydev;

Don't forget to update Documentation/networking/ethtool-netlink.rst.

      Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 06/10] net: ethtool: Introduce a command to list PHYs on an interface
  2023-11-17 16:23   ` Maxime Chevallier
@ 2023-11-21  1:34     ` Andrew Lunn
  -1 siblings, 0 replies; 53+ messages in thread
From: Andrew Lunn @ 2023-11-21  1:34 UTC (permalink / raw
  To: Maxime Chevallier
  Cc: davem, netdev, linux-kernel, thomas.petazzoni, Jakub Kicinski,
	Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index 2540c70952ff..29ef675f45c0 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -57,6 +57,7 @@ Structure of this header is
>    ``ETHTOOL_A_HEADER_DEV_INDEX``  u32     device ifindex
>    ``ETHTOOL_A_HEADER_DEV_NAME``   string  device name
>    ``ETHTOOL_A_HEADER_FLAGS``      u32     flags common for all requests
> +  ``ETHTOOL_A_HEADER_PHY_INDEX``  u32     phy device index
>    ==============================  ======  =============================
>  
>  ``ETHTOOL_A_HEADER_DEV_INDEX`` and ``ETHTOOL_A_HEADER_DEV_NAME`` identify the
> @@ -81,6 +82,12 @@ the behaviour is backward compatible, i.e. requests from old clients not aware
>  of the flag should be interpreted the way the client expects. A client must
>  not set flags it does not understand.
>  
> +``ETHTOOL_A_HEADER_PHY_INDEX`` identify the ethernet PHY the message relates to.
> +As there are numerous commands that are related to PHY configuration, and because
> +we can have more than one PHY on the link, the PHY index can be passed in the
> +request for the commands that needs it. It is however not mandatory, and if it
> +is not passed for commands that target a PHY, the net_device.phydev pointer
> +is used, as a fallback that keeps the legacy behaviour.

O.K, you did document it :-)

But i would make this part of the previous patch.

> +Kernel response contents:
> +
> +  =================================     ======  ==========================
> +  ``ETHTOOL_A_PHY_HEADER``              nested  request header
> +  ``ETHTOOL_A_PHY_INDEX``               u32     the phy's unique index, that can
> +                                                be used for phy-specific requests
> +  ``ETHTOOL_A_PHY_DRVNAME``             string  the phy driver name
> +  ``ETHTOOL_A_PHY_NAME``                string  the phy device name
> +  ``ETHTOOL_A_PHY_UPSTREAM_TYPE``       u32     the type of device this phy is
> +                                                connected to
> +  ``ETHTOOL_A_PHY_UPSTREAM_PHY``        nested  if the phy is connected to another
> +                                                phy, this nest contains info on
> +                                                that connection
> +  ``ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME`` string  if the phy controls an sfp bus,
> +                                                the name of the sfp bus
> +  ``ETHTOOL_A_PHY_ID``                  u32     the phy id if the phy is C22

Maybe a future extension. We could make phy_bus_match() set
phydev->phy_id to the ID it matched to the driver when doing C45. We
would then always have a value here.

> --- a/include/linux/ethtool_netlink.h
> +++ b/include/linux/ethtool_netlink.h
> @@ -118,5 +118,10 @@ static inline bool ethtool_dev_mm_supported(struct net_device *dev)
>  	return false;
>  }
>  
> +int ethnl_phy_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
> +{
> +	return -EOPNOTSUPP;
> +}

This is a header file, so should probably be static inline.

     Andrew

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 06/10] net: ethtool: Introduce a command to list PHYs on an interface
@ 2023-11-21  1:34     ` Andrew Lunn
  0 siblings, 0 replies; 53+ messages in thread
From: Andrew Lunn @ 2023-11-21  1:34 UTC (permalink / raw
  To: Maxime Chevallier
  Cc: davem, netdev, linux-kernel, thomas.petazzoni, Jakub Kicinski,
	Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index 2540c70952ff..29ef675f45c0 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -57,6 +57,7 @@ Structure of this header is
>    ``ETHTOOL_A_HEADER_DEV_INDEX``  u32     device ifindex
>    ``ETHTOOL_A_HEADER_DEV_NAME``   string  device name
>    ``ETHTOOL_A_HEADER_FLAGS``      u32     flags common for all requests
> +  ``ETHTOOL_A_HEADER_PHY_INDEX``  u32     phy device index
>    ==============================  ======  =============================
>  
>  ``ETHTOOL_A_HEADER_DEV_INDEX`` and ``ETHTOOL_A_HEADER_DEV_NAME`` identify the
> @@ -81,6 +82,12 @@ the behaviour is backward compatible, i.e. requests from old clients not aware
>  of the flag should be interpreted the way the client expects. A client must
>  not set flags it does not understand.
>  
> +``ETHTOOL_A_HEADER_PHY_INDEX`` identify the ethernet PHY the message relates to.
> +As there are numerous commands that are related to PHY configuration, and because
> +we can have more than one PHY on the link, the PHY index can be passed in the
> +request for the commands that needs it. It is however not mandatory, and if it
> +is not passed for commands that target a PHY, the net_device.phydev pointer
> +is used, as a fallback that keeps the legacy behaviour.

O.K, you did document it :-)

But i would make this part of the previous patch.

> +Kernel response contents:
> +
> +  =================================     ======  ==========================
> +  ``ETHTOOL_A_PHY_HEADER``              nested  request header
> +  ``ETHTOOL_A_PHY_INDEX``               u32     the phy's unique index, that can
> +                                                be used for phy-specific requests
> +  ``ETHTOOL_A_PHY_DRVNAME``             string  the phy driver name
> +  ``ETHTOOL_A_PHY_NAME``                string  the phy device name
> +  ``ETHTOOL_A_PHY_UPSTREAM_TYPE``       u32     the type of device this phy is
> +                                                connected to
> +  ``ETHTOOL_A_PHY_UPSTREAM_PHY``        nested  if the phy is connected to another
> +                                                phy, this nest contains info on
> +                                                that connection
> +  ``ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME`` string  if the phy controls an sfp bus,
> +                                                the name of the sfp bus
> +  ``ETHTOOL_A_PHY_ID``                  u32     the phy id if the phy is C22

Maybe a future extension. We could make phy_bus_match() set
phydev->phy_id to the ID it matched to the driver when doing C45. We
would then always have a value here.

> --- a/include/linux/ethtool_netlink.h
> +++ b/include/linux/ethtool_netlink.h
> @@ -118,5 +118,10 @@ static inline bool ethtool_dev_mm_supported(struct net_device *dev)
>  	return false;
>  }
>  
> +int ethnl_phy_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
> +{
> +	return -EOPNOTSUPP;
> +}

This is a header file, so should probably be static inline.

     Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 06/10] net: ethtool: Introduce a command to list PHYs on an interface
  2023-11-17 16:23   ` Maxime Chevallier
@ 2023-11-21  1:40     ` Andrew Lunn
  -1 siblings, 0 replies; 53+ messages in thread
From: Andrew Lunn @ 2023-11-21  1:40 UTC (permalink / raw
  To: Maxime Chevallier
  Cc: davem, netdev, linux-kernel, thomas.petazzoni, Jakub Kicinski,
	Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

> +ethnl_phy_reply_size(const struct ethnl_req_info *req_base,
> +		     struct netlink_ext_ack *extack)
> +{
> +	struct phy_device_node *pdn;
> +	struct phy_device *phydev;
> +	struct link_topology *lt;
> +	unsigned long index;
> +	size_t size;
> +
> +	lt = &req_base->dev->link_topo;
> +
> +	size = nla_total_size(0);
> +
> +	xa_for_each(&lt->phys, index, pdn) {
> +		phydev = pdn->phy;
> +
> +		/* ETHTOOL_A_PHY_INDEX */
> +		size += nla_total_size(sizeof(u32));
> +
> +		/* ETHTOOL_A_DRVNAME */
> +		size += nla_total_size(strlen(phydev->drv->name));
> +
> +		/* ETHTOOL_A_NAME */
> +		size += nla_total_size(strlen(dev_name(&phydev->mdio.dev)));
> +
> +		/* ETHTOOL_A_PHY_UPSTREAM_TYPE */
> +		size += nla_total_size(sizeof(u8));
> +
> +		/* ETHTOOL_A_PHY_ID */
> +		size += nla_total_size(sizeof(u32));
> +
> +		if (phy_on_sfp(phydev)) {
> +			/* ETHTOOL_A_PHY_UPSTREAM_SFP_NAME */
> +			if (sfp_get_name(pdn->parent_sfp_bus))
> +				size += nla_total_size(strlen(sfp_get_name(pdn->parent_sfp_bus)));

Have you tried a modular build?

sfp_get_name() could be in a module, and then you will get linker
errors. It is all a bit messy calling into phylib from ethtool :-(

This might actually be the only function you need? If so, its small
enough you can move it into a header as a static inline function.

       Andrew

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 06/10] net: ethtool: Introduce a command to list PHYs on an interface
@ 2023-11-21  1:40     ` Andrew Lunn
  0 siblings, 0 replies; 53+ messages in thread
From: Andrew Lunn @ 2023-11-21  1:40 UTC (permalink / raw
  To: Maxime Chevallier
  Cc: davem, netdev, linux-kernel, thomas.petazzoni, Jakub Kicinski,
	Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

> +ethnl_phy_reply_size(const struct ethnl_req_info *req_base,
> +		     struct netlink_ext_ack *extack)
> +{
> +	struct phy_device_node *pdn;
> +	struct phy_device *phydev;
> +	struct link_topology *lt;
> +	unsigned long index;
> +	size_t size;
> +
> +	lt = &req_base->dev->link_topo;
> +
> +	size = nla_total_size(0);
> +
> +	xa_for_each(&lt->phys, index, pdn) {
> +		phydev = pdn->phy;
> +
> +		/* ETHTOOL_A_PHY_INDEX */
> +		size += nla_total_size(sizeof(u32));
> +
> +		/* ETHTOOL_A_DRVNAME */
> +		size += nla_total_size(strlen(phydev->drv->name));
> +
> +		/* ETHTOOL_A_NAME */
> +		size += nla_total_size(strlen(dev_name(&phydev->mdio.dev)));
> +
> +		/* ETHTOOL_A_PHY_UPSTREAM_TYPE */
> +		size += nla_total_size(sizeof(u8));
> +
> +		/* ETHTOOL_A_PHY_ID */
> +		size += nla_total_size(sizeof(u32));
> +
> +		if (phy_on_sfp(phydev)) {
> +			/* ETHTOOL_A_PHY_UPSTREAM_SFP_NAME */
> +			if (sfp_get_name(pdn->parent_sfp_bus))
> +				size += nla_total_size(strlen(sfp_get_name(pdn->parent_sfp_bus)));

Have you tried a modular build?

sfp_get_name() could be in a module, and then you will get linker
errors. It is all a bit messy calling into phylib from ethtool :-(

This might actually be the only function you need? If so, its small
enough you can move it into a header as a static inline function.

       Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 03/10] net: phy: add helpers to handle sfp phy connect/disconnect
  2023-11-21  0:57     ` Andrew Lunn
@ 2023-11-21 10:08       ` Russell King (Oracle)
  -1 siblings, 0 replies; 53+ messages in thread
From: Russell King (Oracle) @ 2023-11-21 10:08 UTC (permalink / raw
  To: Andrew Lunn
  Cc: Maxime Chevallier, davem, netdev, linux-kernel, thomas.petazzoni,
	Jakub Kicinski, Eric Dumazet, Paolo Abeni, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

On Tue, Nov 21, 2023 at 01:57:24AM +0100, Andrew Lunn wrote:
> > +/**
> > + * 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;
> 
> Will this function only ever be called from a PHY driver? If so, we
> know upstream is PHY. So we can avoid using void * and make it a
> struct phy_device *. 

No. This function is hooked into the .connect_phy method of
sfp_upstream_ops, and the SFP bus layer has no idea what the
"upstream" is. In this case, it's a PHY. In the case of phylink,
it's the phylink struct. So no, "struct phy_device *" here will
cause build errors.

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

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 03/10] net: phy: add helpers to handle sfp phy connect/disconnect
@ 2023-11-21 10:08       ` Russell King (Oracle)
  0 siblings, 0 replies; 53+ messages in thread
From: Russell King (Oracle) @ 2023-11-21 10:08 UTC (permalink / raw
  To: Andrew Lunn
  Cc: Maxime Chevallier, davem, netdev, linux-kernel, thomas.petazzoni,
	Jakub Kicinski, Eric Dumazet, Paolo Abeni, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

On Tue, Nov 21, 2023 at 01:57:24AM +0100, Andrew Lunn wrote:
> > +/**
> > + * 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;
> 
> Will this function only ever be called from a PHY driver? If so, we
> know upstream is PHY. So we can avoid using void * and make it a
> struct phy_device *. 

No. This function is hooked into the .connect_phy method of
sfp_upstream_ops, and the SFP bus layer has no idea what the
"upstream" is. In this case, it's a PHY. In the case of phylink,
it's the phylink struct. So no, "struct phy_device *" here will
cause build errors.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 04/10] net: sfp: Add helper to return the SFP bus name
  2023-11-21  1:00     ` Andrew Lunn
@ 2023-11-21 10:20       ` Russell King (Oracle)
  -1 siblings, 0 replies; 53+ messages in thread
From: Russell King (Oracle) @ 2023-11-21 10:20 UTC (permalink / raw
  To: Andrew Lunn
  Cc: Maxime Chevallier, davem, netdev, linux-kernel, thomas.petazzoni,
	Jakub Kicinski, Eric Dumazet, Paolo Abeni, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

On Tue, Nov 21, 2023 at 02:00:58AM +0100, Andrew Lunn wrote:
> > +const char *sfp_get_name(struct sfp_bus *bus)
> > +{
> > +	if (bus->sfp_dev)
> > +		return dev_name(bus->sfp_dev);
> > +
> > +	return NULL;
> > +}
> 
> Locking? Do you assume rtnl? Does this function need to take rtnl?

Yes, rtnl needs to be held to safely access bus->sfp_dev, and that
either needs to happen in this function, or be documented as being
requried (and ASSERT_RTNL() added here.)

The reason is that sfp_dev is the SFP socket device which can be
unbound via sfp_unregister_socket(), which will set bus->sfp_dev to
NULL. This could race with the above.

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

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 04/10] net: sfp: Add helper to return the SFP bus name
@ 2023-11-21 10:20       ` Russell King (Oracle)
  0 siblings, 0 replies; 53+ messages in thread
From: Russell King (Oracle) @ 2023-11-21 10:20 UTC (permalink / raw
  To: Andrew Lunn
  Cc: Maxime Chevallier, davem, netdev, linux-kernel, thomas.petazzoni,
	Jakub Kicinski, Eric Dumazet, Paolo Abeni, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

On Tue, Nov 21, 2023 at 02:00:58AM +0100, Andrew Lunn wrote:
> > +const char *sfp_get_name(struct sfp_bus *bus)
> > +{
> > +	if (bus->sfp_dev)
> > +		return dev_name(bus->sfp_dev);
> > +
> > +	return NULL;
> > +}
> 
> Locking? Do you assume rtnl? Does this function need to take rtnl?

Yes, rtnl needs to be held to safely access bus->sfp_dev, and that
either needs to happen in this function, or be documented as being
requried (and ASSERT_RTNL() added here.)

The reason is that sfp_dev is the SFP socket device which can be
unbound via sfp_unregister_socket(), which will set bus->sfp_dev to
NULL. This could race with the above.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 03/10] net: phy: add helpers to handle sfp phy connect/disconnect
  2023-11-21 10:08       ` Russell King (Oracle)
@ 2023-11-21 14:35         ` Andrew Lunn
  -1 siblings, 0 replies; 53+ messages in thread
From: Andrew Lunn @ 2023-11-21 14:35 UTC (permalink / raw
  To: Russell King (Oracle)
  Cc: Maxime Chevallier, davem, netdev, linux-kernel, thomas.petazzoni,
	Jakub Kicinski, Eric Dumazet, Paolo Abeni, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

On Tue, Nov 21, 2023 at 10:08:00AM +0000, Russell King (Oracle) wrote:
> On Tue, Nov 21, 2023 at 01:57:24AM +0100, Andrew Lunn wrote:
> > > +/**
> > > + * 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;
> > 
> > Will this function only ever be called from a PHY driver? If so, we
> > know upstream is PHY. So we can avoid using void * and make it a
> > struct phy_device *. 
> 
> No. This function is hooked into the .connect_phy method of
> sfp_upstream_ops, and the SFP bus layer has no idea what the
> "upstream" is. In this case, it's a PHY. In the case of phylink,
> it's the phylink struct. So no, "struct phy_device *" here will
> cause build errors.

O.K, thanks for checking this. It would of been nice to have some
compile time checking what is passed is what we expect in terms of
type, but C does not allow that in this case.

	 Andrew

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 03/10] net: phy: add helpers to handle sfp phy connect/disconnect
@ 2023-11-21 14:35         ` Andrew Lunn
  0 siblings, 0 replies; 53+ messages in thread
From: Andrew Lunn @ 2023-11-21 14:35 UTC (permalink / raw
  To: Russell King (Oracle)
  Cc: Maxime Chevallier, davem, netdev, linux-kernel, thomas.petazzoni,
	Jakub Kicinski, Eric Dumazet, Paolo Abeni, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

On Tue, Nov 21, 2023 at 10:08:00AM +0000, Russell King (Oracle) wrote:
> On Tue, Nov 21, 2023 at 01:57:24AM +0100, Andrew Lunn wrote:
> > > +/**
> > > + * 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;
> > 
> > Will this function only ever be called from a PHY driver? If so, we
> > know upstream is PHY. So we can avoid using void * and make it a
> > struct phy_device *. 
> 
> No. This function is hooked into the .connect_phy method of
> sfp_upstream_ops, and the SFP bus layer has no idea what the
> "upstream" is. In this case, it's a PHY. In the case of phylink,
> it's the phylink struct. So no, "struct phy_device *" here will
> cause build errors.

O.K, thanks for checking this. It would of been nice to have some
compile time checking what is passed is what we expect in terms of
type, but C does not allow that in this case.

	 Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 01/10] net: phy: Introduce ethernet link topology representation
  2023-11-21  0:24     ` Andrew Lunn
@ 2023-11-23 13:34       ` Maxime Chevallier
  -1 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-23 13:34 UTC (permalink / raw
  To: Andrew Lunn
  Cc: davem, netdev, linux-kernel, thomas.petazzoni, Jakub Kicinski,
	Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

Hello Andrew,

On Tue, 21 Nov 2023 01:24:47 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > +int link_topo_add_phy(struct link_topology *lt, struct phy_device *phy,
> > +		      enum phy_upstream upt, void *upstream)
> > +{
> > +	ret = xa_alloc_cyclic(&lt->phys, &phy->phyindex, pdn, xa_limit_32b,
> > +			      &lt->next_phy_index, GFP_KERNEL);
> > +	if (ret)
> > +		goto err;
> > +
> > +	return 0;  
> 
> It looks like that could be just return xa_alloc_cyclic(...);

Indeed, nice catch

> 
> > diff --git a/include/linux/link_topology.h b/include/linux/link_topology.h  
> 
> I think this filename is too generic. Maybe phy_link_topology.h, or
> move it into include/net.
> 
> > +struct phy_device *link_topo_get_phy(struct link_topology *lt, int phyindex);
> > +int link_topo_add_phy(struct link_topology *lt, struct phy_device *phy,
> > +		      enum phy_upstream upt, void *upstream);
> > +
> > +void link_topo_del_phy(struct link_topology *lt, struct phy_device *phy);  
> 
> What is the locking for these functions? Are you assuming RTNL? Maybe
> add ASSERT_RTNL(); into them to make this clear.

Indeed I assume it, I'll add an assert and document that clearly then.

> 
> > diff --git a/include/linux/link_topology_core.h b/include/linux/link_topology_core.h  
> 
> Again, i think this filename is too generic.
> 
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index a16c9cc063fe..7021a0d3d982 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -40,7 +40,6 @@
> >  #include <net/dcbnl.h>
> >  #endif
> >  #include <net/netprio_cgroup.h>
> > -
> >  #include <linux/netdev_features.h>  
> 
> Whitespace change.
> 
> >  #include <linux/neighbour.h>
> >  #include <uapi/linux/netdevice.h>
> > @@ -52,6 +51,7 @@
> >  #include <net/net_trackers.h>
> >  #include <net/net_debug.h>
> >  #include <net/dropreason-core.h>
> > +#include <linux/link_topology_core.h>
> >  
> >  struct netpoll_info;
> >  struct device;
> > @@ -2405,6 +2405,7 @@ struct net_device {
> >  #if IS_ENABLED(CONFIG_CGROUP_NET_PRIO)
> >  	struct netprio_map __rcu *priomap;
> >  #endif
> > +	struct link_topology	link_topo;
> >  	struct phy_device	*phydev;
> >  	struct sfp_bus		*sfp_bus;
> >  	struct lock_class_key	*qdisc_tx_busylock;
> > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > index 3cc52826f18e..d698180b1df0 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;  
> 
> Is this int, or unsigned int? Is a negative value possible and legal?

You're right this should be unsigned. I've also missed properly
documenting correct values and their meaning (like phyindex 0 would be
no index assigned).

> 
> > +enum phy_upstream {
> > +	PHY_UPSTREAM_MAC,
> > +	PHY_UPSTREAM_SFP,
> > +	PHY_UPSTREAM_PHY,
> > +};  
> 
> Please document what these actually mean.

I'll do this as well.

Thank you for reviewing,

Maxime

> 
>        Andrew


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 01/10] net: phy: Introduce ethernet link topology representation
@ 2023-11-23 13:34       ` Maxime Chevallier
  0 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-23 13:34 UTC (permalink / raw
  To: Andrew Lunn
  Cc: davem, netdev, linux-kernel, thomas.petazzoni, Jakub Kicinski,
	Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

Hello Andrew,

On Tue, 21 Nov 2023 01:24:47 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > +int link_topo_add_phy(struct link_topology *lt, struct phy_device *phy,
> > +		      enum phy_upstream upt, void *upstream)
> > +{
> > +	ret = xa_alloc_cyclic(&lt->phys, &phy->phyindex, pdn, xa_limit_32b,
> > +			      &lt->next_phy_index, GFP_KERNEL);
> > +	if (ret)
> > +		goto err;
> > +
> > +	return 0;  
> 
> It looks like that could be just return xa_alloc_cyclic(...);

Indeed, nice catch

> 
> > diff --git a/include/linux/link_topology.h b/include/linux/link_topology.h  
> 
> I think this filename is too generic. Maybe phy_link_topology.h, or
> move it into include/net.
> 
> > +struct phy_device *link_topo_get_phy(struct link_topology *lt, int phyindex);
> > +int link_topo_add_phy(struct link_topology *lt, struct phy_device *phy,
> > +		      enum phy_upstream upt, void *upstream);
> > +
> > +void link_topo_del_phy(struct link_topology *lt, struct phy_device *phy);  
> 
> What is the locking for these functions? Are you assuming RTNL? Maybe
> add ASSERT_RTNL(); into them to make this clear.

Indeed I assume it, I'll add an assert and document that clearly then.

> 
> > diff --git a/include/linux/link_topology_core.h b/include/linux/link_topology_core.h  
> 
> Again, i think this filename is too generic.
> 
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index a16c9cc063fe..7021a0d3d982 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -40,7 +40,6 @@
> >  #include <net/dcbnl.h>
> >  #endif
> >  #include <net/netprio_cgroup.h>
> > -
> >  #include <linux/netdev_features.h>  
> 
> Whitespace change.
> 
> >  #include <linux/neighbour.h>
> >  #include <uapi/linux/netdevice.h>
> > @@ -52,6 +51,7 @@
> >  #include <net/net_trackers.h>
> >  #include <net/net_debug.h>
> >  #include <net/dropreason-core.h>
> > +#include <linux/link_topology_core.h>
> >  
> >  struct netpoll_info;
> >  struct device;
> > @@ -2405,6 +2405,7 @@ struct net_device {
> >  #if IS_ENABLED(CONFIG_CGROUP_NET_PRIO)
> >  	struct netprio_map __rcu *priomap;
> >  #endif
> > +	struct link_topology	link_topo;
> >  	struct phy_device	*phydev;
> >  	struct sfp_bus		*sfp_bus;
> >  	struct lock_class_key	*qdisc_tx_busylock;
> > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > index 3cc52826f18e..d698180b1df0 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;  
> 
> Is this int, or unsigned int? Is a negative value possible and legal?

You're right this should be unsigned. I've also missed properly
documenting correct values and their meaning (like phyindex 0 would be
no index assigned).

> 
> > +enum phy_upstream {
> > +	PHY_UPSTREAM_MAC,
> > +	PHY_UPSTREAM_SFP,
> > +	PHY_UPSTREAM_PHY,
> > +};  
> 
> Please document what these actually mean.

I'll do this as well.

Thank you for reviewing,

Maxime

> 
>        Andrew


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 04/10] net: sfp: Add helper to return the SFP bus name
  2023-11-21 10:20       ` Russell King (Oracle)
@ 2023-11-23 13:35         ` Maxime Chevallier
  -1 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-23 13:35 UTC (permalink / raw
  To: Russell King (Oracle)
  Cc: Andrew Lunn, davem, netdev, linux-kernel, thomas.petazzoni,
	Jakub Kicinski, Eric Dumazet, Paolo Abeni, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

Hi Andrew, Russell,

On Tue, 21 Nov 2023 10:20:43 +0000
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Tue, Nov 21, 2023 at 02:00:58AM +0100, Andrew Lunn wrote:
> > > +const char *sfp_get_name(struct sfp_bus *bus)
> > > +{
> > > +	if (bus->sfp_dev)
> > > +		return dev_name(bus->sfp_dev);
> > > +
> > > +	return NULL;
> > > +}  
> > 
> > Locking? Do you assume rtnl? Does this function need to take rtnl?  
> 
> Yes, rtnl needs to be held to safely access bus->sfp_dev, and that
> either needs to happen in this function, or be documented as being
> requried (and ASSERT_RTNL() added here.)
> 
> The reason is that sfp_dev is the SFP socket device which can be
> unbound via sfp_unregister_socket(), which will set bus->sfp_dev to
> NULL. This could race with the above.
> 

That's right, I'll add an assert and document it, thanks for spotting
this.

Maxime

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 04/10] net: sfp: Add helper to return the SFP bus name
@ 2023-11-23 13:35         ` Maxime Chevallier
  0 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-23 13:35 UTC (permalink / raw
  To: Russell King (Oracle)
  Cc: Andrew Lunn, davem, netdev, linux-kernel, thomas.petazzoni,
	Jakub Kicinski, Eric Dumazet, Paolo Abeni, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

Hi Andrew, Russell,

On Tue, 21 Nov 2023 10:20:43 +0000
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Tue, Nov 21, 2023 at 02:00:58AM +0100, Andrew Lunn wrote:
> > > +const char *sfp_get_name(struct sfp_bus *bus)
> > > +{
> > > +	if (bus->sfp_dev)
> > > +		return dev_name(bus->sfp_dev);
> > > +
> > > +	return NULL;
> > > +}  
> > 
> > Locking? Do you assume rtnl? Does this function need to take rtnl?  
> 
> Yes, rtnl needs to be held to safely access bus->sfp_dev, and that
> either needs to happen in this function, or be documented as being
> requried (and ASSERT_RTNL() added here.)
> 
> The reason is that sfp_dev is the SFP socket device which can be
> unbound via sfp_unregister_socket(), which will set bus->sfp_dev to
> NULL. This could race with the above.
> 

That's right, I'll add an assert and document it, thanks for spotting
this.

Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 05/10] net: ethtool: Allow passing a phy index for some commands
  2023-11-21  1:08     ` Andrew Lunn
@ 2023-11-23 13:36       ` Maxime Chevallier
  -1 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-23 13:36 UTC (permalink / raw
  To: Andrew Lunn
  Cc: davem, netdev, linux-kernel, thomas.petazzoni, Jakub Kicinski,
	Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

On Tue, 21 Nov 2023 02:08:37 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > +	if (dev) {
> > +		if (tb[ETHTOOL_A_HEADER_PHY_INDEX]) {
> > +			u32 phy_index = nla_get_u32(tb[ETHTOOL_A_HEADER_PHY_INDEX]);
> > +
> > +			phydev = link_topo_get_phy(&dev->link_topo, phy_index);  
> 
> struct phy_device *link_topo_get_phy(struct link_topology *lt, int phyindex)
> 
> We have u32 vs int here for phyindex. It would be good to have the
> same type everywhere.

Indeed I messed-up the typing for that variable, shame as it's the core
of that series :(

I'll get it right for the next version.

> 
> > +			if (!phydev) {
> > +				NL_SET_ERR_MSG_ATTR(extack, header, "no phy matches phy index");
> > +				return -EINVAL;
> > +			}
> > +		} else {
> > +			/* If we need a PHY but no phy index is specified, fallback
> > +			 * to dev->phydev
> > +			 */
> > +			phydev = dev->phydev;
> > +		}
> > +	}
> > +
> > +	req_info->phydev = phydev;  
> 
> Don't forget to update Documentation/networking/ethtool-netlink.rst.

Yep I'll squeeze the documentation bit here.

Thanks for the review,

Maxime

>       Andrew


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 05/10] net: ethtool: Allow passing a phy index for some commands
@ 2023-11-23 13:36       ` Maxime Chevallier
  0 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-23 13:36 UTC (permalink / raw
  To: Andrew Lunn
  Cc: davem, netdev, linux-kernel, thomas.petazzoni, Jakub Kicinski,
	Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

On Tue, 21 Nov 2023 02:08:37 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > +	if (dev) {
> > +		if (tb[ETHTOOL_A_HEADER_PHY_INDEX]) {
> > +			u32 phy_index = nla_get_u32(tb[ETHTOOL_A_HEADER_PHY_INDEX]);
> > +
> > +			phydev = link_topo_get_phy(&dev->link_topo, phy_index);  
> 
> struct phy_device *link_topo_get_phy(struct link_topology *lt, int phyindex)
> 
> We have u32 vs int here for phyindex. It would be good to have the
> same type everywhere.

Indeed I messed-up the typing for that variable, shame as it's the core
of that series :(

I'll get it right for the next version.

> 
> > +			if (!phydev) {
> > +				NL_SET_ERR_MSG_ATTR(extack, header, "no phy matches phy index");
> > +				return -EINVAL;
> > +			}
> > +		} else {
> > +			/* If we need a PHY but no phy index is specified, fallback
> > +			 * to dev->phydev
> > +			 */
> > +			phydev = dev->phydev;
> > +		}
> > +	}
> > +
> > +	req_info->phydev = phydev;  
> 
> Don't forget to update Documentation/networking/ethtool-netlink.rst.

Yep I'll squeeze the documentation bit here.

Thanks for the review,

Maxime

>       Andrew


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 06/10] net: ethtool: Introduce a command to list PHYs on an interface
  2023-11-21  1:40     ` Andrew Lunn
@ 2023-11-23 13:40       ` Maxime Chevallier
  -1 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-23 13:40 UTC (permalink / raw
  To: Andrew Lunn
  Cc: davem, netdev, linux-kernel, thomas.petazzoni, Jakub Kicinski,
	Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

On Tue, 21 Nov 2023 02:40:27 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > +ethnl_phy_reply_size(const struct ethnl_req_info *req_base,
> > +		     struct netlink_ext_ack *extack)
> > +{
> > +	struct phy_device_node *pdn;
> > +	struct phy_device *phydev;
> > +	struct link_topology *lt;
> > +	unsigned long index;
> > +	size_t size;
> > +
> > +	lt = &req_base->dev->link_topo;
> > +
> > +	size = nla_total_size(0);
> > +
> > +	xa_for_each(&lt->phys, index, pdn) {
> > +		phydev = pdn->phy;
> > +
> > +		/* ETHTOOL_A_PHY_INDEX */
> > +		size += nla_total_size(sizeof(u32));
> > +
> > +		/* ETHTOOL_A_DRVNAME */
> > +		size += nla_total_size(strlen(phydev->drv->name));
> > +
> > +		/* ETHTOOL_A_NAME */
> > +		size += nla_total_size(strlen(dev_name(&phydev->mdio.dev)));
> > +
> > +		/* ETHTOOL_A_PHY_UPSTREAM_TYPE */
> > +		size += nla_total_size(sizeof(u8));
> > +
> > +		/* ETHTOOL_A_PHY_ID */
> > +		size += nla_total_size(sizeof(u32));
> > +
> > +		if (phy_on_sfp(phydev)) {
> > +			/* ETHTOOL_A_PHY_UPSTREAM_SFP_NAME */
> > +			if (sfp_get_name(pdn->parent_sfp_bus))
> > +				size += nla_total_size(strlen(sfp_get_name(pdn->parent_sfp_bus)));  
> 
> Have you tried a modular build?

Now that you mention it, no. I did try with CONFIG_SFP disabled, but
not as a module, I'll add it to my test suite.

> 
> sfp_get_name() could be in a module, and then you will get linker
> errors. It is all a bit messy calling into phylib from ethtool :-(
> 
> This might actually be the only function you need? If so, its small
> enough you can move it into a header as a static inline function.

It's the only one indeed, so as add it as a header function then.

Thanks,

Maxime

>        Andrew


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 06/10] net: ethtool: Introduce a command to list PHYs on an interface
@ 2023-11-23 13:40       ` Maxime Chevallier
  0 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-23 13:40 UTC (permalink / raw
  To: Andrew Lunn
  Cc: davem, netdev, linux-kernel, thomas.petazzoni, Jakub Kicinski,
	Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

On Tue, 21 Nov 2023 02:40:27 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > +ethnl_phy_reply_size(const struct ethnl_req_info *req_base,
> > +		     struct netlink_ext_ack *extack)
> > +{
> > +	struct phy_device_node *pdn;
> > +	struct phy_device *phydev;
> > +	struct link_topology *lt;
> > +	unsigned long index;
> > +	size_t size;
> > +
> > +	lt = &req_base->dev->link_topo;
> > +
> > +	size = nla_total_size(0);
> > +
> > +	xa_for_each(&lt->phys, index, pdn) {
> > +		phydev = pdn->phy;
> > +
> > +		/* ETHTOOL_A_PHY_INDEX */
> > +		size += nla_total_size(sizeof(u32));
> > +
> > +		/* ETHTOOL_A_DRVNAME */
> > +		size += nla_total_size(strlen(phydev->drv->name));
> > +
> > +		/* ETHTOOL_A_NAME */
> > +		size += nla_total_size(strlen(dev_name(&phydev->mdio.dev)));
> > +
> > +		/* ETHTOOL_A_PHY_UPSTREAM_TYPE */
> > +		size += nla_total_size(sizeof(u8));
> > +
> > +		/* ETHTOOL_A_PHY_ID */
> > +		size += nla_total_size(sizeof(u32));
> > +
> > +		if (phy_on_sfp(phydev)) {
> > +			/* ETHTOOL_A_PHY_UPSTREAM_SFP_NAME */
> > +			if (sfp_get_name(pdn->parent_sfp_bus))
> > +				size += nla_total_size(strlen(sfp_get_name(pdn->parent_sfp_bus)));  
> 
> Have you tried a modular build?

Now that you mention it, no. I did try with CONFIG_SFP disabled, but
not as a module, I'll add it to my test suite.

> 
> sfp_get_name() could be in a module, and then you will get linker
> errors. It is all a bit messy calling into phylib from ethtool :-(
> 
> This might actually be the only function you need? If so, its small
> enough you can move it into a header as a static inline function.

It's the only one indeed, so as add it as a header function then.

Thanks,

Maxime

>        Andrew


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 01/10] net: phy: Introduce ethernet link topology representation
  2023-11-21  0:24     ` Andrew Lunn
@ 2023-11-23 13:44       ` Maxime Chevallier
  -1 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-23 13:44 UTC (permalink / raw
  To: Andrew Lunn
  Cc: davem, netdev, linux-kernel, thomas.petazzoni, Jakub Kicinski,
	Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

On Tue, 21 Nov 2023 01:24:47 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

[...]
> 
> > diff --git a/include/linux/link_topology.h b/include/linux/link_topology.h  
> 
> I think this filename is too generic. Maybe phy_link_topology.h, or
> move it into include/net.

Yeah naming again, phy_link_topology would make sense indeed. I know
that Florian suggested phy_devices_list last time, However I'd like
this to be more than just about PHYs, to keep track of ports, mii-muxes
and such. So, phy_link_topology sounds good to me :)

Maxime

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH net-next v2 01/10] net: phy: Introduce ethernet link topology representation
@ 2023-11-23 13:44       ` Maxime Chevallier
  0 siblings, 0 replies; 53+ messages in thread
From: Maxime Chevallier @ 2023-11-23 13:44 UTC (permalink / raw
  To: Andrew Lunn
  Cc: davem, netdev, linux-kernel, thomas.petazzoni, Jakub Kicinski,
	Eric Dumazet, Paolo Abeni, Russell King, linux-arm-kernel,
	Christophe Leroy, Herve Codina, Florian Fainelli, Heiner Kallweit,
	Vladimir Oltean, Köry Maincent, Jesse Brandeburg

On Tue, 21 Nov 2023 01:24:47 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

[...]
> 
> > diff --git a/include/linux/link_topology.h b/include/linux/link_topology.h  
> 
> I think this filename is too generic. Maybe phy_link_topology.h, or
> move it into include/net.

Yeah naming again, phy_link_topology would make sense indeed. I know
that Florian suggested phy_devices_list last time, However I'd like
this to be more than just about PHYs, to keep track of ports, mii-muxes
and such. So, phy_link_topology sounds good to me :)

Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 53+ messages in thread

end of thread, other threads:[~2023-11-23 13:45 UTC | newest]

Thread overview: 53+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-17 16:23 [RFC PATCH net-next v2 00/10] Introduce PHY listing and link_topology tracking Maxime Chevallier
2023-11-17 16:23 ` Maxime Chevallier
2023-11-17 16:23 ` [RFC PATCH net-next v2 01/10] net: phy: Introduce ethernet link topology representation Maxime Chevallier
2023-11-17 16:23   ` Maxime Chevallier
2023-11-21  0:24   ` Andrew Lunn
2023-11-21  0:24     ` Andrew Lunn
2023-11-23 13:34     ` Maxime Chevallier
2023-11-23 13:34       ` Maxime Chevallier
2023-11-23 13:44     ` Maxime Chevallier
2023-11-23 13:44       ` Maxime Chevallier
2023-11-17 16:23 ` [RFC PATCH net-next v2 02/10] net: sfp: pass the phy_device when disconnecting an sfp module's PHY Maxime Chevallier
2023-11-17 16:23   ` Maxime Chevallier
2023-11-17 16:23 ` [RFC PATCH net-next v2 03/10] net: phy: add helpers to handle sfp phy connect/disconnect Maxime Chevallier
2023-11-17 16:23   ` Maxime Chevallier
2023-11-21  0:57   ` Andrew Lunn
2023-11-21  0:57     ` Andrew Lunn
2023-11-21 10:08     ` Russell King (Oracle)
2023-11-21 10:08       ` Russell King (Oracle)
2023-11-21 14:35       ` Andrew Lunn
2023-11-21 14:35         ` Andrew Lunn
2023-11-17 16:23 ` [RFC PATCH net-next v2 04/10] net: sfp: Add helper to return the SFP bus name Maxime Chevallier
2023-11-17 16:23   ` Maxime Chevallier
2023-11-18  4:44   ` kernel test robot
2023-11-21  1:00   ` Andrew Lunn
2023-11-21  1:00     ` Andrew Lunn
2023-11-21 10:20     ` Russell King (Oracle)
2023-11-21 10:20       ` Russell King (Oracle)
2023-11-23 13:35       ` Maxime Chevallier
2023-11-23 13:35         ` Maxime Chevallier
2023-11-17 16:23 ` [RFC PATCH net-next v2 05/10] net: ethtool: Allow passing a phy index for some commands Maxime Chevallier
2023-11-17 16:23   ` Maxime Chevallier
2023-11-21  1:08   ` Andrew Lunn
2023-11-21  1:08     ` Andrew Lunn
2023-11-23 13:36     ` Maxime Chevallier
2023-11-23 13:36       ` Maxime Chevallier
2023-11-17 16:23 ` [RFC PATCH net-next v2 06/10] net: ethtool: Introduce a command to list PHYs on an interface Maxime Chevallier
2023-11-17 16:23   ` Maxime Chevallier
2023-11-18  4:44   ` kernel test robot
2023-11-18  5:34   ` kernel test robot
2023-11-21  1:34   ` Andrew Lunn
2023-11-21  1:34     ` Andrew Lunn
2023-11-21  1:40   ` Andrew Lunn
2023-11-21  1:40     ` Andrew Lunn
2023-11-23 13:40     ` Maxime Chevallier
2023-11-23 13:40       ` Maxime Chevallier
2023-11-17 16:23 ` [RFC PATCH net-next v2 07/10] net: ethtool: plca: Target the command to the requested PHY Maxime Chevallier
2023-11-17 16:23   ` Maxime Chevallier
2023-11-17 16:23 ` [RFC PATCH net-next v2 08/10] net: ethtool: pse-pd: " Maxime Chevallier
2023-11-17 16:23   ` Maxime Chevallier
2023-11-17 16:23 ` [RFC PATCH net-next v2 09/10] net: ethtool: cable-test: " Maxime Chevallier
2023-11-17 16:23   ` Maxime Chevallier
2023-11-17 16:23 ` [RFC PATCH net-next v2 10/10] net: ethtool: strset: Allow querying phy stats by index Maxime Chevallier
2023-11-17 16:23   ` Maxime Chevallier

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.