Re: [PATCH net-next V2 2/5] net/mlx5: nodnic: Implement data path

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 3 · Medium: 6 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] mlx5_nodnic_activate_queues() (en_cfg.c) calls
  napi_enable_locked() first.
- [High] mlx5_nodnic_deactivate_queues() (en_cfg.c, added in this patch)
  calls mlx5_nodnic_disable_port() only if…
- [High] TX stop/wake between mlx5_nodnic_xmit() and
  mlx5_nodnic_complete_send_wqe() uses plain, unannotated sq->cc/sq->pc…
- [Medium] mlx5_nodnic_handle_err_cqe()
  (drivers/net/ethernet/mellanox/mlx5/nodnic/en.c) handles…
- [Medium] mlx5_nodnic_napi_poll() (en.c) does not handle budget == 0.
- [Medium] In mlx5_nodnic_napi_poll() (en.c), when '++tx_work_done >=
  MLX5_NODNIC_TX_CQ_POLL_BUDGET' (128) the loop jumps to poll_done while…
- [Medium] The CQ consumer-index doorbell record (cq->dbr.db[0], set_ci)
  is written only in mlx5_nodnic_arm_cq(), which runs only after…
- [Medium] mlx5_nodnic_handle_recv_cqe() (rx.c) passes the device-
  reported byte_cnt to page_pool_dma_sync_for_cpu() and skb_put()…
- [Medium] mlx5_nodnic_enable_port() makes three separate PORT_NETWORK
  writes: data/event MSI-X enable, then network_en plus…

--

net/mlx5: nodnic: Implement data path

This commit implements the TX and RX data paths for the mlx5_nodnic driver.
It adds WQE posting with a BlueFlame doorbell, page pool backed RX, and a
single NAPI context that dispatches TX and RX CQEs. It also wires up
ndo_start_xmit and ndo_features_check and enables TX/RX checksum offload.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/nodnic/en.c b/drivers/net/ethernet/mellanox/mlx5/nodnic/en.c
> new file mode 100644
> index 0000000000000..3572aff50194f
> --- /dev/null
> +++ b/drivers/net/ethernet/mellanox/mlx5/nodnic/en.c

[ ... ]

> +static void mlx5_nodnic_handle_err_cqe(struct mlx5_nodnic_cq *cq,
> +				       struct mlx5_cqe64 *cqe64,
> +				       int napi_budget)
> +{

[ ... ]

> +	netdev_err(priv->netdev,
> +		   "%s CQE error: syndrome=0x%02x (%s), vendor_syndrome=0x%02x, wqe_counter=%u, cq_ci=%d\n",
> +		   queue_type,
> +		   syndrome, mlx5_nodnic_cqe_err_syndrome_str(syndrome),
> +		   vendor_syndrome, wqe_counter, cq->cc);
> +
> +	print_hex_dump(KERN_ERR, "mlx5_nodnic err CQE: ", DUMP_PREFIX_OFFSET,
> +		       16, 1, err_cqe, sizeof(*err_cqe), false);
> +
> +	if (opcode == OPCODE_CQ_SEND_ERR) {
> +		struct mlx5_nodnic_sq *sq = &priv->sq;
> +		u16 sq_pi = mlx5_nodnic_sq_get_pi(sq, wqe_counter);
> +
> +		mlx5_nodnic_complete_send_wqe(sq, &sq->wqe_info[sq_pi],
> +					      napi_budget);
> +	} else {
> +		struct mlx5_nodnic_rq *rq = &priv->rq;
> +		u16 ci = wqe_counter & (rq->ring.num_entries - 1);
> +
> +		mlx5_nodnic_release_rx_wqe(rq, ci);
> +	}
> +}

[Severity: Medium]
Is there a recovery path for the SQ or RQ after an error CQE?

mlx5_nodnic_handle_err_cqe() calls netdev_err() and does a 64 byte
print_hex_dump() at KERN_ERR. Neither is rate limited. It then completes or
releases the one WQE named by wqe_counter and carries on. Nothing stops,
resets or recovers the queue.

