Re: [PATCH iwl-next v2 08/14] idpf: refactor idpf to use libie controlq and Xn APIs

[Larysa Zaremba]

On Mon, Apr 28, 2025 at 07:03:49PM +0100, Simon Horman wrote:
> On Thu, Apr 24, 2025 at 01:32:31PM +0200, Larysa Zaremba wrote:
> > From: Pavan Kumar Linga <pavan.kumar.linga@xxxxxxxxx>
> > 
> > Support to initialize and configure controlq, Xn manager,
> > MMIO and reset APIs was introduced in libie. As part of it,
> > most of the existing controlq structures are renamed and
> > modified. Use those APIs in idpf and make all the necessary changes.
> > 
> > Previously for the send and receive virtchnl messages, there
> > used to be a memcpy involved in controlq code to copy the buffer
> > info passed by the send function into the controlq specific
> > buffers. There was no restriction to use automatic memory
> > in that case. The new implementation in libie removed copying
> > of the send buffer info and introduced DMA mapping of the
> > send buffer itself. To accommodate it, use dynamic memory for
> > the send buffers. In case of receive, idpf receives a page pool
> > buffer allocated by the libie and care should be taken to
> > release it after use in the idpf.
> > 
> > The changes are fairly trivial and localized, with a notable exception
> > being the consolidation of idpf_vc_xn_shutdown and idpf_deinit_dflt_mbx
> > under the latter name. This has some additional consequences that are
> > addressed in the following patches.
> > 
> > Reviewed-by: Michal Kubiak <michal.kubiak@xxxxxxxxx>
> > Signed-off-by: Pavan Kumar Linga <pavan.kumar.linga@xxxxxxxxx>
> > Co-developed-by: Larysa Zaremba <larysa.zaremba@xxxxxxxxx>
> > Signed-off-by: Larysa Zaremba <larysa.zaremba@xxxxxxxxx>
> > ---
> >  drivers/net/ethernet/intel/idpf/Kconfig       |    1 +
> >  drivers/net/ethernet/intel/idpf/Makefile      |    2 -
> >  drivers/net/ethernet/intel/idpf/idpf.h        |   42 +-
> >  .../net/ethernet/intel/idpf/idpf_controlq.c   |  624 -------
> >  .../net/ethernet/intel/idpf/idpf_controlq.h   |  130 --
> >  .../ethernet/intel/idpf/idpf_controlq_api.h   |  177 --
> >  .../ethernet/intel/idpf/idpf_controlq_setup.c |  171 --
> >  drivers/net/ethernet/intel/idpf/idpf_dev.c    |   91 +-
> >  drivers/net/ethernet/intel/idpf/idpf_lib.c    |   49 +-
> >  drivers/net/ethernet/intel/idpf/idpf_main.c   |   87 +-
> >  drivers/net/ethernet/intel/idpf/idpf_mem.h    |   20 -
> >  drivers/net/ethernet/intel/idpf/idpf_txrx.h   |    2 +-
> >  drivers/net/ethernet/intel/idpf/idpf_vf_dev.c |   89 +-
> >  .../net/ethernet/intel/idpf/idpf_virtchnl.c   | 1622 ++++++-----------
> >  .../net/ethernet/intel/idpf/idpf_virtchnl.h   |   89 +-
> >  .../ethernet/intel/idpf/idpf_virtchnl_ptp.c   |  303 ++-
> >  16 files changed, 886 insertions(+), 2613 deletions(-)
> 
> This patch is rather large.
> Is there a way it could be split up into more easily reviewable chunks?
>

I tried to divide it in a sensible way, but due to us replacing the data 
structures, coming up with the patches that build properly and do not bloat the 
diff is rather hard. What could be done relatively easily is separating 
libie_pci related changes, but they constitute a very small portion of the 
overall refactor and are rather straightforward.
 
