RE: [PATCH v3 iwl-next 08/10] idpf: add Tx timestamp flows

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Olech, Milena wrote:
> On 01/10/2025 10:34PM Willem de Bruijn 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@intel.com>
> > > > > Signed-off-by: Josh Hay <joshua.a.hay@intel.com>
> > > > > Signed-off-by: Milena Olech <milena.olech@intel.com>
> > > > > ---
> > > > > v2 -> v3: change get_timestamp_filter function name, split stats
> > > > > into vport-based and tx queue-based
> > > > > v1 -> v2: add timestamping stats, use ndo_hwtamp_get/ndo_hwstamp_set
> > > > > 
> > > > >  drivers/net/ethernet/intel/idpf/idpf.h        |  20 ++
> > > > >  .../net/ethernet/intel/idpf/idpf_ethtool.c    |  69 ++++-
> > > > >  .../net/ethernet/intel/idpf/idpf_lan_txrx.h   |  13 +-
> > > > >  drivers/net/ethernet/intel/idpf/idpf_lib.c    |  47 ++++
> > > > >  drivers/net/ethernet/intel/idpf/idpf_ptp.c    | 236 +++++++++++++++++-
> > > > >  drivers/net/ethernet/intel/idpf/idpf_ptp.h    |  52 ++++
> > > > >  drivers/net/ethernet/intel/idpf/idpf_txrx.c   | 139 ++++++++++-
> > > > >  drivers/net/ethernet/intel/idpf/idpf_txrx.h   |  11 +-
> > > > >  .../net/ethernet/intel/idpf/idpf_virtchnl.c   |   6 +-
> > > > >  .../ethernet/intel/idpf/idpf_virtchnl_ptp.c   | 235 +++++++++++++++++
> > > > >  10 files changed, 814 insertions(+), 14 deletions(-)
> > > > > 
> > > > 
> > > > > +/**
> > > > > + * idpf_get_timestamp_filters - Get the supported timestamping mode
> > > > > + * @vport: Virtual port structure
> > > > > + * @info: ethtool timestamping info structure
> > > > > + *
> > > > > + * Get the Tx/Rx timestamp filters.
> > > > > + */
> > > > > +static void idpf_get_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;
> > > > 
> > > > Is making SOF_TIMESTAMPING_RX_HARDWARE and SOF_TIMESTAMPING_RAW_HARDWARE
> > > > conditional on tx_tstamp_access intentional?
> > > 
> > > Hmmm, good catch.
> > > I guess I will change the SOF_TIMESTAMPING_RX_HARDWARE to be not
> > > conditional.
> > > 
> > > About the SOF_TIMESTAMPING_RAW_HARDWARE - it should rely on the
> > > tx_statmp_access.
> >
> > For Tx, but it also applies to Rx.
> 
> Right, there was a misunderstanding because the documentation says:
> "Report hardware timestamps as generated by
>  SOF_TIMESTAMPING_TX_HARDWARE when available."
> 
> So I assumed that it's Tx only.

Indeed. This was fixed fairly recently in commit e503f82e304b. It now
reads

"
SOF_TIMESTAMPING_RAW_HARDWARE:
  Report hardware timestamps as generated by
  SOF_TIMESTAMPING_TX_HARDWARE or SOF_TIMESTAMPING_RX_HARDWARE
  when available.
"

 
> > > > > +
> > > > > +	info->so_timestamping = SOF_TIMESTAMPING_TX_SOFTWARE |
> > > > > +				SOF_TIMESTAMPING_TX_HARDWARE |
> > > > > +				SOF_TIMESTAMPING_RX_HARDWARE |
> > > > > +				SOF_TIMESTAMPING_RAW_HARDWARE;
> > > > > +
> > > > > +	info->tx_types = BIT(HWTSTAMP_TX_OFF) | BIT(HWTSTAMP_TX_ON);
> > > > > +}
> > > > > +
> > > > > +
> > > > > +static int idpf_hwtstamp_get(struct net_device *netdev,
> > > > > +			     struct kernel_hwtstamp_config *config)
> > > > > +{
> > > > > +	struct idpf_adapter *adapter = idpf_netdev_to_adapter(netdev);
> > > > > +	struct idpf_vport *vport;
> > > > > +
> > > > > +	idpf_vport_cfg_lock(adapter);
> > > > > +	vport = idpf_netdev_to_vport(netdev);
> > > > > +
> > > > > +	if (!idpf_ptp_get_vport_tstamp_capability(vport)) {
> > > > > +		idpf_vport_cfg_unlock(adapter);
> > > > > +		return -EOPNOTSUPP;
> > > > 
> > > > Should this return an empty config and return 0, rather than return
> > > > error?
> > > > 
> > > 
> > > I'd prefer to return NOTSUPP, as the feature itself relies on the CP
> > > policy.
> > 
> > Does that make ethtool -T $DEV fail? It should ideally succeed while
> > only advertising software receive timestamping (which is implemented
> > device independent).
> 
> Ok, I'll change then.

I did not actually check whether it would return in failure at the
application layer, to be clear. Just saying that if that would be the
consequence, then that would not be good.

> > > > > +/**
> > > > > + * 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;
> > > > > +	}
> > > > 
> > > > If using s64 delta and ns, no need for branch?
> > > 
> > > To make it more readable, I'll change the condition in v4.
> > > I'll check if the phc_lo is greater than in_stamp.
> > > 
> > > If phc_lo is bigger, then tx tstamp value was captured before
> > > caching PHC time and case 1 will be applicable.
> > 
> > My point is that with signed arithmetic there is no need to
> > invert delta and switch from addition to deletion.
> 
> This ns value is used - at the end of the day - in ns_to_ktime function,
> which requires u64, so IMO it is easier to differentiate at this point,
> rather than introducing new logic in f.e. idpf_ptp_extend_ts.
> 
> But to remove this condition signed delta is enough, and it may work.

Just a suggestion. Feel free to leave as is if you prefer.