linux-kernel - Re: [PATCH -next 0/8] drm/amd/amdgpu: fix comparison pointer to bool warning

Open Source and information security mailing list archives

Message-ID: <CADnq5_OS814FxmadZ9yBkrY6ije8h7FxswWstsxNr3rw=Bum=Q@mail.gmail.com>
Date:   Wed, 16 Sep 2020 09:36:28 -0400
From:   Alex Deucher <alexdeucher@...il.com>
To:     Christian Koenig <christian.koenig@....com>,
        Ville Syrjälä <ville.syrjala@...ux.intel.com>,
        Alex Deucher <alexdeucher@...il.com>, yi.zhang@...wei.com,
        Dave Airlie <airlied@...ux.ie>,
        LKML <linux-kernel@...r.kernel.org>,
        amd-gfx list <amd-gfx@...ts.freedesktop.org>,
        Zheng Bin <zhengbin13@...wei.com>,
        Maling list - DRI developers 
        <dri-devel@...ts.freedesktop.org>,
        "Deucher, Alexander" <alexander.deucher@....com>
Subject: Re: [PATCH -next 0/8] drm/amd/amdgpu: fix comparison pointer to bool warning

On Wed, Sep 16, 2020 at 3:51 AM Daniel Vetter <daniel@...ll.ch> wrote:
>
> On Wed, Sep 16, 2020 at 09:38:34AM +0200, Christian König wrote:
> > Am 15.09.20 um 21:35 schrieb Ville Syrjälä:
> > > On Tue, Sep 15, 2020 at 03:16:32PM -0400, Alex Deucher wrote:
> > > > I question the value of these warnings.  Why even have a boolean type
> > > > if you are going to get warnings when you use them...
> > > > That said, applied to avoid getting these patches again and again
> > > > every time someone sees this.
> > > if (this_is_sparta)
> > > if (this_is_sparta == true)
> > > if (this_is_sparta != false)
> > >
> > > I think the first one reads the best, and avoids having to
> > > decide between truth and falsehood :)
> >
> > +1
>
> +1, especially because we also have the inversion when using negative
> errno codes for failures and 0 as success, which results in
>
>         if (errno == 0) /* success case */
>
> but
>         if (bool == 0) /* failure case */
>
> now creative people do sometimes
>
>         if (!errno) /* success case */
>
> which I think is horribly confusing. So imo for more easier telling apart
> of these too I think consistently using the short form for booleans, and
> consistently using the more explicit long form for errno checks is a Very
> Good Pattern :-)

I don't disagree with your logic, but we regularly get patches to
convert errno checks to drop the direct comparison because that is the
"preferred kernel style".  Arguably, we should be explicit in all
cases as that avoids all confusion.  With that in mind, my original
point stands.  Why have a type when comparisons against valid settings
for that type produce errors?

Alex

>
> Cheers, Daniel
>
> >
> > Christian.
> >
> > >
> > > > Alex
> > > >
> > > > On Wed, Sep 9, 2020 at 9:21 AM Christian König <christian.koenig@....com> wrote:
> > > > > Acked-by: Christian König <christian.koenig@....com> for the series.
> > > > >
> > > > > Am 09.09.20 um 15:07 schrieb Zheng Bin:
> > > > > > Zheng Bin (8):
> > > > > >     drm/amd/amdgpu: fix comparison pointer to bool warning in gfx_v9_0.c
> > > > > >     drm/amd/amdgpu: fix comparison pointer to bool warning in gfx_v10_0.c
> > > > > >     drm/amd/amdgpu: fix comparison pointer to bool warning in sdma_v5_0.c
> > > > > >     drm/amd/amdgpu: fix comparison pointer to bool warning in sdma_v5_2.c
> > > > > >     drm/amd/amdgpu: fix comparison pointer to bool warning in si.c
> > > > > >     drm/amd/amdgpu: fix comparison pointer to bool warning in uvd_v6_0.c
> > > > > >     drm/amd/amdgpu: fix comparison pointer to bool warning in
> > > > > >       amdgpu_atpx_handler.c
> > > > > >     drm/amd/amdgpu: fix comparison pointer to bool warning in sdma_v4_0.c
> > > > > >
> > > > > >    drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c | 4 ++--
> > > > > >    drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c           | 2 +-
> > > > > >    drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c            | 2 +-
> > > > > >    drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c           | 4 ++--
> > > > > >    drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c           | 2 +-
> > > > > >    drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c           | 2 +-
> > > > > >    drivers/gpu/drm/amd/amdgpu/si.c                  | 2 +-
> > > > > >    drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c            | 4 ++--
> > > > > >    8 files changed, 11 insertions(+), 11 deletions(-)
> > > > > >
> > > > > > --
> > > > > > 2.26.0.106.g9fadedd
> > > > > >
> > > > > _______________________________________________
> > > > > amd-gfx mailing list
> > > > > amd-gfx@...ts.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@...ts.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@...ts.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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