The driver decodes MLX5_CQE_SYNDROME_WR_FLUSH_ERR, so it seems to expect
mlx5 flush-after-error behaviour. If so, one non-flush error (for example
LOCAL_LENGTH_ERR) would make every later TX WQE come back as a flush error.

mlx5_nodnic_napi_poll() also reposts every released RX WQE through
mlx5_nodnic_rq_post_wqes(). An errored RQ would flush them straight back.

Could one hardware error then become a flood of KERN_ERR output from
softirq context? TX and RX would stay dead until the interface is bounced,
while the queue and carrier still look up.

[ ... ]

> +int mlx5_nodnic_napi_poll(struct napi_struct *napi, int budget)
> +{

[ ... ]

> +	while (true) {
> +		cqe = mlx5_nodnic_cq_get_cqe(cq);
> +		if (!cqe)
> +			break;
> +
> +		result = mlx5_nodnic_handle_cqe(cq, cqe, budget);
> +		switch (result) {
> +		case MLX5_NODNIC_CQE_TX:
> +			if (++tx_work_done >= MLX5_NODNIC_TX_CQ_POLL_BUDGET)
> +				goto poll_done;
> +			break;
> +		case MLX5_NODNIC_CQE_RX:
> +			if (++rx_work_done >= budget)
> +				goto poll_done;
> +			break;

[Severity: Medium]
Does this poll handle budget == 0? netpoll calls it with a zero budget:

net/core/netpoll.c:poll_one_napi() {
    ...
	work = napi->poll(napi, 0);
    ...
}

The next CQE is fully handled before the budget is checked.

For an RX CQE, mlx5_nodnic_handle_recv_cqe() calls
page_pool_dma_sync_for_cpu(), napi_build_skb() and napi_gro_receive(). For
an RX error CQE, mlx5_nodnic_release_rx_wqe() calls
page_pool_put_full_page() with allow_direct set to true.

Only after that does ++rx_work_done >= budget exit the loop. poll_done then
always calls mlx5_nodnic_rq_post_wqes(), which allocates from the page pool.

Documentation/networking/napi.rst says that with a zero budget only TX
completions should be processed, and page pool and RX APIs must not be
used.

With netconsole on this netdev, netpoll can run from hardirq or printk
context. Can this path then touch the page pool's lockless cache, the
per-CPU NAPI skb cache and GRO? Could those get corrupted if it interrupts
a softirq user on the same CPU?

> +		case MLX5_NODNIC_CQE_IGNORED:
> +			break;
> +		}
> +	}
> +
> +poll_done:
> +	if (!mlx5_nodnic_rq_post_wqes(priv))
> +		return budget;
> +
> +	if (rx_work_done >= budget)
> +		return budget;

[Severity: Medium]
On both return budget paths, does the device ever see an updated CQ
consumer index?

The CQ doorbell record consumer index is only written in
mlx5_nodnic_arm_cq():

	cq->dbr.db[0] = cpu_to_be32(ci);

That only runs after napi_complete_done() succeeds.

Under sustained RX that fills the budget on every pass, NAPI never
completes. cq->cc keeps advancing in software, but db[0] keeps the value
from the last arm.

Meanwhile mlx5_nodnic_rq_post_wqes() keeps reposting RX WQEs based on the
software rq->cc. mlx5_nodnic_complete_send_wqe() keeps freeing SQ slots
and waking TX.

Suppose the device uses set_ci for CQ overrun detection like other mlx5
devices. Could it then see more outstanding CQEs than the CQ size and
report an overrun or stall, even though software has consumed every entry?

For comparison, mlx5e updates the CQ doorbell record with
mlx5_cqwq_update_db_record() at the end of every poll pass.

> +
> +	if (napi_complete_done(napi, rx_work_done))
> +		mlx5_nodnic_arm_cq(cq);
> +
> +	return rx_work_done;
> +}

[Severity: Medium]
When the loop exits because tx_work_done reached
MLX5_NODNIC_TX_CQ_POLL_BUDGET, more CQEs may still be pending:

		case MLX5_NODNIC_CQE_TX:
			if (++tx_work_done >= MLX5_NODNIC_TX_CQ_POLL_BUDGET)
				goto poll_done;

