Re: [PATCH v5 2/7] drm/xe/sysctrl: Return error codes from sysctrl_wait_bit_set()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] The patch introduces usage of an undefined macro `xe_log_err`, a missing header `xe_log.h`, and an undeclared identifier `SYSCTRL`, resulting in immediate compilation failure.

Pre-existing issues:
- [High] Failure to drain the firmware mailbox on truncated reads hangs the hardware sysctrl mailbox, leading to state machine desynchronization and a permanent Denial of Service (DoS) for the sysctrl subsystem.
--

--- Patch [2]: [PATCH v5 2/7] drm/xe/sysctrl: Return error codes from sysctrl_wait_bit_set() ---
commit a0d9dbca39d7d03a2a03ce09604444ffe68818bd
Author: Mallesh Koujalagi <mallesh.koujalagi@intel.com>

drm/xe/sysctrl: Return error codes from sysctrl_wait_bit_set()

This patch modifies sysctrl_wait_bit_set() to return an error code instead
of a bool, and updates the caller in sysctrl_process_frame() to propagate
the error using xe_log_err().

> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
> index f97dec375efe8..b5412ca42bb81 100644
> --- a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
> +++ b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c

[ ... ]

> @@ -193,9 +189,10 @@ static int sysctrl_process_frame(struct xe_sysctrl *sc, void *out,
>  	struct xe_device *xe = sc_to_xe(sc);
>  	int ret;
>  
> -	if (!sysctrl_wait_bit_set(sc, SYSCTRL_MB_CTRL_RUN_BUSY_OUT, timeout_ms)) {
> -		xe_err(xe, "sysctrl: Response frame timeout\n");
> -		return -ETIMEDOUT;
> +	ret = sysctrl_wait_bit_set(sc, SYSCTRL_MB_CTRL_RUN_BUSY_OUT, timeout_ms);
> +	if (ret) {
> +		xe_log_err(xe, SYSCTRL, ret, "Response frame timeout\n");

[Severity: Low]
Is xe_log_err defined here? It appears this patch introduces usage of an
undefined macro xe_log_err and an undeclared identifier SYSCTRL, resulting
in an immediate compilation failure. Could there be a missing prerequisite
patch in this series that adds the xe_log infrastructure?

> +		return ret;
>  	}
>  
>  	ctrl_reg = xe_mmio_read32(sc->mmio, SYSCTRL_MB_CTRL);

[Severity: High]
This isn't a bug introduced by this patch, but looking at the caller of
sysctrl_process_frame():

sysctrl_receive_frames() {
    ...
    while (!done && remain) {
        frame_size = min_t(size_t, remain, XE_SYSCTRL_MB_FRAME_SIZE);
        ret = sysctrl_process_frame(sc, out, frame_size, timeout_ms,
                                    &done);
        if (ret)
            return ret;
        remain -= frame_size;
        out += frame_size;
    }
    ...
}

If the firmware sends a response payload larger than the data provided by
the caller, remain will reach 0 before done is true.

Doesn't this cause the while loop to exit early without draining the
remaining frames from the hardware mailbox?

If the SYSCTRL_MB_CTRL_RUN_BUSY_OUT bit isn't cleared, won't this leave the
hardware state machine stuck and cause a permanent DoS of the system
controller interface for subsequent commands?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825110542.1359776-9-mallesh.koujalagi@intel.com?part=2