Message-ID: <CAK8P3a3GqsXcdA59V7XGd_yFr_68yCaftdc-wMM6bHG8NEE1+g@mail.gmail.com>
Date:   Tue, 27 Oct 2020 17:22:48 +0100
From:   Arnd Bergmann <arnd@...nel.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Waiman Long <longman@...hat.com>,
        Matthew Wilcox <willy@...radead.org>,
        Ingo Molnar <mingo@...nel.org>, Will Deacon <will@...nel.org>,
        Will Deacon <will.deacon@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Nicholas Piggin <npiggin@...il.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        linux-arch <linux-arch@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] qspinlock: use signed temporaries for cmpxchg

On Tue, Oct 27, 2020 at 11:32 AM Peter Zijlstra <peterz@...radead.org> wrote:
>
> On Tue, Oct 27, 2020 at 09:33:32AM +0100, Arnd Bergmann wrote:
> > On Tue, Oct 27, 2020 at 8:47 AM Peter Zijlstra <peterz@...radead.org> wrote:
> > > On Mon, Oct 26, 2020 at 02:03:06PM -0400, Waiman Long wrote:
> > > > On 10/26/20 12:57 PM, Arnd Bergmann wrote:
> > > > Yes, it shouldn't really matter if the value is defined as int or u32.
> > > > However, the only caveat that I see is queued_spin_lock_slowpath() is
> > > > expecting a u32 argument. Maybe you should cast it back to (u32) when
> > > > calling it.
> > >
> > > No, we're not going to confuse the code. That stuff is hard enough as it
> > > is. This warning is garbage and just needs to stay off.
> >
> > Ok, so the question then becomes: should we drop -Wpointer-sign from
> > W=2 and move it to W=3, or instead disable it locally. I could add
> > __diag_ignore(GCC, 4, "-Wpointer-sign") in the couple of header files
> > that produce this kind of warning if there is a general feeling that it
> > still helps to have this for drivers.
>
> What is an actual geniune bug that this warning helps find?

I've gone through the git history to find something mentioning this
warning, but there was no evidence of a real bug that could
have been prevented with this warning, and lots of work wasted
on shutting up the compiler.

The best case was
https://lore.kernel.org/lkml/20201026213040.3889546-6-arnd@kernel.org/
where changing the types led to also making it 'const'.

> If you add that __diag_ignore() it should go in atomic.h I suppose,
> because all of atomic hard relies on this, and then the question becomes
> how much code is left that doesn't include that header and consequently
> doesn't ignore that warning.

I don't think it would work: the __diag_ignore() has to be in the caller,
not the function that is called.

> So, is it useful to begin with in finding actual problems? and given we
> have to annotate away a bucket-load, how much coverage will there remain
> if we squish the known false-positives?

In an x86 allmodconfig build, I see 113618 -Wpointer-sign warnings, 68318
of those in qspinlock.h and qrwlock.h. With the six patches I sent, the
total number goes down to 15201, which of course is still fairly pointless
to go through. Almost all of these are in drivers that have less than
10 warnings, and few of them are in headers included by other drivers.

I looked at the top remaining files, but couldn't find any actual bugs there.
If there are real bugs, they are certainly hard to find among the
false positives.

I have already sent patches to move -Wnested-externs and
-Wcast-align from W=2 to W=3, and I guess -Wpointer-sign
could be handled the same way to make the W=2 level useful
again.

      Arnd

----
   1764 ../drivers/staging/rtl8723bs/hal/HalHWImg8723B_RF.c
    810 ../drivers/net/wireless/ath/ath11k/debugfs_htt_stats.c
    411 ../include/linux/moduleparam.h
    230 ../drivers/net/ethernet/neterion/vxge/vxge-ethtool.h
    184 ../include/linux/nls.h
    150 ../drivers/scsi/esas2r/esas2r.h
    146 ../include/net/tls.h
    144 ../sound/soc/codecs/wm5100.c
    135 ../drivers/scsi/ufs/ufs-sysfs.c
    130 ../include/sound/hda_regmap.h
    125 ../drivers/scsi/myrs.c
    121 ../include/linux/fscrypt.h
    113 ../drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
    105 ../drivers/net/wireless/ath/ath9k/hw.h
     81 ../drivers/staging/media/allegro-dvt/nal-h264.c
     75 ../drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
     68 ../drivers/scsi/esas2r/esas2r_init.c
     56 ../sound/soc/sof/ipc.c
     56 ../drivers/staging/qlge/qlge_dbg.c
     50 ../sound/usb/mixer.c
     50 ../drivers/net/ethernet/brocade/bna/bnad_ethtool.c
     50 ../drivers/isdn/capi/capiutil.c
     47 ../fs/nfs/internal.h
     46 ../drivers/scsi/esas2r/esas2r_int.c
     45 ../drivers/scsi/qla2xxx/qla_init.c
     44 ../drivers/platform/x86/sony-laptop.c
     44 ../drivers/lightnvm/pblk.h
     43 ../drivers/staging/rtl8723bs/hal/HalBtc8723b1Ant.c
     43 ../drivers/media/pci/cx25821/cx25821-medusa-video.c
     42 ../sound/pci/au88x0/au88x0_core.c

Powered by blists - more mailing lists