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:

quoted

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