Message-ID: <PH7PR11MB5885BBAC43E3D85550852C278E272@PH7PR11MB5885.namprd11.prod.outlook.com>
Date: Mon, 18 Nov 2024 15:07:09 +0000
From: "Olech, Milena" <milena.olech@...el.com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
	"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>, "Kitszel, Przemyslaw"
	<przemyslaw.kitszel@...el.com>, "Hay, Joshua A" <joshua.a.hay@...el.com>,
	"Lobakin, Aleksander" <aleksander.lobakin@...el.com>
Subject: RE: [PATCH iwl-net 08/10] idpf: add Tx timestamp flows

On 11/14/2024 1:52 PM, Vadim Fedorenko wrote:

> On 13/11/2024 15:46, Milena Olech wrote:
> > Add functions to request Tx timestamp for the PTP packets, read the Tx
> > timestamp when the completion tag for that packet is being received,
> > extend the Tx timestamp value and set the supported timestamping modes.
> > 
> > Tx timestamp is requested for the PTP packets by setting a TSYN bit and
> > index value in the Tx context descriptor. The driver assumption is that
> > the Tx timestamp value is ready to be read when the completion tag is
> > received. Then the driver schedules delayed work and the Tx timestamp
> > value read is requested through virtchnl message. At the end, the Tx
> > timestamp value is extended to 64-bit and provided back to the skb.
> > 
> > Co-developed-by: Josh Hay <joshua.a.hay@...el.com>
> > Signed-off-by: Josh Hay <joshua.a.hay@...el.com>
> > Reviewed-by: Alexander Lobakin <aleksander.lobakin@...el.com>
> > Signed-off-by: Milena Olech <milena.olech@...el.com>
> > ---
> >   drivers/net/ethernet/intel/idpf/idpf.h        |   4 +
> >   .../net/ethernet/intel/idpf/idpf_ethtool.c    |  63 +++++
> >   .../net/ethernet/intel/idpf/idpf_lan_txrx.h   |  13 +-
> >   drivers/net/ethernet/intel/idpf/idpf_lib.c    |  40 +++
> >   drivers/net/ethernet/intel/idpf/idpf_ptp.c    | 265 +++++++++++++++++-
> >   drivers/net/ethernet/intel/idpf/idpf_ptp.h    |  57 ++++
> >   drivers/net/ethernet/intel/idpf/idpf_txrx.c   | 136 ++++++++-
> >   drivers/net/ethernet/intel/idpf/idpf_txrx.h   |  10 +-
> >   .../net/ethernet/intel/idpf/idpf_virtchnl.c   |   6 +-
> >   .../ethernet/intel/idpf/idpf_virtchnl_ptp.c   | 232 +++++++++++++++
> >   10 files changed, 813 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/idpf/idpf.h b/drivers/net/ethernet/intel/idpf/idpf.h
> > index 1135be31b97c..76ae7f3980e6 100644
> > --- a/drivers/net/ethernet/intel/idpf/idpf.h
> > +++ b/drivers/net/ethernet/intel/idpf/idpf.h
> > @@ -293,6 +293,8 @@ struct idpf_port_stats {
> >    * @link_up: True if link is up
> >    * @sw_marker_wq: workqueue for marker packets
> >    * @tx_tstamp_caps: The capabilities negotiated for Tx timestamping
> > + * @tstamp_config: The Tx tstamp config
> > + * @tstamp_task: Tx timestamping task
> >    */
> >   struct idpf_vport {
> >   	u16 num_txq;
> > @@ -339,6 +341,8 @@ struct idpf_vport {
> >   	wait_queue_head_t sw_marker_wq;
> >   
> >   	struct idpf_ptp_vport_tx_tstamp_caps *tx_tstamp_caps;
> > +	struct hwtstamp_config tstamp_config;
> > +	struct work_struct tstamp_task;
> >   };
> >   
> >   /**
> > diff --git a/drivers/net/ethernet/intel/idpf/idpf_ethtool.c b/drivers/net/ethernet/intel/idpf/idpf_ethtool.c
> > index 59b1a1a09996..9b6ca4c1b3a1 100644
> > --- a/drivers/net/ethernet/intel/idpf/idpf_ethtool.c
> > +++ b/drivers/net/ethernet/intel/idpf/idpf_ethtool.c
> > @@ -2,6 +2,7 @@
> >   /* Copyright (C) 2023 Intel Corporation */
> >
> >   #include "idpf.h"
> > +#include "idpf_ptp.h"
> >
> >   /**
> >    * idpf_get_rxnfc - command to get RX flow classification rules
> > @@ -1312,6 +1313,67 @@ static int idpf_get_link_ksettings(struct net_device *netdev,
> >   	return 0;
> >   }
> >   
> > +/**
> > + * idpf_set_timestamp_filters - Set the supported timestamping mode
> > + * @vport: Virtual port structure
> > + * @info: ethtool timestamping info structure
> > + *
> > + * Set the Tx/Rx timestamp filters.
> > + */
> > +static void idpf_set_timestamp_filters(const struct idpf_vport *vport,
> > +				       struct kernel_ethtool_ts_info *info)
> > +{
> > +	if (!vport->tx_tstamp_caps ||
> > +	    vport->adapter->ptp->tx_tstamp_access == IDPF_PTP_NONE)
> > +		return;
> > +
> > +	info->so_timestamping = SOF_TIMESTAMPING_TX_SOFTWARE |
> > +				SOF_TIMESTAMPING_RX_SOFTWARE |
> > +				SOF_TIMESTAMPING_SOFTWARE |
> > +				SOF_TIMESTAMPING_TX_HARDWARE |
> > +				SOF_TIMESTAMPING_RX_HARDWARE |
> > +				SOF_TIMESTAMPING_RAW_HARDWARE;
> > +
> > +	info->tx_types = BIT(HWTSTAMP_TX_OFF) | BIT(HWTSTAMP_TX_ON);
> > +}
> > +
> > +/**
> > + * idpf_get_ts_info - Get device PHC association
> > + * @netdev: network interface device structure
> > + * @info: ethtool timestamping info structure
> > + *
> > + * Return: 0 on success, -errno otherwise.
> > + */
> > +static int idpf_get_ts_info(struct net_device *netdev,
> > +			    struct kernel_ethtool_ts_info *info)
> > +{
> > +	struct idpf_vport *vport;
> > +	int err = 0;
> > +
> > +	idpf_vport_ctrl_lock(netdev);
> > +	vport = idpf_netdev_to_vport(netdev);
> > +
> > +	if (!vport->adapter->ptp) {
> > +		err = -EOPNOTSUPP;
> > +		goto unlock;
> > +	}
> > +
> > +	idpf_set_timestamp_filters(vport, info);
> > +
> > +	if (idpf_is_cap_ena(vport->adapter, IDPF_OTHER_CAPS, VIRTCHNL2_CAP_PTP) &&
> > +	    vport->adapter->ptp->clock) {
> > +		info->phc_index = ptp_clock_index(vport->adapter->ptp->clock);
> > +	} else {
> > +		pci_dbg(vport->adapter->pdev, "PTP clock not detected\n");
> > +		err = ethtool_op_get_ts_info(netdev, info);
> > +	}
> > +
> > +unlock:
> > +	idpf_vport_ctrl_unlock(netdev);
> > +
> > +	return err;
> > +}
> > +
> >   static const struct ethtool_ops idpf_ethtool_ops = {
> >   	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
> >   				     ETHTOOL_COALESCE_USE_ADAPTIVE,
> > @@ -1336,6 +1398,7 @@ static const struct ethtool_ops idpf_ethtool_ops = {
> >   	.get_ringparam		= idpf_get_ringparam,
> >   	.set_ringparam		= idpf_set_ringparam,
> >   	.get_link_ksettings	= idpf_get_link_ksettings,
> > +	.get_ts_info		= idpf_get_ts_info,
> >   };
> >   
> >   /**
> > diff --git a/drivers/net/ethernet/intel/idpf/idpf_lan_txrx.h b/drivers/net/ethernet/intel/idpf/idpf_lan_txrx.h
> > index 8c7f8ef8f1a1..7492d1713243 100644
> > --- a/drivers/net/ethernet/intel/idpf/idpf_lan_txrx.h
> > +++ b/drivers/net/ethernet/intel/idpf/idpf_lan_txrx.h
> > @@ -282,7 +282,18 @@ struct idpf_flex_tx_tso_ctx_qw {
> >   	u8 flex;
> >   };
> >   
> > -struct idpf_flex_tx_ctx_desc {
> > +union idpf_flex_tx_ctx_desc {
> > +	/* DTYPE = IDPF_TX_DESC_DTYPE_CTX (0x01) */
> > +	struct {
> > +		__le64 qw0;
> > +#define IDPF_TX_CTX_L2TAG2_M	GENMASK_ULL(47, 32)
> > +		__le64 qw1;
> > +#define IDPF_TX_CTX_DTYPE_M	GENMASK_ULL(3, 0)
> > +#define IDPF_TX_CTX_CMD_M	GENMASK_ULL(15, 4)
> > +#define IDPF_TX_CTX_TSYN_REG_M	GENMASK_ULL(47, 30)
> > +#define IDPF_TX_CTX_MSS_M	GENMASK_ULL(50, 63)
> > +	} tsyn;
> > +
> >   	/* DTYPE = IDPF_TX_DESC_DTYPE_FLEX_TSO_CTX (0x05) */
> >   	struct {
> >   		struct idpf_flex_tx_tso_ctx_qw qw0;
> > diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > index b4fbb99bfad2..467f380c8fd4 100644
> > --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > @@ -3,6 +3,7 @@
> >   
> >   #include "idpf.h"
> >   #include "idpf_virtchnl.h"
> > +#include "idpf_ptp.h"
> >   
> >   static const struct net_device_ops idpf_netdev_ops;
> >   
> > @@ -2305,6 +2306,44 @@ static int idpf_set_mac(struct net_device *netdev, void *p)
> >   	return err;
> >   }
> >   
> > +/**
> > + * idpf_eth_ioctl - Access the hwtstamp interface
> > + * @netdev: network interface device structure
> > + * @ifr: interface request data
> > + * @cmd: ioctl command
> > + *
> > + * Return: 0 on success, negative otherwise.
> > + */
> > +static int idpf_eth_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
> > +{
> > +	struct idpf_vport *vport;
> > +	int err;
> > +
> > +	idpf_vport_ctrl_lock(netdev);
> > +	vport = idpf_netdev_to_vport(netdev);
> > +
> > +	if (!idpf_ptp_get_vport_tstamp_capability(vport)) {
> > +		idpf_vport_ctrl_unlock(netdev);
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	switch (cmd) {
> > +	case SIOCGHWTSTAMP:
> > +		err = idpf_ptp_get_ts_config(vport, ifr);
> > +		break;
> > +	case SIOCSHWTSTAMP:
> > +		err = idpf_ptp_set_ts_config(vport, ifr);
> > +		break;
> > +	default:
> > +		err = -EOPNOTSUPP;
> > +		break;
> > +	}
> > +
> > +	idpf_vport_ctrl_unlock(netdev);
> > +
> > +	return err;
> > +}
> > +
> >   /**
> >    * idpf_alloc_dma_mem - Allocate dma memory
> >    * @hw: pointer to hw struct
> > @@ -2351,4 +2390,5 @@ static const struct net_device_ops idpf_netdev_ops = {
> >   	.ndo_get_stats64 = idpf_get_stats64,
> >   	.ndo_set_features = idpf_set_features,
> >   	.ndo_tx_timeout = idpf_tx_timeout,
> > +	.ndo_eth_ioctl = idpf_eth_ioctl,
> 
> ioctl interface is deprecated, should not add any new drivers with it.
> It's better to use .ndo_hwtamp_get/.ndo_hwstamp_set

Good catch, I'll fix in v2.

> 
> >   };
> > diff --git a/drivers/net/ethernet/intel/idpf/idpf_ptp.c b/drivers/net/ethernet/intel/idpf/idpf_ptp.c
> > index 997528176b0f..f34642d10768 100644
> > --- a/drivers/net/ethernet/intel/idpf/idpf_ptp.c
> > +++ b/drivers/net/ethernet/intel/idpf/idpf_ptp.c
> > @@ -317,6 +317,37 @@ static int idpf_ptp_gettimex64(struct ptp_clock_info *info,
> >   	return 0;
> >   }
> >   
> > +/**
> > + * idpf_ptp_update_cached_phctime - Update the cached PHC time values
> > + * @adapter: Driver specific private structure
> > + *
> > + * This function updates the system time values which are cached in the adapter
> > + * structure.
> > + *
> > + * This function must be called periodically to ensure that the cached value
> > + * is never more than 2 seconds old.
> > + *
> > + * Return 0 on success, negative otherwise.
> > + */
> > +static int idpf_ptp_update_cached_phctime(struct idpf_adapter *adapter)
> > +{
> > +	u64 systime;
> > +	int err;
> > +
> > +	err = idpf_ptp_read_src_clk_reg(adapter, &systime, NULL);
> > +	if (err)
> > +		return -EACCES;
> > +
> > +	/* Update the cached PHC time stored in the adapter structure.
> > +	 * These values are used to extend Tx timestamp values to 64 bit
> > +	 * expected by the stack.
> > +	 */
> > +	WRITE_ONCE(adapter->ptp->cached_phc_time, systime);
> > +	WRITE_ONCE(adapter->ptp->cached_phc_jiffies, jiffies);
> > +
> > +	return 0;
> > +}
> > +
> >   /**
> >    * idpf_ptp_settime64 - Set the time of the clock
> >    * @info: the driver's PTP info structure
> > @@ -347,6 +378,11 @@ static int idpf_ptp_settime64(struct ptp_clock_info *info,
> >   		return err;
> >   	}
> >   
> > +	err = idpf_ptp_update_cached_phctime(adapter);
> > +	if (err)
> > +		pci_warn(adapter->pdev,
> > +			 "Unable to immediately update cached PHC time\n");
> > +
> >   	return 0;
> >   }
> >   
> > @@ -402,6 +438,11 @@ static int idpf_ptp_adjtime(struct ptp_clock_info *info, s64 delta)
> >   		return err;
> >   	}
> >   
> > +	err = idpf_ptp_update_cached_phctime(adapter);
> > +	if (err)
> > +		pci_warn(adapter->pdev,
> > +			 "Unable to immediately update cached PHC time\n");
> > +
> >   	return 0;
> >   }
> >   
> > @@ -465,6 +506,196 @@ static int idpf_ptp_gpio_enable(struct ptp_clock_info *info,
> >   	return -EOPNOTSUPP;
> >   }
> >   
> > +/**
> > + * idpf_ptp_tstamp_extend_32b_to_64b - Convert a 32b nanoseconds Tx timestamp
> > + *				       to 64b.
> > + * @cached_phc_time: recently cached copy of PHC time
> > + * @in_timestamp: Ingress/egress 32b nanoseconds timestamp value
> > + *
> > + * Hardware captures timestamps which contain only 32 bits of nominal
> > + * nanoseconds, as opposed to the 64bit timestamps that the stack expects.
> > + *
> > + * Return: Tx timestamp value extended to 64 bits based on cached PHC time.
> > + */
> > +u64 idpf_ptp_tstamp_extend_32b_to_64b(u64 cached_phc_time, u32 in_timestamp)
> > +{
> > +	u32 delta, phc_lo;
> > +	u64 ns;
> > +
> > +	phc_lo = lower_32_bits(cached_phc_time);
> > +	delta = in_timestamp - phc_lo;
> > +
> > +	if (delta > S32_MAX) {
> > +		delta = phc_lo - in_timestamp;
> > +		ns = cached_phc_time - delta;
> > +	} else {
> > +		ns = cached_phc_time + delta;
> > +	}
> > +
> > +	return ns;
> > +}
> > +
> > +/**
> > + * idpf_ptp_extend_ts - Convert a 40b timestamp to 64b nanoseconds
> > + * @adapter: Driver specific private structure
> > + * @in_tstamp: Ingress/egress timestamp value
> > + *
> > + * It is assumed that the caller verifies the timestamp is valid prior to
> > + * calling this function.
> > + *
> > + * Extract the 32bit nominal nanoseconds and extend them. Use the cached PHC
> > + * time stored in the device private PTP structure as the basis for timestamp
> > + * extension.
> > + *
> > + * Return: Tx timestamp value extended to 64 bits.
> > + */
> > +u64 idpf_ptp_extend_ts(const struct idpf_adapter *adapter, u64 in_tstamp)
> > +{
> > +	unsigned long discard_time;
> > +
> > +	discard_time = adapter->ptp->cached_phc_jiffies + 2 * HZ;
> > +
> > +	if (time_is_before_jiffies(discard_time))
> > +		return 0;
> 
> It might be a good idea to count such events, just to provide at least
> some information to the client regarding zero timestamp?

You mean to calculate skipped Tx timestamps?

>
> > +
> > +	return idpf_ptp_tstamp_extend_32b_to_64b(adapter->ptp->cached_phc_time,
> > +						 lower_32_bits(in_tstamp));
> > +}
> 
> 
> [... skip ...]

Thanks,
Milena

Powered by blists - more mailing lists