Re: [net v3 PATCH] octeontx2-pf: Avoid typecasts by simplifying otx2_atomic64_add macro

[Subbaraya Sundeep <sbhatta@marvell.com>]

Hi Simon,

On 2025-05-28 at 15:03:33, Simon Horman (horms@kernel.org) wrote:
> On Wed, May 28, 2025 at 10:10:42AM +0530, Subbaraya Sundeep wrote:
> > Just because otx2_atomic64_add is using u64 pointer as argument
> > all callers has to typecast __iomem void pointers which inturn
> > causing sparse warnings. Fix those by changing otx2_atomic64_add
> > argument to void pointer.
> > 
> > Fixes: caa2da34fd25 ("octeontx2-pf: Initialize and config queues")
> > Signed-off-by: Subbaraya Sundeep <sbhatta@marvell.com>
> > ---
> > v3:
> >  Make otx2_atomic64_add as nop for architectures other than ARM64
> >  to fix sparse warnings
> > v2:
> >  Fixed x86 build error of void pointer dereference reported by
> >  kernel test robot
> 
> Sorry, I seem to have made some some comments on v2 after v3 was posted.
> 
> 1) I'm wondering if you considered changing the type of the 2nd parameter
>    of otx2_atomic64_add to u64 __iomem * and, correspondingly, the type of

My intention is to fix sparse warnings (no __force) and avoid typecasts
so that code is correct and looks cleaner. If I change 2nd param of
otx2_atomics64_add as u64 __iomem * then I still have to use
__force to make sparse happy. This way only otx2_atomic64_add looks odd
internally with assembly stuff.

>    the local variables updated by this patch. Perhaps that isn't so clean
>    for some reason. But if it can be done cleanly it does seem slightly
>    nicer to me.
>
> 2) I wonder if this is more of a clean-up for net-next (once it re-opens,
>    no Fixes tag) than a fix.
> 
Sure. Will post as net-next material later.

Thanks,
Sundeep

> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> 
> ...
> 
> > @@ -747,7 +748,11 @@ static inline u64 otx2_atomic64_add(u64 incr, u64 *ptr)
> >  
> >  #else
> >  #define otx2_write128(lo, hi, addr)		writeq((hi) | (lo), addr)
> > -#define otx2_atomic64_add(incr, ptr)		({ *ptr += incr; })
> > +
> > +static inline u64 otx2_atomic64_add(u64 incr, void __iomem *addr)
> > +{
> > +	return 0;
> 
> Is it intentional that no increment is occurring here,
> whereas there was one in the macro version this replaces?
> 
> > +}
> >  #endif
> 
> ...