Re: [PATCH 7/8] enic : cleanup of enic wq request completion path

[Paolo Abeni <pabeni@redhat.com>]

On 2/28/25 1:30 AM, Satish Kharat wrote:
> diff --git a/drivers/net/ethernet/cisco/enic/enic_wq.c b/drivers/net/ethernet/cisco/enic/enic_wq.c
> index 88fdc462839a8360000eb8526be64118ea35c0e2..37e8f6eeae3fabd3391b8fcacc5f3420ad091b17 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_wq.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_wq.c
> @@ -6,8 +6,12 @@
>  #include "enic.h"
>  #include "enic_wq.h"
>  
> -static void cq_desc_dec(const struct cq_desc *desc_arg, u8 *type, u8 *color,
> -			u16 *q_number, u16 *completed_index)
> +#define ENET_CQ_DESC_COMP_NDX_BITS 14
> +#define ENET_CQ_DESC_COMP_NDX_MASK GENMASK(ENET_CQ_DESC_COMP_NDX_BITS - 1, 0)
> +
> +static inline void enic_wq_cq_desc_dec(const struct cq_desc *desc_arg, bool ext_wq,
> +				       u8 *type, u8 *color, u16 *q_number,
> +				       u16 *completed_index)

Please avoid 'inline' function in c files.

> @@ -111,7 +81,41 @@ int enic_wq_service(struct vnic_dev *vdev, struct cq_desc *cq_desc, u8 type,
>  	}
>  
>  	spin_unlock(&enic->wq[q_number].lock);
> -
> -	return 0;
>  }
>  
> +unsigned int enic_wq_cq_service(struct enic *enic, unsigned int cq_index, unsigned int work_to_do)
> +{
> +	u16 q_number, completed_index;
> +	u8 type, color;
> +	unsigned int work_done = 0;
> +	struct vnic_cq *cq = &enic->cq[cq_index];
> +	struct cq_desc *cq_desc = (struct cq_desc *)((u8 *)cq->ring.descs +
> +		cq->ring.desc_size * cq->to_clean);

I think the code would be more readable creating an helper to access the
`cq->to_clean` descriptor.

> +
> +	bool ext_wq = cq->ring.size > ENIC_MAX_WQ_DESCS;

Please respect the revers christmas tree order for variable definition,
and avoid empty lines in the variable definition area.

The above also applies to other patches.

Please include the target tree name (net-next in this case) in the subj
prefix in the next iteration.

Thanks,

Paolo