Re: [PATCH net-next v8 3/6] idpf: support pacing offload

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] Divergent EDT state across a hard reset: the netdev-side
  pacing horizon survives while the adapter-side timestamp granularity…

--

idpf: support pacing offload

Adds EDT (earliest departure time) support to idpf: the txtime from
skb->tstamp is bounds checked and programmed into the flow scheduling Tx
descriptor. The feature is negotiated with the device through a new
VIRTCHNL2_OP_GET_EDT_CAPS exchange, and the resulting horizon is exposed
as netdev->max_pacing_offload_horizon.

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> index 827c795afcb6..c2ad3fdf72aa 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> @@ -890,6 +890,12 @@ static int idpf_cfg_netdev(struct idpf_vport *vport)
>  	netdev->min_mtu = ETH_MIN_MTU;
>  	netdev->max_mtu = vport->max_mtu;
>  
> +	if (adapter->edt_caps.time_horizon_ns &&
> +	    idpf_is_queue_model_split(vport->dflt_qv_rsrc.txq_model) &&
> +	    !idpf_is_cap_ena(adapter, IDPF_OTHER_CAPS,
> +			     VIRTCHNL2_CAP_SPLITQ_QSCHED))
> +		netdev->max_pacing_offload_horizon = adapter->edt_caps.time_horizon_ns;
> +
>  	dflt_features = NETIF_F_SG	|
>  			NETIF_F_HIGHDMA;
>  
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> index 24b91be25676..d80b6f5a0fde 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> @@ -1742,6 +1742,7 @@ static int idpf_txq_group_alloc(struct idpf_vport *vport,
>  			q->desc_count = rsrc->txq_desc_count;
>  			q->tx_max_bufs = idpf_get_max_tx_bufs(adapter);
>  			q->tx_min_pkt_len = idpf_get_min_tx_pkt_len(adapter);
> +			q->ts_gran_pow2 = adapter->edt_caps.tstamp_granularity_pow2;
>  			q->netdev = vport->netdev;
>  			q->txq_grp = tx_qgrp;
>  			q->rel_q_id = j;

[ ... ]

> @@ -3011,6 +3017,60 @@ static bool idpf_tx_splitq_need_re(struct idpf_tx_queue *tx_q)
>  	return gap >= IDPF_TX_SPLITQ_RE_MIN_GAP;
>  }
>  
> +static void idpf_tx_splitq_set_txtime(const struct sk_buff *skb,
> +				      const struct idpf_tx_queue *tx_q,
> +				      struct idpf_tx_splitq_params *tx_params)
> +{
> +	const int offload_slack_ns = 400;
> +	u64 ts, now, horizon;
> +
> +	horizon = READ_ONCE(skb->dev->pacing_offload_horizon);
> +	if (!horizon)
> +		return;
> +
[ ... ]
> +	ts >>= tx_q->ts_gran_pow2;
> +
> +	/* 0 is valid 23b timestamp, but also means field unset.
> +	 * Increase by one to avoid this case
> +	 */
> +	if ((ts & 0x7fffff) == 0) {
> +		tx_params->offload.desc_ts[0] = 1;
> +		return;
> +	}
> +
> +	tx_params->offload.desc_ts[0] = ts & 0xff;
> +	tx_params->offload.desc_ts[1] = (ts >> 8) & 0xff;
> +	tx_params->offload.desc_ts[2] = ((ts >> 16) & 0x7f);
> +}
> +

