| 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
| ||
|
Message-ID:
<DS0PR18MB536880099C7682CAD84604CBA0952@DS0PR18MB5368.namprd18.prod.outlook.com>
Date: Wed, 28 Aug 2024 05:39:48 +0000
From: Srujana Challa <schalla@...vell.com>
To: Simon Horman <horms@...nel.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"davem@...emloft.net"
<davem@...emloft.net>,
"kuba@...nel.org" <kuba@...nel.org>,
"pabeni@...hat.com" <pabeni@...hat.com>,
"edumazet@...gle.com"
<edumazet@...gle.com>,
Sunil Kovvuri Goutham <sgoutham@...vell.com>,
Linu
Cherian <lcherian@...vell.com>,
Geethasowjanya Akula <gakula@...vell.com>,
Jerin Jacob <jerinj@...vell.com>,
Hariprasad Kelam <hkelam@...vell.com>,
Subbaraya Sundeep Bhatta <sbhatta@...vell.com>,
Bharat Bhushan
<bbhushan2@...vell.com>,
Nithin Kumar Dabilpuram <ndabilpuram@...vell.com>
Subject: RE: [EXTERNAL] Re: [PATCH net-next,1/2] octeontx2-af: reduce cpt flt
interrupt vectors for CN10KB
> Subject: [EXTERNAL] 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
> 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.
Sure, I will make the change.
>
> >
> > 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.
No, it couldn't overflow. I will change the return type to u16.
>
> > +}
> > +
> > +/* 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.
Ack.
>
> > +
> > + 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.
Ack.
>
> > +
> > + 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.
Ack.
>
> 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.
Both are int only.
>
> > + 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.
Ack.
>
> > 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
Thanks for reviewing the patch.
Powered by blists - more mailing lists