Message-ID: <CADnq5_OWUn3Y2RA68pT-Sw1yRKSY0Eqtz=TAoPOXZ5V-KY5EWA@mail.gmail.com>
Date:   Tue, 6 Aug 2019 10:10:25 -0400
From:   Alex Deucher <alexdeucher@...il.com>
To:     "Quan, Evan" <Evan.Quan@....com>
Cc:     Nathan Chancellor <natechancellor@...il.com>,
        "Deucher, Alexander" <Alexander.Deucher@....com>,
        "Koenig, Christian" <Christian.Koenig@....com>,
        "Zhou, David(ChunMing)" <David1.Zhou@....com>,
        "clang-built-linux@...glegroups.com" 
        <clang-built-linux@...glegroups.com>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "amd-gfx@...ts.freedesktop.org" <amd-gfx@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/amd/powerplay: Zero initialize some variables

Applied.  Thanks!

Alex

On Sun, Aug 4, 2019 at 9:21 PM Quan, Evan <Evan.Quan@....com> wrote:
>
> Thanks Nathan. The patch is reviewed-by: Evan Quan <evan.quan@....com>
>
> > -----Original Message-----
> > From: Nathan Chancellor <natechancellor@...il.com>
> > Sent: Monday, August 05, 2019 4:37 AM
> > To: Quan, Evan <Evan.Quan@....com>; Deucher, Alexander
> > <Alexander.Deucher@....com>; Koenig, Christian
> > <Christian.Koenig@....com>; Zhou, David(ChunMing)
> > <David1.Zhou@....com>
> > Cc: amd-gfx@...ts.freedesktop.org; dri-devel@...ts.freedesktop.org; linux-
> > kernel@...r.kernel.org; clang-built-linux@...glegroups.com; Nathan
> > Chancellor <natechancellor@...il.com>
> > Subject: [PATCH] drm/amd/powerplay: Zero initialize some variables
> >
> > Clang warns (only Navi warning shown but Arcturus warns as well):
> >
> > drivers/gpu/drm/amd/amdgpu/../powerplay/navi10_ppt.c:1534:4: warning:
> > variable 'asic_default_power_limit' is used uninitialized whenever '?:'
> > condition is false [-Wsometimes-uninitialized]
> >                         smu_read_smc_arg(smu, &asic_default_power_limit);
> >                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/gpu/drm/amd/amdgpu/../powerplay/inc/amdgpu_smu.h:588:3:
> > note:
> > expanded from macro 'smu_read_smc_arg'
> >         ((smu)->funcs->read_smc_arg? (smu)->funcs->read_smc_arg((smu),
> > (arg)) : 0)
> >          ^~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/gpu/drm/amd/amdgpu/../powerplay/navi10_ppt.c:1550:30: note:
> > uninitialized use occurs here
> >                 smu->default_power_limit = asic_default_power_limit;
> >                                            ^~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/gpu/drm/amd/amdgpu/../powerplay/navi10_ppt.c:1534:4: note:
> > remove the '?:' if its condition is always true
> >                         smu_read_smc_arg(smu, &asic_default_power_limit);
> >                         ^
> > drivers/gpu/drm/amd/amdgpu/../powerplay/inc/amdgpu_smu.h:588:3:
> > note:
> > expanded from macro 'smu_read_smc_arg'
> >         ((smu)->funcs->read_smc_arg? (smu)->funcs->read_smc_arg((smu),
> > (arg)) : 0)
> >          ^
> > drivers/gpu/drm/amd/amdgpu/../powerplay/navi10_ppt.c:1517:35: note:
> > initialize the variable 'asic_default_power_limit' to silence this warning
> >         uint32_t asic_default_power_limit;
> >                                          ^
> >                                           = 0
> > 1 warning generated.
> >
> > As the code is currently written, if read_smc_arg were ever NULL, arg would
> > fail to be initialized but the code would continue executing as normal
> > because the return value would just be zero.
> >
> > There are a few different possible solutions to resolve this class of warnings
> > which have appeared in these drivers before:
> >
> > 1. Assume the function pointer will never be NULL and eliminate the
> >    wrapper macros.
> >
> > 2. Have the wrapper macros initialize arg when the function pointer is
> >    NULL.
> >
> > 3. Have the wrapper macros return an error code instead of 0 when the
> >    function pointer is NULL so that the callsites can properly bail out
> >    before arg can be used.
> >
> > 4. Initialize arg at the top of its function.
> >
> > Number four is the path of least resistance right now as every other change
> > will be driver wide so do that here. I only make the comment now as food for
> > thought.
> >
> > Fixes: b4af964e75c4 ("drm/amd/powerplay: make power limit retrieval as
> > asic specific")
> > Link: https://github.com/ClangBuiltLinux/linux/issues/627
> > Signed-off-by: Nathan Chancellor <natechancellor@...il.com>
> > ---
> >  drivers/gpu/drm/amd/powerplay/arcturus_ppt.c | 2 +-
> >  drivers/gpu/drm/amd/powerplay/navi10_ppt.c   | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> > b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> > index 215f7173fca8..b92eded7374f 100644
> > --- a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> > +++ b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> > @@ -1326,7 +1326,7 @@ static int arcturus_get_power_limit(struct
> > smu_context *smu,
> >                                    bool asic_default)
> >  {
> >       PPTable_t *pptable = smu->smu_table.driver_pptable;
> > -     uint32_t asic_default_power_limit;
> > +     uint32_t asic_default_power_limit = 0;
> >       int ret = 0;
> >       int power_src;
> >
> > diff --git a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
> > b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
> > index 106352a4fb82..d844bc8411aa 100644
> > --- a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
> > +++ b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
> > @@ -1514,7 +1514,7 @@ static int navi10_get_power_limit(struct
> > smu_context *smu,
> >                                    bool asic_default)
> >  {
> >       PPTable_t *pptable = smu->smu_table.driver_pptable;
> > -     uint32_t asic_default_power_limit;
> > +     uint32_t asic_default_power_limit = 0;
> >       int ret = 0;
> >       int power_src;
> >
> > --
> > 2.23.0.rc1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Powered by blists - more mailing lists