netdev - Re: [Intel-wired-lan] [PATCH iwl-next v8 13/14] iavf: handle set and get timestamps ops

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: <2ab61b07-a3b1-4a33-a3fb-e9c711b0587c@intel.com>
Date: Tue, 30 Jul 2024 17:12:03 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>, Jacob Keller
	<jacob.e.keller@...el.com>, Wojciech Drewek <wojciech.drewek@...el.com>,
	Rahul Rameshbabu <rrameshbabu@...dia.com>, Simon Horman <horms@...nel.org>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next v8 13/14] iavf: handle set and
 get timestamps ops

From: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
Date: Tue, 30 Jul 2024 05:15:08 -0400

> From: Jacob Keller <jacob.e.keller@...el.com>
> 
> Add handlers for the .ndo_hwtstamp_get and .ndo_hwtstamp_set ops which allow
> userspace to request timestamp enablement for the device. This support allows
> standard Linux applications to request the timestamping desired.

[...]

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_ptp.c b/drivers/net/ethernet/intel/iavf/iavf_ptp.c
> index 9eb3161575d5..7754f4f24052 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_ptp.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.c
> @@ -3,6 +3,136 @@
>  
>  #include "iavf.h"
>  
> +/**
> + * iavf_ptp_disable_rx_tstamp - Disable timestamping in Rx rings
> + * @adapter: private adapter structure
> + *
> + * Disable timestamp reporting for all Rx rings.
> + */
> +static void iavf_ptp_disable_rx_tstamp(struct iavf_adapter *adapter)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < adapter->num_active_queues; i++)

	for (u32 i = 0; ...)

> +		adapter->rx_rings[i].flags &= ~IAVF_TXRX_FLAGS_HW_TSTAMP;
> +}
> +
> +/**
> + * iavf_ptp_enable_rx_tstamp - Enable timestamping in Rx rings
> + * @adapter: private adapter structure
> + *
> + * Enable timestamp reporting for all Rx rings.
> + */
> +static void iavf_ptp_enable_rx_tstamp(struct iavf_adapter *adapter)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < adapter->num_active_queues; i++)

(same)

> +		adapter->rx_rings[i].flags |= IAVF_TXRX_FLAGS_HW_TSTAMP;
> +}
> +
> +/**
> + * iavf_ptp_set_timestamp_mode - Set device timestamping mode
> + * @adapter: private adapter structure
> + * @config: pointer to kernel_hwtstamp_config
> + *
> + * Set the timestamping mode requested from the userspace.
> + *
> + * Note: this function always translates Rx timestamp requests for any packet
> + * category into HWTSTAMP_FILTER_ALL.
> + *
> + * Return: zero.
> + */
> +static int iavf_ptp_set_timestamp_mode(struct iavf_adapter *adapter,
> +				       struct kernel_hwtstamp_config *config)
> +{
> +	/* Reserved for future extensions. */
> +	if (config->flags)
> +		return -EINVAL;
> +
> +	switch (config->tx_type) {
> +	case HWTSTAMP_TX_OFF:
> +		break;
> +	case HWTSTAMP_TX_ON:
> +		return -EOPNOTSUPP;
> +	default:
> +		return -ERANGE;
> +	}
> +
> +	switch (config->rx_filter) {
> +	case HWTSTAMP_FILTER_NONE:
> +		iavf_ptp_disable_rx_tstamp(adapter);
> +		break;
> +	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
> +	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
> +	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
> +	case HWTSTAMP_FILTER_PTP_V2_EVENT:
> +	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
> +	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
> +	case HWTSTAMP_FILTER_PTP_V2_SYNC:
> +	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
> +	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
> +	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
> +	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
> +	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
> +	case HWTSTAMP_FILTER_NTP_ALL:
> +	case HWTSTAMP_FILTER_ALL:

Since these definitions are uAPI, their values can't change, only new
ones can be added.
So, you can use case range:

	case FILTER_ALL:
	case V1_L4_EVENT ... NTP_ALL:
		

> +		if (!(iavf_ptp_cap_supported(adapter,
> +					     VIRTCHNL_1588_PTP_CAP_RX_TSTAMP)))
> +			return -EOPNOTSUPP;
> +		config->rx_filter = HWTSTAMP_FILTER_ALL;
> +		iavf_ptp_enable_rx_tstamp(adapter);
> +		break;
> +	default:
> +		return -ERANGE;
> +	}

or even simpler:

	if (rx_filter == NONE)
		// disable
		return;

	if (rx_filter == FILTER_SOME || rx_filter > NTP_ALL)
		return -ERANGE;

	// here you will have your 14 supported cases from above,
	// proceed with configuration

> +
> +	return 0;
> +}
> +
> +/**
> + * iavf_ptp_get_ts_config - Get timestamping configuration
> + * @adapter: private adapter structure
> + * @config: pointer to kernel_hwtstamp_config
> + *
> + * Return the current hardware timestamping configuration back to userspace.
> + *
> + * Return: zero.
> + */
> +int iavf_ptp_get_ts_config(struct iavf_adapter *adapter,
> +			   struct kernel_hwtstamp_config *config)
> +{
> +	*config = adapter->ptp.hwtstamp_config;
> +
> +	return 0;

If it always returns 0, void then?

> +}
> +
> +/**
> + * iavf_ptp_set_ts_config - Set timestamping configuration
> + * @adapter: private adapter structure
> + * @config: pointer to kernel_hwtstamp_config structure
> + * @extack: pointer to netlink_ext_ack structure
> + *
> + * Program the requested timestamping configuration to the device.
> + *
> + * Return: zero.

But it can also return @err...

> + */
> +int iavf_ptp_set_ts_config(struct iavf_adapter *adapter,
> +			   struct kernel_hwtstamp_config *config,
> +			   struct netlink_ext_ack *extack)
> +{
> +	int err;
> +
> +	err = iavf_ptp_set_timestamp_mode(adapter, config);
> +	if (err)
> +		return err;
> +
> +	/* Save successful settings for future reference */
> +	adapter->ptp.hwtstamp_config = *config;
> +
> +	return 0;
> +}

[...]

Thanks,
Olek