linux-kernel - Re: [PATCH] irqdomain: fix -Wshadow warning

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]

Date:   Mon, 16 Nov 2020 16:26:41 +0100
From:   Arnd Bergmann <arnd@...nel.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Marc Zyngier <maz@...nel.org>, Arnd Bergmann <arnd@...db.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] irqdomain: fix -Wshadow warning

On Mon, Nov 16, 2020 at 3:03 PM Thomas Gleixner <tglx@...utronix.de> wrote:
> On Mon, Oct 26 2020 at 17:20, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@...db.de>
> >
> > When building with W=2, there are tons of warnings about conflicting
> > local and global definitions of 'nr_irqs' in common header files:
> >
> > include/linux/irqdomain.h: In function 'irq_domain_alloc_irqs':
> > include/linux/irqdomain.h:482:17: warning: declaration of 'nr_irqs' shadows a global declaration [-Wshadow]
> >   482 |    unsigned int nr_irqs, int node, void *arg)
> >       |    ~~~~~~~~~~~~~^~~~~~~
> > In file included from ../include/linux/interrupt.h:10,
> >                  from ../include/linux/kernel_stat.h:9,
> >                  from ../include/linux/cgroup.h:26,
> >                  from ../include/linux/perf_event.h:57,
> >                  from ../include/linux/trace_events.h:10,
> >                  from ../include/trace/syscall.h:7,
> >                  from ../include/linux/syscalls.h:84,
> >                  from ../init/main.c:21:
> > include/linux/irqnr.h:8:12: note: shadowed declaration is here
> >     8 | extern int nr_irqs;
> >       |            ^~~~~~~
> >
> > Rename the local in irqdomain.h to shut up those warnings
>
> That's lame tinkering. There are gazillion of other functions which have
> the very same problem and nr_irqs is used all over the place for local
> variables.
>
> So instead of trying to chase all these places we really want to rename
> the global 'nr_irqs' variable.

Fair enough, yes.

> Something like the uncompiled below which is purely mechanical and does
> not even try to look at some of the places which use it for the very
> wrong reasons and purpose * Shudder *.

Looks good to me.

If we rename it, we might want to pick an even longer name to make
it stand out in code review when it does get used, and easier to
grep for.

I don't have my build testing infrastructure running at the moment,
once I get that back up, I can start testing your patch.

> --- a/arch/arm/kernel/irq.c
> +++ b/arch/arm/kernel/irq.c
> @@ -100,7 +100,7 @@ void __init init_IRQ(void)
>  #ifdef CONFIG_SPARSE_IRQ
>  int __init arch_probe_nr_irqs(void)
>  {
> -       nr_irqs = machine_desc->nr_irqs ? machine_desc->nr_irqs : NR_IRQS;
> -       return nr_irqs;
> +       max_nr_irqs = machine_desc->nr_irqs ? machine_desc->nr_irqs : NR_IRQS;
> +       return max_nr_irqs;
>  }
>  #endif

I think this one can just lose the assignment and return the value.

> --- a/arch/powerpc/platforms/cell/axon_msi.c
> +++ b/arch/powerpc/platforms/cell/axon_msi.c
> @@ -111,7 +111,7 @@ static void axon_msi_cascade(struct irq_
>                 pr_devel("axon_msi: woff %x roff %x msi %x\n",
>                           write_offset, msic->read_offset, msi);
>
> -               if (msi < nr_irqs && irq_get_chip_data(msi) == msic) {
> +               if (msi < max_nr_irqs && irq_get_chip_data(msi) == msic) {
>                         generic_handle_irq(msi);
>                         msic->fifo_virt[idx] = cpu_to_le32(0xffffffff);
>                 } else {

Most of the ones like this seem to have been simply converted from
old sanity checks using NR_IRQS and would work just as well without
the checks.

Actually removing the checks would have a small regression
potential.

> --- a/drivers/pcmcia/at91_cf.c
> +++ b/drivers/pcmcia/at91_cf.c

potential.

> @@ -312,7 +312,7 @@ static int at91_cf_probe(struct platform
>                         goto fail0a;
>                 cf->socket.pci_irq = gpio_to_irq(board->irq_pin);
>         } else
> -               cf->socket.pci_irq = nr_irqs + 1;
> +               cf->socket.pci_irq = max_nr_irqs + 1;
>
>         /*
>          * pcmcia layer only remaps "real" memory not iospace

This one would seem to actually warrant a bugfix, setting
the field to zero.

      Arnd