Message-ID: <673685bc9ef98_3379ce2948@willemb.c.googlers.com.notmuch>
Date: Thu, 14 Nov 2024 18:20:28 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Milena Olech <milena.olech@...el.com>, 
 intel-wired-lan@...ts.osuosl.org
Cc: netdev@...r.kernel.org, 
 anthony.l.nguyen@...el.com, 
 przemyslaw.kitszel@...el.com, 
 Milena Olech <milena.olech@...el.com>, 
 Josh Hay <joshua.a.hay@...el.com>, 
 Alexander Lobakin <aleksander.lobakin@...el.com>
Subject: Re: [PATCH iwl-net 08/10] idpf: add Tx timestamp flows

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(-)
> 

> +/**
> + * 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;
> +

Drivers no longer need to set various software timestamping options
since commit "ethtool: RX software timestamp for all"

> +	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,
>  };

> +/**
> + * 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;
> +}

Consider a standard timecounter to estimate a device clock.

> +#if (IS_ENABLED(CONFIG_PTP_1588_CLOCK))
> +/**
> + * idpf_tx_tstamp - set up context descriptor for hardware timestamp
> + * @tx_q: queue to send buffer on
> + * @skb: pointer to the SKB we're sending
> + * @off: pointer to the offload struct
> + *
> + * Return: Positive index number on success, negative otherwise.
> + */
> +static int idpf_tx_tstamp(struct idpf_tx_queue *tx_q, struct sk_buff *skb,
> +			  struct idpf_tx_offload_params *off)
> +{
> +	int err, idx;
> +
> +	if (!idpf_ptp_get_txq_tstamp_capability(tx_q))
> +		return -1;
> +
> +	/* Tx timestamps cannot be sampled when doing TSO */
> +	if (off->tx_flags & IDPF_TX_FLAGS_TSO)
> +		return -1;
> +
> +	/* only timestamp the outbound packet if the user has requested it */
> +	if (likely(!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)))
> +		return -1;

Make this the first test. This function is being called on every
outgoing packet. It should be as cheap as possible in the likely
case where the PTP timestamp is not requested.

Even move this test to the callee if this does not get inlined.

>  /**
>   * idpf_tx_splitq_frame - Sends buffer on Tx ring using flex descriptors
>   * @skb: send buffer
> @@ -2743,9 +2859,10 @@ static netdev_tx_t idpf_tx_splitq_frame(struct sk_buff *skb,
>  					struct idpf_tx_queue *tx_q)
>  {
>  	struct idpf_tx_splitq_params tx_params = { };
> +	union idpf_flex_tx_ctx_desc *ctx_desc;
>  	struct idpf_tx_buf *first;
>  	unsigned int count;
> -	int tso;
> +	int tso, idx;
>  
>  	count = idpf_tx_desc_count_required(tx_q, skb);
>  	if (unlikely(!count))
> @@ -2765,8 +2882,7 @@ static netdev_tx_t idpf_tx_splitq_frame(struct sk_buff *skb,
>  
>  	if (tso) {
>  		/* If tso is needed, set up context desc */
> -		struct idpf_flex_tx_ctx_desc *ctx_desc =
> -			idpf_tx_splitq_get_ctx_desc(tx_q);
> +		ctx_desc = idpf_tx_splitq_get_ctx_desc(tx_q);
>  
>  		ctx_desc->tso.qw1.cmd_dtype =
>  				cpu_to_le16(IDPF_TX_DESC_DTYPE_FLEX_TSO_CTX |
> @@ -2784,6 +2900,12 @@ static netdev_tx_t idpf_tx_splitq_frame(struct sk_buff *skb,
>  		u64_stats_update_end(&tx_q->stats_sync);
>  	}
>  
> +	idx = idpf_tx_tstamp(tx_q, skb, &tx_params.offload);
> +	if (idx != -1) {
> +		ctx_desc = idpf_tx_splitq_get_ctx_desc(tx_q);
> +		idpf_tx_set_tstamp_desc(ctx_desc, idx);
> +	}
> +

Called here

> +/**
> + * idpf_ptp_get_tx_tstamp_async_handler - Async callback for getting Tx tstamps
> + * @adapter: Driver specific private structure
> + * @xn: transaction for message
> + * @ctlq_msg: received message
> + *
> + * Read the tstamps Tx tstamp values from a received message and put them
> + * directly to the skb. The number of timestamps to read is specified by
> + * the virtchnl message.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int
> +idpf_ptp_get_tx_tstamp_async_handler(struct idpf_adapter *adapter,
> +				     struct idpf_vc_xn *xn,
> +				     const struct idpf_ctlq_msg *ctlq_msg)
> +{
> +	struct virtchnl2_ptp_get_vport_tx_tstamp_latches *recv_tx_tstamp_msg;
> +	struct idpf_ptp_vport_tx_tstamp_caps *tx_tstamp_caps;
> +	struct virtchnl2_ptp_tx_tstamp_latch tstamp_latch;
> +	struct idpf_ptp_tx_tstamp *ptp_tx_tstamp;
> +	struct idpf_vport *tstamp_vport = NULL;
> +	struct list_head *head;
> +	u16 num_latches;
> +	u32 vport_id;
> +	int err;
> +
> +	recv_tx_tstamp_msg = ctlq_msg->ctx.indirect.payload->va;
> +	vport_id = le32_to_cpu(recv_tx_tstamp_msg->vport_id);
> +
> +	idpf_for_each_vport(adapter, vport) {
> +		if (!vport)
> +			continue;
> +
> +		if (vport->vport_id == vport_id) {
> +			tstamp_vport = vport;
> +			break;
> +		}
> +	}

idpf_vid_to_vport?

> +
> +	if (!tstamp_vport || !tstamp_vport->tx_tstamp_caps)
> +		return -EINVAL;

Powered by blists - more mailing lists