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

[sashiko-bot@kernel.org]

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?

> -							     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);

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