[PATCH net] bnxt_en: eliminate the compile warning in bnxt_request_irq due to CONFIG_RFS_ACCEL

[PATCH net] bnxt_en: eliminate the compile warning in bnxt_request_irq due to CONFIG_RFS_ACCEL

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

I received a kernel-test-bot report[1] that shows the
[-Wunused-but-set-variable] warning. Since the previous commit[2] I made
gives users an option to turn on and off the CONFIG_RFS_ACCEL, the issue
then can be discovered and reproduced. Move the @i into the protection
of CONFIG_RFS_ACCEL.

[1]
All warnings (new ones prefixed by >>):

   drivers/net/ethernet/broadcom/bnxt/bnxt.c: In function 'bnxt_request_irq':
>> drivers/net/ethernet/broadcom/bnxt/bnxt.c:10703:9: warning: variable 'j' set but not used [-Wunused-but-set-variable]
   10703 |  int i, j, rc = 0;
         |         ^

[2]
commit 9b6a30febddf ("net: allow rps/rfs related configs to be switched")

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202506282102.x1tXt0qz-lkp@intel.com/
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 869580b6f70d..7369b39380d0 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -11538,9 +11538,10 @@ static void bnxt_free_irq(struct bnxt *bp)
 
 static int bnxt_request_irq(struct bnxt *bp)
 {
-	int i, j, rc = 0;
+	int i, rc = 0;
 	unsigned long flags = 0;
 #ifdef CONFIG_RFS_ACCEL
+	int j = 0;
 	struct cpu_rmap *rmap;
 #endif
 
@@ -11559,7 +11560,7 @@ static int bnxt_request_irq(struct bnxt *bp)
 	if (!rc)
 		bp->tph_mode = PCI_TPH_ST_IV_MODE;
 
-	for (i = 0, j = 0; i < bp->cp_nr_rings; i++) {
+	for (i = 0; i < bp->cp_nr_rings; i++) {
 		int map_idx = bnxt_cp_num_to_irq_num(bp, i);
 		struct bnxt_irq *irq = &bp->irq_tbl[map_idx];
 
-- 
2.41.3

Re: [PATCH net] bnxt_en: eliminate the compile warning in bnxt_request_irq due to CONFIG_RFS_ACCEL

[Simon Horman]

On Sun, Jun 29, 2025 at 08:36:16AM +0800, Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> I received a kernel-test-bot report[1] that shows the
> [-Wunused-but-set-variable] warning. Since the previous commit[2] I made
> gives users an option to turn on and off the CONFIG_RFS_ACCEL, the issue
> then can be discovered and reproduced. Move the @i into the protection
> of CONFIG_RFS_ACCEL.
> 
> [1]
> All warnings (new ones prefixed by >>):
> 
>    drivers/net/ethernet/broadcom/bnxt/bnxt.c: In function 'bnxt_request_irq':
> >> drivers/net/ethernet/broadcom/bnxt/bnxt.c:10703:9: warning: variable 'j' set but not used [-Wunused-but-set-variable]
>    10703 |  int i, j, rc = 0;
>          |         ^
> 
> [2]
> commit 9b6a30febddf ("net: allow rps/rfs related configs to be switched")
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202506282102.x1tXt0qz-lkp@intel.com/
> Signed-off-by: Jason Xing <kernelxing@tencent.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Not for net, but it would be nice to factor the #ifdefs out of this
function entirely.  E.g. by using a helper to perform that part of the
initialisation.

...

Re: [PATCH net] bnxt_en: eliminate the compile warning in bnxt_request_irq due to CONFIG_RFS_ACCEL

[Jason Xing]

On Mon, Jun 30, 2025 at 7:09 PM Simon Horman <horms@kernel.org> wrote:
>
> On Sun, Jun 29, 2025 at 08:36:16AM +0800, Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > I received a kernel-test-bot report[1] that shows the
> > [-Wunused-but-set-variable] warning. Since the previous commit[2] I made
> > gives users an option to turn on and off the CONFIG_RFS_ACCEL, the issue
> > then can be discovered and reproduced. Move the @i into the protection
> > of CONFIG_RFS_ACCEL.
> >
> > [1]
> > All warnings (new ones prefixed by >>):
> >
> >    drivers/net/ethernet/broadcom/bnxt/bnxt.c: In function 'bnxt_request_irq':
> > >> drivers/net/ethernet/broadcom/bnxt/bnxt.c:10703:9: warning: variable 'j' set but not used [-Wunused-but-set-variable]
> >    10703 |  int i, j, rc = 0;
> >          |         ^
> >
> > [2]
> > commit 9b6a30febddf ("net: allow rps/rfs related configs to be switched")
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202506282102.x1tXt0qz-lkp@intel.com/
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
>
> Reviewed-by: Simon Horman <horms@kernel.org>

Thanks.

>
> Not for net, but it would be nice to factor the #ifdefs out of this
> function entirely.  E.g. by using a helper to perform that part of the
> initialisation.

Got it. I will cook a patch after this patch is landed on the net-next branch.

Thanks,
Jason

Re: [PATCH net] bnxt_en: eliminate the compile warning in bnxt_request_irq due to CONFIG_RFS_ACCEL

[Jakub Kicinski]

On Mon, 30 Jun 2025 19:47:47 +0800 Jason Xing wrote:
> > Not for net, but it would be nice to factor the #ifdefs out of this
> > function entirely.  E.g. by using a helper to perform that part of the
> > initialisation.  
> 
> Got it. I will cook a patch after this patch is landed on the net-next branch.

Maybe we can fix it right already. The compiler should not complain if
it sees the read:

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index f621a5bab1ea..6bbe875132b0 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -11616,11 +11616,9 @@ static void bnxt_free_irq(struct bnxt *bp)
 
 static int bnxt_request_irq(struct bnxt *bp)
 {
+       struct cpu_rmap *rmap = NULL;
        int i, j, rc = 0;
        unsigned long flags = 0;
-#ifdef CONFIG_RFS_ACCEL
-       struct cpu_rmap *rmap;
-#endif
 
        rc = bnxt_setup_int_mode(bp);
        if (rc) {
@@ -11641,15 +11639,15 @@ static int bnxt_request_irq(struct bnxt *bp)
                int map_idx = bnxt_cp_num_to_irq_num(bp, i);
                struct bnxt_irq *irq = &bp->irq_tbl[map_idx];
 
-#ifdef CONFIG_RFS_ACCEL
-               if (rmap && bp->bnapi[i]->rx_ring) {
+               if (IS_ENABLED(CONFIG_RFS_ACCEL) &&
+                   rmap && bp->bnapi[i]->rx_ring) {
                        rc = irq_cpu_rmap_add(rmap, irq->vector);
                        if (rc)
                                netdev_warn(bp->dev, "failed adding irq rmap for ring %d\n",
                                            j);
                        j++;
                }
-#endif
+
                rc = request_irq(irq->vector, irq->handler, flags, irq->name,
                                 bp->bnapi[i]);
                if (rc)
-- 
pw-bot: cr

Re: [PATCH net] bnxt_en: eliminate the compile warning in bnxt_request_irq due to CONFIG_RFS_ACCEL

[Jason Xing]

On Wed, Jul 2, 2025 at 8:15 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 30 Jun 2025 19:47:47 +0800 Jason Xing wrote:
> > > Not for net, but it would be nice to factor the #ifdefs out of this
> > > function entirely.  E.g. by using a helper to perform that part of the
> > > initialisation.
> >
> > Got it. I will cook a patch after this patch is landed on the net-next branch.
>
> Maybe we can fix it right already. The compiler should not complain if
> it sees the read:
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index f621a5bab1ea..6bbe875132b0 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -11616,11 +11616,9 @@ static void bnxt_free_irq(struct bnxt *bp)
>
>  static int bnxt_request_irq(struct bnxt *bp)
>  {
> +       struct cpu_rmap *rmap = NULL;
>         int i, j, rc = 0;
>         unsigned long flags = 0;
> -#ifdef CONFIG_RFS_ACCEL
> -       struct cpu_rmap *rmap;
> -#endif

Sorry, Jakub. I failed to see the positive point of this kind of
change comparatively.

>
>         rc = bnxt_setup_int_mode(bp);
>         if (rc) {

Probably in this position, you expect 'rmap = bp->dev->rx_cpu_rmap;'
to stay there even when CONFIG_RFS_ACCEL is off?

The report says it's 'j' that causes the complaint.

Thanks,
Jason

> @@ -11641,15 +11639,15 @@ static int bnxt_request_irq(struct bnxt *bp)
>                 int map_idx = bnxt_cp_num_to_irq_num(bp, i);
>                 struct bnxt_irq *irq = &bp->irq_tbl[map_idx];
>
> -#ifdef CONFIG_RFS_ACCEL
> -               if (rmap && bp->bnapi[i]->rx_ring) {
> +               if (IS_ENABLED(CONFIG_RFS_ACCEL) &&
> +                   rmap && bp->bnapi[i]->rx_ring) {
>                         rc = irq_cpu_rmap_add(rmap, irq->vector);
>                         if (rc)
>                                 netdev_warn(bp->dev, "failed adding irq rmap for ring %d\n",
>                                             j);
>                         j++;
>                 }
> -#endif
> +
>                 rc = request_irq(irq->vector, irq->handler, flags, irq->name,
>                                  bp->bnapi[i]);
>                 if (rc)
> --
> pw-bot: cr

Re: [PATCH net] bnxt_en: eliminate the compile warning in bnxt_request_irq due to CONFIG_RFS_ACCEL

[Jakub Kicinski]

On Wed, 2 Jul 2025 08:47:08 +0800 Jason Xing wrote:
> >  static int bnxt_request_irq(struct bnxt *bp)
> >  {
> > +       struct cpu_rmap *rmap = NULL;
> >         int i, j, rc = 0;
> >         unsigned long flags = 0;
> > -#ifdef CONFIG_RFS_ACCEL
> > -       struct cpu_rmap *rmap;
> > -#endif  
> 
> Sorry, Jakub. I failed to see the positive point of this kind of
> change comparatively.

Like Simon said -- fewer #ifdefs leads to fewer bugs of this nature.
Or do you mean that you don't understand how my fix works?

> >         rc = bnxt_setup_int_mode(bp);
> >         if (rc) {  
> 
> Probably in this position, you expect 'rmap = bp->dev->rx_cpu_rmap;'
> to stay there even when CONFIG_RFS_ACCEL is off?

no, dev->rx_cpu_rmap doesn't exist if RDS_ACCEL=n

> The report says it's 'j' that causes the complaint.

Re: [PATCH net] bnxt_en: eliminate the compile warning in bnxt_request_irq due to CONFIG_RFS_ACCEL

[Jason Xing]

On Wed, Jul 2, 2025 at 8:56 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 2 Jul 2025 08:47:08 +0800 Jason Xing wrote:
> > >  static int bnxt_request_irq(struct bnxt *bp)
> > >  {
> > > +       struct cpu_rmap *rmap = NULL;
> > >         int i, j, rc = 0;
> > >         unsigned long flags = 0;
> > > -#ifdef CONFIG_RFS_ACCEL
> > > -       struct cpu_rmap *rmap;
> > > -#endif
> >
> > Sorry, Jakub. I failed to see the positive point of this kind of
> > change comparatively.
>
> Like Simon said -- fewer #ifdefs leads to fewer bugs of this nature.

Agree on this point.

> Or do you mean that you don't understand how my fix works?

I understand but my previous thought was not like this.

>
> > >         rc = bnxt_setup_int_mode(bp);
> > >         if (rc) {
> >
> > Probably in this position, you expect 'rmap = bp->dev->rx_cpu_rmap;'
> > to stay there even when CONFIG_RFS_ACCEL is off?
>
> no, dev->rx_cpu_rmap doesn't exist if RDS_ACCEL=n

I was trying to say 'readability' of rmap so I used 'stay' on purpose.
As to the core of the patch you provided, it works for sure.

I will send a v2 as you advised :)

Thanks,
Jason

>
> > The report says it's 'j' that causes the complaint.

Re: [PATCH net] bnxt_en: eliminate the compile warning in bnxt_request_irq due to CONFIG_RFS_ACCEL

[Jason Xing]

On Wed, Jul 2, 2025 at 9:07 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> On Wed, Jul 2, 2025 at 8:56 AM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Wed, 2 Jul 2025 08:47:08 +0800 Jason Xing wrote:
> > > >  static int bnxt_request_irq(struct bnxt *bp)
> > > >  {
> > > > +       struct cpu_rmap *rmap = NULL;
> > > >         int i, j, rc = 0;
> > > >         unsigned long flags = 0;
> > > > -#ifdef CONFIG_RFS_ACCEL
> > > > -       struct cpu_rmap *rmap;
> > > > -#endif
> > >
> > > Sorry, Jakub. I failed to see the positive point of this kind of
> > > change comparatively.
> >
> > Like Simon said -- fewer #ifdefs leads to fewer bugs of this nature.
>
> Agree on this point.
>
> > Or do you mean that you don't understand how my fix works?

Oh, thanks to the word 'fix' you mentioned. It seems that I'm supposed
to add Fixes: tag...

Re: [PATCH net] bnxt_en: eliminate the compile warning in bnxt_request_irq due to CONFIG_RFS_ACCEL

[Jakub Kicinski]

On Wed, 2 Jul 2025 09:11:35 +0800 Jason Xing wrote:
> > >
> > > Like Simon said -- fewer #ifdefs leads to fewer bugs of this nature.  
> >
> > Agree on this point.
> >  
> > > Or do you mean that you don't understand how my fix works?  
> 
> Oh, thanks to the word 'fix' you mentioned. It seems that I'm supposed
> to add Fixes: tag...

Yes, change the commit reference to a full Fixes tag.
Also -- may be worth mentioning that it happens with GCC specifically.
My first try to reproduce was with clang and it seems not to show this 
warning even with W=1

Re: [PATCH net] bnxt_en: eliminate the compile warning in bnxt_request_irq due to CONFIG_RFS_ACCEL

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 1103 bytes --]

On Sat, Jun 28, 2025 at 5:36 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> From: Jason Xing <kernelxing@tencent.com>
>
> I received a kernel-test-bot report[1] that shows the
> [-Wunused-but-set-variable] warning. Since the previous commit[2] I made
> gives users an option to turn on and off the CONFIG_RFS_ACCEL, the issue
> then can be discovered and reproduced. Move the @i into the protection
> of CONFIG_RFS_ACCEL.
>
> [1]
> All warnings (new ones prefixed by >>):
>
>    drivers/net/ethernet/broadcom/bnxt/bnxt.c: In function 'bnxt_request_irq':
> >> drivers/net/ethernet/broadcom/bnxt/bnxt.c:10703:9: warning: variable 'j' set but not used [-Wunused-but-set-variable]
>    10703 |  int i, j, rc = 0;
>          |         ^
>
> [2]
> commit 9b6a30febddf ("net: allow rps/rfs related configs to be switched")
>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202506282102.x1tXt0qz-lkp@intel.com/
> Signed-off-by: Jason Xing <kernelxing@tencent.com>

Thanks.
Reviewed-by: Michael Chan <michael.chan@broadcom.com>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4196 bytes --]