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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241111151232.6827f48b@kernel.org>
Date: Mon, 11 Nov 2024 15:12:32 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Kory Maincent <kory.maincent@...tlin.com>
Cc: Florian Fainelli <florian.fainelli@...adcom.com>, Broadcom internal
 kernel review list <bcm-kernel-feedback-list@...adcom.com>, Andrew Lunn
 <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>, Russell King
 <linux@...linux.org.uk>, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Richard
 Cochran <richardcochran@...il.com>, Radu Pirea
 <radu-nicolae.pirea@....nxp.com>, Jay Vosburgh <j.vosburgh@...il.com>, Andy
 Gospodarek <andy@...yhouse.net>, Nicolas Ferre
 <nicolas.ferre@...rochip.com>, Claudiu Beznea <claudiu.beznea@...on.dev>,
 Willem de Bruijn <willemdebruijn.kernel@...il.com>, Jonathan Corbet
 <corbet@....net>, Horatiu Vultur <horatiu.vultur@...rochip.com>,
 UNGLinuxDriver@...rochip.com, Simon Horman <horms@...nel.org>, Vladimir
 Oltean <vladimir.oltean@....com>, donald.hunter@...il.com,
 danieller@...dia.com, ecree.xilinx@...il.com, Andrew Lunn
 <andrew+netdev@...n.ch>, Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
 linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
 linux-doc@...r.kernel.org, Maxime Chevallier
 <maxime.chevallier@...tlin.com>, Rahul Rameshbabu <rrameshbabu@...dia.com>,
 Willem de Bruijn <willemb@...gle.com>, Shannon Nelson
 <shannon.nelson@....com>, Alexandra Winter <wintera@...ux.ibm.com>, Jacob
 Keller <jacob.e.keller@...el.com>
Subject: Re: [PATCH net-next v19 08/10] net: ethtool: tsinfo: Add support
 for reading tsinfo for a specific hwtstamp provider

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