[net-next,v13,05/13] net: ethtool: Allow passing a phy index for some commands

Commit Message

Maxime Chevallier June 7, 2024, 7:18 a.m. UTC

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 <[e-mail]>
Reviewed-by: Andrew Lunn <[e-mail]>
---
 Documentation/networking/ethtool-netlink.rst |  7 +++
 include/uapi/linux/ethtool_netlink.h         |  1 +
 net/ethtool/netlink.c                        | 48 +++++++++++++++++++-
 net/ethtool/netlink.h                        |  5 ++
 4 files changed, 59 insertions(+), 2 deletions(-)

Comments

Jakub Kicinski June 14, 2024, 1:26 a.m. UTC | #1

On Fri,  7 Jun 2024 09:18:18 +0200 Maxime Chevallier wrote:
> +		if (tb[ETHTOOL_A_HEADER_PHY_INDEX]) {
> +			struct nlattr *phy_id;
> +
> +			phy_id = tb[ETHTOOL_A_HEADER_PHY_INDEX];
> +			phydev = phy_link_topo_get_phy(dev,
> +						       nla_get_u32(phy_id));

Sorry for potentially repeating question (please put the answer in the
commit message) - are phys guaranteed not to disappear, even if the
netdev gets closed? this has no rtnl protection

> +			if (!phydev) {
> +				NL_SET_BAD_ATTR(extack, phy_id);
> +				return -ENODEV;
> +			}
> +		} else {
> +			/* If we need a PHY but no phy index is specified, fallback
> +			 * to dev->phydev

please double check the submission for going over 80 chars, this one
appears to be particularly pointlessly over 80 chars...

> +			 */
> +			phydev = dev->phydev;

Andrew Lunn June 16, 2024, 3:21 p.m. UTC | #2

On Sun, Jun 16, 2024 at 06:02:31PM +0200, Maxime Chevallier wrote:
> Hello Jakub,
> 
> On Thu, 13 Jun 2024 18:26:13 -0700
> Jakub Kicinski <[e-mail]> wrote:
> 
> > On Fri,  7 Jun 2024 09:18:18 +0200 Maxime Chevallier wrote:
> > > +		if (tb[ETHTOOL_A_HEADER_PHY_INDEX]) {
> > > +			struct nlattr *phy_id;
> > > +
> > > +			phy_id = tb[ETHTOOL_A_HEADER_PHY_INDEX];
> > > +			phydev = phy_link_topo_get_phy(dev,
> > > +						       nla_get_u32(phy_id));  
> > 
> > Sorry for potentially repeating question (please put the answer in the
> > commit message) - are phys guaranteed not to disappear, even if the
> > netdev gets closed? this has no rtnl protection
> 
> I'll answer here so that people can correct me if I'm wrong, but I'll
> also add it in the commit logs as well (and possibly with some fixes
> depending on how this discussion goes)
> 
> While a PHY can be attached to/detached from a netdevice at open/close,
> the phy_device itself will keep on living, as its lifetime is tied to
> the underlying mdio_device (however phy_attach/detach take a ref on the
> phy_device, preventing it from vanishing while it's attached to a
> netdev)

It gets interesting with copper SFP. They contain a PHY, and that PHY
can physically disappear at any time. What i don't know is when the
logical representation of the PHY will disappear after the hotunplug
event.

	Andrew

Maxime Chevallier June 16, 2024, 4:02 p.m. UTC | #3

Hello Jakub,

On Thu, 13 Jun 2024 18:26:13 -0700
Jakub Kicinski <[e-mail]> wrote:

> On Fri,  7 Jun 2024 09:18:18 +0200 Maxime Chevallier wrote:
> > +		if (tb[ETHTOOL_A_HEADER_PHY_INDEX]) {
> > +			struct nlattr *phy_id;
> > +
> > +			phy_id = tb[ETHTOOL_A_HEADER_PHY_INDEX];
> > +			phydev = phy_link_topo_get_phy(dev,
> > +						       nla_get_u32(phy_id));  
> 
> Sorry for potentially repeating question (please put the answer in the
> commit message) - are phys guaranteed not to disappear, even if the
> netdev gets closed? this has no rtnl protection

I'll answer here so that people can correct me if I'm wrong, but I'll
also add it in the commit logs as well (and possibly with some fixes
depending on how this discussion goes)

While a PHY can be attached to/detached from a netdevice at open/close,
the phy_device itself will keep on living, as its lifetime is tied to
the underlying mdio_device (however phy_attach/detach take a ref on the
phy_device, preventing it from vanishing while it's attached to a
netdev)

I think the worst that could happen is that phy_detach() gets
called (at ndo_close() for example, but that's not the only possible
call site for that), and right after we manually unbind the PHY, which
will drop its last refcount, while we hold a pointer to it :

			phydev = phy_link_topo_get_phy()
 phy_detach(phydev)
 unbind on phydev
			/* access phydev */
			
PHY device lifetime is, from my understanding, not protected by
rtnl() so should a lock be added, I don't think rtnl_lock() would be
the one to use.

Maybe instead we should grab a reference to the phydev when we add it
to the topology ?

> 
> > +			if (!phydev) {
> > +				NL_SET_BAD_ATTR(extack, phy_id);
> > +				return -ENODEV;
> > +			}
> > +		} else {
> > +			/* If we need a PHY but no phy index is specified, fallback
> > +			 * to dev->phydev  
> 
> please double check the submission for going over 80 chars, this one
> appears to be particularly pointlessly over 80 chars...

Arg yes sorry about this one...
 
> > +			 */
> > +			phydev = dev->phydev;  

Thanks,

Maxime

Russell King (Oracle) June 16, 2024, 4:07 p.m. UTC | #4

On Sun, Jun 16, 2024 at 05:21:25PM +0200, Andrew Lunn wrote:
> On Sun, Jun 16, 2024 at 06:02:31PM +0200, Maxime Chevallier wrote:
> > Hello Jakub,
> > 
> > On Thu, 13 Jun 2024 18:26:13 -0700
> > Jakub Kicinski <[e-mail]> wrote:
> > 
> > > On Fri,  7 Jun 2024 09:18:18 +0200 Maxime Chevallier wrote:
> > > > +		if (tb[ETHTOOL_A_HEADER_PHY_INDEX]) {
> > > > +			struct nlattr *phy_id;
> > > > +
> > > > +			phy_id = tb[ETHTOOL_A_HEADER_PHY_INDEX];
> > > > +			phydev = phy_link_topo_get_phy(dev,
> > > > +						       nla_get_u32(phy_id));  
> > > 
> > > Sorry for potentially repeating question (please put the answer in the
> > > commit message) - are phys guaranteed not to disappear, even if the
> > > netdev gets closed? this has no rtnl protection
> > 
> > I'll answer here so that people can correct me if I'm wrong, but I'll
> > also add it in the commit logs as well (and possibly with some fixes
> > depending on how this discussion goes)
> > 
> > While a PHY can be attached to/detached from a netdevice at open/close,
> > the phy_device itself will keep on living, as its lifetime is tied to
> > the underlying mdio_device (however phy_attach/detach take a ref on the
> > phy_device, preventing it from vanishing while it's attached to a
> > netdev)
> 
> It gets interesting with copper SFP. They contain a PHY, and that PHY
> can physically disappear at any time. What i don't know is when the
> logical representation of the PHY will disappear after the hotunplug
> event.

On a SFP module unplug, the following upstream device methods will be
called in order:
1. link_down
2. module_stop
3. disconnect_phy

At this point, the PHY device will be removed (phy_device_remove()) and
freed (phy_device_free()), and shortly thereafter, the MDIO bus is
unregistered and thus destroyed.

In response to the above, phylink will, respectively for each method:

1. disable the netdev carrier and call mac_link_down()
2. call phy_stop() on the attached PHY
3. remove the PHY from phylink, and then call phy_disconnect(),
   disconnecting it from the netdev.

Thus, when a SFP PHY is being removed, phylib will see in order the
following calls:

	phy_disconnect()
	phy_device_remove()
	phy_device_free()

Provided the topology linkage is removed on phy_disconnect() which
disassociates the PHY from the netdev, SFP PHYs should be fine.

Russell King (Oracle) June 16, 2024, 4:15 p.m. UTC | #5

On Sun, Jun 16, 2024 at 06:02:31PM +0200, Maxime Chevallier wrote:
> Hello Jakub,
> 
> On Thu, 13 Jun 2024 18:26:13 -0700
> Jakub Kicinski <[e-mail]> wrote:
> 
> > On Fri,  7 Jun 2024 09:18:18 +0200 Maxime Chevallier wrote:
> > > +		if (tb[ETHTOOL_A_HEADER_PHY_INDEX]) {
> > > +			struct nlattr *phy_id;
> > > +
> > > +			phy_id = tb[ETHTOOL_A_HEADER_PHY_INDEX];
> > > +			phydev = phy_link_topo_get_phy(dev,
> > > +						       nla_get_u32(phy_id));  
> > 
> > Sorry for potentially repeating question (please put the answer in the
> > commit message) - are phys guaranteed not to disappear, even if the
> > netdev gets closed? this has no rtnl protection
> 
> I'll answer here so that people can correct me if I'm wrong, but I'll
> also add it in the commit logs as well (and possibly with some fixes
> depending on how this discussion goes)
> 
> While a PHY can be attached to/detached from a netdevice at open/close,
> the phy_device itself will keep on living, as its lifetime is tied to
> the underlying mdio_device (however phy_attach/detach take a ref on the
> phy_device, preventing it from vanishing while it's attached to a
> netdev)
> 
> I think the worst that could happen is that phy_detach() gets
> called (at ndo_close() for example, but that's not the only possible
> call site for that), and right after we manually unbind the PHY, which
> will drop its last refcount, while we hold a pointer to it :
> 
> 			phydev = phy_link_topo_get_phy()
>  phy_detach(phydev)
>  unbind on phydev
> 			/* access phydev */
> 			
> PHY device lifetime is, from my understanding, not protected by
> rtnl() so should a lock be added, I don't think rtnl_lock() would be
> the one to use.

... and that will cause deadlocks. For example, ethernet drivers can
call phy_disconnect() from their .ndo_close method, which will be
called with the RTNL lock held. This calls phy_detach(), so
phy_detach() also gets called while the RTNL lock is held.

SFP will call all phylib methods while holding the RTNL lock as well
(because that's the only safe way to add or remove a PHY, as it stops
other changes to the config that may conflict, and also ensures that
e.g. paths in phylib will not be in-use when the PHY is being
destroyed.)

So, rather than thinking that phylib should add RTNL locking, it
would be much more sensible to do what phylink does, and enforce
that the RTNL will be held when netdev related methods are called,
but also require that paths that end up changing phylib's configuration
(e.g. removing a PHY driver) end up taking the RTNL lock - because
that is the only way to be sure that none of the phylib methods
that call into the driver are currently executing in another thread.

Maxime Chevallier June 16, 2024, 9:31 p.m. UTC | #6

Hi Russell,

> > It gets interesting with copper SFP. They contain a PHY, and that PHY
> > can physically disappear at any time. What i don't know is when the
> > logical representation of the PHY will disappear after the hotunplug
> > event.

I'm replying to your mail to summarize what the topology code does,
which I think is correct according to these explanations.

> 
> On a SFP module unplug, the following upstream device methods will be
> called in order:
> 1. link_down
> 2. module_stop
> 3. disconnect_phy

Patch 03 adds a phy_sfp_connect_phy() / phy_sfp_disconnect_phy() set of
helpers that new PHY drivers supporting downstream SFP busses must
specify in their sfp_upstream_ops, which will add/remove the SFP phy
to/from the topology. I realize now that I need to add some clear
documentation so that new drivers get that right.

That is because in this situation, the SFP PHY won't be attached to the
netdev (as the media-converter PHY already is), so relying on the
phy_attach / phy_detach paths won't cover these cases.

> 
> At this point, the PHY device will be removed (phy_device_remove()) and
> freed (phy_device_free()), and shortly thereafter, the MDIO bus is
> unregistered and thus destroyed.
> 
> In response to the above, phylink will, respectively for each method:
> 
> 1. disable the netdev carrier and call mac_link_down()
> 2. call phy_stop() on the attached PHY
> 3. remove the PHY from phylink, and then call phy_disconnect(),
>    disconnecting it from the netdev.
> 
> Thus, when a SFP PHY is being removed, phylib will see in order the
> following calls:
> 
> 	phy_disconnect()
> 	phy_device_remove()
> 	phy_device_free()
> 
> Provided the topology linkage is removed on phy_disconnect() which
> disassociates the PHY from the netdev, SFP PHYs should be fine.
> 

And here in that case, there's a hook in phy_detach() to remove the phy
from the topology as well, which deals with the case where phylink
deals with the sfp_upstream_ops. I think this covers all cases :)

Thanks,

Maxime

Maxime Chevallier June 23, 2024, 1:21 a.m. UTC | #7

Hello Jakub, Andrew, Russell,

On Thu, 13 Jun 2024 18:26:13 -0700
Jakub Kicinski <[e-mail]> wrote:

> On Fri,  7 Jun 2024 09:18:18 +0200 Maxime Chevallier wrote:
> > +		if (tb[ETHTOOL_A_HEADER_PHY_INDEX]) {
> > +			struct nlattr *phy_id;
> > +
> > +			phy_id = tb[ETHTOOL_A_HEADER_PHY_INDEX];
> > +			phydev = phy_link_topo_get_phy(dev,
> > +						       nla_get_u32(phy_id));  
> 
> Sorry for potentially repeating question (please put the answer in the
> commit message) - are phys guaranteed not to disappear, even if the
> netdev gets closed? this has no rtnl protection

After scratching my head maybe a bit too hard and re-reading the
replies from Andrew and Russell, I think there's indeed a problem. The
SFP case as described by Russell, from my understanding, leads me to
believe that the way PHY's are tracked by phy_link_topology is correct,
but that doesn't mean that what I try do to in this exact patch is
right.

After the phydev has been retrieved from the topology and stored in the
req_info, nothing guarantees that the PHY won't vanish between the
moment we get it here and the moment we use it in the ethnl command
handling (SFP removal being a good example, and probably(?) the only
problematic case).

A solution would be, as Russell says, to make sure we get the PHY and
do whatever we need to do with it with rtnl held. Fortunately that
shouldn't require significant rework of individual netlink commands
that use the phydev, as they already manipulate it while holding rtnl().

So, I'll ditch this idea of storing the phydev pointer in
the req_info, I'll just store the phy_index (if it was passed by user)
and grab the phy whenever we need to.

Let me know if you find some flaw in my analysis, and thanks for
spotting this.

Best regards,

Maxime

Patch

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 160bfb0ae8ba..8bc71f249448 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`` identifies the Ethernet PHY the message relates to.
+As there are numerous commands that are related to PHY configuration, and because
+there may be 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.
 
 Bit sets
 ========
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index b49b804b9495..f17dbe54bf5e 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -132,6 +132,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 bd04f28d5cf4..f5b4adf324bc 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/phy_link_topology.h>
 
 static struct genl_family ethtool_genl_family;
 
@@ -30,6 +31,24 @@  const struct nla_policy ethnl_header_policy_stats[] = {
 							  ETHTOOL_FLAGS_STATS),
 };
 
+const struct nla_policy ethnl_header_policy_phy[] = {
+	[ETHTOOL_A_HEADER_DEV_INDEX]	= { .type = NLA_U32 },
+	[ETHTOOL_A_HEADER_DEV_NAME]	= { .type = NLA_NUL_STRING,
+					    .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_phy_stats[] = {
+	[ETHTOOL_A_HEADER_DEV_INDEX]	= { .type = NLA_U32 },
+	[ETHTOOL_A_HEADER_DEV_NAME]	= { .type = NLA_NUL_STRING,
+					    .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)
 {
 	int ret;
@@ -89,8 +108,9 @@  int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info,
 			       const struct nlattr *header, struct net *net,
 			       struct netlink_ext_ack *extack, bool require_dev)
 {
-	struct nlattr *tb[ARRAY_SIZE(ethnl_header_policy)];
+	struct nlattr *tb[ARRAY_SIZE(ethnl_header_policy_phy)];
 	const struct nlattr *devname_attr;
+	struct phy_device *phydev = NULL;
 	struct net_device *dev = NULL;
 	u32 flags = 0;
 	int ret;
@@ -104,7 +124,7 @@  int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info,
 	/* No validation here, command policy should have a nested policy set
 	 * for the header, therefore validation should have already been done.
 	 */
-	ret = nla_parse_nested(tb, ARRAY_SIZE(ethnl_header_policy) - 1, header,
+	ret = nla_parse_nested(tb, ARRAY_SIZE(ethnl_header_policy_phy) - 1, header,
 			       NULL, extack);
 	if (ret < 0)
 		return ret;
@@ -145,6 +165,30 @@  int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info,
 		return -EINVAL;
 	}
 
+	if (dev) {
+		if (tb[ETHTOOL_A_HEADER_PHY_INDEX]) {
+			struct nlattr *phy_id;
+
+			phy_id = tb[ETHTOOL_A_HEADER_PHY_INDEX];
+			phydev = phy_link_topo_get_phy(dev,
+						       nla_get_u32(phy_id));
+			if (!phydev) {
+				NL_SET_BAD_ATTR(extack, phy_id);
+				return -ENODEV;
+			}
+		} else {
+			/* If we need a PHY but no phy index is specified, fallback
+			 * to dev->phydev
+			 */
+			phydev = dev->phydev;
+		}
+	} else if (tb[ETHTOOL_A_HEADER_PHY_INDEX]) {
+		NL_SET_ERR_MSG_ATTR(extack, header,
+				    "can't target a PHY without a netdev");
+		return -EINVAL;
+	}
+
+	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..d57a890b5d9e 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,12 @@  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_phy[ETHTOOL_A_HEADER_PHY_INDEX + 1];
+extern const struct nla_policy ethnl_header_policy_phy_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];