Re: [PATCH v5 5/7] drm/xe/sysctrl: Improve firmware response error logging

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 8/27/2026 2:50 PM, Mallesh, Koujalagi wrote:
> 
> On 27-08-2026 04:27 pm, Michal Wajdeczko wrote:
>>
>> On 8/25/2026 1:05 PM, Mallesh Koujalagi wrote:
>>> Use xe_log_err() helper and include additional details when a
>>> response header mismatch occurs.
>>>
>>> Signed-off-by: Mallesh Koujalagi <mallesh.koujalagi@intel.com>
>>> ---
>>> v5:
>>> - Use %#x after REG_FIELD_GET change. (Michal)
>>> ---
>>>   drivers/gpu/drm/xe/xe_sysctrl_mailbox.c | 18 +++++++++++-------
>>>   1 file changed, 11 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
>>> index f8b518dcafa2..5032d766b314 100644
>>> --- a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
>>> +++ b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
>>> @@ -24,16 +24,16 @@ struct xe_sysctrl_mailbox_msg_hdr {
>>>   } __packed;
>>>     #define XE_SYSCTRL_HDR_GROUP_ID(hdr) \
>>> -    FIELD_GET(SYSCTRL_HDR_GROUP_ID_MASK, le32_to_cpu((hdr)->data))
>>> +    REG_FIELD_GET(SYSCTRL_HDR_GROUP_ID_MASK, le32_to_cpu((hdr)->data))
>>>     #define XE_SYSCTRL_HDR_COMMAND(hdr) \
>>> -    FIELD_GET(SYSCTRL_HDR_COMMAND_MASK, le32_to_cpu((hdr)->data))
>>> +    REG_FIELD_GET(SYSCTRL_HDR_COMMAND_MASK, le32_to_cpu((hdr)->data))
>>>     #define XE_SYSCTRL_HDR_IS_RESPONSE(hdr) \
>>> -    FIELD_GET(SYSCTRL_HDR_IS_RESPONSE, le32_to_cpu((hdr)->data))
>>> +    REG_FIELD_GET(SYSCTRL_HDR_IS_RESPONSE, le32_to_cpu((hdr)->data))
>>>     #define XE_SYSCTRL_HDR_RESULT(hdr) \
>>> -    FIELD_GET(SYSCTRL_HDR_RESULT_MASK, le32_to_cpu((hdr)->data))
>>> +    REG_FIELD_GET(SYSCTRL_HDR_RESULT_MASK, le32_to_cpu((hdr)->data))
>> this should be mentioned in this commit message
>>
>> nit: or maybe above changes should be done in its own patch?
> Sure! will make different patch for that.
>>
>>>     static int sysctrl_wait_bit_clear(struct xe_sysctrl *sc, u32 bit_mask,
>>>                     unsigned int timeout_ms)
>>> @@ -237,13 +237,17 @@ static int sysctrl_receive_frames(struct xe_sysctrl *sc,
>>>       if (!XE_SYSCTRL_HDR_IS_RESPONSE(hdr) ||
>>>           XE_SYSCTRL_HDR_GROUP_ID(hdr) != XE_SYSCTRL_HDR_GROUP_ID(req) ||
>>>           XE_SYSCTRL_HDR_COMMAND(hdr) != XE_SYSCTRL_HDR_COMMAND(req)) {
>>> -        xe_err(xe, "sysctrl: Response header mismatch\n");
>>> +        xe_log_err(xe, SYSCTRL, -EPROTO,
>>> +               "Response header mismatch: got group=%#x cmd=%#x is_resp=%u, expected group=%#x cmd=%#x\n",
>>> +               XE_SYSCTRL_HDR_GROUP_ID(hdr), XE_SYSCTRL_HDR_COMMAND(hdr),
>>> +               XE_SYSCTRL_HDR_IS_RESPONSE(hdr),
>>> +               XE_SYSCTRL_HDR_GROUP_ID(req), XE_SYSCTRL_HDR_COMMAND(req));
>> nit: as admin may not know what is "group" "cmd" "resp"
>> maybe we can just log raw hdr value?
>>
>>     "Response header mismatch: %#x\n", hdr
> 
> "Response header mismatch: got=%#x expected=%#x\n", le32_to_cpu(hdr->data), le32_to_cpu(req->data)); please let me know, is this fine?

looks fine, but it could be even simpler:

     "Response header mismatch: %#x != %#x\n"

> 
> Thanks,
> 
> -/Mallesh
> 
>>>           return -EPROTO;
>>>       }
>>>         if (XE_SYSCTRL_HDR_RESULT(hdr) != 0) {
>>> -        xe_err(xe, "sysctrl: Firmware error: 0x%02lx\n",
>>> -               XE_SYSCTRL_HDR_RESULT(hdr));
>>> +        xe_log_err(xe, SYSCTRL, -EIO, "Firmware error: %#04x\n",
>>> +               XE_SYSCTRL_HDR_RESULT(hdr));
>>>           return -EIO;
>>>       }
>>>