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

[Andrew Lunn <andrew@lunn.ch>]

On Mon, Apr 14, 2025 at 04:38:53PM +0000, Sai Krishna Gajula wrote:
> > -----Original Message-----
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Wednesday, March 12, 2025 2:52 AM
> > To: Sai Krishna Gajula <saikrishnag@marvell.com>
> > Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> > pabeni@redhat.com; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> > Sunil Kovvuri Goutham <sgoutham@marvell.com>; Geethasowjanya Akula
> > <gakula@marvell.com>; Linu Cherian <lcherian@marvell.com>; Jerin Jacob
> > <jerinj@marvell.com>; Hariprasad Kelam <hkelam@marvell.com>; Subbaraya
> > Sundeep Bhatta <sbhatta@marvell.com>; andrew+netdev@lunn.ch; Bharat
> > Bhushan <bbhushan2@marvell.com>; nathan@kernel.org;
> > ndesaulniers@google.com; morbo@google.com; justinstitt@google.com;
> > llvm@lists.linux.dev; horms@kernel.org; kernel test robot <lkp@intel.com>
> > 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.

Please configure your email client to correctly wrap emails.

Did you check the performance of memcpy_fromio()? That is the API you
are supposed to be using with an __iomem. I _think_ memcpy is only
going to work for some architectures and other architectures will give
you problems. That is the whole point of the __iomem, to make sure you
use the correct API which works across architectures.

> > 
> > >  	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;
> 

Teach this function to accept an __iomem. Worst case, you remove the
cast here, before going into assembly.

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

What actually happens if you keep the attribute? My guess is
nothing. These are sparse markups, and gcc/as never seems them.

	Andrew