Re: [PATCH v4 1/7] drm/xe/sysctrl: Return error codes from sysctrl_wait_bit_clear()

["Tauro, Riana" <riana.tauro@intel.com>]

On 04-09-2026 15:48, Mallesh, Koujalagi wrote:
>
> On 04-09-2026 02:36 pm, Tauro, Riana wrote:
>>
>> On 04-09-2026 13:55, Mallesh, Koujalagi wrote:
>>>
>>> On 25-08-2026 01:05 pm, Tauro, Riana wrote:
>>>>
>>>> On 24-08-2026 16:34, Michal Wajdeczko wrote:
>>>>>
>>>>> On 8/24/2026 7:44 AM, Tauro, Riana wrote:
>>>>>> Hi Mallesh/Michal
>>>>>>
>>>>>> On 20-08-2026 15:46, Mallesh Koujalagi wrote:
>>>>>>> Make sysctrl_wait_bit_clear() return an error code rather than a 
>>>>>>> bool.
>>>>>>> and update callers to use xe_log_err() with the propagated error 
>>>>>>> code.
>>>>>> Bit confused here on the usage of SIGID. My assumption based on 
>>>>>> documentation and discussion was
>>>>>> that SIG ID is only needed for error logs based on severity and 
>>>>>> requires a resolution which can be documented.
>>>>>> Am i missing something?
>>>>> from "How to pick a SIGID (the uniqueness rule)"
>>>>>
>>>>>   * A single underlying failure therefore legitimately produces a
>>>>>   * *chain* of reports from different layers, each with its own 
>>>>> SIGID -- e.g. a
>>>>>   * GuC communication failure is reported as %XE_SIGID_RUNTIME_FW 
>>>>> by the firmware
>>>>>   * path, the failed recovery as %XE_SIGID_GT_TDR by the reset 
>>>>> path, and an
>>>>>   * aborted bind as %XE_SIGID_PROBE by the probe path. That chain 
>>>>> lets triage
>>>>>   * follow a fault from origin to final effect; it is not a 
>>>>> duplicate.
>>>>>
>>>>> so in this case, some sysctrl timeout may eventually lead to a 
>>>>> reset and/or
>>>>> wedge and/or runtime-survivability-mode, and this intermediate FW 
>>>>> SIGID may
>>>>> help in any postmortem triage, and if we don't reach that final 
>>>>> state then
>>>>> we can still provide some generic resolution based on site and 
>>>>> errno (COLLECT)
>>>>
>>>> What if the paths are also used in non-critical scenarios for ex: 
>>>> sysctrl commands are used for
>>>> general counter telemetry, gpu health querying and firmware status.
>>>> Wouldn't this be lead to a lot of sigids in dmesg in case of some 
>>>> unrelated failure ..
>>>> Why not only add sigid for critical paths when we are sure it will 
>>>> cause a chain of failure
>>>> instead of these prints on every trivial command failure.
>>>
>>> Ops I miss this.
>>>
>>> I agree that we should not add SIGID for every minor or noisy 
>>> failure. The intent is not to log every non-critical cases,
>>>
>>> but to capture important intermediate failures that can help explain 
>>> the real root cause. If we log only the final critical
>>>
>>> failure, we may miss the earlier cases that actually caused the 
>>> issue. That makes triage and postmortem harder, since
>>>
>>> we lose visibility into how the failure occurred. The idea is to use 
>>> SIGID only for meaningful failure in key execution paths.
>>>
>>> Even when these failure are not fatal, they can provide valuable 
>>> context, make debugging easier and reduce
>>>
>>> mean-time-to triage (MTTT).
>>
>>
>> Suppose a user uses invalid entries for gpu health or counter 
>> management, won't we be polluting the dmesg
>> logs with sig id and cper logs unnecessarily since these are not fatal.
>
> No, we should not emit SIGID/CPER for bad user input  or invalid query 
> parameters in gpu health/counter management path.

But the system controller interface is same for all where you have added 
sig id logs. A failure in sysctrl_send_cmd or receive/send frames will be
displayed in general non fatal cases too. And it could be due to invalid 
request.

This is not a call chain that leads to a critical failure always. Need 
conclusion on this because one of my patch has a similar condition.

Riana

