Re: [PATCH v27 net-next 03/10] net/nebula-matrix: add channel layer

[netdev-bot+sashiko@kernel.org]

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

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

- [High] nbl_chan_quiesce_and_reclaim_tx() (TX timeout recovery) resets
  the software doorbell counter txq->tail_ptr to 0 to match the…
- [Medium] The commit message justifies leaving the mailbox BAR2 register
  path unlocked by asserting that the multi-dword QINFO config sequences…
- [Medium] nbl_chan_recv_ack_msg() completes a request slot (copies the
  response, sets wait_head->acked = 1) but never transitions…
- [Medium] nbl_chan_quiesce_and_reclaim_tx() uses wmb() with the comment
  "Ensure reset write is posted before reclaiming descriptors" and then…
- [Medium] nbl_hw_cfg_mailbox_qinfo() builds the QINFO map dword from
  scratch (FUNCTION/DEVID/BUS only) and writes it, unconditionally…
- [Low] Concern (verified FALSE POSITIVE): nbl_chan_send_msg() never
  checks chan_info->active, so a send issued before…
- [Low] Concern (verified FALSE POSITIVE): nbl_chan_teardown_queue()
  frees nothing but clears chan_info->active, so a subsequent…
- [Low] Concern (verified FALSE POSITIVE): the commit message advertises
  working capabilities (dual RX modes, complete queue lifecycle, PF…
- [Low] Concern (verified FALSE POSITIVE): nbl_common_get_hash_node()
  returns hash_node->data after dropping the per-bucket spinlock while…
- [Low] Concern (verified FALSE POSITIVE): setup_queue/teardown_queue
  check and modify chan_info->active, shutdn, the state bitmap,…
- [Low] Concern (verified FALSE POSITIVE): nbl_chan_update_txqueue()
  accepts arg_len > 0 with arg == NULL; in the external-buffer branch it…
- [Low] Concern (verified FALSE POSITIVE): nbl_chan_remove_common()
  destroys the workqueue and frees the handler table but never tears…

--

net/nebula-matrix: add channel layer

This adds the mailbox-based inter-PF channel layer for the Nebula Matrix
adapter, including the TX/RX descriptor ring management, a hash table of
message handlers, and the hardware ops that program the mailbox QINFO
tables and the tail pointer doorbell.

A few questions about the TX timeout recovery path, the ACK completion
handshake, and the QINFO map table write.

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_channel/nbl_channel.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_channel/nbl_channel.c
> new file mode 100644
> index 0000000000000..879e7bab14e94
> --- /dev/null
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_channel/nbl_channel.c

[ ... ]

> +static int nbl_chan_alloc_all_tx_bufs(struct nbl_channel_mgt *chan_mgt,
> +				      struct nbl_chan_info *chan_info)
> +{

[ ... ]

> +	txq->next_to_clean = 0;
> +	txq->next_to_use = 0;
> +	txq->tail_ptr = 0;
> +
> +	return 0;
> +}

[ ... ]

> +static int nbl_chan_update_txqueue(struct nbl_channel_mgt *chan_mgt,
> +				   struct nbl_chan_info *chan_info,
> +				   struct nbl_chan_tx_param *param)
> +{

[ ... ]

> +	/* Ensure descriptor data visible to device before AVAIL flag */
> +	dma_wmb();
> +	tx_desc->flags = cpu_to_le16(BIT(NBL_CHAN_TX_DESC_AVAIL));
> +
> +	txq->next_to_use =
> +		NBL_NEXT_ID(txq->next_to_use, chan_info->num_txq_entries - 1);
> +	txq->tail_ptr++;
> +
> +	return 0;
> +}
> +
> +/*
> + * Quiesce the TX mailbox queue and reclaim all outstanding
> + * descriptors.  Called from the timeout path of nbl_chan_kick_tx_ring()
> + * with txq_lock held.
> + *
> + * The device failed to fetch/complete the current descriptor within the
> + * polling window.  We assert QUEUE_RST to stop further DMA fetches,
> + * reclaim every descriptor between next_to_clean and next_to_use,
> + * reset the software tail_ptr counter to match the hardware reset state,
> + * and re-enable the queue so subsequent sends can proceed.
> + */
> +static void nbl_chan_quiesce_and_reclaim_tx(struct nbl_channel_mgt *chan_mgt,
> +					    struct nbl_chan_info *chan_info)
> +{
> +	struct nbl_hw_ops *hw_ops = chan_mgt->hw_ops_tbl->ops;
> +	struct nbl_hw_mgt *hw_priv = chan_mgt->hw_ops_tbl->priv;
> +	struct nbl_chan_ring *txq = &chan_info->txq;
> +	struct nbl_chan_tx_desc *tx_desc;
> +
> +	/* Assert QUEUE_RST to stop hardware fetching new descriptors */
> +	hw_ops->stop_mailbox_txq(hw_priv);
> +	/* Ensure reset write is posted before reclaiming descriptors */
> +	wmb();

[Severity: Medium]
Does this wmb() actually guarantee the posted QUEUE_RST write has reached
the device before the descriptor flags below are rewritten?

writel() already contains the MMIO ordering barriers (__io_bw() and
__io_aw() in include/asm-generic/io.h), so the extra wmb() adds nothing
with respect to the device.  nbl_hw_stop_mailbox_txq() only performs
posted BAR2 writes with no read-back, and nbl_chan_config_queue() below
de-asserts RST and sets EN in the immediately following writes:

	cfg_tbl.data[3] = FIELD_PREP(NBL_MAILBOX_QINFO_CFG_QUEUE_RST_MASK, 0) |
			  FIELD_PREP(NBL_MAILBOX_QINFO_CFG_QUEUE_EN_MASK, 1);

The QINFO_CFG layout added in this patch exposes RST/EN/DIF_ERR/PTR_ERR
but no reset-done bit, and nothing reads the register back.

Would a read from the same BAR, or a device status poll, be needed before
mutating descriptors the device may still own and before re-enabling the
queue?

> +
> +	/*
> +	 * Reclaim all outstanding descriptors between next_to_clean and
> +	 * next_to_use.  Under txq_lock there is at most one in-flight
> +	 * descriptor, but iterate the full range for robustness.
> +	 */
> +	while (txq->next_to_clean != txq->next_to_use) {
> +		tx_desc = NBL_CHAN_TX_RING_TO_DESC(txq,
> +						   txq->next_to_clean);
> +		WRITE_ONCE(tx_desc->flags, 0);
> +		txq->next_to_clean =
> +			NBL_NEXT_ID(txq->next_to_clean,
> +				    chan_info->num_txq_entries - 1);
> +	}
> +
> +	/*
> +	 * Hardware tail_ptr counter is cleared by QUEUE_RST.  Reset
> +	 * software counter to match so the next doorbell update does
> +	 * not produce a false 16-bit wrap delta.
> +	 */
> +	txq->tail_ptr = 0;

[Severity: High]
tail_ptr is reset to 0 here, but next_to_use and next_to_clean keep their
pre-timeout values.  Can this leave the ring index and the doorbell
counter permanently out of step?

Everywhere else the two advance together:

nbl_chan_alloc_all_tx_bufs():
	txq->next_to_clean = 0;
	txq->next_to_use = 0;
	txq->tail_ptr = 0;

nbl_chan_update_txqueue():
	txq->next_to_use =
		NBL_NEXT_ID(txq->next_to_use, chan_info->num_txq_entries - 1);
	txq->tail_ptr++;

After a timeout on slot X the reclaim loop above leaves
next_to_use == next_to_clean == X + 1 while tail_ptr == 0.

The next send then writes descriptor X + 1 and rings the doorbell with
tail_ptr == 1.  If QUEUE_RST cleared the hardware fetch pointer as the
comment states, the device looks at slot 0, whose flags the reclaim loop
just zeroed, while nbl_chan_kick_tx_ring() polls descriptor X + 1 for
NBL_CHAN_TX_DESC_USED, times out again and re-enters this same recovery.
Would that make every subsequent nbl_chan_send_msg() return -ETIMEDOUT?

And if QUEUE_RST does not clear the hardware counter, does writing
tail_ptr = 0 produce exactly the bogus 16-bit delta the comment is trying
to avoid?

Should next_to_use and next_to_clean be reset to 0 here as well?

> +
> +	/* Re-enable queue with current ring base and size */
> +	nbl_chan_config_queue(chan_mgt, chan_info, true);
> +}

[Severity: Medium]
The commit message states:

  Mailbox BAR2 register access is intentionally left unlocked: single
  dword mailbox writes are atomic, and the multi-dword QINFO config
  sequence (config_mailbox_txq/rxq, stop_mailbox_txq/rxq) only runs
  during queue setup/teardown when chan_info->active is false, with no
  concurrent producers.

Does this recovery path match that description?  It issues
stop_mailbox_txq() and config_mailbox_txq() at runtime from the send
path, and chan_info->active is only cleared at the very end of
nbl_chan_teardown_queue(), so active is true throughout.

Related to that, nbl_chan_teardown_queue() calls nbl_chan_stop_queue()
without taking txq_lock, and it does so even when the drain times out:

	if (!ret) {
		dev_warn(chan_mgt->common->dev,
			 "teardown: inflight tx drain timeout\n");
		ret = -ETIMEDOUT;
	}
	...
	/* After all TX drained, stop hardware queue */
	nbl_chan_stop_queue(chan_mgt);

Can teardown's 4-dword QINFO sequence then interleave with a sender still
executing the stop + config sequence above, leaving QUEUE_EN=1 on a ring
the driver considers torn down?

> +
> +static int nbl_chan_kick_tx_ring(struct nbl_channel_mgt *chan_mgt,
> +				 struct nbl_chan_info *chan_info)
> +{
> +	struct nbl_hw_ops *hw_ops = chan_mgt->hw_ops_tbl->ops;
> +	struct nbl_chan_ring *txq = &chan_info->txq;
> +	struct device *dev = chan_mgt->common->dev;
> +	int max_retries = NBL_CHAN_TX_WAIT_TIMES;
> +	struct nbl_chan_tx_desc *tx_desc;
> +	int retry_count = 0;
> +	u16 msg_type;
> +
> +	nbl_chan_update_tail_ptr(hw_ops, chan_mgt->hw_ops_tbl->priv,
> +				 txq->tail_ptr, NBL_MB_TX_QID);
> +
> +	tx_desc = NBL_CHAN_TX_RING_TO_DESC(txq, txq->next_to_clean);

[ ... ]

> +		retry_count++;
> +		if (retry_count == max_retries) {
> +			msg_type = le16_to_cpu(READ_ONCE(tx_desc->msg_type));
> +			dev_err_ratelimited(dev, "chan send msg type: %d timeout\n",
> +					    msg_type);
> +			/*
> +			 * Device failed to complete this descriptor.
> +			 * Quiesce the queue, reclaim the timed-out
> +			 * descriptor, and re-enable so future sends can
> +			 * proceed instead of stalling the ring full.
> +			 */
> +			nbl_chan_quiesce_and_reclaim_tx(chan_mgt,
> +							chan_info);
> +			return -ETIMEDOUT;
> +		}
> +		usleep_range(NBL_CHAN_TX_WAIT_US, NBL_CHAN_TX_WAIT_US_MAX);
> +	}
> +
> +	txq->next_to_clean = txq->next_to_use;
> +
> +	return 0;
> +}

