Re: [PATCH 08/10] drm/xe: Add infrastructure for debug configfs parameters

On Thu, 2026-08-20 at 20:19 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> - [Low] Global namespace pollution from generic attribute names
> --
> 
> --- Patch [8]: [PATCH 08/10] drm/xe: Add infrastructure for debug
> configfs parameters ---
> commit c6663e692904c84b3875225b65be4210162f1f5e
> Author: Stuart Summers <[email protected]>
> 
> drm/xe: Add infrastructure for debug configfs parameters
> 
> This commit introduces an X-macro pattern for defining debug configfs
> parameters
> in the Xe driver. It automatically generates backing structs,
> configuration
> attributes, and accessors from a single parameter list to simplify
> adding new
> debug entries.
> 
> > diff --git a/drivers/gpu/drm/xe/xe_configfs_debug_params.c
> > b/drivers/gpu/drm/xe/xe_configfs_debug_params.c
> > new file mode 100644
> > index 0000000000000..00e5ff175c0af
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xe/xe_configfs_debug_params.c
> 
> [ ... ]
> 
> > +/*
> > + * Define the configfs_attribute objects. These are non-static so
> > they
> > + * can be referenced from xe_configfs_debug.c.
> > + */
> > +#define _XE_PARAM_DEFINE_ATTR(_T, _name, _def, _val, _vis, _get,
> > _fallback) \
> > +struct configfs_attribute attr_##_name =
> > {                              \
> 
> [Severity: Low]
> Could this naming convention cause global symbol collisions? 
> 
> Since these objects are non-static (defined in the global namespace),
> naming 
> them attr_##_name could lead to link-time multiple definition errors
> if another 
> subsystem happens to export a variable with a generic name like
> attr_timeout or 
> attr_enable.
> 
> Would it be safer to use a subsystem-specific prefix, such as 
> xe_configfs_attr_##_name, to prevent namespace pollution?
> 
> > +       .ca_name        =
> > __stringify(_name),                           \
> > +       .ca_owner       =
> > THIS_MODULE,                                  \
> > +       .ca_mode        =
> > 0644,                                         \
> > +       .show           =
> > _name##_show,                                 \
> > +       .store          =
> > _name##_store,                                \
> > +};
> > +
> > +XE_CONFIGFS_DEBUG_PARAMS_FOR_EACH(_XE_PARAM_DEFINE_ATTR)
> 
> [ ... ]
> 
> > diff --git a/drivers/gpu/drm/xe/xe_configfs_debug_params.h
> > b/drivers/gpu/drm/xe/xe_configfs_debug_params.h
> > new file mode 100644
> > index 0000000000000..954ca4343136d
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xe/xe_configfs_debug_params.h
> 
> [ ... ]
> 
> > +/*
> > + * Forward declarations of the per-parameter configfs_attribute
> > objects.
> > + *
> > + * The objects themselves are defined in
> > xe_configfs_debug_params.c (as
> > + * non-static so they can be referenced from xe_configfs_debug.c's
> > + * xe_configfs_debug_attrs[] array and is_visible() callback).
> > They live
> > + * in the same translation unit as the X-macro-generated
> > show/store
> > + * handlers; consumers should reference them via attr_<name>.
> > + */
> > +#define _XE_PARAM_DECLARE_ATTR(_T, _name, _def, _val, _vis, _get,
> > _fallback) \
> > +       extern struct configfs_attribute attr_##_name;
> 
> [Severity: Low]
> Similar to the definition above, this macro exposes attr_##_name in
> the global 
> namespace. Adding a prefix like xe_configfs_attr_##_name here would
> align with 
> the definition change and ensure namespace safety.

The other review comments from Sashiko were all pre-existing issues I'm
not planning on addressing in this series, but I decided to address
these naming suggestions in the latest series..

I still need to update the IGT, but I think otherwise these should be
ready to review.

Thanks,
Stuart

> 
> > +XE_CONFIGFS_DEBUG_PARAMS_FOR_EACH(_XE_PARAM_DECLARE_ATTR)
> > +#undef _XE_PARAM_DECLARE_ATTR
>