Message-ID: <CAL_JsqJYPJVTxH=d5nYK78+ZWhZAfh4VhEyBLqzyg4MzMhrX0g@mail.gmail.com>
Date:   Wed, 20 Sep 2023 07:44:04 -0500
From:   Rob Herring <robh@...nel.org>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     kernel test robot <lkp@...el.com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        oe-kbuild-all@...ts.linux.dev, linux-kernel@...r.kernel.org,
        Wim Van Sebroeck <wim@...ux-watchdog.org>
Subject: Re: arch/m68k/include/asm/raw_io.h:91:13: warning: array subscript 0
 is outside array bounds of 'volatile u16[0]' {aka 'volatile short unsigned int[]'}

On Tue, Sep 19, 2023 at 6:14 PM Guenter Roeck <linux@...ck-us.net> wrote:
>
> On 9/19/23 11:37, Rob Herring wrote:
> > On Tue, Sep 19, 2023 at 7:09 AM kernel test robot <lkp@...el.com> wrote:
> >>
> >> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> >> head:   2cf0f715623872823a72e451243bbf555d10d032
> >> commit: f1a43aadb5a690e141a3b6700e2a40c1d4dbe088 watchdog: Enable COMPILE_TEST for more drivers
> >> date:   5 weeks ago
> >> config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230919/202309192013.vI4DKHmw-lkp@intel.com/config)
> >> compiler: m68k-linux-gcc (GCC) 13.2.0
> >> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230919/202309192013.vI4DKHmw-lkp@intel.com/reproduce)
> >>
> >> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> >> the same patch/commit), kindly add following tags
> >> | Reported-by: kernel test robot <lkp@...el.com>
> >> | Closes: https://lore.kernel.org/oe-kbuild-all/202309192013.vI4DKHmw-lkp@intel.com/
> >>
> >> All warnings (new ones prefixed by >>):
> >>
> >>     In file included from arch/m68k/include/asm/io_mm.h:25,
> >>                      from arch/m68k/include/asm/io.h:8,
> >>                      from include/linux/io.h:13,
> >>                      from drivers/watchdog/machzwd.c:39:
> >>     In function 'zf_set_timer',
> >>         inlined from 'zf_timer_on' at drivers/watchdog/machzwd.c:218:2:
> >>>> arch/m68k/include/asm/raw_io.h:91:13: warning: array subscript 0 is outside array bounds of 'volatile u16[0]' {aka 'volatile short unsigned int[]'} [-Warray-bounds=]
> >>        91 |         __w = ((*(__force volatile u16 *) ((_addr & 0xFFFF0000UL) + ((__v >> 8)<<1)))); \
> >>           |         ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>     arch/m68k/include/asm/io_mm.h:228:20: note: in expansion of macro 'rom_out_le16'
> >>       228 |                  : rom_out_le16(isa_itw(port), (val)))
> >>           |                    ^~~~~~~~~~~~
> >>     arch/m68k/include/asm/io_mm.h:356:42: note: in expansion of macro 'isa_rom_outw'
> >>       356 | #define outw(val, port) ((port) < 1024 ? isa_rom_outw((val), (port)) : out_le16((port), (val)))
> >>           |                                          ^~~~~~~~~~~~
> >>     drivers/watchdog/machzwd.c:74:53: note: in expansion of macro 'outw'
> >>        74 | #define zf_writew(port, data)  { outb(port, INDEX); outw(data, DATA_W); }
> >>           |                                                     ^~~~
> >>     drivers/watchdog/machzwd.c:173:17: note: in expansion of macro 'zf_writew'
> >>       173 |                 zf_writew(COUNTER_1, new);
> >>           |                 ^~~~~~~~~
> >>     In function 'zf_timer_on':
> >>     cc1: note: source object is likely at address zero
> >
> > This seems to be some newish check in gcc which looks for fixed
> > pointers below 4KB[1]. The linked issue says more was planned for
> > gcc-13, but I haven't found what that is. AFAICT, that shouldn't
> > happen with this config because isa_itw() should be variable and the
> > compiler shouldn't be able to determine the value of _addr. However, a
> > config with CONFIG_Q40=n, CONFIG_AMIGA_PCMCIA=n, and
> > CONFIG_ATARI_ROM_ISA=n would have a fixed NULL value and could trigger
> > the warning. This should also have warnings everywhere outw() (and
> > others) are used with a constant port value.
> >
> > Rob
> >
> > [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
>
> A long time ago, when someone submitted a "cleanup: patch for the machzwd
> watchdog driver, I approved it but added this comment.
>
>  > If anyone is still using the HW supported by this driver, it would
>  > be a prime target for conversion to use the watchdog subsystem.
>  > This would reduce code size and improve driver stability.
>  > _That_ would be useful.
>
>  > The only reason for replacing 0 with NULL is to make a static checker
>  > happy. This kind of change adds zero value to the code. Instead, it
>  > takes time from those who have to review the patches and apply them.
>
>  > If we ignore such patches, we'll get them again and again, taking
>  > away even more time. If we don't ignore them, we encourage submitters
>  > and get even more useless patches. If we try to discourage them, we
>  > risk being accused of being unfriendly. This is a perfect lose-lose
>  > situation for maintainers.

Agreed. I keep getting one to fix "of of" in a comment. The fix is
always to drop an "of", but that's actually wrong because it's
supposed to be "OF". I keep pointing this out and *never* get the
right fix. I don't fix it myself because at this point, I find
continuing to get "fixes" entertaining.

> I do wonder if enabling BUILD_TEST on such drivers is any better.

I think that and trivial fixes on a specific driver are completely
different. If you want to test build one driver, that's not too hard.
Find its kconfig value, turn it on and build the right arch. For tree
wide stuff, it's a real pain to ensure you built everything. For
example, the only way to build many Cavium Octeon drivers is with the
Octeon defconfig which is just one of dozens configs for MIPS (which
is still a bigger mess than arm32 ever was).

Rob

Powered by blists - more mailing lists