All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jakub Kicinski <kuba@kernel.org>
To: Maciej Machnikowski <maciej.machnikowski@intel.com>
Cc: netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
	richardcochran@gmail.com, abyagowi@fb.com,
	anthony.l.nguyen@intel.com, davem@davemloft.net,
	linux-kselftest@vger.kernel.org
Subject: Re: [PATCH net-next 1/2] rtnetlink: Add new RTM_GETEECSTATE message to get SyncE status
Date: Tue, 31 Aug 2021 06:44:10 -0700	[thread overview]
Message-ID: <20210831064410.635eb329@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> (raw)
In-Reply-To: <20210831115233.239720-2-maciej.machnikowski@intel.com>

On Tue, 31 Aug 2021 13:52:32 +0200 Maciej Machnikowski wrote:
> This patch series introduces basic interface for reading the Ethernet
> Equipment Clock (EEC) state on a SyncE capable device. This state gives
> information about the source of the syntonization signal and the state
> of EEC. This interface is required to implement Synchronization Status
> Messaging on upper layers.
> 
> Initial implementation returns:
>  - SyncE EEC state
>  - Source of signal driving SyncE EEC (SyncE, GNSS, PTP or External)
>  - Current index of the pin driving the EEC to track multiple recovered
>    clock paths
> 
> SyncE EEC state read needs to be implemented as ndo_get_eec_state
> function.
> 
> Signed-off-by: Maciej Machnikowski <maciej.machnikowski@intel.com>

> +#define EEC_FLAG_STATE_VAL	(1 << 0)
> +#define EEC_FLAG_SRC_VAL	(1 << 1)
> +#define EEC_FLAG_PIN_VAL	(1 << 2)
> +
> +struct if_eec_state_msg {
> +	__u8 flags;
> +	__u8 state;
> +	__u8 src;
> +	__u8 pin;
> +	__u32 ifindex;
> +};

Break it up into attributes, please. It's the idiomatic way of dealing
with multiple values these days in netlink. Makes validation,
extensiblility, feature discover etc. etc. much easier down the line.

> +static int rtnl_fill_eec_state(struct sk_buff *msg, struct net_device *dev,
> +			       u32 portid, u32 seq, struct netlink_callback *cb,
> +			       int flags, struct netlink_ext_ack *extack)
> +{
> +	const struct net_device_ops *ops = dev->netdev_ops;
> +	struct if_eec_state_msg *state;
> +	struct nlmsghdr *nlh;
> +	int err;
> +
> +	ASSERT_RTNL();
> +
> +	if (!ops->ndo_get_eec_state)
> +		return -EOPNOTSUPP;
> +
> +	nlh = nlmsg_put(msg, portid, seq, RTM_GETEECSTATE,
> +			sizeof(*state), flags);
> +	if (!nlh)
> +		return -EMSGSIZE;
> +
> +	state = nlmsg_data(nlh);
> +
> +	memset(state, 0, sizeof(*state));
> +	err = ops->ndo_get_eec_state(dev, state, extack);
> +	if (err)
> +		return err;

return ops->ndo_get_eec_state(dev, state, extack);

Even tho it's perfectly fine as is IMO there are bots out there
matching on this pattern so let's not feed them.

> +	return 0;
> +}
> +
> +static int rtnl_eec_state_get(struct sk_buff *skb, struct nlmsghdr *nlh,
> +			      struct netlink_ext_ack *extack)
> +{
> +	struct net *net = sock_net(skb->sk);
> +	struct if_eec_state_msg *state;
> +	struct net_device *dev;
> +	struct sk_buff *nskb;
> +	int err;
> +
> +	state = nlmsg_data(nlh);
> +	if (state->ifindex > 0)
> +		dev = __dev_get_by_index(net, state->ifindex);
> +	else
> +		return -EINVAL;

Keep the expected path unindented:

	if (state->ifindex <= 0)
		return -EINVAL;

	dev = __dev_get_by_index(net, state->ifindex);
	if (!dev)
		return -ENODEV;

That said I'm not sure why rtnl_stats_get() checks the ifindex is
positive in the first place (which is what I assumed inspired you).
We can just call __dev_get_by_index() and have it fail AFAICS.

> +	if (!dev)
> +		return -ENODEV;
> +
> +	nskb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> +	if (!nskb)
> +		return -ENOBUFS;
> +
> +	err = rtnl_fill_eec_state(nskb, dev, NETLINK_CB(skb).portid,
> +				  nlh->nlmsg_seq, NULL, nlh->nlmsg_flags,
> +				  extack);
> +	if (err < 0)
> +		kfree_skb(nskb);
> +	else
> +		err = rtnl_unicast(nskb, net, NETLINK_CB(skb).portid);
> +
> +	return err;
> +}

