diff mbox series

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

Message ID 20230907092407.647139-5-maxime.chevallier@bootlin.com (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series net: phy: introduce phy numbering | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 1336 this patch: 21
netdev/cc_maintainers success CCed 9 of 9 maintainers
netdev/build_clang fail Errors and warnings before: 1353 this patch: 17
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 1359 this patch: 21
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? WARNING: line length of 83 exceeds 80 columns WARNING: line length of 84 exceeds 80 columns WARNING: line length of 88 exceeds 80 columns WARNING: line length of 89 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Maxime Chevallier Sept. 7, 2023, 9:24 a.m. UTC 
Introduce a new netlink message that lists all PHYs on a given interface
at a given time.

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

Comments

Russell King (Oracle) Sept. 7, 2023, 10 a.m. UTC | #1 
On Thu, Sep 07, 2023 at 11:24:02AM +0200, Maxime Chevallier wrote:
> +#define PHY_MAX_ENTRIES	16
> +
> +struct phy_list_reply_data {
> +	struct ethnl_reply_data		base;
> +	u8 n_phys;
> +	u32 phy_indices[PHY_MAX_ENTRIES];

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

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

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

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

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

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

      Andrew
Andrew Lunn Sept. 12, 2023, 4:29 p.m. UTC | #3 
> +static int phy_list_fill_reply(struct sk_buff *skb,
> +			       const struct ethnl_req_info *req_base,
> +			       const struct ethnl_reply_data *reply_base)
> +{
> +	const struct phy_list_reply_data *data = PHY_LIST_REPDATA(reply_base);
> +
> +	if (nla_put_u8(skb, ETHTOOL_A_PHY_LIST_COUNT, data->n_phys))
> +		return -EMSGSIZE;
> +
> +	if (!data->n_phys)
> +		return 0;
> +
> +	if (nla_put(skb, ETHTOOL_A_PHY_LIST_INDEX, sizeof(u32) * data->n_phys,
> +		    data->phy_indices))
> +		return -EMSGSIZE;
> +

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

	Andrew
diff mbox series

Patch

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