[RFC PATCH net-next 0/3] RTNL: Add link-down reason reporting

[RFC PATCH net-next 0/3] RTNL: Add link-down reason reporting

[Petr Machata]

In general, after a port is put administratively up, certain handshake
protocols have to finish successfully, otherwise the port is left in a
NO-CARRIER or DORMANT state. When that happens, it would be useful to
communicate the reasons to the administrator, so that the problem that
prevents the link from being established can be corrected.

This patch set adds two new RTNL attributes, IFLA_LINK_DOWN_REASON_MAJOR
and _MINOR, to carry the information. Major reason codes are drawn from
a well-known enum that is part of the kernel interface. They serve as
broad categories intended to convey a general idea of where the problem
is. Minor codes are arbitrary numbers specific for the driver in
question that add detail to the major reasons.

The hope is that an average user will not need to dive into the minor
reason codes. It is for example largely immaterial what it is that makes
any given cable unsupported, because the administrator will just take
another cable anyway. The minor code may still be provided though, for
the cases where further information is actually necessary.

The party with visibility into details of this process is the driver.
Therefore add two new RTNL hooks, link_down_reason_get_size and
fill_link_down_reason, to provide the necessary information.

Link-down reason is not included if the port is up or administratively
down, because those two state are easy to discover through existing
interfaces. The new interface is intended for debugging of the
transition between these two states.

This is all in patch #1. Patches #2 and #3 add implementation of the new
interfaces for mlxsw.

A preliminary iproute patch that implements display of the new
attributes is available here:

    https://github.com/pmachata/iproute2/tree/link_down_reason

And this is an example output:

    # ip -d link show dev sw1p1
    393: sw1p1: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 1000
        link/ether 7c:fe:90:f5:a3:7d brd ff:ff:ff:ff:ff:ff promiscuity 0 minmtu 0 maxmtu 65535
        mlxsw addrgenmode eui64 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 \
        portname p1 switchid 7cfe90f5a340 down_reason NO_CABLE 1024

Petr Machata (3):
  net: rtnetlink: Add link-down reason to RTNL messages
  mlxsw: reg: Add Port Diagnostics Database Register
  mlxsw: spectrum: Add rtnl_link_ops

 drivers/net/ethernet/mellanox/mlxsw/reg.h      |  54 +++++++++++++
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 108 +++++++++++++++++++++++++
 include/net/rtnetlink.h                        |   3 +
 include/uapi/linux/if_link.h                   |  16 ++++
 net/core/rtnetlink.c                           |  22 +++++
 5 files changed, 203 insertions(+)

-- 
2.4.11

[RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

[Petr Machata]

In general, after a port is put administratively up, certain handshake
protocols have to finish successfully, otherwise the port is left in a
NO-CARRIER or DORMANT state. When that happens, it would be useful to
communicate the reasons to the administrator, so that the problem that
prevents the link from being established can be corrected.

Introduce two new link attributes: IFLA_LINK_DOWN_REASON_MAJOR and
_MINOR. Major reason code serve as broad categories intended to convey a
general idea of where the problem is. Minor codes are arbitrary numbers
specific for the driver that add detail to the major reasons. Add enum
rtnl_link_down_reason_major to define the well-known major reason codes.

The party with visibility into details of this process is the driver.
Therefore add two new RTNL hooks, link_down_reason_get_size and
fill_link_down_reason, to provide the necessary information.

Link-down reason is not included if the port is up or administratively
down, because those two state are easy to discover through existing
interfaces.

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 include/net/rtnetlink.h      |  3 +++
 include/uapi/linux/if_link.h | 16 ++++++++++++++++
 net/core/rtnetlink.c         | 22 ++++++++++++++++++++++
 3 files changed, 41 insertions(+)

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index e2091bb2b3a8..cfd9e86ff0ca 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -110,6 +110,9 @@ struct rtnl_link_ops {
 	int			(*fill_linkxstats)(struct sk_buff *skb,
 						   const struct net_device *dev,
 						   int *prividx, int attr);
+	size_t			(*link_down_reason_get_size)(const struct net_device *dev);
+	int			(*fill_link_down_reason)(struct sk_buff *skb,
+							 const struct net_device *dev);
 };
 
 int __rtnl_link_register(struct rtnl_link_ops *ops);
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 5b225ff63b48..a47f42e79741 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -167,6 +167,8 @@ enum {
 	IFLA_NEW_IFINDEX,
 	IFLA_MIN_MTU,
 	IFLA_MAX_MTU,
+	IFLA_LINK_DOWN_REASON_MAJOR,  /* enum rtnl_link_down_reason_major */
+	IFLA_LINK_DOWN_REASON_MINOR,
 	__IFLA_MAX
 };
 
@@ -239,6 +241,20 @@ enum in6_addr_gen_mode {
 	IN6_ADDR_GEN_MODE_RANDOM,
 };
 
+enum rtnl_link_down_reason_major {
+	RTNL_LDR_OTHER,
+	RTNL_LDR_NO_CABLE,
+	RTNL_LDR_UNSUPPORTED_CABLE,
+	RTNL_LDR_AUTONEG_FAILURE,
+	RTNL_LDR_NO_LINK_PARTNER,
+	RTNL_LDR_LINK_TRAINING_FAILURE,
+	RTNL_LDR_LOGICAL_MISMATCH,
+	RTNL_LDR_REMOTE_FAULT,
+	RTNL_LDR_BAD_SIGNAL_INTEGRITY,
+	RTNL_LDR_CALIBRATION_FAILURE,
+	RTNL_LDR_POWER_BUDGET_EXCEEDED,
+};
+
 /* Bridge section */
 
 enum {
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a51cab95ba64..206795f13850 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -979,6 +979,22 @@ static size_t rtnl_xdp_size(void)
 	return xdp_size;
 }
 
+static bool rtnl_should_fill_link_down_reason(const struct net_device *dev)
+{
+	return (dev->flags & IFF_UP) && !netif_oper_up(dev) &&
+		dev->rtnl_link_ops &&
+		dev->rtnl_link_ops->link_down_reason_get_size &&
+		dev->rtnl_link_ops->fill_link_down_reason;
+}
+
+static size_t rtnl_link_down_reason_get_size(const struct net_device *dev)
+{
+	if (!rtnl_should_fill_link_down_reason(dev))
+		return 0;
+
+	return dev->rtnl_link_ops->link_down_reason_get_size(dev);
+}
+
 static noinline size_t if_nlmsg_size(const struct net_device *dev,
 				     u32 ext_filter_mask)
 {
@@ -1026,6 +1042,7 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev,
 	       + nla_total_size(4)  /* IFLA_CARRIER_DOWN_COUNT */
 	       + nla_total_size(4)  /* IFLA_MIN_MTU */
 	       + nla_total_size(4)  /* IFLA_MAX_MTU */
+	       + rtnl_link_down_reason_get_size(dev)
 	       + 0;
 }
 
@@ -1683,6 +1700,9 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
 	    nla_put_s32(skb, IFLA_NEW_IFINDEX, new_ifindex) < 0)
 		goto nla_put_failure;
 
+	if (rtnl_should_fill_link_down_reason(dev) &&
+	    dev->rtnl_link_ops->fill_link_down_reason(skb, dev))
+		goto nla_put_failure;
 
 	rcu_read_lock();
 	if (rtnl_fill_link_af(skb, dev, ext_filter_mask))
@@ -1742,6 +1762,8 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
 	[IFLA_CARRIER_DOWN_COUNT] = { .type = NLA_U32 },
 	[IFLA_MIN_MTU]		= { .type = NLA_U32 },
 	[IFLA_MAX_MTU]		= { .type = NLA_U32 },