>
> These are usage errors, not real device or firmware failures.
>
>
> Thanks,
>
> -/Mallesh
>
>>
>> Thanks
>> Riana
>>
>>
>>>
>>>
>>> Thanks,
>>>
>>> -/Mallesh
>>>
>>>>
>>>> Thanks
>>>> Riana
>>>>
>>>>>
>>>>>> How do these errors indicate the need for a resolution.
>>>>>> Do we need it for all kmd logs with components or for errors that 
>>>>>> need a recovery or can be recoverable?
>>>>>>
>>>>>> Thanks
>>>>>> Riana
>>>>>>
>>>>>>> Signed-off-by: Mallesh Koujalagi <mallesh.koujalagi@intel.com>
>>>>>>> ---
>>>>>>>    drivers/gpu/drm/xe/xe_sysctrl_mailbox.c | 26 
>>>>>>> ++++++++++++-------------
>>>>>>>    1 file changed, 13 insertions(+), 13 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c 
>>>>>>> b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
>>>>>>> index e13eebaac1d0..ef847f0a8f2c 100644
>>>>>>> --- a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
>>>>>>> +++ b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
>>>>>>> @@ -11,6 +11,7 @@
>>>>>>>      #include "regs/xe_sysctrl_regs.h"
>>>>>>>    #include "xe_device.h"
>>>>>>> +#include "xe_log.h"
>>>>>>>    #include "xe_mmio.h"
>>>>>>>    #include "xe_pm.h"
>>>>>>>    #include "xe_printk.h"
>>>>>>> @@ -34,15 +35,11 @@ struct xe_sysctrl_mailbox_msg_hdr {
>>>>>>>    #define XE_SYSCTRL_HDR_RESULT(hdr) \
>>>>>>>        FIELD_GET(SYSCTRL_HDR_RESULT_MASK, le32_to_cpu((hdr)->data))
>>>>>>>    -static bool sysctrl_wait_bit_clear(struct xe_sysctrl *sc, 
>>>>>>> u32 bit_mask,
>>>>>>> -                   unsigned int timeout_ms)
>>>>>>> +static int sysctrl_wait_bit_clear(struct xe_sysctrl *sc, u32 
>>>>>>> bit_mask,
>>>>>>> +                  unsigned int timeout_ms)
>>>>>>>    {
>>>>>>> -    int ret;
>>>>>>> -
>>>>>>> -    ret = xe_mmio_wait32_not(sc->mmio, SYSCTRL_MB_CTRL, 
>>>>>>> bit_mask, bit_mask,
>>>>>>> +    return xe_mmio_wait32_not(sc->mmio, SYSCTRL_MB_CTRL, 
>>>>>>> bit_mask, bit_mask,
>>>>>>>                     timeout_ms * 1000, NULL, false);
>>>>>>> -
>>>>>>> -    return ret == 0;
>>>>>>>    }
>>>>>>>      static bool sysctrl_wait_bit_set(struct xe_sysctrl *sc, u32 
>>>>>>> bit_mask,
>>>>>>> @@ -145,12 +142,14 @@ static int sysctrl_send_frames(struct 
>>>>>>> xe_sysctrl *sc,
>>>>>>>        struct xe_device *xe = sc_to_xe(sc);
>>>>>>>        u32 ctrl_reg, total_frames, frame;
>>>>>>>        size_t bytes_sent, frame_size;
>>>>>>> +    int ret;
>>>>>>>          total_frames = DIV_ROUND_UP(cmd_size, 
>>>>>>> XE_SYSCTRL_MB_FRAME_SIZE);
>>>>>>>    -    if (!sysctrl_wait_bit_clear(sc, 
>>>>>>> SYSCTRL_MB_CTRL_RUN_BUSY, timeout_ms)) {
>>>>>>> -        xe_err(xe, "sysctrl: Mailbox busy\n");
>>>>>>> -        return -EBUSY;
>>>>>>> +    ret = sysctrl_wait_bit_clear(sc, SYSCTRL_MB_CTRL_RUN_BUSY, 
>>>>>>> timeout_ms);
>>>>>>> +    if (ret) {
>>>>>>> +        xe_log_err(xe, SYSCTRL, ret, "Mailbox busy\n");
>>>>>>> +        return ret;
>>>>>>>        }
>>>>>>>          sc->phase_bit ^= 1;
>>>>>>> @@ -173,10 +172,11 @@ static int sysctrl_send_frames(struct 
>>>>>>> xe_sysctrl *sc,
>>>>>>>              xe_mmio_write32(sc->mmio, SYSCTRL_MB_CTRL, ctrl_reg);
>>>>>>>    -        if (!sysctrl_wait_bit_clear(sc, 
>>>>>>> SYSCTRL_MB_CTRL_RUN_BUSY, timeout_ms)) {
>>>>>>> -            xe_err(xe, "sysctrl: Frame %u acknowledgment 
>>>>>>> timeout\n", frame);
>>>>>>> +        ret = sysctrl_wait_bit_clear(sc, 
>>>>>>> SYSCTRL_MB_CTRL_RUN_BUSY, timeout_ms);
>>>>>>> +        if (ret) {
>>>>>>> +            xe_log_err(xe, SYSCTRL, ret, "Frame %u 
>>>>>>> acknowledgment timeout\n", frame);
>>>>>>>                sc->phase_bit = 0;
>>>>>>> -            return -ETIMEDOUT;
>>>>>>> +            return ret;
>>>>>>>            }
>>>>>>>              bytes_sent += frame_size;