netdev - Re: [PATCH net-next v3 3/5] eth: fbnic: add RX packets timestamping support

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: <6015e3d3-e35b-4e6c-b6cf-3348e8b6d4f6@intel.com>
Date: Fri, 4 Oct 2024 16:18:47 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Vadim Fedorenko <vadfed@...a.com>, Vadim Fedorenko
	<vadim.fedorenko@...ux.dev>, Jakub Kicinski <kuba@...nel.org>, David Ahern
	<dsahern@...nel.org>, Paolo Abeni <pabeni@...hat.com>, "David S. Miller"
	<davem@...emloft.net>, Alexander Duyck <alexanderduyck@...com>
CC: <netdev@...r.kernel.org>, Richard Cochran <richardcochran@...il.com>
Subject: Re: [PATCH net-next v3 3/5] eth: fbnic: add RX packets timestamping
 support



On 10/3/2024 5:39 AM, Vadim Fedorenko wrote:
> Add callbacks to support timestamping configuration via ethtool.
> Add processing of RX timestamps.
> 
> Signed-off-by: Vadim Fedorenko <vadfed@...a.com>
> ---
> +static int fbnic_hwtstamp_set(struct net_device *netdev,
> +			      struct kernel_hwtstamp_config *config,
> +			      struct netlink_ext_ack *extack)
> +{
> +	struct fbnic_net *fbn = netdev_priv(netdev);
> +	int old_rx_filter;
> +
> +	if (config->source != HWTSTAMP_SOURCE_NETDEV)
> +		return -EOPNOTSUPP;
> +
> +	if (!kernel_hwtstamp_config_changed(config, &fbn->hwtstamp_config))
> +		return 0;
> +
> +	/* Upscale the filters */
> +	switch (config->rx_filter) {
> +	case HWTSTAMP_FILTER_NONE:
> +	case HWTSTAMP_FILTER_ALL:
> +	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
> +	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
> +	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
> +	case HWTSTAMP_FILTER_PTP_V2_EVENT:
> +		break;
> +	case HWTSTAMP_FILTER_NTP_ALL:
> +		config->rx_filter = HWTSTAMP_FILTER_ALL;
> +		break;
> +	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
> +	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
> +		config->rx_filter = HWTSTAMP_FILTER_PTP_V1_L4_EVENT;
> +		break;
> +	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
> +	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
> +		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L4_EVENT;
> +		break;
> +	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
> +	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
> +		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
> +		break;
> +	case HWTSTAMP_FILTER_PTP_V2_SYNC:
> +	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
> +		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
> +		break;
> +	default:
> +		return -ERANGE;
> +	}
> +
> +	/* Configure */
> +	old_rx_filter = fbn->hwtstamp_config.rx_filter;
> +	memcpy(&fbn->hwtstamp_config, config, sizeof(*config));
> +
> +	if (old_rx_filter != config->rx_filter && netif_running(fbn->netdev)) {
> +		fbnic_rss_reinit(fbn->fbd, fbn);
> +		fbnic_write_rules(fbn->fbd);
> +	}
> +
> +	/* Save / report back filter configuration
> +	 * Note that our filter configuration is inexact. Instead of
> +	 * filtering for a specific UDP port or L2 Ethertype we are
> +	 * filtering in all UDP or all non-IP packets for timestamping. So
> +	 * if anything other than FILTER_ALL is requested we report
> +	 * FILTER_SOME indicating that we will be timestamping a few
> +	 * additional packets.
> +	 */

Is there any benefit to implementing anything other than
HWTSTAMP_FILTER_ALL?

Those are typically considered legacy with the primary reason being to
support hardware which does not support timestamping all frames.

I suppose if you have measurement that supporting them is valuable (i.e.
because of performance impact on timestamping all frames?) it makes
sense to support. But otherwise I'm not sure its worth the extra complexity.

Upgrading the filtering modes to HWTSTAMP_FILTER_ALL is acceptable and
is done by a few drivers.