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

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

On 07-09-2026 19:54, Michal Wajdeczko wrote:
>
> On 9/7/2026 3:52 PM, Tauro, Riana wrote:
>> On 04-09-2026 17:27, Michal Wajdeczko wrote:
>>> On 9/4/2026 1:11 PM, Tauro, Riana wrote:
>>>> 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.
>>> we can discuss what to do in case of explicit error (failure) codes returned by the FW on specific commands, as it looks that unlike the GuC, such errors/failures could be caused by the invalid data provided by the user.
>>>
>>> but IMO if the FW sends corrupted frames, which I assume is a part of the low-level FW communication channel, or we can't send some random command to FW - isn't that a good indication that there is a problem with the FW communication that should be reported?
>> In that case, shouldn't sigid be applicable to guc, huc, i2c and all low level failures.
> it is applicable, see pending series [1]
>
> [1] https://patchwork.freedesktop.org/series/173177/
>
>
>>    What would be the documented recovery mechanism in case one of these fails especially if it is a one time occurence?
> in case of GuC I assume we already trigger a GT reset, which logs its own SIGID
> and if that fails, then we wedge, again logged with its own SIGID
> so all 3x SIGID reports gives us a better picture what just happen
>
>> Wouldn't this approach result in excessive numeric logging and sigid being used in the entire driver ?
> in general we don't expect any failures, but if something fails, IMO we should report any abnormal situation
> so yes, all FW/HW errors that we today report with xe_err() should likely be converted to xe_log_err() family

This conflicts with our initial discussion and series.
  However, if this is the rule for SIGID usage post the new design, I 
have no concerns. In that case, every sysfs,  user interface with 
firmware/hw that fails must generate a SIGID log.

Thanks
Riana

>
>>  From what i understood in the initial series and mallesh's response, this was meant for errors that require a recovery
>> or fatal errors. Am i missing something here?
> then we would just need either PROBE or WEDGED/SURVABILITY logs, as only those are fatal, no?
>
> but since we have RUNTIME/DEVICE FW SIGIDs, when should we use them if not for the error conditions related to code that interacts with that FW?
>
>>>> 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;