Re: [PATCH v6 05/10] drm/xe/cper: Log CPER records for aggregate counter retrieval

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/xe/ras: leak offlined_pages XArray on xe_cper_sysfs_init() error
--

commit 6077a66c6338e4db5c918ef6aa2f82fb393dd9be
Author: Badal Nilawar <badal.nilawar@intel.com>

drm/xe/cper: Log CPER records for aggregate counter retrieval

This commit adds a cper_on_query sysfs attribute to control whether CPER
records are emitted when userspace queries RAS error counters, and updates
xe_ras_get_counter() and xe_ras_init() to support it.

> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> index 0555b86b63b27..6664153b3cae7 100644
> --- a/drivers/gpu/drm/xe/xe_ras.c
> +++ b/drivers/gpu/drm/xe/xe_ras.c

[ ... ]

> @@ -1246,5 +1284,9 @@ int xe_ras_init(struct xe_device *xe)
>  	if (ret)
>  		xe_err(xe, "Failed to create GPU health sysfs, err=%d\n", ret);
>  
> +	ret = xe_cper_sysfs_init(xe);
> +	if (ret)
> +		return ret;
> +
>  	return devm_add_action_or_reset(xe->drm.dev, ras_fini, xe);
>  }

[Severity: Medium]
Does returning early here on xe_cper_sysfs_init() failure leak the entries
allocated in state->offlined_pages?

Earlier in xe_ras_init(), state->offlined_pages is initialized and populated:

drivers/gpu/drm/xe/xe_ras.c:xe_ras_init() {
	...
	xa_init(&state->offlined_pages);

	/* Fetch the offline list before queue to avoid duplication */
	get_offline_list(xe);
	get_offline_queue(xe);
	...
	xe_ras_process_errors(xe);
}

Those helpers store entries into state->offlined_pages via xa_store(...,
GFP_KERNEL), which is normally cleaned up by ras_fini():

static void ras_fini(void *arg)
{
	struct xe_device *xe = arg;

	xa_destroy(&xe->ras.state.offlined_pages);
}

If xe_cper_sysfs_init() fails and returns before devm_add_action_or_reset()
registers ras_fini(), nothing calls xa_destroy(&state->offlined_pages). Should
devm_add_action_or_reset(xe->drm.dev, ras_fini, xe) be registered right after
xa_init(), or should xa_destroy() be called on this error path (or should
xe_cper_sysfs_init() failure be logged as non-fatal like gpu_health_group
above)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028095723.734387-12-badal.nilawar@intel.com?part=5