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

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

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 <pavan.kumar.linga@intel.com>
>     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.
> > 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
> [ ... ]
> > @@ -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
> [ ... ]
> > - */
> > -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.

[...]

> >  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

[...]

> > 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
> [ ... ]
> > @@ -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():
> > @@ -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),
> >  	};
> [ ... ]
> > -	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),
> >  	};
> [ ... ]
> > -	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.