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

[sashiko-bot@kernel.org]

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 <stuart.summers@intel.com>

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.

> +XE_CONFIGFS_DEBUG_PARAMS_FOR_EACH(_XE_PARAM_DECLARE_ATTR)
> +#undef _XE_PARAM_DECLARE_ATTR

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820200719.84681-12-stuart.summers@intel.com?part=8