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: <Y2tXzlBDxgg9WMl7@mail.google.com>
In-Reply-To: <Y2tXzlBDxgg9WMl7@mail.google.com>
From:   Alex Deucher <alexdeucher@gmail.com>
Date:   Thu, 10 Nov 2022 10:57:58 -0500
Message-ID: <CADnq5_NJNa41y28-+PPjA6hUPrYGXc87K50LeiEYGnyGyackUQ@mail.gmail.com>
Subject: Re: [PATCH] [next] drm/amdgpu: Replace one-element array with
 flex-array member
To:     Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
Cc:     Harry Wentland <harry.wentland@amd.com>,
        Leo Li <sunpeng.li@amd.com>,
        Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>,
        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>,
        Rongguang Wei <weirongguang@kylinos.cn>,
        Slark Xiao <slark_xiao@163.com>, 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 Wed, Nov 9, 2022 at 2:33 AM 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 structs _ATOM_CONNECTOR_DEVICE_TAG_RECORD,
> _ATOM_OBJECT_GPIO_CNTL_RECORD, _ATOM_BRACKET_LAYOUT_RECORD,
> _ATOM_BRACKET_LAYOUT_RECORD, _ATOM_LEAKAGE_VOLTAGE_OBJECT_V3,
> _ATOM_FUSION_SYSTEM_INFO_V3, _ATOM_I2C_DATA_RECORD,
> _ATOM_I2C_DEVICE_SETUP_INFO, _ATOM_ASIC_MVDD_INFO and refactor the
> rest of the code accordingly. While at it, removed a redundant casting.
>
> Important to mention is 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].
>
> Link: https://github.com/KSPP/linux/issues/79
> Link: https://github.com/KSPP/linux/issues/238
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 [1]
>
> Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
> ---
>
> Alex, I noticed a few structs in atombios.h that were not referenced. Is
> there any appetite for removing them? Or is that one of those cases
> where the structs are there should one driver ever need it?

A lot of userspace tools use these tables and I'd like to keep the
headers compatible.  Also, as I mentioned in the other thread, the
atombios command tables often parse these tables when initializing the
hardware so when you are debugging a command table, it helps to have
the relevant structures defined so it's easier to understand what the
command tables are doing.

Alex


