RE: [PATCH v3 net-next 2/6] rtnetlink: Add new RTM_GETEECSTATE message to get SyncE status

-----Original Message-----
From: Sabrina Dubroca <sd@queasysnail.net>
Sent: Thursday, November 11, 2021 5:01 PM
To: Machnikowski, Maciej <redacted>
Subject: Re: [PATCH v3 net-next 2/6] rtnetlink: Add new RTM_GETEECSTATE
message to get SyncE status

Hello Maciej,

2021-11-10, 12:44:44 +0100, Maciej Machnikowski wrote:

quoted

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 5888492a5257..1d8662afd6bd 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h

@@ -185,6 +185,9 @@ enum {
 	RTM_GETNEXTHOPBUCKET,
 #define RTM_GETNEXTHOPBUCKET	RTM_GETNEXTHOPBUCKET

+	RTM_GETEECSTATE = 124,
+#define RTM_GETEECSTATE	RTM_GETEECSTATE

I'm not sure about this. All the other RTM_GETxxx are such that
RTM_GETxxx % 4 == 2. Following the current pattern, 124 should be
reserved for RTM_NEWxxx, and RTM_GETEECSTATE would be 126.

Also, why are you leaving a gap (which you end up filling in patch
4/6)?

Hmmm I missed that - is there any guide how to number them?
I'd be happy to follow the pattern there - will fix in next revision.

The gap is there as this was developed first - but most likely this part
Will be removed in next revision in favor of DPLL subsystem.

quoted

+
 	__RTM_MAX,
 #define RTM_MAX		(((__RTM_MAX + 3) & ~3) - 1)
 };

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 2af8aeeadadf..03bc773d0e69 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c

@@ -5467,6 +5467,83 @@ static int rtnl_stats_dump(struct sk_buff *skb,

struct netlink_callback *cb)

quoted

 	return skb->len;
 }

+static int rtnl_fill_eec_state(struct sk_buff *skb, struct net_device *dev,
+			       u32 portid, u32 seq, struct netlink_callback *cb,
+			       int flags, struct netlink_ext_ack *extack)
+{

[...]

quoted

+	nlh = nlmsg_put(skb, portid, seq, RTM_GETEECSTATE,

sizeof(*state_msg),

quoted

+			flags);
+	if (!nlh)
+		return -EMSGSIZE;
+
+	state_msg = nlmsg_data(nlh);
+	state_msg->ifindex = dev->ifindex;

Why stuff this in a struct instead of using an attribute?

Since it's the required parameter to identify what port is in question.
 

quoted

+
+	if (nla_put_u32(skb, IFLA_EEC_STATE, state))
+		return -EMSGSIZE;
+
+	if (!ops->ndo_get_eec_src)
+		goto end_msg;
+
+	err = ops->ndo_get_eec_src(dev, &src_idx, extack);
+	if (err)
+		return err;
+
+	if (nla_put_u32(skb, IFLA_EEC_SRC_IDX, src_idx))
+		return -EMSGSIZE;
+
+end_msg:
+	nlmsg_end(skb, nlh);
+	return 0;
+}
+

Thanks,

--
Sabrina