| 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
| ||
|
Message-ID: <PH7PR11MB5885993E499F9302530E17F58E052@PH7PR11MB5885.namprd11.prod.outlook.com>
Date: Wed, 18 Dec 2024 15:33:06 +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>
Subject: RE: [PATCH v2 iwl-next 08/10] idpf: add Tx timestamp flows
On 11/29/2024 4:46 PM 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@...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.
>
I named it "set" because in fact we're setting some timestamp filters,
however I do agree that it's not compliant with the get_ts_info,
where we also assign something to info struct - phc_index.
I'll change the name to get_timestamp_filters.
>> +{
>> + 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.
Moved in v3.
>> +
>> + 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?
>
After reviewing statistics again, I decided to split into two parts:
1) Tx queue statistics
I've done it similarly to other tx queue statistics (in v3), so
I'm incrementing for each Tx queue separately to sum up at the end.
Here I'll monitor skipped Tx timestamps, where the proper Tx timestamp
index was not found.
2) Vport statistics (protected with mutex)
- flushed - removed due to closing the interface
- discarded - too old to provide back to the skb
These values are calculated for vports.
>> + return -1;
>> + }
>> +
>> + off->tx_flags |= IDPF_TX_FLAGS_TSYN;
>> +
>> + return idx;
>> +}
Thanks,
Milena
Powered by blists - more mailing lists