Re: [PATCH net-next v15 13/14] net: ethtool: tsinfo: Add support for hwtstamp provider and get/set hwtstamp config

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 12 Jun 2024 17:04:13 +0200 Kory Maincent wrote:
> Enhance 'get' command to retrieve tsinfo of hwtstamp providers within a
> network topology and read current hwtstamp configuration.
> 
> Introduce support for ETHTOOL_MSG_TSINFO_SET ethtool netlink socket to
> configure hwtstamp of a PHC provider. Note that simultaneous hwtstamp
> isn't supported; configuring a new one disables the previous setting.
> 
> Also, add support for a specific dump command to retrieve all hwtstamp
> providers within the network topology, with added functionality for
> filtered dump to target a single interface.

Could you split this up, a little bit? It's rather large for a core
change.

>  Desired behavior is passed into the kernel and to a specific device by
> -calling ioctl(SIOCSHWTSTAMP) with a pointer to a struct ifreq whose
> -ifr_data points to a struct hwtstamp_config. The tx_type and
> -rx_filter are hints to the driver what it is expected to do. If
> -the requested fine-grained filtering for incoming packets is not
> +calling the tsinfo netlink socket ETHTOOL_MSG_TSINFO_SET.
> +The ETHTOOL_A_TSINFO_TX_TYPES, ETHTOOL_A_TSINFO_RX_FILTERS and
> +ETHTOOL_A_TSINFO_HWTSTAMP_FLAGS netlink attributes are then used to set the
> +struct hwtstamp_config accordingly.

nit: EHTOOL_A* defines in `` `` quotes?

> +		if (hwtstamp && ptp_clock_phydev(hwtstamp->ptp) == phydev) {
> +			rcu_assign_pointer(dev->hwtstamp, NULL);
> +			synchronize_rcu();
>  			kfree(hwtstamp);

Could you add an rcu_head to this struct and use kfree_rcu()
similarly later use an rcu call to do the dismantle?
synchronize_rcu() can be slow.

> +enum {
> +	ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_UNSPEC,
> +	ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_INDEX,		/* u32 */
> +	ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_QUALIFIER,		/* u32 */
> +
> +	__ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_CNT,
> +	ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_MAX = (__ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_CNT - 1)
> +};
> +
>  

nit: double new line

> +const struct nla_policy ethnl_tsinfo_get_policy[ETHTOOL_A_TSINFO_MAX + 1] = {
>  	[ETHTOOL_A_TSINFO_HEADER]		=
>  		NLA_POLICY_NESTED(ethnl_header_policy_stats),
> +	[ETHTOOL_A_TSINFO_GHWTSTAMP] =
> +		NLA_POLICY_MAX(NLA_U8, 1),

I think this can be an NLA_FLAG, but TBH I'm also confused about 
the semantics. Can you explain what it does from user perspective?

> +	[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER] =
> +		NLA_POLICY_NESTED(ethnl_tsinfo_hwtstamp_provider_policy),
>  };
>  
> +static int tsinfo_parse_hwtstamp_provider(const struct nlattr *nest,
> +					  struct hwtst_provider *hwtst,
> +					  struct netlink_ext_ack *extack,
> +					  bool *mod)
> +{
> +	struct nlattr *tb[ARRAY_SIZE(ethnl_tsinfo_hwtstamp_provider_policy)];

Could you find a more sensible name for this policy?

> +	int ret;
> +
> +	ret = nla_parse_nested(tb,
> +			       ARRAY_SIZE(ethnl_tsinfo_hwtstamp_provider_policy) - 1,
> +			       nest,
> +			       ethnl_tsinfo_hwtstamp_provider_policy, extack);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (NL_REQ_ATTR_CHECK(extack, nest, tb, ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_INDEX) ||
> +	    NL_REQ_ATTR_CHECK(extack, nest, tb, ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_QUALIFIER))

nit: wrap at 80 chars, if you can, please

> +		return -EINVAL;
> +
> +	ethnl_update_u32(&hwtst->index,
> +			 tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_INDEX],
> +			 mod);
> +	ethnl_update_u32(&hwtst->qualifier,
> +			 tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_QUALIFIER],
> +			 mod);
> +
> +	return 0;
> +}

>  static int tsinfo_prepare_data(const struct ethnl_req_info *req_base,
>  			       struct ethnl_reply_data *reply_base,
>  			       const struct genl_info *info)
>  {
>  	struct tsinfo_reply_data *data = TSINFO_REPDATA(reply_base);
> +	struct tsinfo_req_info *req = TSINFO_REQINFO(req_base);
>  	struct net_device *dev = reply_base->dev;
>  	int ret;
>  
>  	ret = ethnl_ops_begin(dev);
>  	if (ret < 0)
>  		return ret;
> +
> +	if (req->get_hwtstamp) {
> +		struct kernel_hwtstamp_config cfg = {};
> +
> +		if (!dev->netdev_ops->ndo_hwtstamp_get) {
> +			ret = -EOPNOTSUPP;
> +			goto out;
> +		}
> +
> +		ret = dev_get_hwtstamp_phylib(dev, &cfg);
> +		data->hwtst_config.tx_type = BIT(cfg.tx_type);
> +		data->hwtst_config.rx_filter = BIT(cfg.rx_filter);
> +		data->hwtst_config.flags = BIT(cfg.flags);
> +		goto out;

This is wrong AFAICT, everything up to this point was a nit pick ;)
Please take a look at 89e281ebff72e6, I think you're reintroducing a
form of the same bug. If ETHTOOL_FLAG_STATS was set, you gotta run stats
init.

Perhaps you can move the stats getting up, and turn this code into if
/ else if / else, without the goto.

> +int ethnl_tsinfo_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
> +{
> +	struct ethnl_tsinfo_dump_ctx *ctx = (void *)cb->ctx;
> +	struct net *net = sock_net(skb->sk);
> +	struct net_device *dev;
> +	int ret = 0;
> +
> +	rtnl_lock();
> +	if (ctx->req_info->base.dev) {
> +		ret = ethnl_tsinfo_dump_one_dev(skb,
> +						ctx->req_info->base.dev,
> +						cb);
> +	} else {
> +		for_each_netdev_dump(net, dev, ctx->pos_ifindex) {
> +			ret = ethnl_tsinfo_dump_one_dev(skb, dev, cb);
> +			if (ret < 0 && ret != -EOPNOTSUPP)
> +				break;
> +			ctx->pos_phcindex = 0;
> +		}
> +	}
> +	rtnl_unlock();
> +
> +	if (ret == -EMSGSIZE && skb->len)
> +		return skb->len;
> +	return ret;

You can just return ret without the if converting to skb->len
af_netlink will handle the EMSGSIZE errors in the same way.