On Wed, 30 Oct 2024 14:54:50 +0100 Kory Maincent wrote:
> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index 0d62363dbd9d..a50cddd36b6d 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -745,12 +745,45 @@ int ethtool_check_ops(const struct ethtool_ops *ops)
> return 0;
> }
>
> +int ethtool_get_ts_info_by_phc(struct net_device *dev,
> + struct kernel_ethtool_ts_info *info,
> + struct hwtstamp_provider *hwtstamp)
> +{
> + const struct ethtool_ops *ops = dev->ethtool_ops;
> + int err = 0;
> +
> + memset(info, 0, sizeof(*info));
> + info->cmd = ETHTOOL_GET_TS_INFO;
> + info->phc_qualifier = hwtstamp->qualifier;
> + info->phc_index = -1;
> +
> + if (!netdev_support_hwtstamp(dev, hwtstamp))
> + return -ENODEV;
> +
> + if (ptp_clock_from_phylib(hwtstamp->ptp) &&
> + phy_has_tsinfo(ptp_clock_phydev(hwtstamp->ptp)))
> + err = phy_ts_info(ptp_clock_phydev(hwtstamp->ptp), info);
> +
> + if (ptp_clock_from_netdev(hwtstamp->ptp) && ops->get_ts_info)
> + err = ops->get_ts_info(dev, info);
Is it not possibly to cleanly fold this into __ethtool_get_ts_info()?
looks like half of this function is a copy/paste.
> + info->so_timestamping |= SOF_TIMESTAMPING_RX_SOFTWARE |
> + SOF_TIMESTAMPING_SOFTWARE;
> +
> + return err;
> +}
> +++ b/net/ethtool/ts.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef _NET_ETHTOOL_TS_H
> +#define _NET_ETHTOOL_TS_H
> +
> +#include "netlink.h"
> +
> +struct hwtst_provider {
> + int index;
> + u32 qualifier;
> +};
> +
> +static const struct nla_policy
> +ethnl_ts_hwtst_prov_policy[ETHTOOL_A_TS_HWTSTAMP_PROVIDER_MAX + 1] = {
> + [ETHTOOL_A_TS_HWTSTAMP_PROVIDER_INDEX] =
> + NLA_POLICY_MIN(NLA_S32, 0),
> + [ETHTOOL_A_TS_HWTSTAMP_PROVIDER_QUALIFIER] =
> + NLA_POLICY_MAX(NLA_U32, HWTSTAMP_PROVIDER_QUALIFIER_CNT - 1)
> +};
> +
> +static inline int ts_parse_hwtst_provider(const struct nlattr *nest,
why not just put it in tsinfo.c and call it from the tsconfig.c;
or vice versa ??
> + struct hwtst_provider *hwtst,
> + struct netlink_ext_ack *extack,
> + bool *mod)
> +{
> + struct nlattr *tb[ARRAY_SIZE(ethnl_ts_hwtst_prov_policy)];
> + int ret;
> +
> + ret = nla_parse_nested(tb,
> + ARRAY_SIZE(ethnl_ts_hwtst_prov_policy) - 1,
> + nest,
> + if (req->hwtst.index != -1) {
> + struct hwtstamp_provider hwtstamp;
please name the hwtstamp_provider variables something more sensible
Maybe tsprov or hwprov? We already call the timestamps and the config
hwtstamp. It makes the code much harder to read.
> - if (ts_info->phc_index >= 0)
> + if (ts_info->phc_index >= 0) {
> + /* _TSINFO_HWTSTAMP_PROVIDER */
> + len += 2 * nla_total_size(sizeof(u32));
and a nest?
> len += nla_total_size(sizeof(u32)); /* _TSINFO_PHC_INDEX */
> + }
> if (req_base->flags & ETHTOOL_FLAG_STATS)
> len += nla_total_size(0) + /* _TSINFO_STATS */
> nla_total_size_64bit(sizeof(u64)) * ETHTOOL_TS_STAT_CNT;
> + reply_data->base.dev = NULL;
> + if (!ret && ehdr)
> + genlmsg_end(skb, ehdr);
> + else
> + genlmsg_cancel(skb, ehdr);
please use goto and a separate path for error handling
clarity > LoC