>
> Ex.:
>         struct _ATOM_I2C_DATA_RECORD
>         struct _ATOM_I2C_DEVICE_SETUP_INFO
>         struct _ATOM_ASIC_MVDD_INFO
> ---
>  .../gpu/drm/amd/display/dc/bios/bios_parser.c    |  5 ++---
>  drivers/gpu/drm/amd/include/atombios.h           | 16 ++++++++--------
>  2 files changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/bios/bios_parser.c b/drivers/gpu/drm/amd/display/dc/bios/bios_parser.c
> index 39dd8b2dc254..6b9e64cd4379 100644
> --- a/drivers/gpu/drm/amd/display/dc/bios/bios_parser.c
> +++ b/drivers/gpu/drm/amd/display/dc/bios/bios_parser.c
> @@ -2606,8 +2606,7 @@ static enum bp_result update_slot_layout_info(
>
>         for (;;) {
>
> -               record_header = (ATOM_COMMON_RECORD_HEADER *)
> -                       GET_IMAGE(ATOM_COMMON_RECORD_HEADER, record_offset);
> +               record_header = GET_IMAGE(ATOM_COMMON_RECORD_HEADER, record_offset);
>                 if (record_header == NULL) {
>                         result = BP_RESULT_BADBIOSTABLE;
>                         break;
> @@ -2621,7 +2620,7 @@ static enum bp_result update_slot_layout_info(
>
>                 if (record_header->ucRecordType ==
>                         ATOM_BRACKET_LAYOUT_RECORD_TYPE &&
> -                       sizeof(ATOM_BRACKET_LAYOUT_RECORD)
> +                       struct_size(record, asConnInfo, 1)
>                         <= record_header->ucRecordSize) {
>                         record = (ATOM_BRACKET_LAYOUT_RECORD *)
>                                 (record_header);
> diff --git a/drivers/gpu/drm/amd/include/atombios.h b/drivers/gpu/drm/amd/include/atombios.h
> index 55ae93c1e365..60c44a8a067f 100644
> --- a/drivers/gpu/drm/amd/include/atombios.h
> +++ b/drivers/gpu/drm/amd/include/atombios.h
> @@ -4733,7 +4733,7 @@ typedef struct  _ATOM_CONNECTOR_DEVICE_TAG_RECORD
>    ATOM_COMMON_RECORD_HEADER   sheader;
>    UCHAR                       ucNumberOfDevice;
>    UCHAR                       ucReserved;
> -  ATOM_CONNECTOR_DEVICE_TAG   asDeviceTag[1];         //This Id is same as "ATOM_DEVICE_XXX_SUPPORT", 1 is only for allocation
> +  ATOM_CONNECTOR_DEVICE_TAG   asDeviceTag[];          //This Id is same as "ATOM_DEVICE_XXX_SUPPORT"
>  }ATOM_CONNECTOR_DEVICE_TAG_RECORD;
>
>
> @@ -4793,7 +4793,7 @@ typedef struct  _ATOM_OBJECT_GPIO_CNTL_RECORD
>    ATOM_COMMON_RECORD_HEADER   sheader;
>    UCHAR                       ucFlags;                // Future expnadibility
>    UCHAR                       ucNumberOfPins;         // Number of GPIO pins used to control the object
> -  ATOM_GPIO_PIN_CONTROL_PAIR  asGpio[1];              // the real gpio pin pair determined by number of pins ucNumberOfPins
> +  ATOM_GPIO_PIN_CONTROL_PAIR  asGpio[];               // the real gpio pin pair determined by number of pins ucNumberOfPins
>  }ATOM_OBJECT_GPIO_CNTL_RECORD;
>
>  //Definitions for GPIO pin state
> @@ -4982,7 +4982,7 @@ typedef struct  _ATOM_BRACKET_LAYOUT_RECORD
>    UCHAR                       ucWidth;
>    UCHAR                       ucConnNum;
>    UCHAR                       ucReserved;
> -  ATOM_CONNECTOR_LAYOUT_INFO  asConnInfo[1];
> +  ATOM_CONNECTOR_LAYOUT_INFO  asConnInfo[];
>  }ATOM_BRACKET_LAYOUT_RECORD;
>
>
> @@ -5161,7 +5161,7 @@ typedef struct  _ATOM_GPIO_VOLTAGE_OBJECT_V3
>     UCHAR  ucPhaseDelay;                      // phase delay in unit of micro second
>     UCHAR  ucReserved;
>     ULONG  ulGpioMaskVal;                     // GPIO Mask value
> -   VOLTAGE_LUT_ENTRY_V2 asVolGpioLut[1];
> +   VOLTAGE_LUT_ENTRY_V2 asVolGpioLut[];
>  }ATOM_GPIO_VOLTAGE_OBJECT_V3;
>
>  typedef struct  _ATOM_LEAKAGE_VOLTAGE_OBJECT_V3
> @@ -5171,7 +5171,7 @@ typedef struct  _ATOM_LEAKAGE_VOLTAGE_OBJECT_V3
>     UCHAR    ucLeakageEntryNum;               // indicate the entry number of LeakageId/Voltage Lut table
>     UCHAR    ucReserved[2];
>     ULONG    ulMaxVoltageLevel;
> -   LEAKAGE_VOLTAGE_LUT_ENTRY_V2 asLeakageIdLut[1];
> +   LEAKAGE_VOLTAGE_LUT_ENTRY_V2 asLeakageIdLut[];
>  }ATOM_LEAKAGE_VOLTAGE_OBJECT_V3;
>
>
> @@ -6599,7 +6599,7 @@ typedef struct _ATOM_FUSION_SYSTEM_INFO_V3
>  typedef struct _ATOM_I2C_DATA_RECORD
>  {
>    UCHAR         ucNunberOfBytes;                                              //Indicates how many bytes SW needs to write to the external ASIC for one block, besides to "Start" and "Stop"
> -  UCHAR         ucI2CData[1];                                                 //I2C data in bytes, should be less than 16 bytes usually
> +  UCHAR         ucI2CData[];                                                  //I2C data in bytes, should be less than 16 bytes usually
>  }ATOM_I2C_DATA_RECORD;
>
>
> @@ -6610,14 +6610,14 @@ typedef struct _ATOM_I2C_DEVICE_SETUP_INFO
>    UCHAR                              ucSSChipID;             //SS chip being used
>    UCHAR                              ucSSChipSlaveAddr;      //Slave Address to set up this SS chip
>    UCHAR                           ucNumOfI2CDataRecords;  //number of data block
> -  ATOM_I2C_DATA_RECORD            asI2CData[1];
> +  ATOM_I2C_DATA_RECORD            asI2CData[];
>  }ATOM_I2C_DEVICE_SETUP_INFO;
>
>  //==========================================================================================
>  typedef struct  _ATOM_ASIC_MVDD_INFO
>  {
>    ATOM_COMMON_TABLE_HEADER         sHeader;
> -  ATOM_I2C_DEVICE_SETUP_INFO      asI2CSetup[1];
> +  ATOM_I2C_DEVICE_SETUP_INFO      asI2CSetup[];
>  }ATOM_ASIC_MVDD_INFO;
>
>  //==========================================================================================
> --
> 2.37.3
>