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

["Poosa, Karthik" <karthik.poosa@intel.com>]

On 28-09-2026 05:41, Rodrigo Vivi wrote:
> On Fri, Sep 25, 2026 at 02:24:17AM +0530, Karthik Poosa wrote:
>> 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.
>>
>> Fixes: 8aa7306631f0 ("drm/xe/hwmon: Fix xe_hwmon_power_max_write")
>> Signed-off-by: Karthik Poosa <karthik.poosa@intel.com>
>> ---
>> v2: Address sashiko-bot@kernel.org review comments. Return -EIO instead of
>>      0 on errors in the sysfs store callback to prevent potential userspace
>>      infinite loops.
>>
>>   drivers/gpu/drm/xe/xe_hwmon.c | 18 ++++++++++++------
>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
>> index 5edeac961ec3..be3fb6733b6f 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;
>> @@ -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 {
>>   			reg_val = xe_mmio_rmw32(mmio, rapl_limit, PWR_LIM_EN, 0);
>> @@ -679,11 +683,13 @@ xe_hwmon_power_max_interval_store(struct device *dev, struct device_attribute *a
>>   
>>   	mutex_lock(&hwmon->hwmon_lock);
>>   
>> -	if (hwmon->xe->info.has_mbx_power_limits)
>> -		xe_hwmon_pcode_rmw_power_limit(hwmon, power_attr, channel, PWR_LIM_TIME, rxy);
>> -	else
>> +	if (hwmon->xe->info.has_mbx_power_limits) {
>> +		if (xe_hwmon_pcode_rmw_power_limit(hwmon, power_attr, channel, PWR_LIM_TIME, rxy))
>> +			count = -EIO;
> please use ret for this, not changing count...
> and you may set ret = count earlier, or something like that...
ok
>
>> +	} else {
>>   		r = xe_mmio_rmw32(mmio, xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, channel),
>>   				  PWR_LIM_TIME, rxy);
>> +	}
>>   
>>   	mutex_unlock(&hwmon->hwmon_lock);
>>   
>> -- 
>> 2.25.1
>>