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

From: "Machnikowski, Maciej" <maciej.machnikowski@intel.com>
To: Sabrina Dubroca <sd@queasysnail.net>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"intel-wired-lan@lists.osuosl.org"
	<intel-wired-lan@lists.osuosl.org>,
	"richardcochran@gmail.com" <richardcochran@gmail.com>,
	"abyagowi@fb.com" <abyagowi@fb.com>,
	"Nguyen, Anthony L" <anthony.l.nguyen@intel.com>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"kuba@kernel.org" <kuba@kernel.org>,
	"linux-kselftest@vger.kernel.org"
	<linux-kselftest@vger.kernel.org>,
	"idosch@idosch.org" <idosch@idosch.org>,
	"mkubecek@suse.cz" <mkubecek@suse.cz>,
	"saeed@kernel.org" <saeed@kernel.org>,
	"michael.chan@broadcom.com" <michael.chan@broadcom.com>,
	"petrm@nvidia.com" <petrm@nvidia.com>
Subject: RE: [PATCH v3 net-next 2/6] rtnetlink: Add new RTM_GETEECSTATE message to get SyncE status
Date: Tue, 16 Nov 2021 14:37:13 +0000	[thread overview]
Message-ID: <MW5PR11MB581243EFB3A84BD2912ACBF8EA999@MW5PR11MB5812.namprd11.prod.outlook.com> (raw)
In-Reply-To: <YY0+PmNU4MSGfgqA@hog>

> -----Original Message-----
> From: Sabrina Dubroca <sd@queasysnail.net>
> Sent: Thursday, November 11, 2021 5:01 PM
> To: Machnikowski, Maciej <maciej.machnikowski@intel.com>
> 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:
> > 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.

> > +
> >  	__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)
> >  	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)
> > +{
> [...]
> > +	nlh = nlmsg_put(skb, portid, seq, RTM_GETEECSTATE,
> sizeof(*state_msg),
> > +			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.

> > +
> > +	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