Message-ID: <PH7PR11MB5885EB42ABAE3CA8023CFC038E272@PH7PR11MB5885.namprd11.prod.outlook.com>
Date: Mon, 18 Nov 2024 15:18:07 +0000
From: "Olech, Milena" <milena.olech@...el.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>,
	"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: [Intel-wired-lan] [PATCH iwl-net 08/10] idpf: add Tx timestamp
 flows

On 11/15/2024 12:20 AM, Willem de Bruijn wrote:

> 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"

Good, thanks for info, I'll get familiar with that commit.

> 
> > +	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.

You mean to rely on standard timecounter instead of cached PHC time?
Can you please clarify?

> 
> > +#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.

Ok, I'll replace.

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