Re: [PATCH v9 01/14] 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 2 potential issue(s) to consider:
- [Medium] drm/xe/hwmon: unchecked return value from xe_hwmon_pcode_rmw_power_limit()
- [Medium] hwmon: ignored pcode read errors fabricate valid sysfs responses
--

commit 6cc9575a02c32b9a8a28e257650ff7b17698865f
Author: Karthik Poosa <karthik.poosa@intel.com>

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

This commit adds a return on read failures inside
xe_hwmon_pcode_rmw_power_limit and updates 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
> @@ -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;
> +	}

[Severity: Medium]
Since this function now returns early on error, skipping the subsequent PCODE
writes, could this cause regressions in xe_hwmon_get_preregistration_info?

In xe_hwmon_get_preregistration_info, there are four calls to this function
that ignore the return value. If these initialization writes fail, the
function still sets hwmon->boot_power_limit_read = true.

Setting this flag alters the subcommand used for later reads, changing it from
READ_PL_FROM_FW to READ_PL_FROM_PCODE. Could this cause userspace to receive
incorrect power limit values if the initial writes were aborted?

[ ... ]
> @@ -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);

[Severity: Medium]
Is it intentional that the return value of xe_hwmon_pcode_read_power_limit is
discarded right after checking the write failure?

If this read fails due to a hardware timeout, it could leave reg_val
uninitialized or zeroed, bypassing the disable verification check that follows.

Additionally, could the ignored read errors in xe_hwmon_power_max_read
fabricate valid sysfs responses? If xe_hwmon_pcode_read_power_limit fails in
that function, it returns fake 0 readings instead of propagating the hardware
error, which might mislead userspace tools into believing the power limit is
disabled.

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