+	[IFLA_LINK_DOWN_REASON_MAJOR] = { .type = NLA_U32 },
+	[IFLA_LINK_DOWN_REASON_MINOR] = { .type = NLA_U32 },
 };
 
 static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
-- 
2.4.11

[RFC PATCH net-next 2/3] mlxsw: reg: Add Port Diagnostics Database Register

[Petr Machata]

The PDDR register enables to read the Phy debug database.

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/reg.h | 54 +++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/reg.h b/drivers/net/ethernet/mellanox/mlxsw/reg.h
index eb4c5e8964cd..fd85d5bd982c 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/reg.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/reg.h
@@ -5210,6 +5210,59 @@ static inline void mlxsw_reg_pspa_pack(char *payload, u8 swid, u8 local_port)
 	mlxsw_reg_pspa_sub_port_set(payload, 0);
 }
 
+/* PDDR - Port Diagnostics Database Register Layout
+ * ------------------------------------------------
+ * This register enables to read the Phy debug database.
+ */
+#define MLXSW_REG_PDDR_ID 0x5031
+#define MLXSW_REG_PDDR_LEN 0x100
+
+MLXSW_REG_DEFINE(pddr, MLXSW_REG_PDDR_ID, MLXSW_REG_PDDR_LEN);
+
+/* reg_pddr_local_port
+ * Local port number.
+ * Access: Index
+ */
+MLXSW_ITEM32(reg, pddr, local_port, 0x00, 16, 8);
+
+enum {
+	MLXSW_REG_PDDR_PAGESEL_TROUBLESHOOTING_INFO = 1,
+};
+
+/* reg_pddr_page_select
+ * Page select index.
+ * Access: Index
+ */
+MLXSW_ITEM32(reg, pddr, page_select, 0x04, 0, 8);
+
+/* Troubleshooting Info Page
+ * - - - - - - - - - - - - -
+ */
+
+enum {
+	MLXSW_REG_PDDR_TRBLSH_GROUP_MONITOR = 0,
+};
+
+/* reg_pddr_group_opcode
+ * Group selector.
+ * Access: Index
+ */
+MLXSW_ITEM32(reg, pddr, trblsh_group_opcode, 0x08, 0, 16);
+
+/* reg_pddr_status_opcode
+ * Group selector.
+ * Access: RO
+ */
+MLXSW_ITEM32(reg, pddr, trblsh_status_opcode, 0x0C, 0, 16);
+
+static inline void mlxsw_reg_pddr_pack(char *payload, u8 local_port,
+				       u8 page_select)
+{
+	MLXSW_REG_ZERO(pddr, payload);
+	mlxsw_reg_pddr_local_port_set(payload, local_port);
+	mlxsw_reg_pddr_page_select_set(payload, page_select);
+}
+
 /* HTGT - Host Trap Group Table
  * ----------------------------
  * Configures the properties for forwarding to CPU.
@@ -9927,6 +9980,7 @@ static const struct mlxsw_reg_info *mlxsw_reg_infos[] = {
 	MLXSW_REG(pptb),
 	MLXSW_REG(pbmc),
 	MLXSW_REG(pspa),
+	MLXSW_REG(pddr),
 	MLXSW_REG(htgt),
 	MLXSW_REG(hpkt),
 	MLXSW_REG(rgcr),
-- 
2.4.11

[RFC PATCH net-next 3/3] mlxsw: spectrum: Add rtnl_link_ops

[Petr Machata]

In order to provide visibility into problems with bringing a link up,
add rtnl_link_ops. Add the mandatory kind field. Add two new functions,
mlxsw_sp_port_link_down_reason_get_size() to calculate the size
necessary for the link-down reason attributes, and
mlxsw_sp_port_fill_link_down_reason() to fetch and decode the link-down
reason from firmware, and dump to the RTNL message. Tie the functions
to, respectively, link_down_reason_get_size and fill_link_down_reason
hooks in rtnl_link_ops.

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 108 +++++++++++++++++++++++++
 1 file changed, 108 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 9eb63300c1d3..b99b958da325 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -3207,6 +3207,113 @@ static const struct ethtool_ops mlxsw_sp_port_ethtool_ops = {
 	.get_module_eeprom	= mlxsw_sp_get_module_eeprom,
 };
 
+struct mlxsw_sp_ldr_opcode_mapping {
+	u16 status_opcode;
+	enum rtnl_link_down_reason_major major;
+};
+
+static const struct mlxsw_sp_ldr_opcode_mapping mlxsw_sp_ldr_opcode_map[] = {
+	{1024, RTNL_LDR_NO_CABLE},
+
+	{16, RTNL_LDR_UNSUPPORTED_CABLE},
+	{20, RTNL_LDR_UNSUPPORTED_CABLE},
+	{24, RTNL_LDR_UNSUPPORTED_CABLE},
+	{25, RTNL_LDR_UNSUPPORTED_CABLE},
+	{26, RTNL_LDR_UNSUPPORTED_CABLE},
+	{27, RTNL_LDR_UNSUPPORTED_CABLE},
+	{28, RTNL_LDR_UNSUPPORTED_CABLE},
+	{29, RTNL_LDR_UNSUPPORTED_CABLE},
+	{30, RTNL_LDR_UNSUPPORTED_CABLE},
+	{31, RTNL_LDR_UNSUPPORTED_CABLE},
+	{32, RTNL_LDR_UNSUPPORTED_CABLE},
+	{1025, RTNL_LDR_UNSUPPORTED_CABLE},
+	{1027, RTNL_LDR_UNSUPPORTED_CABLE},
+	{1029, RTNL_LDR_UNSUPPORTED_CABLE},
+
+	{2, RTNL_LDR_AUTONEG_FAILURE},
+	{3, RTNL_LDR_AUTONEG_FAILURE},
+	{4, RTNL_LDR_AUTONEG_FAILURE},
+	{38, RTNL_LDR_AUTONEG_FAILURE},
+	{39, RTNL_LDR_AUTONEG_FAILURE},
+
+	{36, RTNL_LDR_NO_LINK_PARTNER},
+
+	{5, RTNL_LDR_LINK_TRAINING_FAILURE},
+	{6, RTNL_LDR_LINK_TRAINING_FAILURE},
+	{7, RTNL_LDR_LINK_TRAINING_FAILURE},
+	{8, RTNL_LDR_LINK_TRAINING_FAILURE},
+
+	{9, RTNL_LDR_LOGICAL_MISMATCH},
+	{10, RTNL_LDR_LOGICAL_MISMATCH},
+	{11, RTNL_LDR_LOGICAL_MISMATCH},
+	{12, RTNL_LDR_LOGICAL_MISMATCH},
+	{13, RTNL_LDR_LOGICAL_MISMATCH},
+
+	{14, RTNL_LDR_REMOTE_FAULT},
+
+	{15, RTNL_LDR_BAD_SIGNAL_INTEGRITY},
+	{17, RTNL_LDR_BAD_SIGNAL_INTEGRITY},
+	{34, RTNL_LDR_BAD_SIGNAL_INTEGRITY},
+	{35, RTNL_LDR_BAD_SIGNAL_INTEGRITY},
+	{42, RTNL_LDR_BAD_SIGNAL_INTEGRITY},
+
+	{23, RTNL_LDR_CALIBRATION_FAILURE},
+
+	{1032, RTNL_LDR_POWER_BUDGET_EXCEEDED},
+};
+
+static size_t
+mlxsw_sp_port_link_down_reason_get_size(const struct net_device *dev)
+{
+	return nla_total_size(4) /* IFLA_LINK_DOWN_REASON_MAJOR */
+	     + nla_total_size(4) /* IFLA_LINK_DOWN_REASON_MINOR */
+	     + 0;
+}
+
+static int mlxsw_sp_port_fill_link_down_reason(struct sk_buff *skb,
+					       const struct net_device *dev)
+{
+	enum rtnl_link_down_reason_major major = RTNL_LDR_OTHER;
+	struct mlxsw_sp_port *mlxsw_sp_port = netdev_priv(dev);
+	char pddr_pl[MLXSW_REG_PDDR_LEN];
+	u16 status_opcode;
+	int i;
+	int err;
+
+	mlxsw_reg_pddr_pack(pddr_pl, mlxsw_sp_port->local_port,
+			    MLXSW_REG_PDDR_PAGESEL_TROUBLESHOOTING_INFO);
+	mlxsw_reg_pddr_trblsh_group_opcode_set(pddr_pl,
+				    MLXSW_REG_PDDR_TRBLSH_GROUP_MONITOR);
+
+	err = mlxsw_reg_query(mlxsw_sp_port->mlxsw_sp->core,
+			      MLXSW_REG(pddr), pddr_pl);
+	if (err)
+		return err;
+
+	status_opcode = mlxsw_reg_pddr_trblsh_status_opcode_get(pddr_pl);
+	if (!status_opcode)
+		return 0;
+
+	for (i = 0; i < ARRAY_SIZE(mlxsw_sp_ldr_opcode_map); ++i) {
+		if (mlxsw_sp_ldr_opcode_map[i].status_opcode == status_opcode) {
+			major = mlxsw_sp_ldr_opcode_map[i].major;
+			break;
+		}
+	}
+
+	if (nla_put_u32(skb, IFLA_LINK_DOWN_REASON_MAJOR, major) ||
+	    nla_put_u32(skb, IFLA_LINK_DOWN_REASON_MINOR, status_opcode))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static const struct rtnl_link_ops mlxsw_sp_port_rtnl_link_ops = {
+	.kind			    = "mlxsw",
+	.link_down_reason_get_size  = mlxsw_sp_port_link_down_reason_get_size,
+	.fill_link_down_reason	    = mlxsw_sp_port_fill_link_down_reason,
+};
+
 static int
 mlxsw_sp_port_speed_by_width_set(struct mlxsw_sp_port *mlxsw_sp_port, u8 width)
 {
@@ -3436,6 +3543,7 @@ static int mlxsw_sp_port_create(struct mlxsw_sp *mlxsw_sp, u8 local_port,
 
 	dev->netdev_ops = &mlxsw_sp_port_netdev_ops;
 	dev->ethtool_ops = &mlxsw_sp_port_ethtool_ops;
+	dev->rtnl_link_ops = &mlxsw_sp_port_rtnl_link_ops;
 
 	err = mlxsw_sp_port_module_map(mlxsw_sp_port, module, width, lane);
 	if (err) {
-- 
2.4.11

Re: [RFC PATCH net-next 0/3] RTNL: Add link-down reason reporting

[Andrew Lunn]

> The party with visibility into details of this process is the driver.

Hi Petr

In the general case, i would disagree with this. It is the PHY layer
which knows about these things. phylib and phylink. The MAC driver has
no idea, it just sees that the carrier is off.

There are however some drivers which do PHYs without using the Linux
core code. But there are not so many of them. I would hope this code
is designed for the general case, and can also be used by those that
ignore the core code.

Please could you explain how you see this being linked to phylib and
phylink, without having to modify every MAC driver.

	 Andrew

Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

[Jakub Kicinski]

On Fri, 15 Mar 2019 17:56:07 +0000, Petr Machata wrote:
> diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
> index e2091bb2b3a8..cfd9e86ff0ca 100644
> --- a/include/net/rtnetlink.h
> +++ b/include/net/rtnetlink.h
> @@ -110,6 +110,9 @@ struct rtnl_link_ops {
>  	int			(*fill_linkxstats)(struct sk_buff *skb,
>  						   const struct net_device *dev,
>  						   int *prividx, int attr);
> +	size_t			(*link_down_reason_get_size)(const struct net_device *dev);
> +	int			(*fill_link_down_reason)(struct sk_buff *skb,
> +							 const struct net_device *dev);

IMHO the API is a little heavy for returning, what is effectively a u64
value sliced in two..

Perhaps the core can just assume the reason will be provided if the NDO
is present?  And the "fill" NDO should probably fill in the reason
structure, rather than getting the skb passed and dealing with netlink
directly.

Also perhaps this would be a ethtool-nl candidate (which would
hopefully land soon after the merge window)?

>  };
>  
>  int __rtnl_link_register(struct rtnl_link_ops *ops);
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index 5b225ff63b48..a47f42e79741 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -167,6 +167,8 @@ enum {
>  	IFLA_NEW_IFINDEX,
>  	IFLA_MIN_MTU,
>  	IFLA_MAX_MTU,
> +	IFLA_LINK_DOWN_REASON_MAJOR,  /* enum rtnl_link_down_reason_major */
> +	IFLA_LINK_DOWN_REASON_MINOR,
>  	__IFLA_MAX
>  };

Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

[Andrew Lunn]

Hi Petr

> +enum rtnl_link_down_reason_major {
> +	RTNL_LDR_OTHER,

Does 'other' make any sense? Seem better to just not report anything
at all, or add a comment that more reasons should be added at the end
to reflect whatever the hardware or software can determine.

> +	RTNL_LDR_NO_CABLE,
> +	RTNL_LDR_UNSUPPORTED_CABLE,
> +	RTNL_LDR_AUTONEG_FAILURE,
> +	RTNL_LDR_NO_LINK_PARTNER,
> +	RTNL_LDR_LINK_TRAINING_FAILURE,
> +	RTNL_LDR_LOGICAL_MISMATCH,
> +	RTNL_LDR_REMOTE_FAULT,
> +	RTNL_LDR_BAD_SIGNAL_INTEGRITY,
> +	RTNL_LDR_CALIBRATION_FAILURE,
> +	RTNL_LDR_POWER_BUDGET_EXCEEDED,
> +};

What about SFP cage empty?, i.e. no SFP, SFP+ module in the cage?  An
SFP can also report LOS. That does not appear to be any of the above.
Or that the core SFP code has been unable to read the EEPROM? We have
people reporting this problem at the moment. We also have that the
SERDES has not yet obtained sync to its peer, which you know from
phylink_mac_change. That probably means the peer is using a different
bit rate.

I think it would be good if you handle the general case errors which
phylib and phylink can report, as well as the proprietary cases your
driver can report. We don't want this to be a Mellanox only API.

	Andrew

Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

[Michal Kubecek]

On Fri, Mar 15, 2019 at 07:26:11PM -0700, Jakub Kicinski wrote:
> On Fri, 15 Mar 2019 17:56:07 +0000, Petr Machata wrote:
> > diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
> > index e2091bb2b3a8..cfd9e86ff0ca 100644
> > --- a/include/net/rtnetlink.h
> > +++ b/include/net/rtnetlink.h
> > @@ -110,6 +110,9 @@ struct rtnl_link_ops {
> >  	int			(*fill_linkxstats)(struct sk_buff *skb,
> >  						   const struct net_device *dev,
> >  						   int *prividx, int attr);
> > +	size_t			(*link_down_reason_get_size)(const struct net_device *dev);
> > +	int			(*fill_link_down_reason)(struct sk_buff *skb,
> > +							 const struct net_device *dev);
> 
> IMHO the API is a little heavy for returning, what is effectively a u64
> value sliced in two..

Agreed. Unless we can expect more attributes to follow, repeating the
same code to generate netlink attributes in each driver doesn't feel
right.

> Perhaps the core can just assume the reason will be provided if the NDO
> is present?  And the "fill" NDO should probably fill in the reason
> structure, rather than getting the skb passed and dealing with netlink
> directly.

We could always reserve e.g. 0 as "none" which driver would use e.g. if
it only provides major reason id and not minor.

> Also perhaps this would be a ethtool-nl candidate (which would
> hopefully land soon after the merge window)?

Looking at the suggested values for major reasons, it does indeed seem
to belong to link related information provided by ETHTOOL_GLINKSETTINGS
request. Maybe nesting the reason with link state, i.e.

    ETHA_SETTINGS_LINK_STATE		(nested)
        ETHA_LINKSTATE_LINK		    (u8)
        ETHA_LINKSTATE_DOWN_REASON_MAJOR    (u32)
        ETHA_LINKSTATE_DOWN_REASON_MINOR    (u32)

Michal

Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

[Roopa Prabhu]

On Fri, Mar 15, 2019 at 10:56 AM Petr Machata <petrm@mellanox.com> wrote:
>
> In general, after a port is put administratively up, certain handshake
> protocols have to finish successfully, otherwise the port is left in a
> NO-CARRIER or DORMANT state. When that happens, it would be useful to
> communicate the reasons to the administrator, so that the problem that
> prevents the link from being established can be corrected.
>
> Introduce two new link attributes: IFLA_LINK_DOWN_REASON_MAJOR and
> _MINOR. Major reason code serve as broad categories intended to convey a
> general idea of where the problem is. Minor codes are arbitrary numbers
> specific for the driver that add detail to the major reasons. Add enum
> rtnl_link_down_reason_major to define the well-known major reason codes.
>
> The party with visibility into details of this process is the driver.
> Therefore add two new RTNL hooks, link_down_reason_get_size and
> fill_link_down_reason, to provide the necessary information.
>
> Link-down reason is not included if the port is up or administratively
> down, because those two state are easy to discover through existing
> interfaces.
>
> Signed-off-by: Petr Machata <petrm@mellanox.com>

This looks good and will be use-full. But i have some comments on the
implementation below.
Also, carrier can go down due to protocol down (IFLA_PROTODOWN). And I
get asked about supporting
reason codes or protocol owner for such protodown reason (I have not
given it much thought yet. I will see if there is a way
to use your series for that as well).

> ---
>  include/net/rtnetlink.h      |  3 +++
>  include/uapi/linux/if_link.h | 16 ++++++++++++++++
>  net/core/rtnetlink.c         | 22 ++++++++++++++++++++++
>  3 files changed, 41 insertions(+)
>
> diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
> index e2091bb2b3a8..cfd9e86ff0ca 100644
> --- a/include/net/rtnetlink.h
> +++ b/include/net/rtnetlink.h
> @@ -110,6 +110,9 @@ struct rtnl_link_ops {
>         int                     (*fill_linkxstats)(struct sk_buff *skb,
>                                                    const struct net_device *dev,
>                                                    int *prividx, int attr);
> +       size_t                  (*link_down_reason_get_size)(const struct net_device *dev);
> +       int                     (*fill_link_down_reason)(struct sk_buff *skb,
> +                                                        const struct net_device *dev);
>  };

Any reason to restrict this to network interfaces which support rtnl_link_ops ?.
I also saw that you added rtnl_link_ops to mlxsw for this. Which also
means every driver will now have to declare rtnl_link_ops to use this
?.
I think we should keep rtnl_link_ops to logical links  like bridge,
bonds etc (ie which support newlink and dellink).

Can't we use netdev_ops for this ?. That will allow any driver to just
support this readily.

>
>  int __rtnl_link_register(struct rtnl_link_ops *ops);
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index 5b225ff63b48..a47f42e79741 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -167,6 +167,8 @@ enum {
>         IFLA_NEW_IFINDEX,
>         IFLA_MIN_MTU,
>         IFLA_MAX_MTU,
> +       IFLA_LINK_DOWN_REASON_MAJOR,  /* enum rtnl_link_down_reason_major */
> +       IFLA_LINK_DOWN_REASON_MINOR,
>         __IFLA_MAX
>  };
>
> @@ -239,6 +241,20 @@ enum in6_addr_gen_mode {
>         IN6_ADDR_GEN_MODE_RANDOM,
>  };
>
> +enum rtnl_link_down_reason_major {
> +       RTNL_LDR_OTHER,
> +       RTNL_LDR_NO_CABLE,
> +       RTNL_LDR_UNSUPPORTED_CABLE,
> +       RTNL_LDR_AUTONEG_FAILURE,
> +       RTNL_LDR_NO_LINK_PARTNER,
> +       RTNL_LDR_LINK_TRAINING_FAILURE,
> +       RTNL_LDR_LOGICAL_MISMATCH,
> +       RTNL_LDR_REMOTE_FAULT,
> +       RTNL_LDR_BAD_SIGNAL_INTEGRITY,
> +       RTNL_LDR_CALIBRATION_FAILURE,
> +       RTNL_LDR_POWER_BUDGET_EXCEEDED,
> +};

prefer LINK_DOWN_REASON_* or LINKDOWN_REASON_*
(Though there is no predefined convention, the prefix RTNL makes it
feel like a top-level attribute when its really a value for an IFLA_*
attribute.)


> +
>  /* Bridge section */
>
>  enum {
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index a51cab95ba64..206795f13850 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -979,6 +979,22 @@ static size_t rtnl_xdp_size(void)
>         return xdp_size;
>  }
>
> +static bool rtnl_should_fill_link_down_reason(const struct net_device *dev)
> +{
> +       return (dev->flags & IFF_UP) && !netif_oper_up(dev) &&
> +               dev->rtnl_link_ops &&
> +               dev->rtnl_link_ops->link_down_reason_get_size &&
> +               dev->rtnl_link_ops->fill_link_down_reason;
> +}
> +
> +static size_t rtnl_link_down_reason_get_size(const struct net_device *dev)
> +{
> +       if (!rtnl_should_fill_link_down_reason(dev))
> +               return 0;
> +
> +       return dev->rtnl_link_ops->link_down_reason_get_size(dev);
> +}
> +
>  static noinline size_t if_nlmsg_size(const struct net_device *dev,
>                                      u32 ext_filter_mask)
>  {
> @@ -1026,6 +1042,7 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev,
>                + nla_total_size(4)  /* IFLA_CARRIER_DOWN_COUNT */
>                + nla_total_size(4)  /* IFLA_MIN_MTU */
>                + nla_total_size(4)  /* IFLA_MAX_MTU */
> +              + rtnl_link_down_reason_get_size(dev)
>                + 0;
>  }
>
> @@ -1683,6 +1700,9 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
>             nla_put_s32(skb, IFLA_NEW_IFINDEX, new_ifindex) < 0)
>                 goto nla_put_failure;
>
> +       if (rtnl_should_fill_link_down_reason(dev) &&
> +           dev->rtnl_link_ops->fill_link_down_reason(skb, dev))
> +               goto nla_put_failure;
>
>         rcu_read_lock();
>         if (rtnl_fill_link_af(skb, dev, ext_filter_mask))
> @@ -1742,6 +1762,8 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
>         [IFLA_CARRIER_DOWN_COUNT] = { .type = NLA_U32 },
>         [IFLA_MIN_MTU]          = { .type = NLA_U32 },
>         [IFLA_MAX_MTU]          = { .type = NLA_U32 },
> +       [IFLA_LINK_DOWN_REASON_MAJOR] = { .type = NLA_U32 },
> +       [IFLA_LINK_DOWN_REASON_MINOR] = { .type = NLA_U32 },
>  };
>
>  static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
> --
> 2.4.11
>

Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

[Andrew Lunn]

> Can't we use netdev_ops for this ?. That will allow any driver to just
> support this readily.

Hi Roopa

I argued this is a PHY layer status information. We don't really want
to have to modify every MAC driver to call into phylib/phylink. Yes,
we can have a netdev_ops for those drivers which ignore the Linux PHY
layer, but ideally we want to transparently call into the PHY layer to
get this information, if the MAC is not doing odd things.

	    Andrew

Re: [RFC PATCH net-next 0/3] RTNL: Add link-down reason reporting

[Petr Machata]

Andrew Lunn <andrew@lunn.ch> writes:

>> The party with visibility into details of this process is the driver.
>
> In the general case, i would disagree with this. It is the PHY layer
> which knows about these things. phylib and phylink. The MAC driver has
> no idea, it just sees that the carrier is off.
>
> There are however some drivers which do PHYs without using the Linux
> core code. But there are not so many of them. I would hope this code
> is designed for the general case, and can also be used by those that
> ignore the core code.
>
> Please could you explain how you see this being linked to phylib and
> phylink, without having to modify every MAC driver.

I'll take a look at how to make this work with the phy drivers. Thanks,
Petr

Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

[Petr Machata]

Roopa Prabhu <roopa@cumulusnetworks.com> writes:

> This looks good and will be use-full. But i have some comments on the
> implementation below.
> Also, carrier can go down due to protocol down (IFLA_PROTODOWN). And I
> get asked about supporting
> reason codes or protocol owner for such protodown reason (I have not
> given it much thought yet. I will see if there is a way
> to use your series for that as well).

My thinking was that since protocol down is set from userspace, it's
under admin control, and that's where the reason signalling should be.

>> ---
>>  include/net/rtnetlink.h      |  3 +++
>>  include/uapi/linux/if_link.h | 16 ++++++++++++++++
>>  net/core/rtnetlink.c         | 22 ++++++++++++++++++++++
>>  3 files changed, 41 insertions(+)
>>
>> diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
>> index e2091bb2b3a8..cfd9e86ff0ca 100644
>> --- a/include/net/rtnetlink.h
>> +++ b/include/net/rtnetlink.h
>> @@ -110,6 +110,9 @@ struct rtnl_link_ops {
>>         int                     (*fill_linkxstats)(struct sk_buff *skb,
>>                                                    const struct net_device *dev,
>>                                                    int *prividx, int attr);
>> +       size_t                  (*link_down_reason_get_size)(const struct net_device *dev);
>> +       int                     (*fill_link_down_reason)(struct sk_buff *skb,
>> +                                                        const struct net_device *dev);
>>  };
>
> Any reason to restrict this to network interfaces which support rtnl_link_ops ?.
> I also saw that you added rtnl_link_ops to mlxsw for this. Which also
> means every driver will now have to declare rtnl_link_ops to use this
> ?.
> I think we should keep rtnl_link_ops to logical links  like bridge,
> bonds etc (ie which support newlink and dellink).
>
> Can't we use netdev_ops for this ?. That will allow any driver to just
> support this readily.

I guess you are right.

>> +enum rtnl_link_down_reason_major {
>> +       RTNL_LDR_OTHER,
>> +       RTNL_LDR_NO_CABLE,
>> +       RTNL_LDR_UNSUPPORTED_CABLE,
>> +       RTNL_LDR_AUTONEG_FAILURE,
>> +       RTNL_LDR_NO_LINK_PARTNER,
>> +       RTNL_LDR_LINK_TRAINING_FAILURE,
>> +       RTNL_LDR_LOGICAL_MISMATCH,
>> +       RTNL_LDR_REMOTE_FAULT,
>> +       RTNL_LDR_BAD_SIGNAL_INTEGRITY,
>> +       RTNL_LDR_CALIBRATION_FAILURE,
>> +       RTNL_LDR_POWER_BUDGET_EXCEEDED,
>> +};
>
> prefer LINK_DOWN_REASON_* or LINKDOWN_REASON_*
> (Though there is no predefined convention, the prefix RTNL makes it
> feel like a top-level attribute when its really a value for an IFLA_*
> attribute.)

OK.

Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

[Petr Machata]

Jakub Kicinski <jakub.kicinski@netronome.com> writes:

> On Fri, 15 Mar 2019 17:56:07 +0000, Petr Machata wrote:
>> diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
>> index e2091bb2b3a8..cfd9e86ff0ca 100644
>> --- a/include/net/rtnetlink.h
>> +++ b/include/net/rtnetlink.h
>> @@ -110,6 +110,9 @@ struct rtnl_link_ops {
>>  	int			(*fill_linkxstats)(struct sk_buff *skb,
>>  						   const struct net_device *dev,
>>  						   int *prividx, int attr);
>> +	size_t			(*link_down_reason_get_size)(const struct net_device *dev);
>> +	int			(*fill_link_down_reason)(struct sk_buff *skb,
>> +							 const struct net_device *dev);
>
> IMHO the API is a little heavy for returning, what is effectively a u64
> value sliced in two..

If a year from now someone wants to add string reason, this API will
just extend naturally to cover that case.

> Perhaps the core can just assume the reason will be provided if the NDO
> is present?  And the "fill" NDO should probably fill in the reason
> structure, rather than getting the skb passed and dealing with netlink
> directly.

This copies how other fill APIs are done, in that the responsibility for
filling up the message is deferred to the driver. I think it makes the
API easier to extend: if there ever is richer information available, the
drivers that want to support it just opt in and provide those attributes.

> Also perhaps this would be a ethtool-nl candidate (which would
> hopefully land soon after the merge window)?

Is this the repository with the patches? I'll take a look.

    https://github.com/mkubecek/ethnl

Thanks,
Petr

Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

[Michal Kubecek]

On Mon, Mar 18, 2019 at 12:34:55PM +0000, Petr Machata wrote:
> Jakub Kicinski <jakub.kicinski@netronome.com> writes:
> > Also perhaps this would be a ethtool-nl candidate (which would
> > hopefully land soon after the merge window)?
> 
> Is this the repository with the patches? I'll take a look.
> 
>     https://github.com/mkubecek/ethnl

Yes, that's it.

Michal

Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

[Andrew Lunn]

> This copies how other fill APIs are done, in that the responsibility for
> filling up the message is deferred to the driver. I think it makes the
> API easier to extend: if there ever is richer information available, the
> drivers that want to support it just opt in and provide those attributes.

Hi Petr

Actually, if you look at the PHY layer, none of the drivers have
anything to do with netlink messages. Nor does the phylib or phylink
core. It all happens a layer above. I would keep the API to the driver
simple, just ask it for two u32, and do all the netlink conversion in
the core.

    Andrew

Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

[Petr Machata]

Andrew Lunn <andrew@lunn.ch> writes:

>> +enum rtnl_link_down_reason_major {
>> +	RTNL_LDR_OTHER,
>
> Does 'other' make any sense? Seem better to just not report anything
> at all, or add a comment that more reasons should be added at the end
> to reflect whatever the hardware or software can determine.

You still have the minor code to give you some information.

>> +	RTNL_LDR_NO_CABLE,
>> +	RTNL_LDR_UNSUPPORTED_CABLE,
>> +	RTNL_LDR_AUTONEG_FAILURE,
>> +	RTNL_LDR_NO_LINK_PARTNER,
>> +	RTNL_LDR_LINK_TRAINING_FAILURE,
>> +	RTNL_LDR_LOGICAL_MISMATCH,
>> +	RTNL_LDR_REMOTE_FAULT,
>> +	RTNL_LDR_BAD_SIGNAL_INTEGRITY,
>> +	RTNL_LDR_CALIBRATION_FAILURE,
>> +	RTNL_LDR_POWER_BUDGET_EXCEEDED,
>> +};
>
> What about SFP cage empty?, i.e. no SFP, SFP+ module in the cage?  An

No cable? Maybe the name needs to change...

> SFP can also report LOS. That does not appear to be any of the above.
> Or that the core SFP code has been unable to read the EEPROM? We have

My assumption was that cable with unreadable EEPROM is simply a bad
cable. Does the admin actually care which particular part of the cable
is at fault?

> people reporting this problem at the moment. We also have that the
> SERDES has not yet obtained sync to its peer, which you know from
> phylink_mac_change. That probably means the peer is using a different
> bit rate.

We can add this.

> I think it would be good if you handle the general case errors which
> phylib and phylink can report, as well as the proprietary cases your
> driver can report. We don't want this to be a Mellanox only API.

Sure, I'll take a look at that. I didn't need to deal with PHY so far,
so I need to figure out what's what.

Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

[Andrew Lunn]

On Mon, Mar 18, 2019 at 01:15:41PM +0000, Petr Machata wrote:
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> >> +enum rtnl_link_down_reason_major {
> >> +	RTNL_LDR_OTHER,
> >
> > Does 'other' make any sense? Seem better to just not report anything
> > at all, or add a comment that more reasons should be added at the end
> > to reflect whatever the hardware or software can determine.
> 
> You still have the minor code to give you some information.
> 
> >> +	RTNL_LDR_NO_CABLE,
> >> +	RTNL_LDR_UNSUPPORTED_CABLE,
> >> +	RTNL_LDR_AUTONEG_FAILURE,
> >> +	RTNL_LDR_NO_LINK_PARTNER,
> >> +	RTNL_LDR_LINK_TRAINING_FAILURE,
> >> +	RTNL_LDR_LOGICAL_MISMATCH,
> >> +	RTNL_LDR_REMOTE_FAULT,
> >> +	RTNL_LDR_BAD_SIGNAL_INTEGRITY,
> >> +	RTNL_LDR_CALIBRATION_FAILURE,
> >> +	RTNL_LDR_POWER_BUDGET_EXCEEDED,
> >> +};
> >
> > What about SFP cage empty?, i.e. no SFP, SFP+ module in the cage?  An
> 
> No cable? Maybe the name needs to change...

An SFP module, and the cable plugged into it via LC connectors, are
physically different things. And you can also have an SFP with an RJ45
for 1G copper. I know at higher speeds they can be inseparable, but
this needs to be a generic API and also work with them being two
separate things. 

> 
> > SFP can also report LOS. That does not appear to be any of the above.
> > Or that the core SFP code has been unable to read the EEPROM? We have
> 
> My assumption was that cable with unreadable EEPROM is simply a bad
> cable. Does the admin actually care which particular part of the cable
> is at fault?

Yes. I throw away the SFP module, because its EEPROM is broke, but
don't need to replace the 1KM of fibre cable, or 100m of Cat 6a copper
cable. Classic example would be fibre to the home. Do you really think
they are going to dig up the road again, to replace the cable, when
all they need to do is replace the SFP module?

	Andrew

Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

[Petr Machata]

Andrew Lunn <andrew@lunn.ch> writes:

> On Mon, Mar 18, 2019 at 01:15:41PM +0000, Petr Machata wrote:
>> 
>> Andrew Lunn <andrew@lunn.ch> writes:
>> 
>> >> +enum rtnl_link_down_reason_major {
>> >> +	RTNL_LDR_OTHER,
>> >
>> > Does 'other' make any sense? Seem better to just not report anything
>> > at all, or add a comment that more reasons should be added at the end
>> > to reflect whatever the hardware or software can determine.
>> 
>> You still have the minor code to give you some information.
>> 
>> >> +	RTNL_LDR_NO_CABLE,
>> >> +	RTNL_LDR_UNSUPPORTED_CABLE,
>> >> +	RTNL_LDR_AUTONEG_FAILURE,
>> >> +	RTNL_LDR_NO_LINK_PARTNER,
>> >> +	RTNL_LDR_LINK_TRAINING_FAILURE,
>> >> +	RTNL_LDR_LOGICAL_MISMATCH,
>> >> +	RTNL_LDR_REMOTE_FAULT,
>> >> +	RTNL_LDR_BAD_SIGNAL_INTEGRITY,
>> >> +	RTNL_LDR_CALIBRATION_FAILURE,
>> >> +	RTNL_LDR_POWER_BUDGET_EXCEEDED,
>> >> +};
>> >
>> > What about SFP cage empty?, i.e. no SFP, SFP+ module in the cage?  An
>> 
>> No cable? Maybe the name needs to change...
>
> An SFP module, and the cable plugged into it via LC connectors, are
> physically different things. And you can also have an SFP with an RJ45
> for 1G copper. I know at higher speeds they can be inseparable, but
> this needs to be a generic API and also work with them being two
> separate things.

Understood.

>> 
>> > SFP can also report LOS. That does not appear to be any of the above.
>> > Or that the core SFP code has been unable to read the EEPROM? We have
>> 
>> My assumption was that cable with unreadable EEPROM is simply a bad
>> cable. Does the admin actually care which particular part of the cable
>> is at fault?
>
> Yes. I throw away the SFP module, because its EEPROM is broke, but
> don't need to replace the 1KM of fibre cable, or 100m of Cat 6a copper
> cable. Classic example would be fibre to the home.

OK, gotcha.

Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

[Andrew Lunn]

On Mon, Mar 18, 2019 at 01:15:41PM +0000, Petr Machata wrote:
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> >> +enum rtnl_link_down_reason_major {
> >> +	RTNL_LDR_OTHER,
> >
> > Does 'other' make any sense? Seem better to just not report anything
> > at all, or add a comment that more reasons should be added at the end
> > to reflect whatever the hardware or software can determine.
> 
> You still have the minor code to give you some information.

The problem i have with OTHER, is that you know it is not NO_CABLE,
UNSUPPORTED_CABLE, AUTONEG_FAILURE, etc. But for people to know what
OTHER cannot be, they have to know all the codes.

But then later, some other driver writer does the right thing, adds a
new value to the end for a code they can detect. Say for example
SFP_OVERHEATED.  This happened to be what the previous driver was
using for OTHER. Now we have one driver returning SFP_OVERHEATED and
the older driver OTHER. So OTHER no longer actually mean 'other', it
just means something random, which could actually be the same as one
of the listed codes.

You can stop this from happening by not having OTHER. Always add a new
code if there is something you can report, but there currently is no
code for it. And the userspace tool should just print the decimal
value if it does not know what text to translate it into.

      Andrew

Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

[Stephen Hemminger]

On Mon, 18 Mar 2019 15:02:53 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> On Mon, Mar 18, 2019 at 01:15:41PM +0000, Petr Machata wrote:
> > 
> > Andrew Lunn <andrew@lunn.ch> writes:
> >   
> > >> +enum rtnl_link_down_reason_major {
> > >> +	RTNL_LDR_OTHER,  
> > >
> > > Does 'other' make any sense? Seem better to just not report anything
> > > at all, or add a comment that more reasons should be added at the end
> > > to reflect whatever the hardware or software can determine.  
> > 
> > You still have the minor code to give you some information.  
> 
> The problem i have with OTHER, is that you know it is not NO_CABLE,
> UNSUPPORTED_CABLE, AUTONEG_FAILURE, etc. But for people to know what
> OTHER cannot be, they have to know all the codes.
> 
> But then later, some other driver writer does the right thing, adds a
> new value to the end for a code they can detect. Say for example
> SFP_OVERHEATED.  This happened to be what the previous driver was
> using for OTHER. Now we have one driver returning SFP_OVERHEATED and
> the older driver OTHER. So OTHER no longer actually mean 'other', it
> just means something random, which could actually be the same as one
> of the listed codes.
> 
> You can stop this from happening by not having OTHER. Always add a new
> code if there is something you can report, but there currently is no
> code for it. And the userspace tool should just print the decimal
> value if it does not know what text to translate it into.


Gut feel is that enumerated values are going to grow and grow and be long term API headache.

Would it be possible to use a string like the external ack error message?

Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

[Petr Machata]

Stephen Hemminger <stephen@networkplumber.org> writes:

> On Mon, 18 Mar 2019 15:02:53 +0100
> Andrew Lunn <andrew@lunn.ch> wrote:
>
>> On Mon, Mar 18, 2019 at 01:15:41PM +0000, Petr Machata wrote:
>> >
>> > Andrew Lunn <andrew@lunn.ch> writes:
>> >
>> > >> +enum rtnl_link_down_reason_major {
>> > >> +	RTNL_LDR_OTHER,
>> > >
>> > > Does 'other' make any sense? Seem better to just not report anything
>> > > at all, or add a comment that more reasons should be added at the end
>> > > to reflect whatever the hardware or software can determine.
>> >
>> > You still have the minor code to give you some information.
>>
>> The problem i have with OTHER, is that you know it is not NO_CABLE,
>> UNSUPPORTED_CABLE, AUTONEG_FAILURE, etc. But for people to know what
>> OTHER cannot be, they have to know all the codes.
>>
>> But then later, some other driver writer does the right thing, adds a
>> new value to the end for a code they can detect. Say for example
>> SFP_OVERHEATED.  This happened to be what the previous driver was
>> using for OTHER. Now we have one driver returning SFP_OVERHEATED and
>> the older driver OTHER. So OTHER no longer actually mean 'other', it
>> just means something random, which could actually be the same as one
>> of the listed codes.
>>
>> You can stop this from happening by not having OTHER. Always add a new
>> code if there is something you can report, but there currently is no
>> code for it. And the userspace tool should just print the decimal
>> value if it does not know what text to translate it into.
>
> Gut feel is that enumerated values are going to grow and grow and be
> long term API headache.
>
> Would it be possible to use a string like the external ack error
> message?

It would, but then if any automated tools want to make use of it beyond
just blindly displaying it, they will need to parse it with all the
usual problems. In the end the string itself becomes the API anyway.

Adding a string would make sense as an extra piece of information, not
as the primary channel. Extack is like this as well, the primary channel
there is errno.

Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

[Michal Kubecek]

On Tue, Mar 19, 2019 at 10:18:00AM +0000, Petr Machata wrote:
> Stephen Hemminger <stephen@networkplumber.org> writes:
> > Gut feel is that enumerated values are going to grow and grow and be
> > long term API headache.
> >
> > Would it be possible to use a string like the external ack error
> > message?
> 
> It would, but then if any automated tools want to make use of it beyond
> just blindly displaying it, they will need to parse it with all the
> usual problems. In the end the string itself becomes the API anyway.

Providing only text description doesn't sound right to me either,
I would rather prefer having both text and numeric code in such case.

> Adding a string would make sense as an extra piece of information, not
> as the primary channel. Extack is like this as well, the primary channel
> there is errno.

Not exactly, the primary goal of extack was IMHO to provide better
granularity than the standard error codes allowed (we all remember the
cryptic messages of older iproute2 versions).

A better analogy would be numeric error codes which could be passed via
extack cookie (e.g. nl_set_extack_cookie_u64()) together with a text
representation. But AFAIK only few netlink based interfaces use those.

Michal Kubecek

Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

[Stephen Hemminger]

On Tue, 19 Mar 2019 10:18:00 +0000
Petr Machata <petrm@mellanox.com> wrote:

> Stephen Hemminger <stephen@networkplumber.org> writes:
> 
> > On Mon, 18 Mar 2019 15:02:53 +0100
> > Andrew Lunn <andrew@lunn.ch> wrote:
> >  
> >> On Mon, Mar 18, 2019 at 01:15:41PM +0000, Petr Machata wrote:  
> >> >
> >> > Andrew Lunn <andrew@lunn.ch> writes:
> >> >  
> >> > >> +enum rtnl_link_down_reason_major {
> >> > >> +	RTNL_LDR_OTHER,  
> >> > >
> >> > > Does 'other' make any sense? Seem better to just not report anything
> >> > > at all, or add a comment that more reasons should be added at the end
> >> > > to reflect whatever the hardware or software can determine.  
> >> >
> >> > You still have the minor code to give you some information.  
> >>
> >> The problem i have with OTHER, is that you know it is not NO_CABLE,
> >> UNSUPPORTED_CABLE, AUTONEG_FAILURE, etc. But for people to know what
> >> OTHER cannot be, they have to know all the codes.
> >>
> >> But then later, some other driver writer does the right thing, adds a
> >> new value to the end for a code they can detect. Say for example
> >> SFP_OVERHEATED.  This happened to be what the previous driver was
> >> using for OTHER. Now we have one driver returning SFP_OVERHEATED and
> >> the older driver OTHER. So OTHER no longer actually mean 'other', it
> >> just means something random, which could actually be the same as one
> >> of the listed codes.
> >>
> >> You can stop this from happening by not having OTHER. Always add a new
> >> code if there is something you can report, but there currently is no
> >> code for it. And the userspace tool should just print the decimal
> >> value if it does not know what text to translate it into.  
> >
> > Gut feel is that enumerated values are going to grow and grow and be
> > long term API headache.
> >
> > Would it be possible to use a string like the external ack error
> > message?  
> 
> It would, but then if any automated tools want to make use of it beyond
> just blindly displaying it, they will need to parse it with all the
> usual problems. In the end the string itself becomes the API anyway.
> 
> Adding a string would make sense as an extra piece of information, not
> as the primary channel. Extack is like this as well, the primary channel
> there is errno.

The problem with codes is that without some standard (like IETF) the
values are very system specific and likely to get lots of of version churn.

Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

[Petr Machata]

Stephen Hemminger <stephen@networkplumber.org> writes:

> On Tue, 19 Mar 2019 10:18:00 +0000
> Petr Machata <petrm@mellanox.com> wrote:
>
>> Stephen Hemminger <stephen@networkplumber.org> writes:
>> 
>> > On Mon, 18 Mar 2019 15:02:53 +0100
>> > Andrew Lunn <andrew@lunn.ch> wrote:
>> >  
>> >> On Mon, Mar 18, 2019 at 01:15:41PM +0000, Petr Machata wrote:  
>> >> >
>> >> > Andrew Lunn <andrew@lunn.ch> writes:
>> >> >  
>> >> > >> +enum rtnl_link_down_reason_major {
>> >> > >> +	RTNL_LDR_OTHER,  
>> >> > >
>> >> > > Does 'other' make any sense? Seem better to just not report anything
>> >> > > at all, or add a comment that more reasons should be added at the end
>> >> > > to reflect whatever the hardware or software can determine.  
>> >> >
>> >> > You still have the minor code to give you some information.  
>> >>
>> >> The problem i have with OTHER, is that you know it is not NO_CABLE,
>> >> UNSUPPORTED_CABLE, AUTONEG_FAILURE, etc. But for people to know what
>> >> OTHER cannot be, they have to know all the codes.
>> >>
>> >> But then later, some other driver writer does the right thing, adds a
>> >> new value to the end for a code they can detect. Say for example
>> >> SFP_OVERHEATED.  This happened to be what the previous driver was
>> >> using for OTHER. Now we have one driver returning SFP_OVERHEATED and
>> >> the older driver OTHER. So OTHER no longer actually mean 'other', it
>> >> just means something random, which could actually be the same as one
>> >> of the listed codes.
>> >>
>> >> You can stop this from happening by not having OTHER. Always add a new
>> >> code if there is something you can report, but there currently is no
>> >> code for it. And the userspace tool should just print the decimal
>> >> value if it does not know what text to translate it into.  
>> >
>> > Gut feel is that enumerated values are going to grow and grow and be
>> > long term API headache.
>> >
>> > Would it be possible to use a string like the external ack error
>> > message?  
>> 
>> It would, but then if any automated tools want to make use of it beyond
>> just blindly displaying it, they will need to parse it with all the
>> usual problems. In the end the string itself becomes the API anyway.
>
> The problem with codes is that without some standard (like IETF) the
> values are very system specific and likely to get lots of of version churn.

I think this would be even worse with just strings.

Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

[Petr Machata]

Andrew Lunn <andrew@lunn.ch> writes:

> I argued this is a PHY layer status information. We don't really want
> to have to modify every MAC driver to call into phylib/phylink. Yes,
> we can have a netdev_ops for those drivers which ignore the Linux PHY
> layer, but ideally we want to transparently call into the PHY layer to
> get this information, if the MAC is not doing odd things.

From what I see, there are four places where the information could be
collected:

- MAC driver: dev->ethtool_ops->get_link_down_reason
  (At least mlxsw and mlx5 need this.)

    modified   include/linux/ethtool.h
    @@ -395,4 +395,7 @@ struct ethtool_ops {
                                        struct ethtool_fecparam *);
            void    (*get_ethtool_phy_stats)(struct net_device *,
                                            struct ethtool_stats *, u64 *);
    +       int     (*get_link_down_reason)(const struct net_device *,
    +                                       enum link_down_reason_major *,
    +                                       u32 *minor);
    };

  Return -ENODATA to indicate there's nothing to report.

- PHY driver: dev->phydev->drv->get_link_down_reason
  Certain PHY drivers might want to have a custom handling for some
  PHY-specific insight. Something like:

    modified   include/linux/phy.h
    @@ -636,4 +636,7 @@ struct phy_driver {
                                struct ethtool_tunable *tuna,
                                const void *data);
            int (*set_loopback)(struct phy_device *dev, bool enable);
    +       int (*get_link_down_reason)(struct phy_device *dev,
    +                                   enum link_down_reason_major *major,
    +                                   u32 *minor);
    };

  Return -ENODATA to indicate there's nothing to report.

- Generic PHY
  It would be possible to add a function that e.g. consults the PHY
  state machine to figure out what went wrong. Phy as such may have to
  be extended to keep track of e.g. why the state machine is PHY_HALTED,
  or possibly other problems worthy of reporting.

- phylink
  I'm not sure how to generically plug in the phylink library, because
  it is a private property of a MAC driver. There are currently only
  three drivers using phylink (mvvp2, mvneta, DSA), but it looks as if
  the intention is that phylink is used much more widely. So maybe it
  would make sense to have an NDO like get_phylink and use that to call
  into the phylink library for some generic handling.

Speaking specifically, my patch set would only do the first step above,
because neither mlxsw nor mlx5 use the PHY subsystem. But it would be
easy enough to extend for the other cases above.

Thoughts?

Thanks,
Petr

Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

[Andrew Lunn]

On Thu, Mar 28, 2019 at 05:59:50PM +0000, Petr Machata wrote:

Hi Petr

> - PHY driver: dev->phydev->drv->get_link_down_reason
>   Certain PHY drivers might want to have a custom handling for some
>   PHY-specific insight. Something like:
> 
>     modified   include/linux/phy.h
>     @@ -636,4 +636,7 @@ struct phy_driver {
>                                 struct ethtool_tunable *tuna,
>                                 const void *data);
>             int (*set_loopback)(struct phy_device *dev, bool enable);
>     +       int (*get_link_down_reason)(struct phy_device *dev,
>     +                                   enum link_down_reason_major *major,
>     +                                   u32 *minor);
>     };
> 
>   Return -ENODATA to indicate there's nothing to report.
> 
> - Generic PHY
>   It would be possible to add a function that e.g. consults the PHY
>   state machine to figure out what went wrong. Phy as such may have to
>   be extended to keep track of e.g. why the state machine is PHY_HALTED,
>   or possibly other problems worthy of reporting.

I would suggest changing the order of these two. Put the generic PHY
first. If it sees the driver has a OP to get more specific detail, it
would make the call into the driver. You don't violate any layering
that way, and the locking is done correctly.

The PHY being in state HALTED probably means there is a hardware error
at the MDIO level. That does not happen very often at all.

What you are more interested is state PHY_NOLINK, which indicates
there is no link. Maybe because auto-neg didn't complete, if it was
turned on, or if auto-neg is off and the link is forced, the peer is
using something different, or is not there at all.

> - phylink
>   I'm not sure how to generically plug in the phylink library, because
>   it is a private property of a MAC driver. There are currently only
>   three drivers using phylink (mvvp2, mvneta, DSA), but it looks as if
>   the intention is that phylink is used much more widely. So maybe it
>   would make sense to have an NDO like get_phylink and use that to call
>   into the phylink library for some generic handling.

We need to consider adding a phylink pointer to the netdev structure
soon, in the same way there is a phylib structure.

> Speaking specifically, my patch set would only do the first step above,
> because neither mlxsw nor mlx5 use the PHY subsystem.

Shame. With just mlx, you will get a coverage of ~0. If you did the
generic phylib work, you might get coverage of > 50% of the MAC
drivers. It then becomes something useful and really used.

	 Andrew

Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

[Jiri Pirko]

Thu, Mar 28, 2019 at 08:51:34PM CET, andrew@lunn.ch wrote:
>On Thu, Mar 28, 2019 at 05:59:50PM +0000, Petr Machata wrote:
>
>Hi Petr
>
>> - PHY driver: dev->phydev->drv->get_link_down_reason
>>   Certain PHY drivers might want to have a custom handling for some
>>   PHY-specific insight. Something like:
>> 
>>     modified   include/linux/phy.h
>>     @@ -636,4 +636,7 @@ struct phy_driver {
>>                                 struct ethtool_tunable *tuna,
>>                                 const void *data);
>>             int (*set_loopback)(struct phy_device *dev, bool enable);
>>     +       int (*get_link_down_reason)(struct phy_device *dev,
>>     +                                   enum link_down_reason_major *major,
>>     +                                   u32 *minor);
>>     };
>> 
>>   Return -ENODATA to indicate there's nothing to report.
>> 
>> - Generic PHY
>>   It would be possible to add a function that e.g. consults the PHY
>>   state machine to figure out what went wrong. Phy as such may have to
>>   be extended to keep track of e.g. why the state machine is PHY_HALTED,
>>   or possibly other problems worthy of reporting.
>
>I would suggest changing the order of these two. Put the generic PHY
>first. If it sees the driver has a OP to get more specific detail, it
>would make the call into the driver. You don't violate any layering
>that way, and the locking is done correctly.
>
>The PHY being in state HALTED probably means there is a hardware error
>at the MDIO level. That does not happen very often at all.
>
>What you are more interested is state PHY_NOLINK, which indicates
>there is no link. Maybe because auto-neg didn't complete, if it was
>turned on, or if auto-neg is off and the link is forced, the peer is
>using something different, or is not there at all.
>
>> - phylink
>>   I'm not sure how to generically plug in the phylink library, because
>>   it is a private property of a MAC driver. There are currently only
>>   three drivers using phylink (mvvp2, mvneta, DSA), but it looks as if
>>   the intention is that phylink is used much more widely. So maybe it
>>   would make sense to have an NDO like get_phylink and use that to call
>>   into the phylink library for some generic handling.
>
>We need to consider adding a phylink pointer to the netdev structure
>soon, in the same way there is a phylib structure.
>
>> Speaking specifically, my patch set would only do the first step above,
>> because neither mlxsw nor mlx5 use the PHY subsystem.
>
>Shame. With just mlx, you will get a coverage of ~0. If you did the
>generic phylib work, you might get coverage of > 50% of the MAC
>drivers. It then becomes something useful and really used.

Yeah, that's what it is. But maybe someone would pick up the work and
do the phylib implementation too. Let's see.

>
>	 Andrew