Re: [PATCH net-next v4 04/15] libie: add control queue support

[Larysa Zaremba <larysa.zaremba@intel.com>]

Sashiko has some concerns about this patch.
There are some improvements that I think are nice to have based on that:

commit 1da5bb5c7a7be66fc6226afa94c0f25bb52a57a0
Author: Larysa Zaremba <larysa.zaremba@intel.com>
Date:   Mon Jul 13 15:59:55 2026 +0200

    fixup! libie: add control queue support

diff --git a/drivers/net/ethernet/intel/libie/controlq.c b/drivers/net/ethernet/intel/libie/controlq.c
index 885b4437b4f0..c043c07dbb89 100644
--- a/drivers/net/ethernet/intel/libie/controlq.c
+++ b/drivers/net/ethernet/intel/libie/controlq.c
@@ -327,7 +327,8 @@ libie_ctlq_add(struct libie_ctlq_ctx *ctx,
 {
        struct libie_ctlq_info *ctlq;

-       if (qinfo->id != LIBIE_CTLQ_MBX_ID)
+       if (qinfo->id != LIBIE_CTLQ_MBX_ID ||
+           qinfo->len > FIELD_MAX(LIBIE_CTLQ_MBX_ATQ_LEN))
                return ERR_PTR(-EOPNOTSUPP);

        /* libie_ctlq_init was not called */
@@ -493,8 +494,6 @@ EXPORT_SYMBOL_NS_GPL(libie_ctlq_send_desc_avail, "LIBIE_CP");
  * The caller must hold ctlq->lock. The intended pattern is to first check
  * the number of descriptors available, then fill in the messages and perform
  * send within a single critical section.
- *
- * Return: %0 on success, -%errno on failure.
  */
 void libie_ctlq_send(struct libie_ctlq_info *ctlq, u32 num_q_msg)
 {
@@ -510,6 +509,7 @@ void libie_ctlq_send(struct libie_ctlq_info *ctlq, u32 num_q_msg)
                if (unlikely(++ntu == ctlq->ring_len))
                        ntu = 0;
        }
+       dma_wmb();
        writel(ntu, ctlq->reg.tail);
        ctlq->next_to_use = ntu;
 }

Other than that, I would put feedback into the following categories:
* worrying about patterns not employed by idpf or ixd

> libie_ctlq_recv() only breaks the loop on DD=0, but DD is never
> cleared on consumption. It is cleared only when the slot is
> re-posted by libie_ctlq_post_rx_buffs(), and even there the zeroing
> happens for descriptors that get freshly filled:
> 	while (num_to_post--) {
> 		...
> 		ctlq->descs[ntp] = (struct libie_ctlq_desc) {};
> 		...
> 		libie_ctlq_prep_rx_desc(&ctlq->descs[ntp], addr, fq.truesize);
> 		if (unlikely(++ntp == ctlq->ring_len))
> 			ntp = 0;
> 	}
> The final barrier slot between next_to_clean and next_to_post is
> never zeroed, so it retains DD=1 from the previous rotation.
> With ring_len = 4, after one full cycle ntc=3 and ntp=3; post_rx_buffs
> refills slots 3, 0, 1 and leaves slot 2 (the new barrier) with stale
> DD=1. If a caller then calls libie_ctlq_recv() with
> num_q_msg >= ring_len, the loop can wrap and re-process the stale
> descriptor at slot 2.
> 
If caller calls libie_ctlq_recv() with num_q_msg >= ring_len, this is caller's
programming error, direct ctlq APIs are not exactly safe, and queue cannot
contain more than ring_len - 1 messages. So maybe worth mentioning in the kdoc.
So the stale slot is not a valid concern.

[...]

> Can this returned pointer be freed concurrently by another thread?
> The pointer is returned without acquiring a reference, and the lock that
> protected it (ctlqs_lock) is released immediately before returning.
> If a concurrent thread initiates teardown via libie_ctlq_deinit(), it could
> remove and free the queue while the caller of libie_find_ctlq() is actively
> accessing it, causing a use-after-free.
> 
idpf and ixd do not do that

* Worrying about impossible libeth configurations

> If the page pool uses compound pages and the offset places iov_base into a
> subsequent 4K frame, calling virt_to_netmem(rx_buf->iov_base) yields the tail
> page rather than the head page.
> Page pool metadata is only valid on the head page, so calling
> page_pool_put_full_netmem() on this tail page could read garbage data.
> 
page_pool in libie_cp is created in a way that makes compound pages impossible

> If the page pool ever returns a smaller truesize (for example via
> libeth_rx_page_pool_params_zc), would a hardware-provided data_len
> up to 4K let iov_len point past the end of the actual buffer?
> 
page_pool via libeth is configured exactly in a way that disables any headroom 
or tailroom, so truesize will never be less than data_len.

* Some parts of code that I agree look semi-ugly (like defines vs virtchnl2 
enums, limiting id to LIBIE_CTLQ_MBX_ID, and lack of low-level completion 
function for Tx control queue), but those I think are best addressed withing 
non-default-mailbox-ctlq development, as currently the best way is unclear, but 
how is looks currently is perfectly acceptable.

* Concerns about HW doorbells and such. Same as with previous versions, the flow 
is consistent with what was in idpf beforehand.

* This one is an outlier:

> 	if (unlikely(msg->data_len > LIBIE_CTLQ_MAX_BUF_LEN)) {
> 		msg->data_len = LIBIE_CTLQ_MAX_BUF_LEN;
> 		msg->chnl_retval = U32_MAX;
> 	}
> Can callers distinguish "hardware returned U32_MAX" from "libie
> truncated the buffer"?
> 

Yes, U32_MAX was chosen, so that it never intersects with the valid HW codes