Re: [PATCH v11 01/16] drm/xe/hwmon: Handle pcode read failures of xe_hwmon_pcode_rmw_power_limit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/xe/hwmon: missing error checks for pcode writes during preregistration
--

commit 79685dc42e50f60af65d94b261b8f9a9cccf79d0
Author: Karthik Poosa <karthik.poosa@intel.com>

    drm/xe/hwmon: Handle pcode read failures of xe_hwmon_pcode_rmw_power_limit
    
    Return early when the pcode read operation fails in
    xe_hwmon_pcode_rmw_power_limit() and check the returned error in
    callers to avoid updating power limit settings with invalid data.

> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 5edeac961ec33..9884e06f83e59 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
[ ... ]
> @@ -394,7 +396,9 @@ static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, u32 attr, int channe
>  		if (hwmon->xe->info.has_mbx_power_limits) {
>  			drm_dbg(&hwmon->xe->drm, "disabling %s on channel %d\n",
>  				PWR_ATTR_TO_STR(attr), channel);
> -			xe_hwmon_pcode_rmw_power_limit(hwmon, attr, channel, PWR_LIM_EN, 0);
> +			ret = xe_hwmon_pcode_rmw_power_limit(hwmon, attr, channel, PWR_LIM_EN, 0);
> +			if (ret)
> +				goto unlock;
>  			xe_hwmon_pcode_read_power_limit(hwmon, attr, channel, &reg_val);
>  		} else {

[Severity: Medium]
This problem wasn't introduced by this patch, but since the commit message 
mentions checking the returned error in callers of 
xe_hwmon_pcode_rmw_power_limit() to avoid invalid data, does this patch miss 
the callers in xe_hwmon_get_preregistration_info()?

If any of these writes fail (e.g. due to a pcode timeout), the driver still
sets hwmon->boot_power_limit_read = true. This forces all subsequent power 
limit queries to use READ_PL_FROM_PCODE or READ_PL_ACCEPTED parameters instead 
of READ_PL_FROM_FW:

drivers/gpu/drm/xe/xe_hwmon.c:xe_hwmon_get_preregistration_info() {
    ...
		} else {
			drm_info(&hwmon->xe->drm, "Using mailbox commands for power limits\n");
			/* Write default limits to read from pcode from now on. */
			xe_hwmon_pcode_rmw_power_limit(hwmon, PL1_HWMON_ATTR,
						       CHANNEL_CARD, PWR_LIM | PWR_LIM_TIME,
						       hwmon->pl1_on_boot[CHANNEL_CARD]);
			xe_hwmon_pcode_rmw_power_limit(hwmon, PL1_HWMON_ATTR,
						       CHANNEL_PKG, PWR_LIM | PWR_LIM_TIME,
						       hwmon->pl1_on_boot[CHANNEL_PKG]);
			xe_hwmon_pcode_rmw_power_limit(hwmon, PL2_HWMON_ATTR,
						       CHANNEL_CARD, PWR_LIM | PWR_LIM_TIME,
						       hwmon->pl2_on_boot[CHANNEL_CARD]);
			xe_hwmon_pcode_rmw_power_limit(hwmon, PL2_HWMON_ATTR,
						       CHANNEL_PKG, PWR_LIM | PWR_LIM_TIME,
						       hwmon->pl2_on_boot[CHANNEL_PKG]);
			hwmon->scl_shift_power = PWR_UNIT;
			hwmon->scl_shift_energy = ENERGY_UNIT;
			hwmon->scl_shift_time = TIME_UNIT;
			hwmon->boot_power_limit_read = true;
    ...
}

Because the defaults were never successfully committed to the pcode mailbox, 
can these queries return stale, uninitialized, or invalid limit values, 
causing userspace to see incorrect hardware monitoring data or misconfigure 
power limits?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013020636.447620-1-karthik.poosa@intel.com?part=1