diff mbox series

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

Message ID 20230907092407.647139-7-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: 1875 this patch: 1878
netdev/cc_maintainers warning 1 maintainers not CCed: mailhol.vincent@wanadoo.fr
netdev/build_clang fail Errors and warnings before: 473 this patch: 473
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: 2003 this patch: 1987
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? WARNING: line length of 81 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
Now that we can list PHYs belonging to a netdevice, add a command to get
PHY-specific information, that we can extend as needed to get more data
such as link info, offloading support, etc.

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

Comments

Russell King (Oracle) Sept. 7, 2023, 10:04 a.m. UTC | #1
On Thu, Sep 07, 2023 at 11:24:04AM +0200, Maxime Chevallier wrote:
> +	data->phyindex = req_info->phyindex;
> +	data->drvname = phydev->drv->name;
> +	if (phydev->is_on_sfp_module)

Please use the accessor provided:

	if (phy_on_sfp(phydev))
Jakub Kicinski Sept. 8, 2023, 3:42 p.m. UTC | #2
On Thu,  7 Sep 2023 11:24:04 +0200 Maxime Chevallier wrote:
> +enum phy_upstream_type {
> +	PHY_UPSTREAM_MAC,
> +	PHY_UPSTREAM_SFP,
> +	PHY_UPSTREAM_PHY,
> +};

Would be good to define the enum in the YAML spec, too.
That way YNL users can auto-magically see strings for the values.
Jakub Kicinski Sept. 8, 2023, 3:46 p.m. UTC | #3
On Thu,  7 Sep 2023 11:24:04 +0200 Maxime Chevallier wrote:
>  	ETHTOOL_MSG_PHY_LIST_GET,
> +	ETHTOOL_MSG_PHY_GET,

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

The dump can accept filtering arguments, like ifindex, if you want 
to narrow down the results, that's perfectly fine (you may need to
give up some of the built-in ethtool scaffolding, but it shouldn't 
be all that bad).
Maxime Chevallier Sept. 14, 2023, 9:36 a.m. UTC | #4
Hello Jakub,

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

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

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

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

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

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

Thanks,

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

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

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

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

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

    Andrew
diff mbox series

Patch

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