WARNING: multiple messages have this Message-ID (diff)
From: Jakub Kicinski <kuba@kernel.org>
To: intel-wired-lan@osuosl.org
Subject: [Intel-wired-lan] [PATCH net-next 1/2] rtnetlink: Add new RTM_GETEECSTATE message to get SyncE status
Date: Tue, 31 Aug 2021 06:44:10 -0700	[thread overview]
Message-ID: <20210831064410.635eb329@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> (raw)
In-Reply-To: <20210831115233.239720-2-maciej.machnikowski@intel.com>

On Tue, 31 Aug 2021 13:52:32 +0200 Maciej Machnikowski wrote:
> This patch series introduces basic interface for reading the Ethernet
> Equipment Clock (EEC) state on a SyncE capable device. This state gives
> information about the source of the syntonization signal and the state
> of EEC. This interface is required to implement Synchronization Status
> Messaging on upper layers.
> 
> Initial implementation returns:
>  - SyncE EEC state
>  - Source of signal driving SyncE EEC (SyncE, GNSS, PTP or External)
>  - Current index of the pin driving the EEC to track multiple recovered
>    clock paths
> 
> SyncE EEC state read needs to be implemented as ndo_get_eec_state
> function.
> 
> Signed-off-by: Maciej Machnikowski <maciej.machnikowski@intel.com>

> +#define EEC_FLAG_STATE_VAL	(1 << 0)
> +#define EEC_FLAG_SRC_VAL	(1 << 1)
> +#define EEC_FLAG_PIN_VAL	(1 << 2)
> +
> +struct if_eec_state_msg {
> +	__u8 flags;
> +	__u8 state;
> +	__u8 src;
> +	__u8 pin;
> +	__u32 ifindex;
> +};

Break it up into attributes, please. It's the idiomatic way of dealing
with multiple values these days in netlink. Makes validation,
extensiblility, feature discover etc. etc. much easier down the line.

