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

Jani Nikula
2024-03-27T08:01:19Z

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