Re: [PATCH v2 2/2] drm/xe/hwmon: Use shared pcode default timeout for power limit write

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

On 07-09-2026 14:14, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Missing early return on PCODE mailbox read failure causes hardware misconfiguration and corruption of pcode registers.
> --
>
> commit d67ad2da2fb13ff7d9f642e82666f4e587b00a48
> Author: Karthik Poosa <karthik.poosa@intel.com>
> drm/xe/hwmon: Use shared pcode default timeout for power limit write
>
> This commit drops the local PL_WRITE_MBX_TIMEOUT_MS macro and uses the common
> PCODE_DEFAULT_TIMEOUT_MS for the power limit write mailbox command.
>
>> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
>> index 5284cab6703d8..5edeac961ec33 100644
>> --- a/drivers/gpu/drm/xe/xe_hwmon.c
>> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> [ ... ]
>> @@ -252,7 +247,7 @@ static int xe_hwmon_pcode_rmw_power_limit(const struct xe_hwmon *hwmon, u32 attr
>>   							     (channel == CHANNEL_CARD) ?
>>   							     WRITE_PSYSGPU_POWER_LIMIT :
>>   							     WRITE_PACKAGE_POWER_LIMIT, 0),
> [Severity: High]
> This isn't a bug introduced by this patch, but looking at the earlier read
> operation in xe_hwmon_pcode_rmw_power_limit() in drivers/gpu/drm/xe/xe_hwmon.c,
> is an early return missing upon read failure?
>
> If xe_pcode_read() encounters an error, such as a mailbox timeout, it logs a
> debug message but execution falls through without returning. The val0 and val1
> variables remain 0, and the read-modify-write operation is applied to them:
>
>          ret = xe_pcode_read(..., &val0, &val1);
>          if (ret)
>                  drm_dbg(...);
>
>          if (attr == PL1_HWMON_ATTR)
>                  val0 = (val0 & ~clr) | set;
>          else if (attr == PL2_HWMON_ATTR)
>                  val1 = (val1 & ~clr) | set;
>
> Can this fall-through cause the subsequent write to clear all other fields in
> the power limit registers, like time windows and enable bits, potentially
> leaving the hardware power configuration in a corrupted state?
Yes, this shall be addressed in separate patch.
>
>> -							     val0, val1, PL_WRITE_MBX_TIMEOUT_MS);
>> +							     val0, val1, PCODE_DEFAULT_TIMEOUT_MS);
>>   	if (ret)
>>   		drm_dbg(&hwmon->xe->drm, "write failed ch %d val0 0x%08x, val1 0x%08x, ret %d\n",
>>   			channel, val0, val1, ret);