linux-kernel - Re: [net-next PATCH v3 1/2] octeontx2-af: correct _

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:
 <BY3PR18MB4707C3348715B237C0A95C47A0B32@BY3PR18MB4707.namprd18.prod.outlook.com>
Date: Mon, 14 Apr 2025 16:38:53 +0000
From: Sai Krishna Gajula <saikrishnag@...vell.com>
To: Andrew Lunn <andrew@...n.ch>
CC: "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com"
	<edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        Sunil Kovvuri Goutham <sgoutham@...vell.com>,
        Geethasowjanya Akula <gakula@...vell.com>,
        Linu Cherian
	<lcherian@...vell.com>, Jerin Jacob <jerinj@...vell.com>,
        Hariprasad Kelam
	<hkelam@...vell.com>,
        Subbaraya Sundeep Bhatta <sbhatta@...vell.com>,
        "andrew+netdev@...n.ch" <andrew+netdev@...n.ch>,
        Bharat Bhushan
	<bbhushan2@...vell.com>,
        "nathan@...nel.org" <nathan@...nel.org>,
        "ndesaulniers@...gle.com" <ndesaulniers@...gle.com>,
        "morbo@...gle.com"
	<morbo@...gle.com>,
        "justinstitt@...gle.com" <justinstitt@...gle.com>,
        "llvm@...ts.linux.dev" <llvm@...ts.linux.dev>,
        "horms@...nel.org"
	<horms@...nel.org>,
        kernel test robot <lkp@...el.com>
Subject: Re: [net-next PATCH v3 1/2] octeontx2-af: correct __iomem annotations
 flagged by Sparse

> -----Original Message-----
> From: Andrew Lunn <andrew@...n.ch>
> Sent: Wednesday, March 12, 2025 2:52 AM
> To: Sai Krishna Gajula <saikrishnag@...vell.com>
> Cc: davem@...emloft.net; edumazet@...gle.com; kuba@...nel.org;
> pabeni@...hat.com; netdev@...r.kernel.org; linux-kernel@...r.kernel.org;
> Sunil Kovvuri Goutham <sgoutham@...vell.com>; Geethasowjanya Akula
> <gakula@...vell.com>; Linu Cherian <lcherian@...vell.com>; Jerin Jacob
> <jerinj@...vell.com>; Hariprasad Kelam <hkelam@...vell.com>; Subbaraya
> Sundeep Bhatta <sbhatta@...vell.com>; andrew+netdev@...n.ch; Bharat
> Bhushan <bbhushan2@...vell.com>; nathan@...nel.org;
> ndesaulniers@...gle.com; morbo@...gle.com; justinstitt@...gle.com;
> llvm@...ts.linux.dev; horms@...nel.org; kernel test robot <lkp@...el.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.
> 
> >  	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