Re: [PATCH 01/25] common/idpf: added NVME CPF specific code with defines

[Bruce Richardson <bruce.richardson@intel.com>]

On Tue, May 28, 2024 at 07:28:31AM +0000, Soumyadeep Hore wrote:
> The aim of the changes is to remove NVME dependency on

Hi,

The intro words "The aim of the changes" is unnecessary. Just shorten the
commit log by stating what the patch is for without any intro:

"Remove NVME dependency on memory allocations..."

If rewording, it would be worth including detail about when the define is
expected to be used - will it be defined/configured in a later patch, or is
it expected that it's a build-time setting set by user? If latter case, we
need a doc update here.

One further minor nit below too.

/Bruce



> memory allocations, and to use a prepared buffer instead.
> 
> The changes do not affect other components.
> 
> Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
> ---
>  drivers/common/idpf/base/idpf_controlq.c     | 27 +++++++++++++++++---
>  drivers/common/idpf/base/idpf_controlq_api.h |  9 +++++--
>  2 files changed, 31 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/common/idpf/base/idpf_controlq.c b/drivers/common/idpf/base/idpf_controlq.c
> index a82ca628de..0ba7281a45 100644
> --- a/drivers/common/idpf/base/idpf_controlq.c
> +++ b/drivers/common/idpf/base/idpf_controlq.c
> @@ -1,5 +1,5 @@
>  /* SPDX-License-Identifier: BSD-3-Clause
> - * Copyright(c) 2001-2023 Intel Corporation
> + * Copyright(c) 2001-2024 Intel Corporation
>   */
>  
>  #include "idpf_controlq.h"
> @@ -145,8 +145,12 @@ int idpf_ctlq_add(struct idpf_hw *hw,
>  	    qinfo->buf_size > IDPF_CTLQ_MAX_BUF_LEN)
>  		return -EINVAL;
>  
> +#ifndef NVME_CPF
>  	cq = (struct idpf_ctlq_info *)
>  	     idpf_calloc(hw, 1, sizeof(struct idpf_ctlq_info));
> +#else
> +	cq = *cq_out;
> +#endif
>  	if (!cq)
>  		return -ENOMEM;
>  
> @@ -172,10 +176,15 @@ int idpf_ctlq_add(struct idpf_hw *hw,
>  	}
>  
>  	if (status)
> +#ifdef NVME_CPF
> +		return status;
> +#else
>  		goto init_free_q;
> +#endif
>  
>  	if (is_rxq) {
>  		idpf_ctlq_init_rxq_bufs(cq);
> +#ifndef NVME_CPF
>  	} else {
>  		/* Allocate the array of msg pointers for TX queues */
>  		cq->bi.tx_msg = (struct idpf_ctlq_msg **)
> @@ -185,6 +194,7 @@ int idpf_ctlq_add(struct idpf_hw *hw,
>  			status = -ENOMEM;
>  			goto init_dealloc_q_mem;
>  		}
> +#endif
>  	}
>  
>  	idpf_ctlq_setup_regs(cq, qinfo);
> @@ -195,6 +205,7 @@ int idpf_ctlq_add(struct idpf_hw *hw,
>  
>  	LIST_INSERT_HEAD(&hw->cq_list_head, cq, cq_list);
>  
> +#ifndef NVME_CPF
>  	*cq_out = cq;
>  	return status;
>  
> @@ -205,6 +216,7 @@ int idpf_ctlq_add(struct idpf_hw *hw,
>  	idpf_free(hw, cq);
>  	cq = NULL;
>  
> +#endif
>  	return status;
>  }
>  
> @@ -232,8 +244,13 @@ void idpf_ctlq_remove(struct idpf_hw *hw,
>   * destroyed. This must be called prior to using the individual add/remove
>   * APIs.
>   */
> +#ifdef NVME_CPF
> +int idpf_ctlq_init(struct idpf_hw *hw, u8 num_q,
> +                   struct idpf_ctlq_create_info *q_info, struct idpf_ctlq_info **ctlq)
> +#else
>  int idpf_ctlq_init(struct idpf_hw *hw, u8 num_q,
>  		   struct idpf_ctlq_create_info *q_info)
> +#endif
>  {
>  	struct idpf_ctlq_info *cq = NULL, *tmp = NULL;
>  	int ret_code = 0;
> @@ -244,6 +261,10 @@ int idpf_ctlq_init(struct idpf_hw *hw, u8 num_q,
>  	for (i = 0; i < num_q; i++) {
>  		struct idpf_ctlq_create_info *qinfo = q_info + i;
>  
> +#ifdef NVME_CPF
> +		cq = *(ctlq + i);
> +
> +#endif	
>  		ret_code = idpf_ctlq_add(hw, qinfo, &cq);
>  		if (ret_code)
>  			goto init_destroy_qs;
> @@ -398,7 +419,7 @@ int idpf_ctlq_send(struct idpf_hw *hw, struct idpf_ctlq_info *cq,
>   * ctlq_msgs and free or reuse the DMA buffers.
>   */
>  static int __idpf_ctlq_clean_sq(struct idpf_ctlq_info *cq, u16 *clean_count,
> -				struct idpf_ctlq_msg *msg_status[], bool force)
> +		                struct idpf_ctlq_msg *msg_status[], bool force)
>  {
>  	struct idpf_ctlq_desc *desc;
>  	u16 i = 0, num_to_clean;
> @@ -469,7 +490,7 @@ static int __idpf_ctlq_clean_sq(struct idpf_ctlq_info *cq, u16 *clean_count,
>   * ctlq_msgs and free or reuse the DMA buffers.
>   */
>  int idpf_ctlq_clean_sq_force(struct idpf_ctlq_info *cq, u16 *clean_count,
> -			     struct idpf_ctlq_msg *msg_status[])
> +		             struct idpf_ctlq_msg *msg_status[])
>  {
>  	return __idpf_ctlq_clean_sq(cq, clean_count, msg_status, true);
>  }
> diff --git a/drivers/common/idpf/base/idpf_controlq_api.h b/drivers/common/idpf/base/idpf_controlq_api.h
> index 38f5d2df3c..bce5187981 100644
> --- a/drivers/common/idpf/base/idpf_controlq_api.h
> +++ b/drivers/common/idpf/base/idpf_controlq_api.h
> @@ -1,5 +1,5 @@
>  /* SPDX-License-Identifier: BSD-3-Clause
> - * Copyright(c) 2001-2023 Intel Corporation
> + * Copyright(c) 2001-2024 Intel Corporation
>   */
>  
>  #ifndef _IDPF_CONTROLQ_API_H_
> @@ -158,8 +158,13 @@ enum idpf_mbx_opc {
>  /* Will init all required q including default mb.  "q_info" is an array of
>   * create_info structs equal to the number of control queues to be created.
>   */
> +#ifdef NVME_CPF
> +int idpf_ctlq_init(struct idpf_hw *hw, u8 num_q,
> +                   struct idpf_ctlq_create_info *q_info, struct idpf_ctlq_info **ctlq);
> +#else
>  int idpf_ctlq_init(struct idpf_hw *hw, u8 num_q,
>  		   struct idpf_ctlq_create_info *q_info);
> +#endif
>  
>  /* Allocate and initialize a single control queue, which will be added to the
>   * control queue list; returns a handle to the created control queue
> @@ -186,7 +191,7 @@ int idpf_ctlq_recv(struct idpf_ctlq_info *cq, u16 *num_q_msg,
>  
>  /* Reclaims all descriptors on HW write back */
>  int idpf_ctlq_clean_sq_force(struct idpf_ctlq_info *cq, u16 *clean_count,
> -			     struct idpf_ctlq_msg *msg_status[]);
> +		             struct idpf_ctlq_msg *msg_status[]);

This is a whitespace change that has slipped in unrelated to the rest of
the patch. Not a big deal, just keep an eye out for such things. If there
are a few like this, you can consider just putting them in a misc patch at
the end.

>  
>  /* Reclaims send descriptors on HW write back */
>  int idpf_ctlq_clean_sq(struct idpf_ctlq_info *cq, u16 *clean_count,
> -- 
> 2.43.0
>