Re: [PATCH net-next v4 09/15] idpf: refactor idpf to use libie control queues

Here are some minor changes that came out of Sashiko's review.
The first one is not needed, but does make code more consistent and robust.
The second one is updating kdoc, where Sashiko was concerned about a functional 
change. I have checked that the functional change itself does not affect 
any callers.

diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
index 04a0421c1b77..d768b63700c8 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c

@@ -2931,6 +2931,8 @@ int idpf_init_dflt_mbx(struct idpf_adapter *adapter)
        adapter->arq = libie_find_ctlq(ctx, LIBIE_CTLQ_TYPE_RX,
                                       LIBIE_CTLQ_MBX_ID);
        if (!adapter->asq || !adapter->arq) {
+               adapter->asq = NULL;
+               adapter->arq = NULL;
                libie_ctlq_xn_deinit(params.xnm, ctx);
                return -ENOENT;
        }

@@ -3993,7 +3995,7 @@ int idpf_set_promiscuous(struct idpf_adapter *adapter,
  * @send_msg: message to send
  * @msg_size: size of message to send
  * @recv_msg: message to populate on reception of response
- * @recv_len: length of message copied into recv_msg or 0 on error
+ * @recv_len: length of message copied into recv_msg
  *
  * Return: 0 on success or error code on failure.
  */

Everything else:

commit 9ebd6c429379be8477841f5180023c78d16732d0
Author: Pavan Kumar Linga [off-list ref]
    idpf: refactor idpf to use libie control queues
This refactors idpf to use the libie control-queue API, removing the
in-tree idpf_controlq.[ch] and idpf_controlq_setup.c and reworking every
virtchnl send/receive path to use libie_ctlq_xn_send_params /
libie_ctlq_release_rx_buf(). A few observations below.

quoted

diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
index 357ff3e86b2e..388075a07af7 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c

[ ... ]

quoted

@@ -4128,9 +3731,9 @@ static void idpf_set_mac_type(const u8 *default_mac_addr,
 
 /**
  * idpf_mac_filter_async_handler - Async callback for mac filters

[ ... ]

quoted

- */
-static int idpf_mac_filter_async_handler(struct idpf_adapter *adapter,
-					 struct idpf_vc_xn *xn,
-					 const struct idpf_ctlq_msg *ctlq_msg)
+static void idpf_mac_filter_async_handler(void *ctx,
+					  struct kvec *buff,
+					  int status)
 {
 	struct virtchnl2_mac_addr_list *ma_list;
 	struct idpf_vport_config *vport_config;
 	struct virtchnl2_mac_addr *mac_addr;
+	struct idpf_adapter *adapter = ctx;
 	struct idpf_mac_filter *f, *tmp;
 	struct list_head *ma_list_head;
 	struct idpf_vport *vport;

@@ -4152,18 +3756,18 @@ static int idpf_mac_filter_async_handler(struct idpf_adapter *adapter,
 	int i;
 
 	/* if success we're done, we're only here if something bad happened */
-	if (!ctlq_msg->cookie.mbx.chnl_retval)
-		return 0;
+	if (!status || status == -ETIMEDOUT)
+		return;

Should -ETIMEDOUT really be treated the same as success here?
In the old code the async handler only ran when a reply was received,
so a timeout could not reach this callback. With libie the async path
now delivers -ETIMEDOUT via libie_xn_check_async_timeout(), and this
new short-circuit returns immediately without reconciling the driver's
mac_filter_list with what the CP actually applied.
Note that idpf_add_del_mac_filters() clears f->add / f->remove for each
entry before sending:
	if (add && f->add) {
		ether_addr_copy(mac_addr[i].addr, f->macaddr);
		idpf_set_mac_type(default_mac_addr, &mac_addr[i]);
		i++;
		f->add = false;
So on -ETIMEDOUT those entries look "applied" to the driver while the
CP never acknowledged them. Would it be safer to only skip work when
status == 0 and fall through to the list_del reconciliation on
-ETIMEDOUT the same way as for any other error?

On -ETIMEDOUT we do not have any buffer to handle.
In this case it could be either added or not, we do not know.
Both options lead us into a suboptimal situation, but treating it
as a success is simpler. Also, 2 minute timeout probably means you
should reload something anyway.

[...]

quoted

 int idpf_send_dealloc_vectors_msg(struct idpf_adapter *adapter)
 {
 	struct virtchnl2_alloc_vectors *ac = adapter->req_vec_chunks;
-	struct virtchnl2_vector_chunks *vcs = &ac->vchunks;
-	struct idpf_vc_xn_params xn_params = {};
-	ssize_t reply_sz;
-	int buf_size;
+	struct libie_ctlq_xn_send_params xn_params = {
+		.timeout_ms	= IDPF_VC_XN_DEFAULT_TIMEOUT_MSEC,
+		.chnl_opcode	= VIRTCHNL2_OP_DEALLOC_VECTORS,
+	};
+	struct virtchnl2_vector_chunks *vcs;
+	int buf_size, err;
 
-	buf_size = struct_size(vcs, vchunks, le16_to_cpu(vcs->num_vchunks));
+	buf_size = struct_size(&ac->vchunks, vchunks,
+			       le16_to_cpu(ac->vchunks.num_vchunks));
+	vcs = kmemdup(&ac->vchunks, buf_size, GFP_KERNEL);
+	if (!vcs)
+		return -ENOMEM;
 
-	xn_params.vc_op = VIRTCHNL2_OP_DEALLOC_VECTORS;
-	xn_params.send_buf.iov_base = vcs;
-	xn_params.send_buf.iov_len = buf_size;
-	xn_params.timeout_ms = IDPF_VC_XN_DEFAULT_TIMEOUT_MSEC;
-	reply_sz = idpf_vc_xn_exec(adapter, &xn_params);
-	if (reply_sz < 0)
-		return reply_sz;
+	err = idpf_send_mb_msg_kfree(adapter, &xn_params, vcs, buf_size);
+	if (err)
+		return err;
 
 	kfree(adapter->req_vec_chunks);
 	adapter->req_vec_chunks = NULL;

Does this code leak adapter->req_vec_chunks on the error path?
On success the kfree()/NULL of adapter->req_vec_chunks runs after
idpf_send_mb_msg_kfree(). On error the function returns err early
while adapter->req_vec_chunks is still allocated.
Its caller is idpf_intr_rel():
	void idpf_intr_rel(struct idpf_adapter *adapter)
	{
		...
		idpf_send_dealloc_vectors_msg(adapter);
		idpf_deinit_vector_stack(adapter);
		kfree(adapter->msix_entries);
		adapter->msix_entries = NULL;
		kfree(adapter->rdma_msix_entries);
		adapter->rdma_msix_entries = NULL;
	}
which does not free req_vec_chunks either. Is there another site
that frees adapter->req_vec_chunks when the dealloc virtchnl fails
(for example on a mailbox timeout during teardown), or does it
accumulate across reset/unload attempts?

Seems so, but this is a preexistent issue

[...]

quoted

diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl_ptp.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl_ptp.c
index 8d8fb498e092..1d8dd67e299d 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl_ptp.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl_ptp.c

[ ... ]

quoted

@@ -234,23 +244,18 @@ int idpf_ptp_set_dev_clk_time(struct idpf_adapter *adapter, u64 time)
 	struct virtchnl2_ptp_set_dev_clk_time set_dev_clk_time_msg = {
 		.dev_time_ns = cpu_to_le64(time),
 	};
-	struct idpf_vc_xn_params xn_params = {
-		.vc_op = VIRTCHNL2_OP_PTP_SET_DEV_CLK_TIME,
-		.send_buf.iov_base = &set_dev_clk_time_msg,
-		.send_buf.iov_len = sizeof(set_dev_clk_time_msg),
-		.recv_buf.iov_base = &set_dev_clk_time_msg,
-		.recv_buf.iov_len = sizeof(set_dev_clk_time_msg),
+	struct libie_ctlq_xn_send_params xn_params = {
+		.chnl_opcode = VIRTCHNL2_OP_PTP_SET_DEV_CLK_TIME,
 		.timeout_ms = IDPF_VC_XN_DEFAULT_TIMEOUT_MSEC,
 	};
-	int reply_sz;
+	int err;
 
-	reply_sz = idpf_vc_xn_exec(adapter, &xn_params);
-	if (reply_sz < 0)
-		return reply_sz;
-	if (reply_sz != sizeof(set_dev_clk_time_msg))
-		return -EIO;
+	err = idpf_send_mb_msg(adapter, &xn_params, &set_dev_clk_time_msg,
+			       sizeof(set_dev_clk_time_msg));
+	if (!err)
+		libie_ctlq_release_rx_buf(&xn_params.recv_mem);
 
-	return 0;
+	return err;
 }

Is dropping the reply-size check here intentional?
The pre-patch code explicitly rejected a malformed reply:
	if (reply_sz != sizeof(set_dev_clk_time_msg))
		return -EIO;
The refactored helper only surfaces the return code of
idpf_send_mb_msg() and unconditionally releases the Rx buffer, so a
short/truncated payload from the CP is silently accepted as success.
The same reply-size check has also been dropped in
idpf_ptp_adj_dev_clk_time() and idpf_ptp_adj_dev_clk_fine():

quoted

@@ -267,23 +272,18 @@ int idpf_ptp_adj_dev_clk_time(struct idpf_adapter *adapter, s64 delta)
 	struct virtchnl2_ptp_adj_dev_clk_time adj_dev_clk_time_msg = {
 		.delta = cpu_to_le64(delta),
 	};

[ ... ]

quoted

-	reply_sz = idpf_vc_xn_exec(adapter, &xn_params);
-	if (reply_sz < 0)
-		return reply_sz;
-	if (reply_sz != sizeof(adj_dev_clk_time_msg))
-		return -EIO;
+	err = idpf_send_mb_msg(adapter, &xn_params, &adj_dev_clk_time_msg,
+			       sizeof(adj_dev_clk_time_msg));
+	if (!err)
+		libie_ctlq_release_rx_buf(&xn_params.recv_mem);

@@ -301,23 +301,18 @@ int idpf_ptp_adj_dev_clk_fine(struct idpf_adapter *adapter, u64 incval)
 	struct virtchnl2_ptp_adj_dev_clk_fine adj_dev_clk_fine_msg = {
 		.incval = cpu_to_le64(incval),
 	};

[ ... ]

quoted

-	reply_sz = idpf_vc_xn_exec(adapter, &xn_params);
-	if (reply_sz < 0)
-		return reply_sz;
-	if (reply_sz != sizeof(adj_dev_clk_fine_msg))
-		return -EIO;
+	err = idpf_send_mb_msg(adapter, &xn_params, &adj_dev_clk_fine_msg,
+			       sizeof(adj_dev_clk_fine_msg));
+	if (!err)
+		libie_ctlq_release_rx_buf(&xn_params.recv_mem);

Should these three helpers keep validating that
xn_params.recv_mem.iov_len matches the expected reply size, the way
idpf_ptp_get_caps() and idpf_ptp_get_dev_clk_time() still do?

No need to validate size, if we do not use the buffer.