Re: [PATCH v12 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:
- [High] drm/xe/hwmon: PCODE active configuration corruption due to power limit init short-circuit
--

commit 96c45d5f8e540d8e962d44f94ea42a65f2c0cb61
Author: Karthik Poosa <karthik.poosa@intel.com>

drm/xe/hwmon: Handle pcode read failures of xe_hwmon_pcode_rmw_power_limit

This commit adds error handling for pcode read failures in
xe_hwmon_pcode_rmw_power_limit(). It propagates the error codes back to
callers, including an updated initialization sequence in
xe_hwmon_get_preregistration_info().

> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 5edeac961ec33..0b46572bb3c88 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -232,9 +232,11 @@ static int xe_hwmon_pcode_rmw_power_limit(const struct xe_hwmon *hwmon, u32 attr
>  						  READ_PSYSGPU_POWER_LIMIT :
>  						  READ_PACKAGE_POWER_LIMIT,
>  						  prepare_power_limit_param2(hwmon)), &val0, &val1);
> -	if (ret)
> -		drm_dbg(&hwmon->xe->drm, "read failed ch %d val0 0x%08x, val1 0x%08x, ret %d\n",
> +	if (ret) {
> +		drm_err(&hwmon->xe->drm, "read failed ch %d val0 0x%08x, val1 0x%08x, ret %d\n",
>  			channel, val0, val1, ret);
> +		return ret;
> +	}
>  
>  	if (attr == PL1_HWMON_ATTR)
>  		val0 = (val0 & ~clr) | set;
[ ... ]
> @@ -1510,23 +1518,29 @@ xe_hwmon_get_preregistration_info(struct xe_hwmon *hwmon)
>  				 "Failed to read power limits, check GPU firmware !\n");
>  		} 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;
> +
> +			/* Write default limits to read from pcode from now on. */
> +			if (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])) {
> +				drm_warn(&hwmon->xe->drm,
> +					 "Failed to set default power limits !\n");
> +			} else {
> +				hwmon->boot_power_limit_read = true;
> +			}

[Severity: High]
Does this code inadvertently leave power limit sysfs files exposed with
boot_power_limit_read set to false when an error occurs?

If any of the initializations fail, the logical OR short-circuits. This skips
the remaining initialization calls and leaves boot_power_limit_read as false,
but the driver continues loading.

When userspace later writes to a power limit via xe_hwmon_power_max_write(), it
calls xe_hwmon_pcode_rmw_power_limit(). Because boot_power_limit_read is false,
the read phase fetches firmware defaults (READ_PL_FROM_FW) instead of active
PCODE limits.

However, the write phase targets the active PCODE registers
(WRITE_PACKAGE_POWER_LIMIT). This asymmetrical read-modify-write silently
overwrites the active PCODE register (including sibling power limit enables and
time windows) with the firmware defaults, corrupting the active hardware
configuration.

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