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

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

[Subbaraya Sundeep]

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

 .../net/ethernet/marvell/octeontx2/nic/otx2_common.c    | 17 +++++++++--------
 .../net/ethernet/marvell/octeontx2/nic/otx2_common.h    | 11 ++++++++---
 drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c    |  4 ++--
 drivers/net/ethernet/marvell/octeontx2/nic/qos_sq.c     |  5 +++--
 4 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
index 84cd029..e5be021 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
@@ -28,12 +28,12 @@ static void otx2_nix_rq_op_stats(struct queue_stats *stats,
 				 struct otx2_nic *pfvf, int qidx)
 {
 	u64 incr = (u64)qidx << 32;
-	u64 *ptr;
+	void __iomem *ptr;
 
-	ptr = (u64 *)otx2_get_regaddr(pfvf, NIX_LF_RQ_OP_OCTS);
+	ptr = otx2_get_regaddr(pfvf, NIX_LF_RQ_OP_OCTS);
 	stats->bytes = otx2_atomic64_add(incr, ptr);
 
-	ptr = (u64 *)otx2_get_regaddr(pfvf, NIX_LF_RQ_OP_PKTS);
+	ptr = otx2_get_regaddr(pfvf, NIX_LF_RQ_OP_PKTS);
 	stats->pkts = otx2_atomic64_add(incr, ptr);
 }
 
@@ -41,12 +41,12 @@ static void otx2_nix_sq_op_stats(struct queue_stats *stats,
 				 struct otx2_nic *pfvf, int qidx)
 {
 	u64 incr = (u64)qidx << 32;
-	u64 *ptr;
+	void __iomem *ptr;
 
-	ptr = (u64 *)otx2_get_regaddr(pfvf, NIX_LF_SQ_OP_OCTS);
+	ptr = otx2_get_regaddr(pfvf, NIX_LF_SQ_OP_OCTS);
 	stats->bytes = otx2_atomic64_add(incr, ptr);
 
-	ptr = (u64 *)otx2_get_regaddr(pfvf, NIX_LF_SQ_OP_PKTS);
+	ptr = otx2_get_regaddr(pfvf, NIX_LF_SQ_OP_PKTS);
 	stats->pkts = otx2_atomic64_add(incr, ptr);
 }
 
@@ -860,9 +860,10 @@ void otx2_sqb_flush(struct otx2_nic *pfvf)
 {
 	int qidx, sqe_tail, sqe_head;
 	struct otx2_snd_queue *sq;
-	u64 incr, *ptr, val;
+	void __iomem *ptr;
+	u64 incr, val;
 
-	ptr = (u64 *)otx2_get_regaddr(pfvf, NIX_LF_SQ_OP_STATUS);
+	ptr = otx2_get_regaddr(pfvf, NIX_LF_SQ_OP_STATUS);
 	for (qidx = 0; qidx < otx2_get_total_tx_queues(pfvf); qidx++) {
 		sq = &pfvf->qset.sq[qidx];
 		if (!sq->sqb_ptrs)
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
index d6b4b74..c4a0b45 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
@@ -733,8 +733,9 @@ static inline void otx2_write128(u64 lo, u64 hi, void __iomem *addr)
 			 ::[x0]"r"(lo), [x1]"r"(hi), [p1]"r"(addr));
 }
 
-static inline u64 otx2_atomic64_add(u64 incr, u64 *ptr)
+static inline u64 otx2_atomic64_add(u64 incr, void __iomem *addr)
 {
+	u64 __iomem *ptr = addr;
 	u64 result;
 
 	__asm__ volatile(".cpu   generic+lse\n"
@@ -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;
+}
 #endif
 
 static inline void __cn10k_aura_freeptr(struct otx2_nic *pfvf, u64 aura,
@@ -797,7 +802,7 @@ static inline void cn10k_aura_freeptr(void *dev, int aura, u64 buf)
 /* Alloc pointer from pool/aura */
 static inline u64 otx2_aura_allocptr(struct otx2_nic *pfvf, int aura)
 {
-	u64 *ptr = (__force u64 *)otx2_get_regaddr(pfvf, NPA_LF_AURA_OP_ALLOCX(0));
+	void __iomem *ptr = otx2_get_regaddr(pfvf, NPA_LF_AURA_OP_ALLOCX(0));
 	u64 incr = (u64)aura | BIT_ULL(63);
 
 	return otx2_atomic64_add(incr, ptr);
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
index cfed9ec..b303a03 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
@@ -1305,8 +1305,8 @@ static irqreturn_t otx2_q_intr_handler(int irq, void *data)
 {
 	struct otx2_nic *pf = data;
 	struct otx2_snd_queue *sq;
-	u64 val, *ptr;
-	u64 qidx = 0;
+	void __iomem *ptr;
+	u64 val, qidx = 0;
 
 	/* CQ */
 	for (qidx = 0; qidx < pf->qset.cq_cnt; qidx++) {
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/qos_sq.c b/drivers/net/ethernet/marvell/octeontx2/nic/qos_sq.c
index c5dbae0..d3ae390 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/qos_sq.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/qos_sq.c
@@ -151,9 +151,10 @@ static void otx2_qos_sq_free_sqbs(struct otx2_nic *pfvf, int qidx)
 static void otx2_qos_sqb_flush(struct otx2_nic *pfvf, int qidx)
 {
 	int sqe_tail, sqe_head;
-	u64 incr, *ptr, val;
+	void __iomem *ptr;
+	u64 incr, val;
 
-	ptr = (__force u64 *)otx2_get_regaddr(pfvf, NIX_LF_SQ_OP_STATUS);
+	ptr = otx2_get_regaddr(pfvf, NIX_LF_SQ_OP_STATUS);
 	incr = (u64)qidx << 32;
 	val = otx2_atomic64_add(incr, ptr);
 	sqe_head = (val >> 20) & 0x3F;
-- 
2.7.4

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

[Simon Horman]

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

> 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

...

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

[Subbaraya Sundeep]

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

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

[Simon Horman]

On Thu, May 29, 2025 at 07:04:48AM +0000, Subbaraya Sundeep wrote:
> 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.

Thanks. Based on your remarks above I agree this is a good approach.

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

Again, thanks.