Re: [RESEND v3 2/2] drm: Add CONFIG_DRM_WERROR

[Jani Nikula <jani.nikula@intel.com>]

On Wed, 27 Mar 2024, Maxime Ripard <mripard@kernel.org> wrote:
> Hi,
>
> On Tue, Mar 26, 2024 at 03:56:50PM -0700, Nathan Chancellor wrote:
>> On Tue, Mar 05, 2024 at 11:07:36AM +0200, Jani Nikula wrote:
>> > Add kconfig to enable -Werror subsystem wide. This is useful for
>> > development and CI to keep the subsystem warning free, while avoiding
>> > issues outside of the subsystem that kernel wide CONFIG_WERROR=y might
>> > hit.
>> > 
>> > v2: Don't depend on COMPILE_TEST
>> > 
>> > Reviewed-by: Hamza Mahfooz <hamza.mahfooz@amd.com> # v1
>> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> > ---
>> >  drivers/gpu/drm/Kconfig  | 13 +++++++++++++
>> >  drivers/gpu/drm/Makefile |  3 +++
>> >  2 files changed, 16 insertions(+)
>> > 
>> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> > index 6e853acf15da..c08e18108c2a 100644
>> > --- a/drivers/gpu/drm/Kconfig
>> > +++ b/drivers/gpu/drm/Kconfig
>> > @@ -416,3 +416,16 @@ config DRM_LIB_RANDOM
>> >  config DRM_PRIVACY_SCREEN
>> >  	bool
>> >  	default n
>> > +
>> > +config DRM_WERROR
>> > +	bool "Compile the drm subsystem with warnings as errors"
>> > +	depends on EXPERT
>> > +	default n
>> > +	help
>> > +	  A kernel build should not cause any compiler warnings, and this
>> > +	  enables the '-Werror' flag to enforce that rule in the drm subsystem.
>> > +
>> > +	  The drm subsystem enables more warnings than the kernel default, so
>> > +	  this config option is disabled by default.
>> > +
>> > +	  If in doubt, say N.
>> 
>> While I understand the desire for an easy switch that maintainers and
>> developers can use to ensure that their changes are warning free for the
>> drm subsystem specifically, I think subsystem specific configuration
>> options like this are actively detrimental to developers and continuous
>> integration systems that build test the entire kernel. For example, we
>> turned off CONFIG_WERROR for our Hexagon builds because of warnings that
>> appear with -Wextra that are legitimate but require treewide changes to
>> resolve in a manner sufficient for Linus:
>> 
>> https://github.com/ClangBuiltLinux/linux/issues/1285
>> https://lore.kernel.org/all/CAHk-=wg80je=K7madF4e7WrRNp37e3qh6y10Svhdc7O8SZ_-8g@mail.gmail.com/
>> https://lore.kernel.org/all/20230522105049.1467313-1-schnelle@linux.ibm.com/
>> 
>> But now, due to CONFIG_DRM_WERROR getting enabled by all{mod,yes}config
>> and -Wextra being unconditionally enabled for DRM, those warnings hard
>> break the build despite CONFIG_WERROR=n...
>
> Would making DRM_WERROR depends on WERROR address your concerns?

But then what would be the point of having DRM_WERROR at all? For me the
point is, "werror in drm, ignore the rest, they're someone else's
problem".

An alternative would be to "depends on !COMPILE_TEST" that we have in
i915, but then some folks want to have COMPILE_TEST in drm, because some
drivers are otherwise hard for people to build.

Nathan, we do want to fix any issues switfly. Are you hitting specific
build problems?


BR,
Jani.



>
> Maxime

-- 
Jani Nikula, Intel