> +static int rtnl_fill_eec_state(struct sk_buff *msg, struct net_device *dev,
> +			       u32 portid, u32 seq, struct netlink_callback *cb,
> +			       int flags, struct netlink_ext_ack *extack)
> +{
> +	const struct net_device_ops *ops = dev->netdev_ops;
> +	struct if_eec_state_msg *state;
> +	struct nlmsghdr *nlh;
> +	int err;
> +
> +	ASSERT_RTNL();
> +
> +	if (!ops->ndo_get_eec_state)
> +		return -EOPNOTSUPP;
> +
> +	nlh = nlmsg_put(msg, portid, seq, RTM_GETEECSTATE,
> +			sizeof(*state), flags);
> +	if (!nlh)
> +		return -EMSGSIZE;
> +
> +	state = nlmsg_data(nlh);
> +
> +	memset(state, 0, sizeof(*state));
> +	err = ops->ndo_get_eec_state(dev, state, extack);
> +	if (err)
> +		return err;

return ops->ndo_get_eec_state(dev, state, extack);

Even tho it's perfectly fine as is IMO there are bots out there
matching on this pattern so let's not feed them.

> +	return 0;
> +}
> +
> +static int rtnl_eec_state_get(struct sk_buff *skb, struct nlmsghdr *nlh,
> +			      struct netlink_ext_ack *extack)
> +{
> +	struct net *net = sock_net(skb->sk);
> +	struct if_eec_state_msg *state;
> +	struct net_device *dev;
> +	struct sk_buff *nskb;
> +	int err;
> +
> +	state = nlmsg_data(nlh);
> +	if (state->ifindex > 0)
> +		dev = __dev_get_by_index(net, state->ifindex);
> +	else
> +		return -EINVAL;

Keep the expected path unindented:

	if (state->ifindex <= 0)
		return -EINVAL;

	dev = __dev_get_by_index(net, state->ifindex);
	if (!dev)
		return -ENODEV;

That said I'm not sure why rtnl_stats_get() checks the ifindex is
positive in the first place (which is what I assumed inspired you).
We can just call __dev_get_by_index() and have it fail AFAICS.

> +	if (!dev)
> +		return -ENODEV;
> +
> +	nskb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> +	if (!nskb)
> +		return -ENOBUFS;
> +
> +	err = rtnl_fill_eec_state(nskb, dev, NETLINK_CB(skb).portid,
> +				  nlh->nlmsg_seq, NULL, nlh->nlmsg_flags,
> +				  extack);
> +	if (err < 0)
> +		kfree_skb(nskb);
> +	else
> +		err = rtnl_unicast(nskb, net, NETLINK_CB(skb).portid);
> +
> +	return err;
> +}

  reply	other threads:[~2021-08-31 13:44 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-31 11:52 [PATCH net-next 0/2] Add RTNL interface for SyncE EEC state Maciej Machnikowski
2021-08-31 11:52 ` [Intel-wired-lan] " Maciej Machnikowski
2021-08-31 11:52 ` [PATCH net-next 1/2] rtnetlink: Add new RTM_GETEECSTATE message to get SyncE status Maciej Machnikowski
2021-08-31 11:52   ` [Intel-wired-lan] " Maciej Machnikowski
2021-08-31 13:44   ` Jakub Kicinski [this message]
2021-08-31 13:44     ` Jakub Kicinski
2021-08-31 11:52 ` [PATCH net-next 2/2] ice: add support for reading SyncE DPLL state Maciej Machnikowski
2021-08-31 11:52   ` [Intel-wired-lan] " Maciej Machnikowski
2021-09-03 15:14 [RFC v4 net-next 0/2] Add RTNL interface for SyncE Maciej Machnikowski
2021-09-03 15:14 ` [PATCH net-next 1/2] rtnetlink: Add new RTM_GETEECSTATE message to get SyncE status Maciej Machnikowski
2021-09-03 16:18   ` Stephen Hemminger
2021-09-03 22:20     ` Machnikowski, Maciej
2021-09-03 22:14   ` Jakub Kicinski
2021-09-06 18:30     ` Machnikowski, Maciej
2021-09-06 18:39       ` Jakub Kicinski
2021-09-06 19:01         ` Machnikowski, Maciej
2021-09-07  1:01           ` Jakub Kicinski
2021-09-07  8:50             ` Machnikowski, Maciej
2021-09-07 14:55               ` Jakub Kicinski
2021-09-07 15:47                 ` Machnikowski, Maciej
2021-09-07 19:47                   ` Jakub Kicinski
2021-09-08  8:03                     ` Machnikowski, Maciej
2021-09-08 16:21                       ` Jakub Kicinski
2021-09-08 17:30                         ` Machnikowski, Maciej
2021-09-08 19:34                           ` Andrew Lunn
2021-09-08 20:27                             ` Machnikowski, Maciej
2021-09-08 22:20                             ` Jakub Kicinski
2021-09-08 22:59                               ` Andrew Lunn
2021-09-09  2:09                                 ` Richard Cochran
2021-09-09  8:18                                   ` Machnikowski, Maciej
2021-09-10 14:14                                     ` Richard Cochran
2021-09-08 22:18                           ` Jakub Kicinski
2021-09-08 23:14                             ` Andrew Lunn
2021-09-08 23:58                               ` Jakub Kicinski
2021-09-09  8:26                                 ` Machnikowski, Maciej
2021-09-09  9:24                                   ` Machnikowski, Maciej
2021-09-09 10:15                                     ` David Miller
2021-09-09  8:11                             ` Machnikowski, Maciej
2021-09-13  8:50                         ` Ido Schimmel
2021-09-21 13:36                         ` Ido Schimmel
2021-09-21 13:15   ` Ido Schimmel
2021-09-21 13:37     ` Machnikowski, Maciej
2021-09-21 14:58       ` Ido Schimmel
2021-09-21 21:14         ` Jakub Kicinski
2021-09-22  6:22           ` Ido Schimmel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210831064410.635eb329@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com \
    --to=kuba@kernel.org \
    --cc=abyagowi@fb.com \
    --cc=anthony.l.nguyen@intel.com \
    --cc=davem@davemloft.net \
    --cc=intel-wired-lan@lists.osuosl.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=maciej.machnikowski@intel.com \
    --cc=netdev@vger.kernel.org \
    --cc=richardcochran@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.