Suppose the RQ refill succeeds and rx_work_done < budget. This path then
calls napi_complete_done(), re-arms the CQ and returns less than budget,
even though polling stopped with work left.

Should this return budget instead, as mlx5e does when its TX poll hits its
limit?

As written, the remaining TX completions, and any RX CQEs behind them,
depend on the device raising an event when armed with unconsumed CQEs. If
it only signals on new CQE writes, TX completions could stall with the SQ
stopped. There is no ndo_tx_timeout to recover.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/nodnic/en_cfg.c b/drivers/net/ethernet/mellanox/mlx5/nodnic/en_cfg.c
> index 5724ed41d8b89..9657c43e9958f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/nodnic/en_cfg.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/nodnic/en_cfg.c

[ ... ]

> @@ -573,6 +643,143 @@ void mlx5_nodnic_refresh_carrier(struct mlx5_nodnic_priv *priv)

[ ... ]

> +static int mlx5_nodnic_activate_queues(struct mlx5_nodnic_core_dev *dev)
> +{
> +	struct mlx5_nodnic_cq *cq = &dev->priv->cq;
> +	int err;
> +
> +	napi_enable_locked(&cq->napi);
> +
> +	mlx5_nodnic_init_cqes_ownership(cq);
> +
> +	err = mlx5_nodnic_activate_rq(dev);
> +	if (err)
> +		goto err_napi;
> +
> +	err = mlx5_nodnic_vsc_context_acquire(dev->vsc_ctx);
> +	if (err)
> +		goto err_rq;
> +
> +	err = mlx5_nodnic_enable_port(dev);
> +	mlx5_nodnic_vsc_context_release(dev->vsc_ctx);
> +	if (err)
> +		goto err_rq;

[Severity: Medium]
If mlx5_nodnic_enable_port() fails after its first or second write, is the
port left partly enabled?

mlx5_nodnic_enable_port() makes three separate PORT_NETWORK writes: MSI-X
enable, then network_en plus receive_filter_en, then dma_en. It returns
early on failure and does not roll back.

This unwind goes to err_rq without calling mlx5_nodnic_disable_port().
mlx5_nodnic_open_locked() then reaches:

err_close:
	clear_bit(MLX5_NODNIC_STATE_OPENED, &priv->state);
	mlx5_nodnic_close_queues(dev);

That frees the rings, DBRs and working buffer, but their addresses are
still programmed in the device.

The dma_en write might take effect but still be reported as failed, for
example on a VSC flag-wait timeout. Could the device then DMA into freed
memory?

mlx5_nodnic_disable_port() also notes that data_msix_en and event_msix_en
can't be modified while the port is active. Could the leftover state make
the first enable_port write on the next open ineffective?

> +
> +	mlx5_nodnic_arm_cq(cq);
> +	napi_schedule(&cq->napi);
> +
> +	mlx5_nodnic_activate_sq(dev);
> +
> +	return 0;
> +
> +err_rq:
> +	mlx5_nodnic_deactivate_rq(dev);
> +err_napi:
> +	napi_disable_locked(&cq->napi);
> +
> +	return err;
> +}

[Severity: High]
Is this unwind order safe?

mlx5_nodnic_activate_rq() creates the page pool with
.napi = &priv->cq.napi. On the err_rq path, mlx5_nodnic_deactivate_rq()
frees rq->pages and calls page_pool_destroy() while NAPI is still enabled.
Only after that does err_napi call napi_disable_locked(). The
err_destroy_page_pool path inside mlx5_nodnic_activate_rq() has the same
problem.

page_pool_destroy() reaches this check through
page_pool_disable_direct_recycling()->napi_assert_will_not_race():

	WARN_ON(!test_bit(NAPI_STATE_SCHED, &napi->state));

napi_enable_locked() has already cleared that bit. So wouldn't any open
failure after page_pool_create() hit this WARN? That covers RX page
allocation, VSC acquire and enable_port failures. The pool is also torn
down without synchronizing against a running poll.

mlx5_nodnic_deactivate_queues() uses the other order: it calls
napi_disable_locked() before mlx5_nodnic_deactivate_rq().

