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

[Florian Fainelli <florian.fainelli@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 2905 bytes --]

On 10/9/23 08:51, Köry Maincent wrote:
> From: Kory Maincent <kory.maincent@bootlin.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@bootlin.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


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]