netdev - Re: [PATCH net-next,1/2] octeontx2-af: reduce cpt flt interrupt vectors for CN10KB

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240827163218.GO1368797@kernel.org>
Date: Tue, 27 Aug 2024 17:32:18 +0100
From: Simon Horman <horms@...nel.org>
To: Srujana Challa <schalla@...vell.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
	pabeni@...hat.com, edumazet@...gle.com, sgoutham@...vell.com,
	lcherian@...vell.com, gakula@...vell.com, jerinj@...vell.com,
	hkelam@...vell.com, sbhatta@...vell.com, bbhushan2@...vell.com,
	ndabilpuram@...vell.com
Subject: Re: [PATCH net-next,1/2] octeontx2-af: reduce cpt flt interrupt
 vectors for CN10KB

On Tue, Aug 27, 2024 at 09:55:11AM +0530, Srujana Challa wrote:
> On CN10KB, number of flt interrupt vectors are reduced to
> 2. So, modify the code accordingly for cn10k.

I think it would be nice to state that the patch moves
away from a hard-coded to dynamic number of vectors.
And that this is in order to accommodate the CN10KB,
which has 2 vectors, as opposed to chips supported by
the driver to date, which have 3.

> 
> Signed-off-by: Srujana Challa <schalla@...vell.com>

...

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.c
> index 3e09d2285814..e56d27018828 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.c
> @@ -37,6 +37,44 @@
>  	(_rsp)->free_sts_##etype = free_sts;                        \
>  })
>  
> +#define MAX_AE  GENMASK_ULL(47, 32)
> +#define MAX_IE  GENMASK_ULL(31, 16)
> +#define MAX_SE  GENMASK_ULL(15, 0)

nit: Maybe a blank line here.

> +static u32 cpt_max_engines_get(struct rvu *rvu)
> +{
> +	u16 max_ses, max_ies, max_aes;
> +	u64 reg;
> +
> +	reg = rvu_read64(rvu, BLKADDR_CPT0, CPT_AF_CONSTANTS1);
> +	max_ses = FIELD_GET(MAX_SE, reg);
> +	max_ies = FIELD_GET(MAX_IE, reg);
> +	max_aes = FIELD_GET(MAX_AE, reg);
> +
> +	return max_ses + max_ies + max_aes;

Maybe I am wrong, but it looks like this will perform u16 addition.
Can that overflow? I ask because the return type is u32, implying
values larger than 64k are expected.

> +}
> +
> +/* Number of flt interrupt vectors are depends on number of engines that the
> + * chip has. Each flt vector represents 64 engines.
> + */
> +static int cpt_10k_flt_nvecs_get(struct rvu *rvu)
> +{
> +	u32 max_engs;
> +	int flt_vecs;
> +
> +	max_engs = cpt_max_engines_get(rvu);
> +
> +	flt_vecs = (max_engs / 64);
> +	flt_vecs += (max_engs % 64) ? 1 : 0;

I don't think the parentheses are needed on the lines above.
And likewise elsewhere in this patch.

At any rate, here, I think you can use DIV_ROUND_UP.

> +
> +	if (flt_vecs > CPT_10K_AF_INT_VEC_FLT_MAX) {
> +		dev_warn(rvu->dev, "flt_vecs(%d) exceeds the max vectors(%d)\n",
> +			 flt_vecs, CPT_10K_AF_INT_VEC_FLT_MAX);
> +		flt_vecs = CPT_10K_AF_INT_VEC_FLT_MAX;
> +	}

cpt_max_engines_get seems to get called quite a bit.
I think dev_warn_once() is probably appropriate here.

> +
> +	return flt_vecs;
> +}
> +
>  static irqreturn_t cpt_af_flt_intr_handler(int vec, void *ptr)
>  {
>  	struct rvu_block *block = ptr;
> @@ -150,17 +188,25 @@ static void cpt_10k_unregister_interrupts(struct rvu_block *block, int off)
>  {
>  	struct rvu *rvu = block->rvu;
>  	int blkaddr = block->addr;
> +	u32 max_engs;
> +	u8 nr;
>  	int i;
>  
> +	max_engs = cpt_max_engines_get(rvu);
> +
>  	/* Disable all CPT AF interrupts */
> -	rvu_write64(rvu, blkaddr, CPT_AF_FLTX_INT_ENA_W1C(0), ~0ULL);
> -	rvu_write64(rvu, blkaddr, CPT_AF_FLTX_INT_ENA_W1C(1), ~0ULL);
> -	rvu_write64(rvu, blkaddr, CPT_AF_FLTX_INT_ENA_W1C(2), 0xFFFF);
> +	for (i = CPT_10K_AF_INT_VEC_FLT0; i < cpt_10k_flt_nvecs_get(rvu); i++) {

I think it would be best to cache the value of cpt_10k_flt_nvecs_get()
rather than run it for each iteration of the loop. I'm assuming it has a
non-zero cost as it reads a register, the value of which which I assume
does not change.

Also, the same register is already read by the call to
cpt_max_engines_get(). It would be nice to read it just once in this scope.

Likewise elsewhere.

Also, there is an inconsistency between the type of i and the return type
of cpt_10k_flt_nvecs_get(). Probably harmless, but IMHO it would be nice to
fix.

> +		nr = (max_engs > 64) ? 64 : max_engs;
> +		max_engs -= nr;
> +		rvu_write64(rvu, blkaddr, CPT_AF_FLTX_INT_ENA_W1C(i),
> +			    INTR_MASK(nr));
> +	}
>  
>  	rvu_write64(rvu, blkaddr, CPT_AF_RVU_INT_ENA_W1C, 0x1);
>  	rvu_write64(rvu, blkaddr, CPT_AF_RAS_INT_ENA_W1C, 0x1);
>  
> -	for (i = 0; i < CPT_10K_AF_INT_VEC_CNT; i++)
> +	/* CPT AF interrupt vectors are flt_int, rvu_int and ras_int. */
> +	for (i = 0; i < cpt_10k_flt_nvecs_get(rvu) + 2; i++)

It would be nice to avoid the naked '2' here.
Perhaps a macro is appropriate.

>  		if (rvu->irq_allocated[off + i]) {
>  			free_irq(pci_irq_vector(rvu->pdev, off + i), block);
>  			rvu->irq_allocated[off + i] = false;

...

-- 
pw-bot: cr