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

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

On 13-10-2026 07:53, sashiko-bot@kernel.org wrote:
> 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?
We can check these return and report a warning.
>