On Mon, 26 Feb 2024 14:40:03 +0100 Kory Maincent wrote:
> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index b3f45c307301..37071929128a 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -426,6 +426,7 @@ const char sof_timestamping_names[][ETH_GSTRING_LEN] = {
> [const_ilog2(SOF_TIMESTAMPING_OPT_TX_SWHW)] = "option-tx-swhw",
> [const_ilog2(SOF_TIMESTAMPING_BIND_PHC)] = "bind-phc",
> [const_ilog2(SOF_TIMESTAMPING_OPT_ID_TCP)] = "option-id-tcp",
> + [const_ilog2(SOF_TIMESTAMPING_GHWTSTAMP)] = "get-hwtstamp",
What is this new SOF_TIMESTAMPING_GHWTSTAMP? If there's
a good reason for it to exist it should be documented in
Documentation/networking/timestamping.rst
> +const struct nla_policy ethnl_tsinfo_get_policy[ETHTOOL_A_TSINFO_MAX + 1] = {
> [ETHTOOL_A_TSINFO_HEADER] =
> NLA_POLICY_NESTED(ethnl_header_policy),
> + [ETHTOOL_A_TSINFO_TIMESTAMPING] = { .type = NLA_NESTED },
> + [ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_NEST] = { .type = NLA_NESTED },
link the policy by NLA_POLICY_NESTED() so that user space can inspect
the sub-layers via the control family.
> };
>
> +static int
> +tsinfo_parse_request(struct ethnl_req_info *req_base, struct nlattr **tb,
> + struct netlink_ext_ack *extack)
> +{
> + struct nlattr *hwtst_tb[ARRAY_SIZE(ethnl_tsinfo_hwtstamp_provider_policy)];
> + struct tsinfo_req_info *req = TSINFO_REQINFO(req_base);
> + unsigned long val = 0, mask = 0;
> + int ret;
> +
> + req->hwtst.index = -1;
> +
> + if (tb[ETHTOOL_A_TSINFO_TIMESTAMPING]) {
> + ret = ethnl_parse_bitset(&val, &mask, __SOF_TIMESTAMPING_CNT,
> + tb[ETHTOOL_A_TSINFO_TIMESTAMPING],
> + sof_timestamping_names, extack);
> + if (ret < 0)
> + return ret;
> +
> + if (val & SOF_TIMESTAMPING_GHWTSTAMP) {
> + /* We support only the get of the current hwtstamp config
> + * for now.
> + */
> + if (tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_NEST])
NL_SET_ERR_MSG_ATTR(...)
> + return -EOPNOTSUPP;
> +
> + req->get_hwtstamp = true;
> + return 0;
> + }
> + }
> +
> + if (!tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_NEST])
> + return 0;
> +
> + ret = nla_parse_nested(hwtst_tb,
> + ARRAY_SIZE(ethnl_tsinfo_hwtstamp_provider_policy) - 1,
> + tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_NEST],
> + ethnl_tsinfo_hwtstamp_provider_policy, extack);
> + if (ret < 0)
> + return ret;
> +
> + if (!hwtst_tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_INDEX] ||
> + !hwtst_tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_QUALIFIER])
> + return -EINVAL;
NL_REQ_ATTR_CHECK()
> + ret = nla_get_u32(hwtst_tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_INDEX]);
> + if (ret < 0)
> + return -EINVAL;
How's the get_u32 going to return a negative value?
That's the purpose of this check?
The policy should contain the max expected value - NLA_POLICY_MAX().
> + req->hwtst.index = ret;
> +
> + ret = nla_get_u32(hwtst_tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_QUALIFIER]);
> + if (ret < 0 || HWTSTAMP_PROVIDER_QUALIFIER_CNT <= ret)
> + return -EINVAL;
> + req->hwtst.qualifier = ret;
> +
> + 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;
> - ret = __ethtool_get_ts_info(dev, &data->ts_info);
> +
> + if (!netif_device_present(dev)) {
ethnl_ops_begin() checks for presence
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + 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 goto could be an else
> + if (req->hwtst.index != -1) {
> + struct hwtstamp_provider hwtstamp;
> +
> + hwtstamp.ptp = ptp_clock_get_by_index(req->hwtst.index);
> + if (!hwtstamp.ptp) {
> + ret = -ENODEV;
> + goto out;
> + }
> + hwtstamp.qualifier = req->hwtst.qualifier;
> +
> + ret = ethtool_get_ts_info_by_phc(dev, &data->ts_info,
> + &hwtstamp);
> + } else {
> + ret = __ethtool_get_ts_info(dev, &data->ts_info);
Not sure I grok why we need 3 forms of getting the tstamp config.
Please make sure to always update
Documentation/networking/ethtool-netlink.rst
when extending ethtool-nl.
> + }
> +
> +out:
> ethnl_ops_complete(dev);
>
> return ret;
> }
> + if (ts_info->phc_index >= 0) {
> + /* _TSINFO_HWTSTAMP_PROVIDER_NEST */
> + len += nla_total_size(sizeof(u32) * 2);
That translates to two raw u32s into a single attribute.
Is that what you mean?
> len += nla_total_size(sizeof(u32)); /* _TSINFO_PHC_INDEX */
> + }
>
> return len;
> }
>
> + if (ts_info->phc_index >= 0) {
> + ret = nla_put_u32(skb, ETHTOOL_A_TSINFO_PHC_INDEX,
> + ts_info->phc_index);
> + if (ret)
> + return -EMSGSIZE;
> +
> + nest = nla_nest_start(skb, ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_NEST);
> + if (!nest)
> + return -EMSGSIZE;
> +
> + ret = nla_put_u32(skb,
> + ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_INDEX,
> + ts_info->phc_index);
You can assume nla_put_u32 only returns EMSGSIZE, so doing:
if (nla_put_u32(....) ||
nla_put_u32(....))
return -EMSGSIZE;
is generally considered to be fine.
> + if (ret) {
> + nla_nest_cancel(skb, nest);
> + return ret;
> + }
> +
> + ret = nla_put_u32(skb,
> + ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_QUALIFIER,
> + ts_info->phc_qualifier);
> + if (ret) {
> + nla_nest_cancel(skb, nest);
> + return ret;
> + }
> +
> + nla_nest_end(skb, nest);
> + }
> + return 0;
> +}
> +static int ethnl_set_tsinfo(struct ethnl_req_info *req_base,
> + struct genl_info *info)
> +{
> + struct nlattr *hwtst_tb[ARRAY_SIZE(ethnl_tsinfo_hwtstamp_provider_policy)];
> + unsigned long mask = 0, req_rx_filter, req_tx_type;
> + struct kernel_hwtstamp_config hwtst_config = {0};
> + struct net_device *dev = req_base->dev;
> + struct hwtstamp_provider hwtstamp;
> + struct nlattr **tb = info->attrs;
> + int ret, phc_index = 0;
> + bool mod = false;
> +
> + BUILD_BUG_ON(__HWTSTAMP_TX_CNT > 32);
> + BUILD_BUG_ON(__HWTSTAMP_FILTER_CNT > 32);
> +
> + if (!netif_device_present(dev))
> + return -ENODEV;
> +
> + if (tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_NEST]) {
> + ret = nla_parse_nested(hwtst_tb,
> + ARRAY_SIZE(ethnl_tsinfo_hwtstamp_provider_policy) - 1,
> + tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_NEST],
> + ethnl_tsinfo_hwtstamp_provider_policy, info->extack);
> + if (ret < 0)
> + return ret;
> +
> + if (!hwtst_tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_INDEX] ||
> + !hwtst_tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_QUALIFIER])
> + return -EINVAL;
> +
> + memcpy(&hwtstamp, &dev->hwtstamp, sizeof(hwtstamp));
> + if (hwtstamp.ptp)
> + phc_index = ptp_clock_index(hwtstamp.ptp);
> +
> + ethnl_update_u32(&phc_index,
> + hwtst_tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_INDEX],
> + &mod);
> + ethnl_update_u32(&hwtstamp.qualifier,
> + hwtst_tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_QUALIFIER],
> + &mod);
> +
> + /* Does the hwtstamp supported in the netdev topology */
> + if (mod) {
> + hwtstamp.ptp = ptp_clock_get_by_index(phc_index);
This just returns a pointer without any refcounting, right?
What guarantees the ptp object doesn't disappear?
> + if (!hwtstamp.ptp)
> + return -ENODEV;
> +
> + if (!netdev_support_hwtstamp(dev, &hwtstamp))
> + return -ENODEV;
these need extacks
> + }
> + }
