[PATCH 0/4] Use -Werror by default and fix compilation warnings

[PATCH 0/4] Use -Werror by default and fix compilation warnings

[Rodrigo Siqueira]

We have an internal CI in the display team that uses a stable version of
amd-staging-drm-next to run a set of tests (from compilation to IGT);
usually, we have to create this stable version manually. We are trying
to automate this process, but we are getting failures because we compile
display code with the flag -Werror, which means we cannot ignore
warnings in amd-staging-drm-next. In this patchset, I propose enabling
-Werror flag by default in amdgpu to make it a little bit harder to
ignore warning messages. The first patch of this series updates amdgpu
Makefile, and the other patches fix warning issues in amdgpu.

I tested this series with DCN, DCN disabled, and with `allmodconfig`; I
did not have any compilation issue.

Thanks
Siqueira

Rodrigo Siqueira (4):
  drm/amdgpu: Treat warning as an error
  drm/amdgpu: Fix compilation warning due to double semicolon
  drm/amdgpu: Drop unused variable
  drm/amdgpu: Silent GCC warning

 drivers/gpu/drm/amd/amdgpu/Makefile      | 2 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c  | 1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h  | 1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c | 4 ++--
 4 files changed, 5 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH 1/4] drm/amdgpu: Treat warning as an error

[Rodrigo Siqueira]

We have one internal CI that builds our kernel with the -Werror flag; as
a result, when we try to sync our branch with amd-staging-drm-next we
get some failures due to warnings. This commit tries to alleviate this
problem by forcing a warning to be treated as an error.

Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile
index 7fedbb725e17..158f427c2f2e 100644
--- a/drivers/gpu/drm/amd/amdgpu/Makefile
+++ b/drivers/gpu/drm/amd/amdgpu/Makefile
@@ -27,6 +27,8 @@ FULL_AMD_PATH=$(srctree)/$(src)/..
 DISPLAY_FOLDER_NAME=display
 FULL_AMD_DISPLAY_PATH = $(FULL_AMD_PATH)/$(DISPLAY_FOLDER_NAME)
 
+subdir-ccflags-y += -Werror
+
 ccflags-y := -I$(FULL_AMD_PATH)/include/asic_reg \
 	-I$(FULL_AMD_PATH)/include \
 	-I$(FULL_AMD_PATH)/amdgpu \
-- 
2.25.1

Re: [PATCH 1/4] drm/amdgpu: Treat warning as an error

[Michel Dänzer]

On 2022-01-11 00:39, Rodrigo Siqueira wrote:
> We have one internal CI that builds our kernel with the -Werror flag; as
> a result, when we try to sync our branch with amd-staging-drm-next we
> get some failures due to warnings. This commit tries to alleviate this
> problem by forcing a warning to be treated as an error.
> 
> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/Makefile | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile
> index 7fedbb725e17..158f427c2f2e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
> @@ -27,6 +27,8 @@ FULL_AMD_PATH=$(srctree)/$(src)/..
>  DISPLAY_FOLDER_NAME=display
>  FULL_AMD_DISPLAY_PATH = $(FULL_AMD_PATH)/$(DISPLAY_FOLDER_NAME)
>  
> +subdir-ccflags-y += -Werror

The problem with this is that different compilers, or even different versions of the same compiler, can generate different warnings for the same code. So this will definitely result in some people hitting compile failures for no reason other than using a different compiler (version) than the code had been tested with. Or maybe just a different .config.

Unless you secretly hate your users ;) [0], I'd strongly recommend against enabling this unconditionally.

