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.
