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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <b16a716e-69e3-406f-a0f6-1d62cfa39c42@redhat.com>
Date: Tue, 4 Mar 2025 11:47:41 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Satish Kharat <satishkh@...co.com>, Christian Benvenuti
 <benve@...co.com>, Andrew Lunn <andrew+netdev@...n.ch>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 Nelson Escobar <neescoba@...co.com>, John Daley <johndale@...co.com>
Subject: Re: [PATCH 7/8] enic : cleanup of enic wq request completion path

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