Re: [PATCH] drm/amd/pm: replace 1-element arrays with flexible-array members
On Thu, Nov 9, 2023 at 7:14 AM José Pekkarinen
wrote:
>
> On 2023-11-09 11:06, Greg KH wrote:
> > On Thu, Nov 09, 2023 at 10:43:50AM +0200, José Pekkarinen wrote:
> >> On 2023-11-08 09:29, Greg KH wrote:
> >> > On Wed, Nov 08, 2023 at 08:54:35AM +0200, José Pekkarinen wrote:
> >> > > The following case seems to be safe to be replaced with a flexible
> >> > > array
> >> > > to clean up the added coccinelle warning. This patch will just do it.
> >> > >
> >> > > drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h:76:38-63:
> >> > > WARNING use flexible-array member instead
> >> > > (https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays)
> >> > >
> >> > > Signed-off-by: José Pekkarinen
> >> > > ---
> >> > > drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h | 2 +-
> >> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> >> > >
> >> > > diff --git a/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h
> >> > > b/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h
> >> > > index c7b61222d258..1ce4087005f0 100644
> >> > > --- a/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h
> >> > > +++ b/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h
> >> > > @@ -73,7 +73,7 @@ struct smu8_register_index_data_pair {
> >> > >
> >> > > struct smu8_ih_meta_data {
> >> > > uint32_t command;
> >> > > -struct smu8_register_index_data_pair
> >> > > register_index_value_pair[1];
> >> > > +struct smu8_register_index_data_pair
> >> > > register_index_value_pair[];
> >> >
> >> > Did you just change this structure size without any need to change any
> >> > code as well? How was this tested?
> >>
> >> I didn't find any use of that struct member, if I missed
> >> something here, please let me know and I'll happily address any
> >> needed further work.
> >
> > I don't think this is even a variable array. It's just a one element
> > one, which is fine, don't be confused by the coccinelle "warning" here,
> > it's fired many false-positives and you need to verify this properly
> > with the driver authors first before changing anything.
>
> My apologies to you, and anybody that feels the same, it is not my
> intention to bother with mistaken patches, I just assume that this patch
> or any other from me, will go to review process, where it should be fine
> if the patch is right, wrong, need further work, or further testing
> either
> from my side or anybody else, and at the end of the day I need to do
> patches if I want to find my mentorship patches, and graduate.
>
> > In short, you just changed the size of this structure, are you _sure_
> > you can do that? And yes, it doesn't look like this field is used, but
> > the structure is, so be careful.
>
> I don't know, let check it out together and see where this goes.
I think it may have been used with the SMU firmware. I'll need to
check with that team to determine if this was meant to be a variable
sized array or not.
Alex
Re: [PATCH] drm/amd/pm: replace 1-element arrays with flexible-array members
On 2023-11-09 11:06, Greg KH wrote:
On Thu, Nov 09, 2023 at 10:43:50AM +0200, José Pekkarinen wrote:
On 2023-11-08 09:29, Greg KH wrote:
> On Wed, Nov 08, 2023 at 08:54:35AM +0200, José Pekkarinen wrote:
> > The following case seems to be safe to be replaced with a flexible
> > array
> > to clean up the added coccinelle warning. This patch will just do it.
> >
> > drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h:76:38-63:
> > WARNING use flexible-array member instead
(https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays)
> >
> > Signed-off-by: José Pekkarinen
> > ---
> > drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h
> > b/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h
> > index c7b61222d258..1ce4087005f0 100644
> > --- a/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h
> > +++ b/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h
> > @@ -73,7 +73,7 @@ struct smu8_register_index_data_pair {
> >
> > struct smu8_ih_meta_data {
> > uint32_t command;
> > - struct smu8_register_index_data_pair register_index_value_pair[1];
> > + struct smu8_register_index_data_pair register_index_value_pair[];
>
> Did you just change this structure size without any need to change any
> code as well? How was this tested?
I didn't find any use of that struct member, if I missed
something here, please let me know and I'll happily address any
needed further work.
I don't think this is even a variable array. It's just a one element
one, which is fine, don't be confused by the coccinelle "warning" here,
it's fired many false-positives and you need to verify this properly
with the driver authors first before changing anything.
My apologies to you, and anybody that feels the same, it is not my
intention to bother with mistaken patches, I just assume that this patch
or any other from me, will go to review process, where it should be fine
if the patch is right, wrong, need further work, or further testing
either
from my side or anybody else, and at the end of the day I need to do
patches if I want to find my mentorship patches, and graduate.
In short, you just changed the size of this structure, are you _sure_
you can do that? And yes, it doesn't look like this field is used, but
the structure is, so be careful.
I don't know, let check it out together and see where this goes.
José.
Re: [PATCH] drm/amd/pm: replace 1-element arrays with flexible-array members
On Thu, Nov 09, 2023 at 10:43:50AM +0200, José Pekkarinen wrote:
> On 2023-11-08 09:29, Greg KH wrote:
> > On Wed, Nov 08, 2023 at 08:54:35AM +0200, José Pekkarinen wrote:
> > > The following case seems to be safe to be replaced with a flexible
> > > array
> > > to clean up the added coccinelle warning. This patch will just do it.
> > >
> > > drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h:76:38-63:
> > > WARNING use flexible-array member instead
> > > (https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays)
> > >
> > > Signed-off-by: José Pekkarinen
> > > ---
> > > drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h
> > > b/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h
> > > index c7b61222d258..1ce4087005f0 100644
> > > --- a/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h
> > > +++ b/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h
> > > @@ -73,7 +73,7 @@ struct smu8_register_index_data_pair {
> > >
> > > struct smu8_ih_meta_data {
> > > uint32_t command;
> > > - struct smu8_register_index_data_pair register_index_value_pair[1];
> > > + struct smu8_register_index_data_pair register_index_value_pair[];
> >
> > Did you just change this structure size without any need to change any
> > code as well? How was this tested?
>
> I didn't find any use of that struct member, if I missed
> something here, please let me know and I'll happily address any
> needed further work.
I don't think this is even a variable array. It's just a one element
one, which is fine, don't be confused by the coccinelle "warning" here,
it's fired many false-positives and you need to verify this properly
with the driver authors first before changing anything.
In short, you just changed the size of this structure, are you _sure_
you can do that? And yes, it doesn't look like this field is used, but
the structure is, so be careful.
thanks,
greg k-h
Re: [PATCH] drm/amd/pm: replace 1-element arrays with flexible-array members
On 2023-11-08 09:29, Greg KH wrote:
On Wed, Nov 08, 2023 at 08:54:35AM +0200, José Pekkarinen wrote:
The following case seems to be safe to be replaced with a flexible
array
to clean up the added coccinelle warning. This patch will just do it.
drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h:76:38-63:
WARNING use flexible-array member instead
(https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays)
Signed-off-by: José Pekkarinen
---
drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h
b/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h
index c7b61222d258..1ce4087005f0 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h
+++ b/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h
@@ -73,7 +73,7 @@ struct smu8_register_index_data_pair {
struct smu8_ih_meta_data {
uint32_t command;
- struct smu8_register_index_data_pair register_index_value_pair[1];
+ struct smu8_register_index_data_pair register_index_value_pair[];
Did you just change this structure size without any need to change any
code as well? How was this tested?
I didn't find any use of that struct member, if I missed
something here, please let me know and I'll happily address any
needed further work.
José.
Re: [PATCH] drm/amd/pm: replace 1-element arrays with flexible-array members
On Wed, Nov 08, 2023 at 08:54:35AM +0200, José Pekkarinen wrote:
> The following case seems to be safe to be replaced with a flexible array
> to clean up the added coccinelle warning. This patch will just do it.
>
> drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h:76:38-63: WARNING use
> flexible-array member instead
> (https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays)
>
> Signed-off-by: José Pekkarinen
> ---
> drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h
> b/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h
> index c7b61222d258..1ce4087005f0 100644
> --- a/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h
> +++ b/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h
> @@ -73,7 +73,7 @@ struct smu8_register_index_data_pair {
>
> struct smu8_ih_meta_data {
> uint32_t command;
> - struct smu8_register_index_data_pair register_index_value_pair[1];
> + struct smu8_register_index_data_pair register_index_value_pair[];
Did you just change this structure size without any need to change any
code as well? How was this tested?
thanks,
greg k-h
RE: [PATCH] drm/amd/pm: Replace 1-element arrays with flexible array members
[Public]
> -Original Message-
> From: Samuel Holland
> Sent: Wednesday, August 30, 2023 2:58 PM
> To: Quan, Evan ; Deucher, Alexander
> ; Koenig, Christian
>
> Cc: Samuel Holland ; Daniel Vetter
> ; David Airlie ; Pan, Xinhui
> ; amd-...@lists.freedesktop.org; dri-
> de...@lists.freedesktop.org; linux-ker...@vger.kernel.org
> Subject: [PATCH] drm/amd/pm: Replace 1-element arrays with flexible array
> members
>
> Since commit df8fc4e934c1 ("kbuild: Enable -fstrict-flex-arrays=3"),
> UBSAN_BOUNDS no longer pretends 1-element arrays are unbounded. The
> bounds check is tripped in smu7_get_pp_table_entry_callback_func_v1(),
> while reading from mclk_dep_table.
>
> For consistency, fix all affected struct definitions in this file.
>
> Signed-off-by: Samuel Holland
+ Gustavo
Please make sure any code that uses these structures properly deals with the
change in structure size.
Alex
> ---
>
> .../drm/amd/pm/powerplay/hwmgr/pptable_v1_0.h | 20 +
> --
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/pptable_v1_0.h
> b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/pptable_v1_0.h
> index b0ac4d121adc..fb5e935ef786 100644
> --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/pptable_v1_0.h
> +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/pptable_v1_0.h
> @@ -164,7 +164,7 @@ typedef struct _ATOM_Tonga_State { typedef struct
> _ATOM_Tonga_State_Array {
> UCHAR ucRevId;
> UCHAR ucNumEntries; /* Number of entries. */
> - ATOM_Tonga_State entries[1];/* Dynamically allocate entries. */
> + ATOM_Tonga_State entries[]; /* Dynamically allocate entries. */
> } ATOM_Tonga_State_Array;
>
> typedef struct _ATOM_Tonga_MCLK_Dependency_Record { @@ -179,7
> +179,7 @@ typedef struct _ATOM_Tonga_MCLK_Dependency_Record {
> typedef struct _ATOM_Tonga_MCLK_Dependency_Table {
> UCHAR ucRevId;
> UCHAR ucNumEntries;
> /* Number of entries. */
> - ATOM_Tonga_MCLK_Dependency_Record entries[1];
> /* Dynamically allocate entries. */
> + ATOM_Tonga_MCLK_Dependency_Record entries[];
> /* Dynamically allocate entries. */
> } ATOM_Tonga_MCLK_Dependency_Table;
>
> typedef struct _ATOM_Tonga_SCLK_Dependency_Record { @@ -194,7
> +194,7 @@ typedef struct _ATOM_Tonga_SCLK_Dependency_Record {
> typedef struct _ATOM_Tonga_SCLK_Dependency_Table {
> UCHAR ucRevId;
> UCHAR ucNumEntries;
> /* Number of entries. */
> - ATOM_Tonga_SCLK_Dependency_Record entries[1];
>/* Dynamically allocate entries. */
> + ATOM_Tonga_SCLK_Dependency_Record entries[];
>/* Dynamically allocate entries. */
> } ATOM_Tonga_SCLK_Dependency_Table;
>
> typedef struct _ATOM_Polaris_SCLK_Dependency_Record { @@ -210,7
> +210,7 @@ typedef struct _ATOM_Polaris_SCLK_Dependency_Record {
> typedef struct _ATOM_Polaris_SCLK_Dependency_Table {
> UCHAR ucRevId;
> UCHAR ucNumEntries;
> /* Number of entries. */
> - ATOM_Polaris_SCLK_Dependency_Record entries[1];
>/* Dynamically allocate entries. */
> + ATOM_Polaris_SCLK_Dependency_Record entries[];
>/* Dynamically allocate entries. */
> } ATOM_Polaris_SCLK_Dependency_Table;
>
> typedef struct _ATOM_Tonga_PCIE_Record { @@ -222,7 +222,7 @@ typedef
> struct _ATOM_Tonga_PCIE_Record { typedef struct
> _ATOM_Tonga_PCIE_Table {
> UCHAR ucRevId;
> UCHAR ucNumEntries;
> /* Number of entries. */
> - ATOM_Tonga_PCIE_Record entries[1];
> /* Dynamically allocate entries. */
> + ATOM_Tonga_PCIE_Record entries[];
> /* Dynamically allocate entries. */
> } ATOM_Tonga_PCIE_Table;
>
> typedef struct _ATOM_Polaris10_PCIE_Record { @@ -235,7 +235,7 @@
> typedef struct _ATOM_Polaris10_PCIE_Record { typedef struct
> _ATOM_Polaris10_PCIE_Table {
> UCHAR ucRevId;
> UCHAR ucNumEntries; /* Number
> of entries. */
> - ATOM_Polaris10_PCIE_Record entries[1]; /*
> Dynamically
> allocate entries. */
> + ATOM_Polaris10_PCIE_Record entries[]; /*
> Dynamically
> allocate entries. */
> } ATOM_Polaris10_PCIE_Table;
>
>
> @@ -252,7 +252,7 @@ typedef struct
> _ATOM_Tonga_MM_Dependency_Record { typedef struct
> _ATOM_Tonga_MM_Dependency_Table {
> UCHAR ucRevId;
> UCHAR ucNumEntries;
> /* Number of entries. */
> - ATOM_Tonga_MM_Dependency_Record entries[1];
> /* Dynamically allocate entries. */
> + ATOM_Tonga_MM_Dependency_Record entries[];
> /* Dynamically allocate entries. */
> } ATOM_Tonga_MM_Dependency_Table;
>
> typedef struct _ATOM_Tonga_Voltage_Lookup_Record { @@ -265,7 +265,7
> @@ typedef struct _ATOM_Tonga_Voltage_Lookup_Record { typedef struct
>
