Re: [PATCH iwl-next v2 2/2] idpf: add direct Tx timestamp readout

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] idpf: hrtimer_start() race with hrtimer_forward_now() in
  tx_tstamp_timer
- [Medium] idpf: stale W1C readiness bits left set after latch timeout
  or flush
- [Medium] idpf: premature readiness clear when
  idpf_ptp_read_tx_tstamp() returns 0
- [Medium] idpf: missing IDPF_VPORT_UP check in
  idpf_ptp_tx_tstamp_start() and mode set
--

commit 6ef55bc62ac37c13a04d2106837cebcae6373e12
Author: Przemyslaw Korba <przemyslaw.korba@intel.com>

idpf: add direct Tx timestamp readout

This commit adds direct BAR register readout for Tx timestamps using a
periodic 100 us hrtimer to poll a 64-bit hardware readiness bitmap and
reclaim expired latch requests after 500 ms.

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_ptp.c b/drivers/net/ethernet/intel/idpf/idpf_ptp.c
> index df2da430ff28f..f66ba6fb4ea09 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_ptp.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_ptp.c

[ ... ]

> @@ -851,19 +925,287 @@ int idpf_ptp_request_ts(struct idpf_tx_queue *tx_q, struct sk_buff *skb,
>  	return 0;
>  }
>  
> +/**
> + * idpf_ptp_read_tx_tstamp - Read and extract timestamp from latch registers
> + * @tstamp_caps: Tx timestamp capabilities of the vport
> + * @tx_tstamp: Tx timestamp structure with latch register addresses
> + *
> + * Read the timestamp from the latch registers, whose layout the IDPF
> + * specification defines as:
> + *   low  bit[0]   : valid flag
> + *   low  bit[31:1]: timestamp bits [30:0]
> + *   high bit[7:0] : timestamp bits [38:31]
> + * Together they form a 39-bit timestamp, which is then shifted right by
> + * tstamp_ns_lo_bit to remove sub-nanosecond precision bits.
> + *
> + * The caller has already found the latch index set in the readiness bitmap,
> + * which is preferred over the per-latch valid flag because it covers every
> + * latch in a single pair of reads. The valid flag is still checked here, as
> + * the hardware may overwrite a latch whose readiness bit has been left set.
> + *
> + * @latches_lock must be held across the readiness check, this call and the
> + * subsequent clearing of the readiness bit, to keep the index from being
> + * recycled in between.
> + *
> + * Return: extracted timestamp value in nanoseconds, 0 if the latch does not
> + * hold a valid timestamp.
> + */
> +static u64
> +idpf_ptp_read_tx_tstamp(struct idpf_ptp_vport_tx_tstamp_caps *tstamp_caps,
> +			struct idpf_ptp_tx_tstamp *tx_tstamp)
> +{
> +	u32 tstamp_l, tstamp_h;
> +	u64 tstamp;
> +
> +	lockdep_assert_held(&tstamp_caps->latches_lock);
> +
> +	tstamp_l = readl(tx_tstamp->tx_latch_reg_addr_l);
> +	tstamp_h = readl(tx_tstamp->tx_latch_reg_addr_h);
> +
> +	if (unlikely(!(tstamp_l & IDPF_PTP_TX_TSTAMP_L_VALID_M)))
> +		return 0;
> +
> +	tstamp = FIELD_GET(IDPF_PTP_TX_TSTAMP_L_TSTAMP_M, tstamp_l);
> +	tstamp |= FIELD_PREP(IDPF_PTP_TX_TSTAMP_H_POS_M,
> +			     FIELD_GET(IDPF_PTP_TX_TSTAMP_H_TSTAMP_M, tstamp_h));
> +
> +	return tstamp >> tstamp_caps->tstamp_ns_lo_bit;
> +}

[ ... ]

