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

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

On 10-10-2026 10:44, Poosa, Karthik wrote:
>
> On 09-10-2026 17:44, sashiko-bot@kernel.org wrote:
>> 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?
>>
>> [ ... ]
>
> No, This won't cause a regression.
>
> boot_power_limit_read is set only after all four READ_PL_FROM_FW reads 
> succeed. Later PCODE failures don't change the cached pl1_on_boot and 
> pl2_on_boot values.
>
> If the RMW read fails, nothing is written. If the write fails, PCODE 
> doesn't apply it. Either way, PCODE keeps the firmware defaults.
>>> @@ -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.
> Valid points, but they're unrelated to this patch. this shall be 
> handled in separate patch series.
Raised this patch for this 
https://patchwork.freedesktop.org/patch/759383/?series=175943&rev=1