linux-kernel - Re: [PATCH] mmc: sdhci-of-arasan: Remove uninitialized ret variables

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:   Thu, 16 Apr 2020 14:57:37 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Nathan Chancellor <natechancellor@...il.com>
Cc:     Adrian Hunter <adrian.hunter@...el.com>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Michal Simek <michal.simek@...inx.com>,
        Manish Narani <manish.narani@...inx.com>,
        linux-mmc@...r.kernel.org,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        clang-built-linux <clang-built-linux@...glegroups.com>,
        "kernelci . org bot" <bot@...nelci.org>
Subject: Re: [PATCH] mmc: sdhci-of-arasan: Remove uninitialized ret variables

On Thu, Apr 16, 2020 at 1:24 PM Nathan Chancellor
<natechancellor@...il.com> wrote:
>
> On Thu, Apr 16, 2020 at 01:16:27PM -0700, Nick Desaulniers wrote:
> > On Thu, Apr 16, 2020 at 11:24 AM Nathan Chancellor
> > <natechancellor@...il.com> wrote:
> > >
> > > Clang warns:
> > >
> > > drivers/mmc/host/sdhci-of-arasan.c:784:9: warning: variable 'ret' is
> > > uninitialized when used here [-Wuninitialized]
> > >         return ret;
> > >                ^~~
> > > drivers/mmc/host/sdhci-of-arasan.c:738:9: note: initialize the variable
> > > 'ret' to silence this warning
> > >         int ret;
> > >                ^
> > >                 = 0
> > > drivers/mmc/host/sdhci-of-arasan.c:860:9: warning: variable 'ret' is
> > > uninitialized when used here [-Wuninitialized]
> > >         return ret;
> > >                ^~~
> > > drivers/mmc/host/sdhci-of-arasan.c:810:9: note: initialize the variable
> > > 'ret' to silence this warning
> > >         int ret;
> > >                ^
> > >                 = 0
> > > 2 warnings generated.
> > >
> > > This looks like a copy paste error. Neither function has handling that
> > > needs ret so just remove it and return 0 directly.
> >
> > Forgive me for not taking the time to look into this more carefully,
> > but just a thought:
> >
> > Having functions always return a single integer literal as opposed to
> > having a `void` return type in their function signature is a code
> > smell.  Did you consider the call sites of these functions to see if
> > they do anything with the return value?  I understand it may not be
> > worthwhile/possible if these functions fulfil an interface that
> > requires the int return type function signature.  (It's also probably
>
> Which is the case. These functions are passed to 'struct clk_ops', which
> defines the set_phase member as
>
> int     (*set_phase)(struct clk_hw *hw, int degrees);
>
> so we cannot just change the return to void since there are other
> set_phase functions that do set a return value other than zero.
>
> > faster for me to just look rather than type this all out, but I saw no
> > mention of this consideration in the commit message or patch, so
> > wanted to check that it had been performed).
>
> Yeah, I should have probably mentioned that. I can do so if the
> maintainers feel it worthwhile.

No worries, I should hold my comments until I can give a more thorough
review, which I've now done. LGTM and thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@...gle.com>


-- 
Thanks,
~Nick Desaulniers