On 09/04/2025 13:22, Arnd Bergmann wrote:
> From: Arnd Bergmann <a...@arndb.de>
>
> When CONFIG_DEBUG_FS is disabled, the stid_fmts[] array is not referenced
> anywhere, causing a W=1 warning with gcc:
>
> In file included from drivers/gpu/drm/imagination/pvr_fw_trace.c:7:
> drivers/gpu/drm/imagination/pvr_rogue_fwif_sf.h:75:39: error: 'stid_fmts'
> defined but not used [-Werror=unused-const-variable=]
> 75 | static const struct rogue_km_stid_fmt stid_fmts[] = {
> | ^~~~~~~~~
Hi Arnd,
Thanks for catching this! My dev environment permanently has DEBUG_FS
enabled, we should probably look into a wider variety of testing
configs.
>
> 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?
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).
>
> Signed-off-by: Arnd Bergmann <a...@arndb.de>
> ---
> drivers/gpu/drm/imagination/pvr_fw_trace.c | 8 ++++----
> drivers/gpu/drm/imagination/pvr_fw_trace.h | 2 --
> 2 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/imagination/pvr_fw_trace.c
> b/drivers/gpu/drm/imagination/pvr_fw_trace.c
> index 5dbb636d7d4f..93269299d6a4 100644
> --- a/drivers/gpu/drm/imagination/pvr_fw_trace.c
> +++ b/drivers/gpu/drm/imagination/pvr_fw_trace.c
> @@ -122,8 +122,6 @@ void pvr_fw_trace_fini(struct pvr_device *pvr_dev)
> pvr_fw_object_unmap_and_destroy(fw_trace->tracebuf_ctrl_obj);
> }
>
> -#if defined(CONFIG_DEBUG_FS)
> -
> /**
> * update_logtype() - Send KCCB command to trigger FW to update logtype
> * @pvr_dev: Target PowerVR device
> @@ -447,7 +445,7 @@ static const struct file_operations pvr_fw_trace_fops = {
> void
> pvr_fw_trace_mask_update(struct pvr_device *pvr_dev, u32 old_mask, u32
> new_mask)
> {
> - if (old_mask != new_mask)
> + if (IS_ENABLED(CONFIG_DEBUG_FS) && old_mask != new_mask)
Logically, there's no reason to add the condition here. This function
was only gated behind CONFIG_DEBUG_FS because that was the only
mechanism provided to invoke update_logtype() but since we're relying on
the compiler to figure out when this function is required, we can skip
the IS_ENABLED() check.
> update_logtype(pvr_dev, new_mask);
> }
>
> @@ -457,6 +455,9 @@ pvr_fw_trace_debugfs_init(struct pvr_device *pvr_dev,
> struct dentry *dir)
> struct pvr_fw_trace *fw_trace = &pvr_dev->fw_dev.fw_trace;
> u32 thread_nr;
>
> + if (!IS_ENABLED(CONFIG_DEBUG_FS))
> + return;
> +
> static_assert(ARRAY_SIZE(fw_trace->buffers) <= 10,
> "The filename buffer is only large enough for a
> single-digit thread count");
>
> @@ -469,4 +470,3 @@ pvr_fw_trace_debugfs_init(struct pvr_device *pvr_dev,
> struct dentry *dir)
> &pvr_fw_trace_fops);
> }
> }
> -#endif
> diff --git a/drivers/gpu/drm/imagination/pvr_fw_trace.h
> b/drivers/gpu/drm/imagination/pvr_fw_trace.h
> index 0074d2b18da0..1d0ef937427a 100644
> --- a/drivers/gpu/drm/imagination/pvr_fw_trace.h
> +++ b/drivers/gpu/drm/imagination/pvr_fw_trace.h
> @@ -65,7 +65,6 @@ struct pvr_fw_trace {
> int pvr_fw_trace_init(struct pvr_device *pvr_dev);
> void pvr_fw_trace_fini(struct pvr_device *pvr_dev);
>
> -#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?
>
> @@ -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?
>
> #endif /* PVR_FW_TRACE_H */
--
Matt Coster
E: matt.cos...@imgtec.com
OpenPGP_signature.asc
Description: OpenPGP digital signature
