Re: [PATCH v7 12/20] drm/xe/vsec: Support EAGAIN return value

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Medium] xe_hwmon_energy_get() incorrectly masks the -EAGAIN error by returning 0, which corrupts power calculations and permanently hides sysfs attributes.
--

commit 99e578f62cb6212c825a10a5de5875717c46d4a9
Author: Michael J. Ruhl <michael.j.ruhl@intel.com>

drm/xe/vsec: Support EAGAIN return value

This commit updates call locations of xe_pmt_telem_read() to explicitly
ignore the -EAGAIN return value and prevent it from logging warnings.

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 5284cab6703d8..ecd52a04175d0 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -522,7 +522,9 @@ xe_hwmon_energy_get(struct xe_hwmon *hwmon, int channel, long *energy)
>  					xe_mmio_read32(mmio, PUNIT_TELEMETRY_GUID),
>  					&pmt_val, BMG_ENERGY_STATUS_PMT_OFFSET,	sizeof(pmt_val));
>  		if (ret != sizeof(pmt_val)) {
> -			drm_warn(&hwmon->xe->drm, "energy read from pmt failed, ret %d\n", ret);
> +			if (ret != -EAGAIN)
> +				drm_warn(&hwmon->xe->drm, "energy read from pmt failed, ret %d\n",
> +					 ret);

[Severity: Medium]
This isn't a bug introduced by this patch, but since we are modifying the
error handling here, does swallowing -EAGAIN break sysfs attribute
registration and runtime energy reporting?

During device initialization, xe_hwmon_register() evaluates sysfs attribute
visibility by calling xe_hwmon_energy_get():

xe_hwmon_energy_is_visible() {
    ...
    if (hwmon->xe->info.platform == XE_BATTLEMAGE) {
        xe_hwmon_energy_get(hwmon, channel, &energy);
        return energy ? 0444 : 0;
    }
    ...
}

Because xe_vsec_init() (which sets PMT base_offset) is called after
xe_hwmon_register() in xe_device.c, the PMT read deterministically returns
-EAGAIN during probe.

By swallowing the error here and resetting the output to 0:

>  			*energy = 0;
>  			return;
>  		}

The visibility check sees energy=0 and permanently hides the sysfs attributes
(e.g., energy1_input) for Battlemage GPUs.

Additionally, if -EAGAIN occurs during a later runtime read in
xe_hwmon_energy_read(), returning 0 microjoules instead of propagating the
error code corrupts power monitoring tools that compute power from this
monotonic accumulator.

Could we change xe_hwmon_energy_get() to return an error code instead of
void so callers can distinguish between an actual 0 value and a failure?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909154739.1515514-22-michael.j.ruhl@intel.com?part=12