> +static void idpf_ptp_get_tx_tstamp_direct(struct idpf_vport *vport)
> +{
> +	struct idpf_ptp_vport_tx_tstamp_caps *tstamp_caps;
> +	struct idpf_ptp_tx_tstamp *ptp_tx_tstamp, *tmp;
> +	u64 readiness_bitmap, clear_map = 0;
> +	LIST_HEAD(done);
> +
> +	tstamp_caps = vport->tx_tstamp_caps;
> +
> +	spin_lock(&tstamp_caps->latches_lock);
> +
> +	if (list_empty(&tstamp_caps->latches_in_use)) {
> +		spin_unlock(&tstamp_caps->latches_lock);
> +		return;
> +	}
> +
> +	readiness_bitmap = idpf_ptp_read_tx_tstamp_readiness(tstamp_caps);
> +
> +	list_for_each_entry_safe(ptp_tx_tstamp, tmp,
> +				 &tstamp_caps->latches_in_use, list_member) {
> +		u32 idx = ptp_tx_tstamp->idx;
> +
> +		ptp_tx_tstamp->tstamp = 0;
> +
> +		if (readiness_bitmap & BIT_ULL(idx)) {
> +			ptp_tx_tstamp->tstamp =
> +				idpf_ptp_read_tx_tstamp(tstamp_caps,
> +							ptp_tx_tstamp);
> +			clear_map |= BIT_ULL(idx);
> +		}
> +
> +		/* A latch with no timestamp is kept in use until its deadline
> +		 * passes, as the hardware may still be about to report it.
> +		 * Once it does, drop its readiness bit as well, so that the
> +		 * next user of the index is not given a stale timestamp if
> +		 * the hardware reports one after the latch has been reclaimed.
> +		 */
> +		if (!ptp_tx_tstamp->tstamp) {
> +			if (time_is_after_jiffies(ptp_tx_tstamp->deadline))
> +				continue;
> +
> +			clear_map |= BIT_ULL(idx);
> +		}
> +
> +		list_move(&ptp_tx_tstamp->list_member, &done);
> +	}
> +
> +	/* The indexes are not recycled before the splice below, so the bits
> +	 * of every latch read above can be dropped in a single write.
> +	 */
> +	idpf_ptp_clear_tx_tstamp_ready(tstamp_caps, clear_map);

[Severity: Medium]
Does setting clear_map |= BIT_ULL(idx) before checking
!ptp_tx_tstamp->tstamp strand the latch when idpf_ptp_read_tx_tstamp()
returns 0?

If readiness_bitmap has BIT_ULL(idx) set but idpf_ptp_read_tx_tstamp()
returns 0 (either because IDPF_PTP_TX_TSTAMP_L_VALID_M is not set or
because the shifted timestamp is legitimately 0), the loop continues and
keeps ptp_tx_tstamp on latches_in_use, while
idpf_ptp_clear_tx_tstamp_ready() clears BIT_ULL(idx) in the hardware
readiness register.  Subsequent timer ticks will never see BIT_ULL(idx) set
again, so the latch stalls until its 500 ms deadline expires and the skb is
dropped.

[ ... ]

> +void idpf_ptp_tx_tstamp_start(struct idpf_vport *vport)
> +{
> +	if (!idpf_ptp_is_vport_tx_tstamp_ena(vport) ||
> +	    vport->tx_tstamp_caps->access != IDPF_PTP_DIRECT)
> +		return;
> +
> +	if (READ_ONCE(vport->tstamp_config.tx_type) == HWTSTAMP_TX_ON)
> +		hrtimer_start(&vport->tx_tstamp_timer,
> +			      IDPF_PTP_TX_TSTAMP_PERIOD_NS,
> +			      HRTIMER_MODE_REL_SOFT);
> +}

[Severity: Medium]
Can idpf_hwtstamp_set() -> idpf_ptp_set_timestamp_mode() start
vport->tx_tstamp_timer and dereference a NULL rsrc->rxq_grps while the
vport is down?

idpf_hwtstamp_set() only checks vport->link_up, which
idpf_handle_event_link() sets before checking test_bit(IDPF_VPORT_UP,
np->state) (and which idpf_vport_open() also leaves true if
idpf_config_rss() fails and unwinds queue allocation).  If hardware
timestamping is configured while the vport is in that state,
idpf_ptp_tx_tstamp_start() starts tx_tstamp_timer while the vport is down
and idpf_ptp_set_rx_tstamp() then dereferences NULL
vport->dflt_qv_rsrc.rxq_grps.

> +/**
> + * idpf_ptp_tx_tstamp_stop - Stop collecting the Tx timestamps
> + * @vport: Virtual port structure
> + *
> + * Stop the collection timer and flush the latches that are still waiting for
> + * a timestamp, as nothing would collect them anymore. No new latch is handed
> + * out afterwards, either because the Tx timestamping has been disabled, which
> + * idpf_ptp_request_ts() checks under the latches lock, or because the Tx path
> + * is stopped.
> + */
> +void idpf_ptp_tx_tstamp_stop(struct idpf_vport *vport)
> +{
> +	struct idpf_ptp_vport_tx_tstamp_caps *tstamp_caps;
> +	struct idpf_ptp_tx_tstamp *ptp_tx_tstamp, *tmp;
> +	u64 clear_map = 0;
> +
> +	if (!idpf_ptp_is_vport_tx_tstamp_ena(vport) ||
> +	    vport->tx_tstamp_caps->access != IDPF_PTP_DIRECT)
> +		return;
> +
> +	tstamp_caps = vport->tx_tstamp_caps;
> +
> +	hrtimer_cancel(&vport->tx_tstamp_timer);
> +
> +	spin_lock_bh(&tstamp_caps->latches_lock);
> +
> +	u64_stats_update_begin(&vport->tstamp_stats.stats_sync);
> +	list_for_each_entry_safe(ptp_tx_tstamp, tmp,
> +				 &tstamp_caps->latches_in_use, list_member) {
> +		u64_stats_inc(&vport->tstamp_stats.flushed);
> +
> +		clear_map |= BIT_ULL(ptp_tx_tstamp->idx);
> +
> +		if (ptp_tx_tstamp->skb)
> +			consume_skb(ptp_tx_tstamp->skb);
> +
> +		ptp_tx_tstamp->skb = NULL;
> +		list_move(&ptp_tx_tstamp->list_member,
> +			  &tstamp_caps->latches_free);
> +	}
> +	u64_stats_update_end(&vport->tstamp_stats.stats_sync);
> +
> +	idpf_ptp_clear_tx_tstamp_ready(tstamp_caps, clear_map);
> +
> +	spin_unlock_bh(&tstamp_caps->latches_lock);
> +}

[Severity: Medium]
Can an in-flight packet set the write-1-to-clear readiness bit after its
latch has already been cleared and returned to latches_free?

When a latch is flushed here by idpf_ptp_set_timestamp_mode() while the
vport is still transmitting, or when its 500 ms deadline expires in
idpf_ptp_get_tx_tstamp_direct(), idpf_ptp_clear_tx_tstamp_ready() writes 1
while the hardware readiness bit is still 0 (a no-op on W1C registers) and
moves the latch to latches_free.  If the packet carrying that latch index in
its TSYN context descriptor is still queued in the hardware Tx ring and
transmits afterwards, hardware sets both the readiness bit and
IDPF_PTP_TX_TSTAMP_L_VALID_M while the latch sits on latches_free, so the
next packet to reuse that index will report the previous packet's stale
timestamp.

[ ... ]

> @@ -923,12 +1265,21 @@ int idpf_ptp_set_timestamp_mode(struct idpf_vport *vport,
>  	case HWTSTAMP_TX_ON:
>  		if (!idpf_ptp_is_vport_tx_tstamp_ena(vport))
>  			return -EINVAL;
> +
>  		break;
>  	default:
>  		return -EINVAL;
>  	}
>  
> -	vport->tstamp_config.tx_type = config->tx_type;
> +	/* Publish the new mode first, so that the Tx path stops handing out
> +	 * latches, then flush the ones that are still waiting.
> +	 */
> +	WRITE_ONCE(vport->tstamp_config.tx_type, config->tx_type);
> +	if (config->tx_type == HWTSTAMP_TX_ON)
> +		idpf_ptp_tx_tstamp_start(vport);
> +	else
> +		idpf_ptp_tx_tstamp_stop(vport);

[Severity: Medium]
Can idpf_ptp_set_timestamp_mode() call hrtimer_start() on
vport->tx_tstamp_timer while idpf_ptp_tx_tstamp_timer_cb() is running?

When Tx timestamping is already HWTSTAMP_TX_ON and user space issues another
SIOCSHWTSTAMP or ETHTOOL_MSG_TSCONFIG_SET with HWTSTAMP_TX_ON (for example,
to change rx_filter or when restarting ptp4l), idpf_ptp_tx_tstamp_start()
calls hrtimer_start() without cancelling the running timer first:

CPU0                                  CPU1
<softirq>
idpf_ptp_tx_tstamp_timer_cb()
  idpf_ptp_get_tx_tstamp_direct()
                                      idpf_hwtstamp_set()
                                        idpf_ptp_set_timestamp_mode()
                                          idpf_ptp_tx_tstamp_start()
                                            hrtimer_start(&tx_tstamp_timer)
                                              /* enqueues timer */
  hrtimer_forward_now(timer, ...)
    WARN_ON(timer->is_queued)  <-- modifies expires while queued in rbtree
  return HRTIMER_RESTART;

hrtimer_forward_now() runs without cpu_base->lock held and requires the
timer not to be concurrently enqueued, so this triggers WARN_ON(timer->is_queued)
in hrtimer_forward() and modifies timer->node.expires while the node is in
the active timer rbtree.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109105433.133056-1-przemyslaw.korba@intel.com?part=2