> >  delete mode 100644 drivers/net/ethernet/intel/idpf/idpf_controlq.c
> >  delete mode 100644 drivers/net/ethernet/intel/idpf/idpf_controlq.h
> >  delete mode 100644 drivers/net/ethernet/intel/idpf/idpf_controlq_api.h
> >  delete mode 100644 drivers/net/ethernet/intel/idpf/idpf_controlq_setup.c
> >  delete mode 100644 drivers/net/ethernet/intel/idpf/idpf_mem.h
> 
> ...
> 
> > diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> 
> ...
> 
> > @@ -2520,15 +2045,18 @@ static void idpf_finalize_ptype_lookup(struct libeth_rx_pt *ptype)
> >   */
> >  int idpf_send_get_rx_ptype_msg(struct idpf_vport *vport)
> >  {
> > -	struct virtchnl2_get_ptype_info *get_ptype_info __free(kfree) = NULL;
> > -	struct virtchnl2_get_ptype_info *ptype_info __free(kfree) = NULL;
> > +	struct libie_ctlq_xn_send_params xn_params = {
> > +		.timeout_ms	= IDPF_VC_XN_DEFAULT_TIMEOUT_MSEC,
> > +		.chnl_opcode	= VIRTCHNL2_OP_GET_PTYPE_INFO,
> > +	};
> >  	struct libeth_rx_pt *ptype_lkup __free(kfree) = NULL;
> > +	struct virtchnl2_get_ptype_info *get_ptype_info;
> >  	int max_ptype, ptypes_recvd = 0, ptype_offset;
> >  	struct idpf_adapter *adapter = vport->adapter;
> > -	struct idpf_vc_xn_params xn_params = {};
> > +	struct virtchnl2_get_ptype_info *ptype_info;
> > +	int buf_size = sizeof(*get_ptype_info);
> >  	u16 next_ptype_id = 0;
> > -	ssize_t reply_sz;
> > -	int i, j, k;
> > +	int i, j, k, err = 0;
> >  
> >  	if (vport->rx_ptype_lkup)
> >  		return 0;
> > @@ -2542,22 +2070,11 @@ int idpf_send_get_rx_ptype_msg(struct idpf_vport *vport)
> >  	if (!ptype_lkup)
> >  		return -ENOMEM;
> >  
> > -	get_ptype_info = kzalloc(sizeof(*get_ptype_info), GFP_KERNEL);
> > -	if (!get_ptype_info)
> > -		return -ENOMEM;
> > -
> > -	ptype_info = kzalloc(IDPF_CTLQ_MAX_BUF_LEN, GFP_KERNEL);
> > -	if (!ptype_info)
> > -		return -ENOMEM;
> > -
> > -	xn_params.vc_op = VIRTCHNL2_OP_GET_PTYPE_INFO;
> > -	xn_params.send_buf.iov_base = get_ptype_info;
> > -	xn_params.send_buf.iov_len = sizeof(*get_ptype_info);
> > -	xn_params.recv_buf.iov_base = ptype_info;
> > -	xn_params.recv_buf.iov_len = IDPF_CTLQ_MAX_BUF_LEN;
> > -	xn_params.timeout_ms = IDPF_VC_XN_DEFAULT_TIMEOUT_MSEC;
> > -
> >  	while (next_ptype_id < max_ptype) {
> > +		get_ptype_info = kzalloc(buf_size, GFP_KERNEL);
> > +		if (!get_ptype_info)
> > +			return -ENOMEM;
> > +
> >  		get_ptype_info->start_ptype_id = cpu_to_le16(next_ptype_id);
> >  
> >  		if ((next_ptype_id + IDPF_RX_MAX_PTYPES_PER_BUF) > max_ptype)
> > @@ -2567,13 +2084,15 @@ int idpf_send_get_rx_ptype_msg(struct idpf_vport *vport)
> >  			get_ptype_info->num_ptypes =
> >  				cpu_to_le16(IDPF_RX_MAX_PTYPES_PER_BUF);
> >  
> > -		reply_sz = idpf_vc_xn_exec(adapter, &xn_params);
> > -		if (reply_sz < 0)
> > -			return reply_sz;
> > +		err = idpf_send_mb_msg(adapter, &xn_params, get_ptype_info,
> > +				       buf_size);
> > +		if (err)
> > +			goto free_tx_buf;
> >  
> > +		ptype_info = xn_params.recv_mem.iov_base;
> >  		ptypes_recvd += le16_to_cpu(ptype_info->num_ptypes);
> >  		if (ptypes_recvd > max_ptype)
> > -			return -EINVAL;
> 
> Should err be set to -EINVAL here?
> 
> Flagged by Smatch.
>

Yes, it should. Thanks!

> > +			goto free_rx_buf;
> >  
> >  		next_ptype_id = le16_to_cpu(get_ptype_info->start_ptype_id) +
> >  				le16_to_cpu(get_ptype_info->num_ptypes);
> > @@ -2589,8 +2108,8 @@ int idpf_send_get_rx_ptype_msg(struct idpf_vport *vport)
> >  					((u8 *)ptype_info + ptype_offset);
> >  
> >  			ptype_offset += IDPF_GET_PTYPE_SIZE(ptype);
> > -			if (ptype_offset > IDPF_CTLQ_MAX_BUF_LEN)
> > -				return -EINVAL;
> > +			if (ptype_offset > LIBIE_CTLQ_MAX_BUF_LEN)
> > +				goto free_rx_buf;
> >  
> >  			/* 0xFFFF indicates end of ptypes */
> >  			if (le16_to_cpu(ptype->ptype_id_10) ==
> > @@ -2720,12 +2239,24 @@ int idpf_send_get_rx_ptype_msg(struct idpf_vport *vport)
> >  
> >  			idpf_finalize_ptype_lookup(&ptype_lkup[k]);
> >  		}
> > +		libie_ctlq_release_rx_buf(adapter->arq,
> > +					  &xn_params.recv_mem);
> > +		if (libie_cp_can_send_onstack(buf_size))
> > +			kfree(get_ptype_info);
> >  	}
> >  
> >  out:
> >  	vport->rx_ptype_lkup = no_free_ptr(ptype_lkup);
> >  
> >  	return 0;
> > +
> > +free_rx_buf:
> > +	libie_ctlq_release_rx_buf(adapter->arq, &xn_params.recv_mem);
> > +free_tx_buf:
> > +	if (libie_cp_can_send_onstack(buf_size))
> > +		kfree(get_ptype_info);
> > +
> > +	return err;
> >  }
> >  
> >  /**
> 
> ...