Re: [net-next PATCH v3 1/2] octeontx2-af: correct __iomem annotations flagged by Sparse

[Sai Krishna Gajula]

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Wednesday, March 12, 2025 2:52 AM
> To: Sai Krishna Gajula <saikrishnag@xxxxxxxxxxx>
> Cc: davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
> pabeni@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> Sunil Kovvuri Goutham <sgoutham@xxxxxxxxxxx>; Geethasowjanya Akula
> <gakula@xxxxxxxxxxx>; Linu Cherian <lcherian@xxxxxxxxxxx>; Jerin Jacob
> <jerinj@xxxxxxxxxxx>; Hariprasad Kelam <hkelam@xxxxxxxxxxx>; Subbaraya
> Sundeep Bhatta <sbhatta@xxxxxxxxxxx>; andrew+netdev@xxxxxxx; Bharat
> Bhushan <bbhushan2@xxxxxxxxxxx>; nathan@xxxxxxxxxx;
> ndesaulniers@xxxxxxxxxx; morbo@xxxxxxxxxx; justinstitt@xxxxxxxxxx;
> llvm@xxxxxxxxxxxxxxx; horms@xxxxxxxxxx; kernel test robot <lkp@xxxxxxxxx>
> Subject: Re: [net-next PATCH v3 1/2] octeontx2-af: correct
> __iomem annotations flagged by Sparse
> 
> > if (mbox->mbox. hwbase) > - iounmap(mbox->mbox. hwbase); > +
> > iounmap((void __iomem *)mbox->mbox. hwbase); This looks wrong. If you
> > are unmapping it, you must of mapped it somewhere. And that mapping
> > would of returned an __iomem
> 
> >  	if (mbox->mbox.hwbase)
> > -		iounmap(mbox->mbox.hwbase);
> > +		iounmap((void __iomem *)mbox->mbox.hwbase);
> 
> This looks wrong. If you are unmapping it, you must of mapped it
> somewhere. And that mapping would of returned an __iomem value. So
> mbox.hwbase should be an __iomem value and you would not need this
> cast.
Yes,  mbox->mbox.hwbase is ioremapped with cache (ioremap_wc), while initialization it is declared as __iomem. But this hwbase is actually a DRAM memory mapped to BAR for better accessibility across the system. Since we use large memcpy (64KB and more) to/from this hwbase, we forced it to use as "void */u64" before exiting with unmap. As this is DRAM memory, memory access will not have side effects. Infact the AI applications also recommended similar mechanism. Please suggest if you have any other view and this can be addressed in some other way.
> 
> >  	for (qidx = 0; qidx < pf->qset.cq_cnt; qidx++) {
> > -		ptr = otx2_get_regaddr(pf, NIX_LF_CQ_OP_INT);
> > +		ptr = (__force u64 *)otx2_get_regaddr(pf,
> NIX_LF_CQ_OP_INT);
> >  		val = otx2_atomic64_add((qidx << 44), ptr);
> 
> This also looks questionable. You should be removing casts, not adding them.
> otx2_get_regaddr() returns an __iomem. So maybe
> otx2_atomic64_add() is actually broken here?
Similar to the above case, otx2_atomic64_add is a special case where it uses assembly code as part of definition, hence we had to use typecasting the "ptr". Please suggest if there is any better way.

static inline u64 otx2_atomic64_add(u64 incr, u64 *ptr)
{
        u64 result;

        __asm__ volatile(".cpu   generic+lse\n"
                         "ldadd %x[i], %x[r], [%[b]]"
                         : [r]"=r"(result), "+m"(*ptr)
                         : [i]"r"(incr), [b]"r"(ptr)
                         : "memory");
        return result;
}

Apologies for delayed in response.
> 
>     Andrew
> 
> ---
> pw-bot: cr