(The proper place for -Werror is in CI which gates merging of all changes. E.g. Mesa's upstream GitLab CI does this with great success)


[0] No, this isn't about RHEL — the RHEL kernel build always enables -Werror, so we need to deal with that anyway.

-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer

Re: [PATCH 1/4] drm/amdgpu: Treat warning as an error

[Michel Dänzer]

On 2022-01-11 11:41, Michel Dänzer wrote:
> On 2022-01-11 00:39, Rodrigo Siqueira wrote:
>> We have one internal CI that builds our kernel with the -Werror flag; as
>> a result, when we try to sync our branch with amd-staging-drm-next we
>> get some failures due to warnings. This commit tries to alleviate this
>> problem by forcing a warning to be treated as an error.
>>
>> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/Makefile | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile
>> index 7fedbb725e17..158f427c2f2e 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
>> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
>> @@ -27,6 +27,8 @@ FULL_AMD_PATH=$(srctree)/$(src)/..
>>  DISPLAY_FOLDER_NAME=display
>>  FULL_AMD_DISPLAY_PATH = $(FULL_AMD_PATH)/$(DISPLAY_FOLDER_NAME)
>>  
>> +subdir-ccflags-y += -Werror
> 
> The problem with this is that different compilers, or even different versions of the same compiler, can generate different warnings for the same code. So this will definitely result in some people hitting compile failures for no reason other than using a different compiler (version) than the code had been tested with.

A corollary of this is that your internal CI might still hit compile failures due to warnings, if its compiler (version) is sufficiently different from those used by the critical path of maintainers for merging amdgpu changes.

Since that CI cannot prevent merging of changes with warnings, it cannot enable -Werror without risking this.


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer

Re: [PATCH 1/4] drm/amdgpu: Treat warning as an error

[Rodrigo Siqueira Jordao]

On 2022-01-11 6:00 a.m., Michel Dänzer wrote:
> On 2022-01-11 11:41, Michel Dänzer wrote:
>> On 2022-01-11 00:39, Rodrigo Siqueira wrote:
>>> We have one internal CI that builds our kernel with the -Werror flag; as
>>> a result, when we try to sync our branch with amd-staging-drm-next we
>>> get some failures due to warnings. This commit tries to alleviate this
>>> problem by forcing a warning to be treated as an error.
>>>
>>> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/Makefile | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile
>>> index 7fedbb725e17..158f427c2f2e 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
>>> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
>>> @@ -27,6 +27,8 @@ FULL_AMD_PATH=$(srctree)/$(src)/..
>>>   DISPLAY_FOLDER_NAME=display
>>>   FULL_AMD_DISPLAY_PATH = $(FULL_AMD_PATH)/$(DISPLAY_FOLDER_NAME)
>>>   
>>> +subdir-ccflags-y += -Werror
>>
>> The problem with this is that different compilers, or even different versions of the same compiler, can generate different warnings for the same code. So this will definitely result in some people hitting compile failures for no reason other than using a different compiler (version) than the code had been tested with.
> 
> A corollary of this is that your internal CI might still hit compile failures due to warnings, if its compiler (version) is sufficiently different from those used by the critical path of maintainers for merging amdgpu changes.
> 
> Since that CI cannot prevent merging of changes with warnings, it cannot enable -Werror without risking this.

Hi Michel/Alex,

Those are great points, and I did not think about the different GCC an 
config versions; I tried with GCC and Clang, but the version can 
definitely become a problem.

Thanks a lot for the explanation; I guess I need to find another way to 
handle this issue.

Best Regards
Siqueira

[PATCH 2/4] drm/amdgpu: Fix compilation warning due to double semicolon

[Rodrigo Siqueira]

There is a double semicolon that makes GCC complain about the following
warning:

amdgpu_xgmi.c:953:2: error: ISO C90 forbids mixed declarations and code
  [-Werror=declaration-after-statement]
  953 |  struct ta_ras_trigger_error_input *block_info;

Drop the extra semicolon to get rid of the GCC warning.

Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
index aa8d614009d4..d47a510a7f5d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
@@ -949,8 +949,8 @@ static void amdgpu_xgmi_query_ras_error_count(struct amdgpu_device *adev,
 /* Trigger XGMI/WAFL error */
 static int amdgpu_ras_error_inject_xgmi(struct amdgpu_device *adev,  void *inject_if)
 {
-	int ret = 0;;
-	struct ta_ras_trigger_error_input *block_info =  (struct ta_ras_trigger_error_input *)inject_if;
+	int ret = 0;
+	struct ta_ras_trigger_error_input *block_info = (struct ta_ras_trigger_error_input *)inject_if;
 
 	if (amdgpu_dpm_set_df_cstate(adev, DF_CSTATE_DISALLOW))
 		dev_warn(adev->dev, "Failed to disallow df cstate");
-- 
2.25.1

[PATCH 3/4] drm/amdgpu: Drop unused variable

[Rodrigo Siqueira]

This commit fix the following GCC warning:

drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:945:6: error:
  unused variable ‘i’ [-Werror=unused-variable]

Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
index b1bedfd4febc..517650d286a7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
@@ -942,7 +942,6 @@ int amdgpu_ras_query_error_status(struct amdgpu_device *adev,
 	struct amdgpu_ras_block_object* block_obj = NULL;
 	struct ras_manager *obj = amdgpu_ras_find_obj(adev, &info->head);
 	struct ras_err_data err_data = {0, 0, 0, NULL};
-	int i;
 
 	if (!obj)
 		return -EINVAL;
-- 
2.25.1

[PATCH 4/4] drm/amdgpu: Silent GCC warning

[Rodrigo Siqueira]

We have the following GCC warning:

drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h:400:10:
error: ‘struct amdgpu_iv_entry’ declared inside parameter list will not
be visible outside of this definition or declaration [-Werror]
  400 |   struct amdgpu_iv_entry *entry);

Silent this warning

Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
index f604a2235a9c..62f1f97ef7f4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
@@ -395,6 +395,7 @@ struct ras_err_handler_data {
 	int space_left;
 };
 
+struct amdgpu_iv_entry;
 typedef int (*ras_ih_cb)(struct amdgpu_device *adev,
 		void *err_data,
 		struct amdgpu_iv_entry *entry);
-- 
2.25.1

Re: [PATCH 4/4] drm/amdgpu: Silent GCC warning

[Alex Deucher]

I just sent these same three fixes earlier today:
https://patchwork.freedesktop.org/series/98695/

Alex

On Mon, Jan 10, 2022 at 6:39 PM Rodrigo Siqueira
<Rodrigo.Siqueira@amd.com> wrote:
>
> We have the following GCC warning:
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h:400:10:
> error: ‘struct amdgpu_iv_entry’ declared inside parameter list will not
> be visible outside of this definition or declaration [-Werror]
>   400 |   struct amdgpu_iv_entry *entry);
>
> Silent this warning
>
> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> index f604a2235a9c..62f1f97ef7f4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> @@ -395,6 +395,7 @@ struct ras_err_handler_data {
>         int space_left;
>  };
>
> +struct amdgpu_iv_entry;
>  typedef int (*ras_ih_cb)(struct amdgpu_device *adev,
>                 void *err_data,
>                 struct amdgpu_iv_entry *entry);
> --
> 2.25.1
>