Re: [PATCH net-next v19 08/10] net: ethtool: tsinfo: Add support for reading tsinfo for a specific hwtstamp provider

[Jakub Kicinski <kuba@kernel.org>]

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