Re: [PATCH v3 07/12] drm/xe/cper: Log CPER records for aggregate counter retrival

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Passing incompletely initialized query counter struct to CPER logging instead of the fully populated response counter returned by hardware.
- [Low] The sysfs attribute `cper_on_query` is created with 0600 permissions, contradicting its documentation which states it is readable by all users.

Pre-existing issues:
- [High] The preexisting `xe_emit_hardware_error_cper` function silently aborts when passed a valid hardware sigid, completely breaking the new CPER logging functionality.
--

commit 4840bdedfff2abbfbb8ce20f95bb4403db40e936
Author: Badal Nilawar <badal.nilawar@intel.com>

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

This commit adds support to log CPER records for aggregate counter
retrieval from userspace. It introduces a new sysfs attribute
cper_on_query to control this behavior.

> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> index 7e3e627504485..288dbc0942f58 100644
> --- a/drivers/gpu/drm/xe/xe_ras.c
> +++ b/drivers/gpu/drm/xe/xe_ras.c
[ ... ]
> @@ -612,6 +641,7 @@ enum xe_ras_recovery_action xe_ras_process_errors(struct xe_device *xe)
>   */
>  int xe_ras_get_counter(struct xe_device *xe, u8 severity, u8 component, u32 *value)
>  {
> +	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
>  	struct xe_ras_error_class counter = {0};
>  	struct xe_ras_get_counter_response response = {0};
>  	int ret;
> @@ -623,8 +653,13 @@ int xe_ras_get_counter(struct xe_device *xe, u8 severity, u8 component, u32 *val
>  	ret = xe_ras_get_counter_response(xe, &counter, &response);
>  	if (ret)
>  		return ret;
> -
>  	*value = response.value;
> +
> +	if (xe->ras.cper_on_query)
> +		xe_emit_hardware_error_cper(pdev, ras_sev_to_cper_sev(counter.common.severity),
> +					    ras_comp_to_hw_sigid(counter.common.component),
> +					    (struct xe_ras_error_class *)&counter,

[Severity: Medium]
Does this code pass the incompletely initialized query counter instead of the 
fully populated &response.counter returned by hardware? 

The CPER record emitted will be missing the product-specific hardware
classification fields (like tile, instance, cause), leading to information loss
in RAS logging.

[Severity: High]
This is a pre-existing issue, but xe_emit_hardware_error_cper() silently
aborts when passed a valid hardware sigid (which are all 
>= INTEL_SIGID_GPU_XE_HARDWARE_START) due to a backwards check:

drivers/gpu/drm/xe/xe_cper.c:xe_emit_hardware_error_cper() {
	if ((int)sigid >= INTEL_SIGID_GPU_XE_HARDWARE_START)
		return;
    ...
}

Does this completely break the new CPER logging functionality intended by 
this patch?

> +					    (struct xe_ras_get_counter_response *)&response);
>  	return 0;
>  }
>  
[ ... ]
> @@ -1069,6 +1104,56 @@ static const struct attribute_group gpu_health_group = {
>  	.attrs = gpu_health_attrs,
>  };
>  
> +static ssize_t cper_on_query_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct xe_device *xe = kdev_to_xe_device(dev);
> +
> +	return sysfs_emit(buf, "%u\n", xe->ras.cper_on_query);
> +}
> +
> +static ssize_t cper_on_query_store(struct device *dev, struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	struct xe_device *xe = kdev_to_xe_device(dev);
> +	bool enable;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &enable);
> +	if (ret)
> +		return ret;
> +
> +	xe->ras.cper_on_query = enable;
> +
> +	return count;
> +}
> +static DEVICE_ATTR_ADMIN_RW(cper_on_query);

[Severity: Low]
Does this incorrectly enforce 0600 permissions, preventing non-root users from 
reading the value? 

The documentation below states it should be readable by all users:

> +
> +static struct attribute *cper_on_query_attrs[] = {
> +	&dev_attr_cper_on_query.attr,
> +	NULL
> +};
> +
> +/**
> + * DOC: CPER on query
> + *
> + * On Intel Xe platforms that support the RAS error reporting interface,
> + * the driver can emit a CPER (Common Platform Error Record) each time an
> + * error counter is queried. This behaviour is controlled through the
> + * following sysfs attribute::
> + *
> + *     /sys/bus/pci/devices/<device>/cper_on_query
> + *
> + * The attribute is a boolean (``0`` or ``1``). When set to ``1``, every
> + * counter query emits a CPER record built from the associated info queue
> + * data; when set to ``0`` (default) no record is emitted on query.
> + *
> + * Reading the attribute is available to all users and returns the current
> + * setting, whereas writing is restricted to administrative users.
> + */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906172604.2215987-14-badal.nilawar@intel.com?part=7