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

[Simon Horman <horms@kernel.org>]

On Thu, Apr 24, 2025 at 01:32:31PM +0200, Larysa Zaremba wrote:
> From: Pavan Kumar Linga <pavan.kumar.linga@intel.com>
> 
> 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@intel.com>
> Signed-off-by: Pavan Kumar Linga <pavan.kumar.linga@intel.com>
> Co-developed-by: Larysa Zaremba <larysa.zaremba@intel.com>
> Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
> ---
>  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?

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

> +			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;
>  }
>  
>  /**

...