[ ... ]

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> index 1caf52706973..030ecd9b6c41 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> @@ -573,6 +575,54 @@ static int idpf_send_get_caps_msg(struct idpf_adapter *adapter)
>  	return err;
>  }
>  
[ ... ]
> +	if (xn_params.recv_mem.iov_len < sizeof(caps)) {
> +		err = -EIO;
> +		goto free_rx_buf;
> +	}
> +
> +	memcpy(&caps, xn_params.recv_mem.iov_base, sizeof(caps));
> +	horizon_ns = le64_to_cpu(caps.time_horizon_ns);
> +	gran_ns = le64_to_cpu(caps.tstamp_granularity_ns);
> +	if (horizon_ns > U32_MAX) {
> +		dev_warn(&adapter->pdev->dev, "EDT horizon exceeds U32\n");
> +		err = -EINVAL;
> +		goto free_rx_buf;
> +	}
> +	if (!gran_ns) {
> +		dev_warn(&adapter->pdev->dev, "EDT granularity undefined\n");
> +		err = -EINVAL;
> +		goto free_rx_buf;
> +	}
> +
> +	adapter->edt_caps.time_horizon_ns = horizon_ns;
> +	adapter->edt_caps.tstamp_granularity_pow2 = ilog2(gran_ns);
> +
> +free_rx_buf:
> +	libie_ctlq_release_rx_buf(&xn_params.recv_mem);
> +
> +	return err;
> +}
> +
[ ... ]
> @@ -3085,6 +3135,14 @@ int idpf_vc_core_init(struct idpf_adapter *adapter)
>  		}
>  	}
>  
> +	if (idpf_is_cap_ena(adapter, IDPF_OTHER_CAPS, VIRTCHNL2_CAP_EDT)) {
> +		err = idpf_send_get_edt_caps_msg(adapter);
> +		if (err) {
> +			dev_err(&adapter->pdev->dev, "EDT init failed, err=%d\n", err);
> +			memset(&adapter->edt_caps, 0, sizeof(adapter->edt_caps));
> +		}
> +	}
> +

[Severity: Medium]
When this re-negotiation fails on a reset, edt_caps is zeroed and init
continues, but nothing revokes the pacing horizon already published on the
netdev.  Can that leave the two halves of the EDT state inconsistent?

idpf_init_hard_reset() calls idpf_vc_core_init() again on the same adapter
while the netdevs stay registered (IDPF_VPORT_REG_NETDEV is only cleared in
idpf_decfg_netdev(), on driver remove), so idpf_cfg_netdev() takes its early
return:

	if (test_bit(IDPF_VPORT_REG_NETDEV, vport_config->flags)) {
		netdev = adapter->netdevs[idx];
		...
		return idpf_init_mac_addr(vport, netdev);
	}

and never re-evaluates the new assignment:

	if (adapter->edt_caps.time_horizon_ns && ...)
		netdev->max_pacing_offload_horizon = adapter->edt_caps.time_horizon_ns;

The dev->pacing_offload_horizon value the admin set earlier through
RTM_SETLINK also survives, since it is only validated at set time.

The re-created Tx queues do pick up the zeroed granularity in
idpf_txq_group_alloc():

	q->ts_gran_pow2 = adapter->edt_caps.tstamp_granularity_pow2;

and idpf_tx_splitq_set_txtime() gates only on the netdev horizon:

	horizon = READ_ONCE(skb->dev->pacing_offload_horizon);
	if (!horizon)
		return;
	...
	ts >>= tx_q->ts_gran_pow2;

With ts_gran_pow2 of 0 the shift is a no-op, so the low 23 bits of a raw
CLOCK_TAI nanosecond value end up in the descriptor ts[] field while the
device still interprets that field in its own unchanged granularity.  Would
departure times then land on effectively arbitrary points inside the hardware
window (up to roughly 4.29 s), holding a descriptor and buf_id each, after
only the single dev_err above?

Note that granularity 0 is a value the driver itself rejects from firmware
("EDT granularity undefined"), so it is being used as an invalid state here.

Would it be reasonable to clear dev->pacing_offload_horizon and
max_pacing_offload_horizon on this failure path, or to have
idpf_tx_splitq_set_txtime() also check that tx_q->ts_gran_pow2 is valid?

One more thing on this part of the commit message:

    On device reset, dev->pacing_offload_horizon, fq offload_horizon and
    granularity are not re-negotiated. It is safe to assume that firmware
    does not change these EDT capabilities across resets.

idpf_vc_core_init() runs again on every hard reset and does re-issue
VIRTCHNL2_OP_GET_EDT_CAPS, so granularity is re-negotiated.  Could the
wording be adjusted to match what the code does?

>  	pci_sriov_set_totalvfs(adapter->pdev, idpf_get_max_vfs(adapter));
>  	num_max_vports = idpf_get_max_vports(adapter);
>  	adapter->vports = kzalloc_objs(*adapter->vports, num_max_vports);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902181747.2483351-1-willemdebruijn.kernel%40gmail.com