Re: [PATCH] misc: rtsx: Set setting_reg2 before use.

From: Nathan Chancellor <nathan@kernel.org>
To: Nick Desaulniers <ndesaulniers@google.com>
Cc: Tom Rix <trix@redhat.com>,
	Stephen Rothwell <sfr@canb.auug.org.au>,
	arnd@arndb.de, gregkh@linuxfoundation.org, ricky_wu@realtek.com,
	kai.heng.feng@canonical.com, linux-kernel@vger.kernel.org,
	llvm@lists.linux.dev
Subject: Re: [PATCH] misc: rtsx: Set setting_reg2 before use.
Date: Mon, 16 May 2022 14:57:12 -0700	[thread overview]
Message-ID: <YoLIuKf/1OcRtjdX@dev-arch.thelio-3990X> (raw)
In-Reply-To: <CAKwvOd=-H+rvt83rgrR3GyXdSDe+n0aG8CFLEshij9jNAzCQtw@mail.gmail.com>

On Mon, May 16, 2022 at 02:22:50PM -0700, Nick Desaulniers wrote:
> On Mon, May 16, 2022 at 10:06 AM Tom Rix <trix@redhat.com> wrote:
> >
> >
> > On 5/16/22 8:56 AM, Nathan Chancellor wrote:
> > > On Mon, May 16, 2022 at 09:00:47AM -0400, Tom Rix wrote:
> > >> The clang build fails with
> > >> rts5261.c:406:13: error: variable 'setting_reg2' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
> > >>          } else if (efuse_valid == 0) {
> > >>                     ^~~~~~~~~~~~~~~~
> > >>
> > >> setting_reg2 is set in this block
> > >>    if (efuse_valid == 2 || efuse_valid == 3) {
> > >> ..
> > >>    } else if (efuse_valid == 0) {
> > >>      // default
> > >> ..
> > >>    }
> > >> But efuse_valid can also have a value of 1.
> > >> Change the 'else if' to 'else' to make the second block the default.
> > >>
> > >> Fixes: b1c5f3085149 ("misc: rtsx: add rts5261 efuse function")
> > >> Signed-off-by: Tom Rix <trix@redhat.com>
> > > I am not sure if this fix is correct from a functional standpoint (i.e.
> > > is treating efuse_valid == 1 the same as efuse_valid == 0 correct?) but
> > > it is better than not handling this value altogether. For what it's
> > > worth:
> >
> > I looked at how the code used to work, this seemed better than
> > initializing to NULL.
> >
> > >
> > > Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> > >
> > > As a side note, it is unfortunate that this change made it into -next
> > > when there was an outstanding report about this warning:
> >
> >  From the clang side, this is a build break and my static analysis infra
> > goes down.
> >
> > These build breaks seem to happening every week, is there a precommit
> > clang gating test that could be done for -next ?
> 
> Probably worth asking Stephen, though I don't think there's _any_
> gating (i.e. presubmit testing) for -next, since that'd increase the
> build capacity needed. -next is tested post-merge (i.e. post submit
> testing) IIUC.

Stephen does do build testing during the merges ("Between each merge,
the tree was built with a ppc64_defconfig for powerpc, an allmodconfig
for x86_64, a multi_v7_defconfig for arm and a native build of
tools/perf"), in addition to testing after the final tree is done.

https://lore.kernel.org/20220516205718.2c5a52f9@canb.auug.org.au/

Increasing the "during merge" testing is not a reasonable request (IMO)
due to the additional time it would add to the entire -next process.  I
am not sure that having Stephen do builds with clang post merge is any
different from all the existing build coverage that we already have (the
tree is already done at that point), other than maybe people listen to a
human complaining more than a robot... :)

Cheers,
Nathan