netdev - Re: [PATCH v2 iwl-next 08/10] idpf: add Tx timestamp flows

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: <6749e1cb95bed_23772a2944b@willemb.c.googlers.com.notmuch>
Date: Fri, 29 Nov 2024 10:46:19 -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>
Subject: Re: [PATCH v2 iwl-next 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>
> Signed-off-by: Milena Olech <milena.olech@...el.com>
> ---
> v1 -> v2: add timestamping stats, use ndo_hwtamp_get/ndo_hwstamp_set

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

This is not really a setter. It modifies no vport state.

idpf_get_timestamp_filters? Or just merge into the below caller.

> +{
> +	if (!vport->tx_tstamp_caps ||
> +	    vport->adapter->ptp->tx_tstamp_access == IDPF_PTP_NONE)
> +		return;
> +
> +	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);
> +}
> +
> +/**
> + * 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_adapter *adapter = idpf_netdev_to_adapter(netdev);
> +	struct idpf_vport *vport;
> +	int err = 0;
> +
> +	idpf_vport_cfg_lock(adapter);
> +	vport = idpf_netdev_to_vport(netdev);
> +
> +	if (!vport->adapter->ptp) {
> +		err = -EOPNOTSUPP;
> +		goto unlock;
> +	}
> +
> +	idpf_set_timestamp_filters(vport, info);

Probably move this in the below if, als it gets entirely overwritten
if the else is taken.
> +
> +	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_cfg_unlock(adapter);
> +
> +	return err;
> +}

> +/**
> + * idpf_ptp_extend_ts - Convert a 40b timestamp to 64b nanoseconds
> + * @vport: Virtual port 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(struct idpf_vport *vport, u64 in_tstamp)
> +{
> +	struct idpf_ptp *ptp = vport->adapter->ptp;
> +	unsigned long discard_time;
> +
> +	discard_time = ptp->cached_phc_jiffies + 2 * HZ;
> +
> +	if (time_is_before_jiffies(discard_time)) {
> +		vport->tstamp_stats.tx_hwtstamp_discarded++;
> +		return 0;
> +	}
> +
> +	return idpf_ptp_tstamp_extend_32b_to_64b(ptp->cached_phc_time,
> +						 lower_32_bits(in_tstamp));
> +}

> +#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;
> +
> +	/* only timestamp the outbound packet if the user has requested it */
> +	if (likely(!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)))
> +		return -1;
> +
> +	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;
> +
> +	/* Grab an open timestamp slot */
> +	err = idpf_ptp_request_ts(tx_q, skb, &idx);
> +	if (err) {
> +		tx_q->txq_grp->vport->tstamp_stats.tx_hwtstamp_skipped++;

What is the mutual exclusion on these stats fields?

In ndo_start_xmit the txq lock is held, but no vport wide lock?

> +		return -1;
> +	}
> +
> +	off->tx_flags |= IDPF_TX_FLAGS_TSYN;
> +
> +	return idx;
> +}