[ ... ]

> +static void nbl_chan_recv_ack_msg(void *priv, u16 srcid, u16 msgid, void *data,
> +				  u32 data_len)
> +{

[ ... ]

> +	if (w_status != NBL_MBX_STATUS_WAITING) {
> +		mutex_unlock(&chan_info->pending_lock);
> +		dev_err_ratelimited(dev,
> +				    "Skip ack invalid status, wait msgtype:%u idx:%u status:%d ack msgtype:%u msgid:%u datalen:%u\n",
> +				    w_msgtype, w_msgidx, w_status,
> +				    ack_msgtype, ack_msgid, ack_datalen);
> +		return;
> +	}

[ ... ]

> +ack_done:
> +	/* Guarantee payload data finished before acked flag visible */
> +	smp_wmb();
> +	WRITE_ONCE(wait_head->acked, 1);
> +	mutex_unlock(&chan_info->pending_lock);
> +	wake_up(&wait_head->wait_queue);
> +}

[Severity: Medium]
This completes the slot by setting acked = 1 but leaves
wait_head->status at NBL_MBX_STATUS_WAITING.  Can that make the timeout
paths in nbl_chan_send_msg() discard a delivered ACK?

The interrupt path tests only status:

	if (ret == 0) {
		mutex_lock(&chan_info->pending_lock);
		if (READ_ONCE(wait_head->status) ==
		    NBL_MBX_STATUS_WAITING) {
			WRITE_ONCE(wait_head->status,
				   NBL_MBX_STATUS_TIMEOUT);
			WRITE_ONCE(wait_head->acked, 0);

So if the RX cleanup work runs the code above in the window between
wait_event_timeout() evaluating its condition for the last time and the
sender taking pending_lock, the sender sees WAITING, clears acked and
returns -ETIMEDOUT for a request that was answered.

The polling path has a similar window: it checks acked, then sleeps
NBL_CHAN_TX_WAIT_ACK_US_MIN..MAX, and after the last iteration falls
straight through to ret = -ETIMEDOUT with no further locked check, so an
ACK arriving during that final ~1 ms sleep is dropped.

Since status also stays WAITING, would a duplicate ACK for the same
msgid still pass all the validation above and memcpy into the requester's
response buffer a second time?

Should the ACK handler move the slot to a completed state (or should the
timeout transitions re-check acked under pending_lock)?

> +
> +static int nbl_chan_send_msg(struct nbl_channel_mgt *chan_mgt,
> +			     struct nbl_chan_send_info *chan_send)
> +{

[ ... ]

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.c
> index 0ba0b7f643c64..379dbb9dc0034 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.c
> @@ -10,6 +10,156 @@

[ ... ]

> +static void nbl_hw_stop_mailbox_txq(struct nbl_hw_mgt *hw_mgt)
> +{
> +	struct nbl_mailbox_qinfo_cfg_table cfg_tbl;
> +
> +	memset(&cfg_tbl, 0, sizeof(cfg_tbl));
> +	cfg_tbl.data[3] = FIELD_PREP(NBL_MAILBOX_QINFO_CFG_QUEUE_RST_MASK, 1);
> +	nbl_hw_write_mbx_regs(hw_mgt, NBL_MAILBOX_QINFO_CFG_TX_TABLE_ADDR,
> +			      cfg_tbl.data, sizeof(cfg_tbl));
> +}
> +
> +static void nbl_hw_get_host_pf_mask(struct nbl_hw_mgt *hw_mgt, u32 *pf_mask)
> +{
> +	nbl_hw_rd_regs_lock(hw_mgt, NBL_PCIE_HOST_K_PF_MASK_REG, pf_mask,
> +			    sizeof(*pf_mask));
> +}
> +
> +static void nbl_hw_cfg_mailbox_qinfo(struct nbl_hw_mgt *hw_mgt, u16 func_id,
> +				     u8 bus, u8 devid, u8 function)
> +{
> +	u32 data = 0;
> +
> +	data = FIELD_PREP(NBL_MAILBOX_QINFO_MAP_FUNCTION_MASK, function) |
> +	       FIELD_PREP(NBL_MAILBOX_QINFO_MAP_DEVID_MASK, devid) |
> +	       FIELD_PREP(NBL_MAILBOX_QINFO_MAP_BUS_MASK, bus);
> +	nbl_hw_wr_regs_lock(hw_mgt, NBL_MAILBOX_QINFO_MAP_REG_ARR(func_id),
> +			    &data,
> +			    sizeof(data));
> +}

[Severity: Medium]
This builds the whole dword from FUNCTION/DEVID/BUS and writes it, but
the MSI-X fields share the same dword:

#define NBL_MAILBOX_QINFO_MAP_MSIX_IDX_MASK		GENMASK(28, 16)
#define NBL_MAILBOX_QINFO_MAP_MSIX_IDX_VALID_MASK	BIT(29)

Does this zero the mailbox MSI-X routing of the entries it rewrites?

nbl_chan_cfg_qinfo_map_table() programs an entry for every unmasked PF,
not just the local function:

	for (func_id = 0; func_id < NBL_MAX_PF; func_id++) {
		if (!(pf_mask & (1 << func_id)))
			hw_ops->cfg_mailbox_qinfo(p, func_id, bus,
						  devid, func_id);
	}

so re-probing one PF would clear MSIX_IDX and MSIX_IDX_VALID for its
already-bound siblings.  Those siblings keep NBL_CHAN_IRQ_RDY set, so
each synchronous send would wait out NBL_CHAN_ACK_WAIT_TIME and fail with
-ETIMEDOUT until they are re-bound.

The commit message also says:

  Add per-hw_mgt reg_lock to protect read-modify-write sequences on the
  general-purpose register BAR (hw_addr, BAR0), such as QINFO map table
  entries and k_pf_mask reads

Where is the read-modify-write on the QINFO map entry?  This helper only
writes.  Should it read the entry first and preserve the MSI-X fields?

> +
> +static struct nbl_hw_ops hw_ops = {
> +	.update_mailbox_queue_tail_ptr = nbl_hw_update_mailbox_queue_tail_ptr,
> +	.config_mailbox_rxq = nbl_hw_config_mailbox_rxq,
> +	.config_mailbox_txq = nbl_hw_config_mailbox_txq,
> +	.stop_mailbox_rxq = nbl_hw_stop_mailbox_rxq,
> +	.stop_mailbox_txq = nbl_hw_stop_mailbox_txq,
> +	.get_host_pf_mask = nbl_hw_get_host_pf_mask,
> +	.cfg_mailbox_qinfo = nbl_hw_cfg_mailbox_qinfo,
> +
> +};

[ ... ]

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.h b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.h
> index 1f9e509dc6312..71691e893790e 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.h
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.h
> @@ -11,5 +11,61 @@

[ ... ]

> +/* MAILBOX qinfo_map_table */
> +#define NBL_MAILBOX_QINFO_MAP_FUNCTION_MASK		GENMASK(2, 0)
> +#define NBL_MAILBOX_QINFO_MAP_DEVID_MASK		GENMASK(7, 3)
> +#define NBL_MAILBOX_QINFO_MAP_BUS_MASK			GENMASK(15, 8)
> +#define NBL_MAILBOX_QINFO_MAP_MSIX_IDX_MASK		GENMASK(28, 16)
> +#define NBL_MAILBOX_QINFO_MAP_MSIX_IDX_VALID_MASK	BIT(29)

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907123848.30256-1-illusion.wang%40nebula-matrix.com