netdev - Re: [PATCH net-next v5 15/16] net ethtool: net: Let the active time stamping layer be selectable

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: <ac520b3b-bf70-4643-a259-83e91dd330a6@broadcom.com>
Date: Mon, 9 Oct 2023 14:28:38 -0700
From: Florian Fainelli <florian.fainelli@...adcom.com>
To: Köry Maincent <kory.maincent@...tlin.com>,
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-doc@...r.kernel.org
Cc: Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Jonathan Corbet <corbet@....net>,
 Jay Vosburgh <j.vosburgh@...il.com>, Andy Gospodarek <andy@...yhouse.net>,
 Nicolas Ferre <nicolas.ferre@...rochip.com>,
 Claudiu Beznea <claudiu.beznea@...on.dev>,
 Horatiu Vultur <horatiu.vultur@...rochip.com>, UNGLinuxDriver@...rochip.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>, Richard Cochran <richardcochran@...il.com>,
 Radu Pirea <radu-nicolae.pirea@....nxp.com>,
 Willem de Bruijn <willemdebruijn.kernel@...il.com>,
 Vladimir Oltean <vladimir.oltean@....com>, Michael Walle <michael@...le.cc>,
 Jacob Keller <jacob.e.keller@...el.com>,
 Maxime Chevallier <maxime.chevallier@...tlin.com>
Subject: Re: [PATCH net-next v5 15/16] net ethtool: net: Let the active time
 stamping layer be selectable

On 10/9/23 08:51, Köry Maincent wrote:
> From: Kory Maincent <kory.maincent@...tlin.com>
> 
> Now that the current timestamp is saved in a variable lets add the
> ETHTOOL_MSG_TS_SET ethtool netlink socket to make it selectable.
> 
> Signed-off-by: Kory Maincent <kory.maincent@...tlin.com>
> ---

[snip]

> +static int ethnl_set_ts_validate(struct ethnl_req_info *req_info,
> +				 struct genl_info *info)
> +{
> +	struct nlattr **tb = info->attrs;
> +	const struct net_device_ops *ops = req_info->dev->netdev_ops;
> +
> +	if (!tb[ETHTOOL_A_TS_LAYER])
> +		return 0;
> +
> +	if (!ops->ndo_hwtstamp_set)
> +		return -EOPNOTSUPP;

I would check for this first, in all likelihood this is what most 
drivers currently do not support, no need to event de-reference the 
array of attributes.

> +
> +	return 1;
> +}
> +
> +static int ethnl_set_ts(struct ethnl_req_info *req_info, struct genl_info *info)
> +{
> +	struct net_device *dev = req_info->dev;
> +	const struct ethtool_ops *ops = dev->ethtool_ops;
> +	struct kernel_hwtstamp_config config = {0};
> +	struct nlattr **tb = info->attrs;
> +	bool mod = false;
> +	u32 ts_layer;
> +	int ret;
> +
> +	ts_layer = dev->ts_layer;
> +	ethnl_update_u32(&ts_layer, tb[ETHTOOL_A_TS_LAYER], &mod);
> +
> +	if (!mod)
> +		return 0;
> +
> +	if (ts_layer & NETDEV_TIMESTAMPING && !ops->get_ts_info) {
> +		NL_SET_ERR_MSG_ATTR(info->extack, tb[ETHTOOL_A_TS_LAYER],
> +				    "this device cannot support timestamping");

Maybe expand the extended ack with "this devices does not support 
MAC-based timestamping"

> +		return -EINVAL;
> +	}
> +
> +	if (ts_layer & PHYLIB_TIMESTAMPING && !phy_has_tsinfo(dev->phydev)) {
> +		NL_SET_ERR_MSG_ATTR(info->extack, tb[ETHTOOL_A_TS_LAYER],
> +				    "this device cannot support timestamping");

Likewise, detail which kind of timestamping is not supported.

> +		return -EINVAL;
> +	}
> +
> +	/* Disable time stamping in the current layer. */
> +	if (netif_device_present(dev) &&
> +	    dev->ts_layer & (PHYLIB_TIMESTAMPING | NETDEV_TIMESTAMPING)) {
> +		ret = dev_set_hwtstamp_phylib(dev, &config, info->extack);

Can we still land in this function even if no changes to the 
timestamping configuration has been made? If so, would suggest first 
getting the current configuration and compare it with the user-supplied 
configuration if there are no changes, return.

> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	dev->ts_layer = ts_layer;
> +
> +	return 1;
> +}
> +
>   const struct ethnl_request_ops ethnl_ts_request_ops = {
>   	.request_cmd		= ETHTOOL_MSG_TS_GET,
>   	.reply_cmd		= ETHTOOL_MSG_TS_GET_REPLY,
> @@ -69,6 +132,9 @@ const struct ethnl_request_ops ethnl_ts_request_ops = {
>   	.prepare_data		= ts_prepare_data,
>   	.reply_size		= ts_reply_size,
>   	.fill_reply		= ts_fill_reply,
> +
> +	.set_validate		= ethnl_set_ts_validate,
> +	.set			= ethnl_set_ts,
>   };
>   
>   /* TS_LIST_GET */

-- 
Florian

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4221 bytes)