linux-kernel - Re: [PATCH 01/10] drm/imagination: avoid unused-const-variable warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <997d5df5-fa8e-4b3b-83a9-f86131705b13@app.fastmail.com>
Date: Thu, 10 Apr 2025 13:58:52 +0200
From: "Arnd Bergmann" <arnd@...db.de>
To: "Matt Coster" <Matt.Coster@...tec.com>, "Arnd Bergmann" <arnd@...nel.org>
Cc: "Frank Binns" <Frank.Binns@...tec.com>,
 "Maarten Lankhorst" <maarten.lankhorst@...ux.intel.com>,
 "Maxime Ripard" <mripard@...nel.org>,
 "Thomas Zimmermann" <tzimmermann@...e.de>, "Dave Airlie" <airlied@...il.com>,
 "Simona Vetter" <simona@...ll.ch>,
 "Krzysztof Kozlowski" <krzysztof.kozlowski@...aro.org>,
 "Andy Shevchenko" <andriy.shevchenko@...ux.intel.com>,
 "Andrzej Hajda" <andrzej.hajda@...el.com>,
 "Jani Nikula" <jani.nikula@...el.com>,
 "Alex Deucher" <alexander.deucher@....com>,
 "Alessio Belle" <Alessio.Belle@...tec.com>,
 "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 01/10] drm/imagination: avoid unused-const-variable warning

On Thu, Apr 10, 2025, at 13:22, Matt Coster wrote:
> On 09/04/2025 13:22, Arnd Bergmann wrote:
>> 
>> Rather than adding more #ifdef blocks, address this by changing the
>> existing #ifdef into equivalent IS_ENABLED() checks so gcc can see
>> where the symbol is used but still eliminate it from the object file.
>
> Possibly a silly question, but wouldn't adding __maybe_unused to
> stid_fmts be a simpler change here? Or is there a strong preference to
> avoid #ifdef CONFIG_* and let the compiler figure out what to elide?

All three ways work, and it's mainly a matter a preference. A lot
of developers don't like __maybe_unused, and there are enough developers
that really dislike #ifdef blocks in .c files, though a lot of
others don't really care.

I sent the version that I like best because I find that easier to
read and it gives better compile-time coverage in all configurations,
but I mainly care about having no warnings, so pick whatever approach
works for you. 

> The contents of the pvr_rogue_fwif*.h headers is essentially normative
> outside of company-internal documentation. With types, there's generally
> no warnings emitted when they're not used, but I believe this is the
> only instance of actual data being stored in these headers.
>
> I suppose technically it should even be moved to an associated *.c file,
> but that would (I assume) further confound the compiler's ability to
> decide when it's needed in the final binary (or I guess the linker if
> it's in a separate object).

Moving it next to the user of that definition is geneally best
here, if the variable is only ever used in one place. If that
happens to be inside of an existing #ifdef, it just works, and
if someone later replaces the #ifdef with an IS_ENABLED() check
it keeps working.

>> -#if defined(CONFIG_DEBUG_FS)
>>  /* Forward declaration from <linux/dcache.h>. */
>>  struct dentry;
>
> With the #ifdef removed, there's no reason to keep this forward
> declaration down here. Can you move it up to the top of the file with
> the others?

I don't think it changes much, I usually keep these close to
the function prototype that use them as you have here.

>> @@ -73,6 +72,5 @@ void pvr_fw_trace_mask_update(struct pvr_device *pvr_dev, u32 old_mask,
>>  			      u32 new_mask);
>>  
>>  void pvr_fw_trace_debugfs_init(struct pvr_device *pvr_dev, struct dentry *dir);
>> -#endif /* defined(CONFIG_DEBUG_FS) */
>
> Having said that, surely it makes sense to keep at least
> *_debugfs_init() gated behind CONFIG_DEBUG_FS?

With the IS_ENABLED() check, it's an empty function, so the cost
of the function is very small. Leaving the #ifdef in here would
probably again cause warnings about unused functions or variable.

Ideally the entire file would just be left out of the build
here, with the function decarations in pvr_fw_trace.h
replaced with empty stubs for !CONFIG_DEBUG_FS. I don't
know if that's possible, or if you still need to initialize
the fw trace if there is no debugfs interface for it, so
I did not try this myself.

Can you try to come up with a fix for the warning that you
like best and treat my email as "Reported-by: Arnd Bergmann
<arnd@...db.de>"?

      Arnd