A related question is why napi_enable_locked() runs first. It comes before
mlx5_nodnic_init_cqes_ownership(), before mlx5_nodnic_activate_rq() sets up
rq->page_pool and rq->pages, and before mlx5_nodnic_activate_sq() sets
sq->txq.

napi_enable_locked()->napi_restore_config() re-hashes the napi_id kept from
the previous session. A busy-polling socket can then call
mlx5_nodnic_napi_poll() straight away, without the netdev or state lock.

On reopen, rq->pages still points at the array freed by
mlx5_nodnic_rq_free_pages() during the previous close:

	kvfree(rq->pages);

rq->page_pool still points at the pool destroyed then, and open_rq has
reset rq->pc and rq->cc to 0. Can
mlx5_nodnic_rq_post_wqes()->mlx5_nodnic_rq_post_one() then read
rq->pages[pi] and call page_pool_dev_alloc_pages(rq->page_pool) on freed
memory?

Before the ownership init runs, the zeroed CQ ring has op_own == 0. That
matches software ownership 0 and opcode OPCODE_CQ_SEND. Would a poll in
that window complete a bogus send through wqe_info[0] and push sq->cc past
sq->pc?

> +
> +static void mlx5_nodnic_deactivate_queues(struct mlx5_nodnic_core_dev *dev)
> +{
> +	if (!mlx5_nodnic_vsc_context_acquire(dev->vsc_ctx)) {
> +		mlx5_nodnic_disable_port(dev);
> +		mlx5_nodnic_vsc_context_release(dev->vsc_ctx);
> +	}
> +
> +	mlx5_nodnic_deactivate_sq(&dev->priv->sq);
> +	napi_disable_locked(&dev->priv->cq.napi);
> +	mlx5_nodnic_deactivate_rq(dev);
> +}

[Severity: High]
What happens to the device if mlx5_nodnic_vsc_context_acquire() fails here?

Acquire can fail with -EBUSY when the gateway semaphore is still held after
VSC_MAX_RETRIES in mlx5_nodnic_vsc_gw_lock(). It can also fail with
-EACCES, or when setting the space fails. In all of these cases
mlx5_nodnic_disable_port() is skipped. Even when it is called, it returns
void and only warns if its read or write of PORT_NETWORK fails.

Either way teardown continues. mlx5_nodnic_deactivate_rq() returns the
device-owned RX pages to the page pool and destroys it.
mlx5_nodnic_close_queues() then frees the coherent memory:

	mlx5_nodnic_free_ring(dev, &priv->working_buffer);
	mlx5_nodnic_close_cq(&priv->cq);
	mlx5_nodnic_close_rq(dev);
	mlx5_nodnic_close_sq(dev);
	mlx5_nodnic_free_dbr(dev, &priv->dbr);

At that point the device still has network_en and dma_en set and the ring
and DBR addresses programmed. Its RX WQEs still point at the freed pages.

Can an incoming packet then be DMA-written into pages that are back in the
page allocator? Can the device keep fetching the freed SQ ring and doorbell
records?

Even when the disable write succeeds, nothing reads back or waits for
in-flight DMA to drain.

[ ... ]

> diff --git a/drivers/net/ethernet/mellanox/mlx5/nodnic/rx.c b/drivers/net/ethernet/mellanox/mlx5/nodnic/rx.c
> new file mode 100644
> index 0000000000000..7736a1f24ea5b
> --- /dev/null
> +++ b/drivers/net/ethernet/mellanox/mlx5/nodnic/rx.c

[ ... ]

