| Message ID | 20230609164207.430377-1-hamza.mahfooz@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amd/amdgpu: enable W=1 for amdgpu | expand |
+ Masahiro and linux-kbuild On Fri, Jun 09, 2023 at 12:42:06PM -0400, Hamza Mahfooz wrote: > We have a clean build with W=1 as of > commit 12a15dd589ac ("drm/amd/display/amdgpu_dm/amdgpu_dm_helpers: Move > SYNAPTICS_DEVICE_ID into CONFIG_DRM_AMD_DC_DCN ifdef"). So, let's enable > these checks unconditionally for the entire module to catch these errors > during development. > > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: Nathan Chancellor <nathan@kernel.org> > Signed-off-by: Hamza Mahfooz <hamza.mahfooz@amd.com> I think this is fine, especially since it will help catch issues in amdgpu quickly and hopefully encourage developers to fix their problems before they make it to a tree with wider impact lika -next. However, this is now the third place that W=1 has been effectively enabled (i915 and btrfs are the other two I know of) and it would be nice if this was a little more unified, especially since it is not uncommon for the warnings under W=1 to shift around and keeping them unified will make maintainence over the longer term a little easier. I am not sure if this has been brought up in the past and I don't want to hold up this change but I suspect this sentiment of wanting to enable W=1 on a per-subsystem basis is going to continue to grow. Regardless, for clang 11.1.0 to 16.0.5, I see no warnings when building drivers/gpu/drm/amd/amdgpu/ with Arch Linux's configuration or allmodconfig. Reviewed-by: Nathan Chancellor <nathan@kernel.org> Tested-by: Nathan Chancellor <nathan@kernel.org> > --- > drivers/gpu/drm/amd/amdgpu/Makefile | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile > index 86b833085f19..8d16f280b695 100644 > --- a/drivers/gpu/drm/amd/amdgpu/Makefile > +++ b/drivers/gpu/drm/amd/amdgpu/Makefile > @@ -40,7 +40,18 @@ ccflags-y := -I$(FULL_AMD_PATH)/include/asic_reg \ > -I$(FULL_AMD_PATH)/amdkfd > > subdir-ccflags-y := -Wextra > -subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable) > +subdir-ccflags-y += -Wunused > +subdir-ccflags-y += -Wmissing-prototypes > +subdir-ccflags-y += -Wmissing-declarations > +subdir-ccflags-y += -Wmissing-include-dirs > +subdir-ccflags-y += -Wold-style-definition > +subdir-ccflags-y += -Wmissing-format-attribute > +# Need this to avoid recursive variable evaluation issues > +cond-flags := $(call cc-option, -Wunused-but-set-variable) \ > + $(call cc-option, -Wunused-const-variable) \ > + $(call cc-option, -Wstringop-truncation) \ > + $(call cc-option, -Wpacked-not-aligned) > +subdir-ccflags-y += $(cond-flags) > subdir-ccflags-y += -Wno-unused-parameter > subdir-ccflags-y += -Wno-type-limits > subdir-ccflags-y += -Wno-sign-compare > -- > 2.40.1 >
On Sat, Jun 10, 2023 at 5:17 AM Nathan Chancellor <nathan@kernel.org> wrote: > > + Masahiro and linux-kbuild > > On Fri, Jun 09, 2023 at 12:42:06PM -0400, Hamza Mahfooz wrote: > > We have a clean build with W=1 as of > > commit 12a15dd589ac ("drm/amd/display/amdgpu_dm/amdgpu_dm_helpers: Move > > SYNAPTICS_DEVICE_ID into CONFIG_DRM_AMD_DC_DCN ifdef"). So, let's enable > > these checks unconditionally for the entire module to catch these errors > > during development. > > > > Cc: Alex Deucher <alexander.deucher@amd.com> > > Cc: Nathan Chancellor <nathan@kernel.org> > > Signed-off-by: Hamza Mahfooz <hamza.mahfooz@amd.com> > > I think this is fine, especially since it will help catch issues in > amdgpu quickly and hopefully encourage developers to fix their problems > before they make it to a tree with wider impact lika -next. > > However, this is now the third place that W=1 has been effectively > enabled (i915 and btrfs are the other two I know of) and it would be > nice if this was a little more unified, especially since it is not > uncommon for the warnings under W=1 to shift around and keeping them > unified will make maintainence over the longer term a little easier. I > am not sure if this has been brought up in the past and I don't want to > hold up this change but I suspect this sentiment of wanting to enable > W=1 on a per-subsystem basis is going to continue to grow. I believe this patch is the right way because we will be able to add a new warning option to scripts/Makefile.extrawarn without fixing any code. I remember somebody argued that drivers should be able to do subdir-ccflags-y += $(W1_FLAGS) However, if a new flag, -Wfoo, emits warnings for drivers/gpu/drm/{i915,amd}, you cannot add it to W=1 until fixing the code. If many drivers start to do likewise, W=1 warning will not be W=1 any more. Another good thing for hard-coding warning options is you can lift up a warning flag one by one. Let's say you fixed the entire DRM subsystem so it is -Wunused free now. Then, you can move -Wunused to drivers/gpu/drm/Makefile, while other warning options stay in drivers Makefiles. > > Regardless, for clang 11.1.0 to 16.0.5, I see no warnings when building > drivers/gpu/drm/amd/amdgpu/ with Arch Linux's configuration or > allmodconfig. > > Reviewed-by: Nathan Chancellor <nathan@kernel.org> > Tested-by: Nathan Chancellor <nathan@kernel.org> > > > --- > > drivers/gpu/drm/amd/amdgpu/Makefile | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile > > index 86b833085f19..8d16f280b695 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/Makefile > > +++ b/drivers/gpu/drm/amd/amdgpu/Makefile > > @@ -40,7 +40,18 @@ ccflags-y := -I$(FULL_AMD_PATH)/include/asic_reg \ > > -I$(FULL_AMD_PATH)/amdkfd > > > > subdir-ccflags-y := -Wextra > > -subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable) > > +subdir-ccflags-y += -Wunused > > +subdir-ccflags-y += -Wmissing-prototypes > > +subdir-ccflags-y += -Wmissing-declarations > > +subdir-ccflags-y += -Wmissing-include-dirs > > +subdir-ccflags-y += -Wold-style-definition > > +subdir-ccflags-y += -Wmissing-format-attribute > > +# Need this to avoid recursive variable evaluation issues > > +cond-flags := $(call cc-option, -Wunused-but-set-variable) \ > > + $(call cc-option, -Wunused-const-variable) \ > > + $(call cc-option, -Wstringop-truncation) \ > > + $(call cc-option, -Wpacked-not-aligned) > > +subdir-ccflags-y += $(cond-flags) > > subdir-ccflags-y += -Wno-unused-parameter > > subdir-ccflags-y += -Wno-type-limits > > subdir-ccflags-y += -Wno-sign-compare > > -- > > 2.40.1 > >
On Sat, 10 Jun 2023, Masahiro Yamada <masahiroy@kernel.org> wrote: > On Sat, Jun 10, 2023 at 5:17 AM Nathan Chancellor <nathan@kernel.org> wrote: >> >> + Masahiro and linux-kbuild >> >> On Fri, Jun 09, 2023 at 12:42:06PM -0400, Hamza Mahfooz wrote: >> > We have a clean build with W=1 as of >> > commit 12a15dd589ac ("drm/amd/display/amdgpu_dm/amdgpu_dm_helpers: Move >> > SYNAPTICS_DEVICE_ID into CONFIG_DRM_AMD_DC_DCN ifdef"). So, let's enable >> > these checks unconditionally for the entire module to catch these errors >> > during development. >> > >> > Cc: Alex Deucher <alexander.deucher@amd.com> >> > Cc: Nathan Chancellor <nathan@kernel.org> >> > Signed-off-by: Hamza Mahfooz <hamza.mahfooz@amd.com> >> >> I think this is fine, especially since it will help catch issues in >> amdgpu quickly and hopefully encourage developers to fix their problems >> before they make it to a tree with wider impact lika -next. >> >> However, this is now the third place that W=1 has been effectively >> enabled (i915 and btrfs are the other two I know of) and it would be >> nice if this was a little more unified, especially since it is not >> uncommon for the warnings under W=1 to shift around and keeping them >> unified will make maintainence over the longer term a little easier. I >> am not sure if this has been brought up in the past and I don't want to >> hold up this change but I suspect this sentiment of wanting to enable >> W=1 on a per-subsystem basis is going to continue to grow. > > > > I believe this patch is the right way because > we will be able to add a new warning option to > scripts/Makefile.extrawarn without fixing any code. > > I remember somebody argued that drivers should be > able to do > subdir-ccflags-y += $(W1_FLAGS) Personally, I think this would be the viable way to make the kernel free of W=1 warnings. Make it clean driver and subsystem at a time, with constant progress. Currently, there's haphazard fixing of issues, with new ones creeping back in, because kernel-wide W=1 is too verbose for most developers. It's whac-a-mole. > However, if a new flag, -Wfoo, emits warnings > for drivers/gpu/drm/{i915,amd}, > you cannot add it to W=1 until fixing the code. Or adding -Wno-foo where it breaks, until fixed. > If many drivers start to do likewise, > W=1 warning will not be W=1 any more. I don't know, is the goal to fix the warnings, or keep adding stuff to W=1 so that it'll always emit warnings? :p BR, Jani. > Another good thing for hard-coding warning options > is you can lift up a warning flag one by one. > > > Let's say you fixed the entire DRM subsystem so > it is -Wunused free now. > > Then, you can move -Wunused to drivers/gpu/drm/Makefile, > while other warning options stay in drivers Makefiles. > > > > > > > >> >> Regardless, for clang 11.1.0 to 16.0.5, I see no warnings when building >> drivers/gpu/drm/amd/amdgpu/ with Arch Linux's configuration or >> allmodconfig. >> >> Reviewed-by: Nathan Chancellor <nathan@kernel.org> >> Tested-by: Nathan Chancellor <nathan@kernel.org> >> >> > --- >> > drivers/gpu/drm/amd/amdgpu/Makefile | 13 ++++++++++++- >> > 1 file changed, 12 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile >> > index 86b833085f19..8d16f280b695 100644 >> > --- a/drivers/gpu/drm/amd/amdgpu/Makefile >> > +++ b/drivers/gpu/drm/amd/amdgpu/Makefile >> > @@ -40,7 +40,18 @@ ccflags-y := -I$(FULL_AMD_PATH)/include/asic_reg \ >> > -I$(FULL_AMD_PATH)/amdkfd >> > >> > subdir-ccflags-y := -Wextra >> > -subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable) >> > +subdir-ccflags-y += -Wunused >> > +subdir-ccflags-y += -Wmissing-prototypes >> > +subdir-ccflags-y += -Wmissing-declarations >> > +subdir-ccflags-y += -Wmissing-include-dirs >> > +subdir-ccflags-y += -Wold-style-definition >> > +subdir-ccflags-y += -Wmissing-format-attribute >> > +# Need this to avoid recursive variable evaluation issues >> > +cond-flags := $(call cc-option, -Wunused-but-set-variable) \ >> > + $(call cc-option, -Wunused-const-variable) \ >> > + $(call cc-option, -Wstringop-truncation) \ >> > + $(call cc-option, -Wpacked-not-aligned) >> > +subdir-ccflags-y += $(cond-flags) >> > subdir-ccflags-y += -Wno-unused-parameter >> > subdir-ccflags-y += -Wno-type-limits >> > subdir-ccflags-y += -Wno-sign-compare >> > -- >> > 2.40.1 >> >
diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile index 86b833085f19..8d16f280b695 100644 --- a/drivers/gpu/drm/amd/amdgpu/Makefile +++ b/drivers/gpu/drm/amd/amdgpu/Makefile @@ -40,7 +40,18 @@ ccflags-y := -I$(FULL_AMD_PATH)/include/asic_reg \ -I$(FULL_AMD_PATH)/amdkfd subdir-ccflags-y := -Wextra -subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable) +subdir-ccflags-y += -Wunused +subdir-ccflags-y += -Wmissing-prototypes +subdir-ccflags-y += -Wmissing-declarations +subdir-ccflags-y += -Wmissing-include-dirs +subdir-ccflags-y += -Wold-style-definition +subdir-ccflags-y += -Wmissing-format-attribute +# Need this to avoid recursive variable evaluation issues +cond-flags := $(call cc-option, -Wunused-but-set-variable) \ + $(call cc-option, -Wunused-const-variable) \ + $(call cc-option, -Wstringop-truncation) \ + $(call cc-option, -Wpacked-not-aligned) +subdir-ccflags-y += $(cond-flags) subdir-ccflags-y += -Wno-unused-parameter subdir-ccflags-y += -Wno-type-limits subdir-ccflags-y += -Wno-sign-compare
We have a clean build with W=1 as of commit 12a15dd589ac ("drm/amd/display/amdgpu_dm/amdgpu_dm_helpers: Move SYNAPTICS_DEVICE_ID into CONFIG_DRM_AMD_DC_DCN ifdef"). So, let's enable these checks unconditionally for the entire module to catch these errors during development. Cc: Alex Deucher <alexander.deucher@amd.com> Cc: Nathan Chancellor <nathan@kernel.org> Signed-off-by: Hamza Mahfooz <hamza.mahfooz@amd.com> --- drivers/gpu/drm/amd/amdgpu/Makefile | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)