Received-SPF: pass (google.com: domain of linux-kernel+bounces-91612-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 4 Mar 2024 19:27:33 -0800
From: Jakub Kicinski <kuba@kernel.org>
To: Kory Maincent <kory.maincent@bootlin.com>
Cc: Florian Fainelli <florian.fainelli@broadcom.com>, Broadcom internal
 kernel review list <bcm-kernel-feedback-list@broadcom.com>, Andrew Lunn
 <andrew@lunn.ch>, Heiner Kallweit <hkallweit1@gmail.com>, Russell King
 <linux@armlinux.org.uk>, "David S. Miller" <davem@davemloft.net>, Eric
 Dumazet <edumazet@google.com>, Paolo Abeni <pabeni@redhat.com>, Richard
 Cochran <richardcochran@gmail.com>, Radu Pirea
 <radu-nicolae.pirea@oss.nxp.com>, Jay Vosburgh <j.vosburgh@gmail.com>, Andy
 Gospodarek <andy@greyhouse.net>, Nicolas Ferre
 <nicolas.ferre@microchip.com>, Claudiu Beznea <claudiu.beznea@tuxon.dev>,
 Willem de Bruijn <willemdebruijn.kernel@gmail.com>, Jonathan Corbet
 <corbet@lwn.net>, Horatiu Vultur <horatiu.vultur@microchip.com>,
 UNGLinuxDriver@microchip.com, Simon Horman <horms@kernel.org>, Vladimir
 Oltean <vladimir.oltean@nxp.com>, Thomas Petazzoni
 <thomas.petazzoni@bootlin.com>, netdev@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, Maxime Chevallier
 <maxime.chevallier@bootlin.com>, Rahul Rameshbabu <rrameshbabu@nvidia.com>
Subject: Re: [PATCH net-next v9 12/13] net: ethtool: tsinfo: Add support for
 hwtstamp provider and get/set hwtstamp config
Message-ID: <20240304192733.1e8e08cc@kernel.org>
In-Reply-To: <20240226-feature_ptp_netnext-v9-12-455611549f21@bootlin.com>
References: <20240226-feature_ptp_netnext-v9-0-455611549f21@bootlin.com>
	<20240226-feature_ptp_netnext-v9-12-455611549f21@bootlin.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

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

> +		}
> +	}