Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <Y1trhRE3nK5iAY6q@mail.google.com> <Y1yetX1CHsr+fibp@mail.google.com>
 <CADnq5_Mod90O=tN26+Yi74WPYxpVtss+LG_+_HZyFv2EtzR+MA@mail.gmail.com> <Y2GMEjB4NQ3RYk2C@mail.google.com>
In-Reply-To: <Y2GMEjB4NQ3RYk2C@mail.google.com>
From:   Alex Deucher <alexdeucher@gmail.com>
Date:   Tue, 1 Nov 2022 17:27:51 -0400
Message-ID: <CADnq5_MuYmxOExeuhQAZ_KU4jfOdFhy1Jn4QBuawuL2Tq64n7w@mail.gmail.com>
Subject: Re: [PATCH v2] [next] drm/radeon: Replace one-element array with
 flexible-array member
To:     Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
Cc:     Alex Deucher <alexander.deucher@amd.com>,
        =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>,
        "Pan, Xinhui" <Xinhui.Pan@amd.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>, amd-gfx@lists.freedesktop.org,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        linux-hardening@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Nov 1, 2022 at 5:14 PM Paulo Miguel Almeida
<paulo.miguel.almeida.rodenas@gmail.com> wrote:
>
> On Tue, Nov 01, 2022 at 10:42:14AM -0400, Alex Deucher wrote:
> > On Fri, Oct 28, 2022 at 11:32 PM Paulo Miguel Almeida
> > <paulo.miguel.almeida.rodenas@gmail.com> wrote:
> > >
> > > One-element arrays are deprecated, and we are replacing them with
> > > flexible array members instead. So, replace one-element array with
> > > flexible-array member in struct _ATOM_FAKE_EDID_PATCH_RECORD and
> > > refactor the rest of the code accordingly.
> > >
> > > It's worth mentioning that doing a build before/after this patch results
> > > in no binary output differences.
> > >
> > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE
> > > routines on memcpy() and help us make progress towards globally
> > > enabling -fstrict-flex-arrays=3 [1].
> >
> > This seems like a worthy goal, and I'm not opposed to the patch per
> > se, but it seems a bit at odds with what this header represents.
> > atombios.h represents the memory layout of the data stored in the ROM
> > on the GPU.  This changes the memory layout of that ROM.  We can work
> > around that in the driver code, but if someone were to take this
> > header to try and write some standalone tool or use it for something
> > else in the kernel, it would not reflect reality.
> >
> > Alex
> >
> Hi Alex, thanks for taking the time to review this patch.
>
> I see where you are coming from and why you may be apprehensive with the
> approach. From my humble point of view, I think that the artificial line
> that separates "what we can change at will" and "what we should be extra
> careful not to break/confuse others" is whether the header file is part
> of the UAPI. Given that atombios.h isn't publicly accessible, I tend to
> gravitate towards the first one.

It may not be publicly accessible, but it describes a physical thing
that is burned into millions of GPU boards out in the wild.  There are
tons of open source tools out there that take these headers from the
kernel to be able to parse the date in the ROM on the GPU.  If those
applications sync up with the latest version of the header from the
kernel, it will break their tools.  The next time someone from AMD
syncs up the header with the version maintained by the vbios team, the
change could accidently sneak back in and break the code.  It seems to
me in this particular case that the header should reflect the physical
layout of the ROM since that is what it describes.

Alex

>
> > > +  /* empty fake edid record must be 3 bytes long */
> > +    sizeof(ATOM_FAKE_EDID_PATCH_RECORD) + 1;
>
> I am assuming that this is the part of the patch that makes you feel
> concerned about how devs will get it (should they copy this header),
> is that right?
>
> If so, would a comment on the ATOM_FAKE_EDID_PATCH_RECORD struct
> specifying the size of the struct when empty do the trick?
>
> - Paulo A.
>