Re: [PATCH net-next v19 08/10] net: ethtool: tsinfo: Add support for reading tsinfo for a specific hwtstamp provider
From: Jakub Kicinski <kuba@kernel.org>
Date: 2024-11-11 23:12:35
Also in:
linux-doc, lkml
On Wed, 30 Oct 2024 14:54:50 +0100 Kory Maincent wrote:
quoted hunk ↗ jump to hunk
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; +}
quoted hunk ↗ jump to hunk
+++ 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