| Message ID | 20250409122314.2848028-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | -Wunused-const-variable warning fixes | expand |
On 09/04/2025 13:22, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@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 <arnd@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 */
On Thu, 10 Apr 2025, Matt Coster <Matt.Coster@imgtec.com> wrote: > Having said that, surely it makes sense to keep at least > *_debugfs_init() gated behind CONFIG_DEBUG_FS? If they're basically just calls to debugfs_create_{dir,file}, the compiler pretty much turns them into nops, and you'll get better build coverage. BR, Jani.
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@arndb.de>"? Arnd
On Thu, Apr 10, 2025 at 11:22:05AM +0000, 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? I'm not Arnd (and I just have read his answer), but I would like to add that __maybe_unused should be the last resort as it has more cons than more invasive solutions. In particular, it makes build time increase with a lot of work to be made at link time, and also it might hide the real bugs when somebody simply forgot to use it (depending on the configuration options) or so.
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) 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; @@ -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) */ #endif /* PVR_FW_TRACE_H */