> +void mlx5_nodnic_handle_recv_cqe(struct mlx5_nodnic_cq *cq,
> +				 struct mlx5_cqe64 *cqe)
> +{

[ ... ]

> +	u32 byte_count = be32_to_cpu(cqe->byte_cnt);

[ ... ]

> +	page_pool_dma_sync_for_cpu(rq->page_pool, page, 0, byte_count);
> +
> +	skb = napi_build_skb(page_address(page), PAGE_SIZE);
> +	if (unlikely(!skb))
> +		goto err_recycle;
> +
> +	skb_mark_for_recycle(skb);
> +	rq->pages[ci] = NULL;
> +
> +	skb_reserve(skb, MLX5_NODNIC_SKB_HEADROOM_BYTES);
> +	skb_put(skb, byte_count);

[Severity: Medium]
This is mostly a hardening question. byte_count comes straight from the
CQE and is passed to page_pool_dma_sync_for_cpu() and skb_put(). It is
never checked against the posted buffer size (MLX5_NODNIC_HW_MTU) or the
skb tailroom.

A CQE with byte_cnt above roughly PAGE_SIZE - NET_SKB_PAD minus the
skb_shared_info size would hit skb_over_panic()->BUG().

This needs a buggy or misbehaving device. Every RX WQE is posted with
byte_count = MLX5_NODNIC_HW_MTU, mlx5-family hardware normally reports
oversize frames as LOCAL_LENGTH_ERR, and the mlx5e linear RX path doesn't
check this either. Would a cheap bound check here still be worthwhile?

[ ... ]

> diff --git a/drivers/net/ethernet/mellanox/mlx5/nodnic/tx.c b/drivers/net/ethernet/mellanox/mlx5/nodnic/tx.c
> new file mode 100644
> index 0000000000000..f2efc8d3f5b85
> --- /dev/null
> +++ b/drivers/net/ethernet/mellanox/mlx5/nodnic/tx.c

[ ... ]

> +static int mlx5_nodnic_tx_is_sq_full(struct mlx5_nodnic_sq *sq)
> +{
> +	u16 pending = (u16)(sq->pc - sq->cc);
> +
> +	return pending == sq->ring.num_entries - MLX5_NODNIC_SQ_STOP_ROOM;
> +}

[ ... ]

> +netdev_tx_t mlx5_nodnic_xmit(struct sk_buff *skb, struct net_device *netdev)
> +{

[ ... ]

> +	if (unlikely(mlx5_nodnic_tx_is_sq_full(sq)))
> +		netif_tx_stop_queue(sq->txq);
> +
> +	next_offset = mlx5_nodnic_tx_get_next_sq_offset(sq);
> +
> +	wqe = (struct mlx5_nodnic_tx_wqe *)((u8 *)sq->ring.wq + next_offset);
> +
> +	err = mlx5_nodnic_tx_write_wqe(sq, skb, wqe);
> +	if (err) {
> +		dev_kfree_skb_any(skb);
> +
> +		return NETDEV_TX_OK;
> +	}

[Severity: High]
Is the stop/wake handshake between mlx5_nodnic_xmit() and
mlx5_nodnic_complete_send_wqe() safe on weakly ordered CPUs?

sq->pc and sq->cc use plain loads and stores with no barriers. xmit does
not re-check after stopping, and the full test only matches
pending == num_entries - 1.

When pending == N - 1, xmit stops the queue and still posts, which leaves
the ring full (pending == N). NAPI then does:

	sq->cc++;

	if (netif_tx_queue_stopped(txq))
		netif_tx_wake_queue(txq);

On the next xmit, nothing orders the load of sq->cc after the observation
that XOFF was cleared. On arm64 or POWER that load can return the old cc.
pending is then N, the == N - 1 check fails, and the queue is not stopped.
That post uses the slot just freed, so the ring is now truly full with the
queue still running.

The xmit after that again sees pending == N and does not stop. It writes
the WQE and wqe_info at pc & (N - 1), the slot of the oldest WQE the
hardware still owns.

Can this corrupt an in-flight descriptor, lose its skb and DMA mapping, and
let pc run past cc + N?

On the completion side, cc++ followed by test_bit() of XOFF has no full
barrier to pair with xmit's load of cc followed by set_bit().

The dma mapping failure path above also returns after netif_tx_stop_queue()
without posting anything. With that path in the mix, could the queue end up
stopped with nothing outstanding to wake it?

netif_txq_maybe_stop() and netif_txq_completed_wake() in
include/net/netdev_queues.h provide the smp_mb(), a >= test and the
re-check. Would they fit here?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008171821.660739-1-tariqt%40nvidia.com