Re: [PATCH v3] drm/xe/sysctrl: Add better sysctrl error reporting

["Mallesh, Koujalagi" <mallesh.koujalagi@intel.com>]

On 18-08-2026 07:36 pm, Michal Wajdeczko wrote:
>
> On 8/18/2026 3:10 PM, Mallesh Koujalagi wrote:
>> Switch sysctrl error messages to xe_log_err() with SYSCTRL tags so
> s/Switch sysctr error/Switch the System Controller (sysctrl) error
>
>> tools can reliably detect and categorize common sysctrl failures.
>>
>> Signed-off-by: Mallesh Koujalagi <mallesh.koujalagi@intel.com>
>> ---
>> v2:
>> - Use -EOVERFLOW.
>> - Drop sysctrl in xe_log_err. (Michal)
>> - Drop redundant %pe.
>> - Print response->count.
>>
>> v3:
>> - Rebase.
>> ---
>>   drivers/gpu/drm/xe/xe_sysctrl_event.c   | 12 ++++++----
>>   drivers/gpu/drm/xe/xe_sysctrl_mailbox.c | 29 ++++++++++++++++---------
>>   2 files changed, 27 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_event.c b/drivers/gpu/drm/xe/xe_sysctrl_event.c
>> index da395148ee9d..4341ea99cdc3 100644
>> --- a/drivers/gpu/drm/xe/xe_sysctrl_event.c
>> +++ b/drivers/gpu/drm/xe/xe_sysctrl_event.c
>> @@ -5,6 +5,7 @@
>>   
>>   #include "xe_device.h"
>>   #include "xe_irq.h"
>> +#include "xe_log.h"
>>   #include "xe_printk.h"
>>   #include "xe_ras.h"
>>   #include "xe_sysctrl.h"
>> @@ -25,13 +26,15 @@ static void get_pending_event(struct xe_sysctrl *sc, struct xe_sysctrl_mailbox_c
>>   
>>   		ret = xe_sysctrl_send_command(sc, command, &len);
>>   		if (ret) {
>> -			xe_err(xe, "sysctrl: failed to get pending event %d\n", ret);
>> +			xe_log_err(xe, SYSCTRL, ret,
>> +				   "failed to get pending event\n");
> nit1: no need to break line
Not required, will remove it
>
> nit2: we should be consistent (at least per file) whether
> the error message should start with lowercase or uppercase
Sure, I'll make uppercase to start of each message.
>
>>   			return;
>>   		}
>>   
>>   		if (len != sizeof(*response)) {
>> -			xe_err(xe, "sysctrl: unexpected event response length %zu (expected %zu)\n",
>> -			       len, sizeof(*response));
>> +			xe_log_err(xe, SYSCTRL, -EMSGSIZE,
>> +				   "unexpected event response length %zu (expected %zu)\n",
>> +				   len, sizeof(*response));
>>   			return;
>>   		}
>>   
>> @@ -41,7 +44,8 @@ static void get_pending_event(struct xe_sysctrl *sc, struct xe_sysctrl_mailbox_c
>>   			xe_warn(xe, "sysctrl: unexpected event %#x\n", response->event);
> we do have xe_log_err_info() now - maybe we can use it here?

hmm, why we need to replace xe_warn to xe_log_err_info, since it's part 
of dmesg, user can easily get it.

IMO, adding to many xe_log_* helper will defeat the purpose.

>
>>   
>>   		if (!--count) {
>> -			xe_err(xe, "sysctrl: event flooding\n");
>> +			xe_log_err(xe, SYSCTRL, -EOVERFLOW,
>> +				   "event flooding: %u\n", response->count);
> nit: from the dmesg line it will be hard to guess what this %u really mean
I'll add "Event flooding, response count as: %u\n"
>
>>   			return;
>>   		}
>>   
>> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
>> index e13eebaac1d0..a96ef2864526 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"
>> @@ -115,7 +116,8 @@ static int sysctrl_prepare_command(struct xe_device *xe,
>>   	xe_assert(xe, command <= SYSCTRL_HDR_COMMAND_MAX);
>>   
>>   	if (data_in_len > XE_SYSCTRL_MB_MAX_MESSAGE_SIZE - sizeof(*hdr)) {
>> -		xe_err(xe, "sysctrl: Input data too large: %zu bytes\n", data_in_len);
>> +		xe_log_err(xe, SYSCTRL, -EINVAL,
>> +			   "Input data too large: %zu bytes\n", data_in_len);
> didn't we agree to change that to xe_assert() ?
>
> (this could be done as a preparation patch)
Sure, will provide as a preparation patch.
>
>>   		return -EINVAL;
>>   	}
>>   
>> @@ -149,7 +151,7 @@ static int sysctrl_send_frames(struct xe_sysctrl *sc,
>>   	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)) {
> nit: shouldn't sysctrl_wait_bit_clear() and friends return -errno instead of bool?
> then we wouldn't need to figure out the errno code at the callers side
>
> (this could be done as a preparation patch)
Ok
>
>> -		xe_err(xe, "sysctrl: Mailbox busy\n");
>> +		xe_log_err(xe, SYSCTRL, -EBUSY, "Mailbox busy\n");
>>   		return -EBUSY;
>>   	}
>>   
>> @@ -160,7 +162,8 @@ static int sysctrl_send_frames(struct xe_sysctrl *sc,
>>   		frame_size = min_t(size_t, cmd_size - bytes_sent, XE_SYSCTRL_MB_FRAME_SIZE);
>>   
>>   		if (sysctrl_write_frame(sc, mbox_cmd + bytes_sent, frame_size)) {
> hmm, actually sysctrl_write_frame() always return 0
> either change that function to void
> or use the returned error code
>
> (also as separate patch)
Sure
>> -			xe_err(xe, "sysctrl: Failed to write frame %u\n", frame);
>> +			xe_log_err(xe, SYSCTRL, -EIO,
>> +				   "Failed to write frame %u\n", frame);
>>   			sc->phase_bit = 0;
>>   			return -EIO;
>>   		}
>> @@ -174,7 +177,8 @@ 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);
>> +			xe_log_err(xe, SYSCTRL, -ETIMEDOUT,
>> +				   "Frame %u acknowledgment timeout\n", frame);
>>   			sc->phase_bit = 0;
>>   			return -ETIMEDOUT;
>>   		}
>> @@ -194,7 +198,8 @@ static int sysctrl_process_frame(struct xe_sysctrl *sc, void *out,
>>   	int ret;
>>   
>>   	if (!sysctrl_wait_bit_set(sc, SYSCTRL_MB_CTRL_RUN_BUSY_OUT, timeout_ms)) {
>> -		xe_err(xe, "sysctrl: Response frame timeout\n");
>> +		xe_log_err(xe, SYSCTRL, -ETIMEDOUT,
>> +			   "Response frame timeout\n");
>>   		return -ETIMEDOUT;
>>   	}
>>   
>> @@ -249,13 +254,15 @@ 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\n");
> maybe we should print the unexpected data from the header?
Based on data size, or else unnecessary putting pressure on dmesg.
>>   		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: 0x%02lx\n",
>> +			   XE_SYSCTRL_HDR_RESULT(hdr));
>>   		return -EIO;
>>   	}
>>   
>> @@ -381,7 +388,8 @@ int xe_sysctrl_send_command(struct xe_sysctrl *sc,
>>   				      cmd->data_in, cmd->data_in_len,
>>   				      &mbox_cmd, &cmd_size);
>>   	if (ret) {
>> -		xe_err(xe, "sysctrl: Failed to prepare command: %pe\n", ERR_PTR(ret));
>> +		xe_log_err(xe, SYSCTRL, ret,
>> +			   "Failed to prepare command\n");
> nit: do we need to split the line here?
seems, not required.
> and maybe it is good to know which command actually failed?
>
Sure, will add that cmd.
>>   		return ret;
>>   	}
>>   
>> @@ -391,7 +399,8 @@ int xe_sysctrl_send_command(struct xe_sysctrl *sc,
>>   				   cmd->data_out, cmd->data_out_len, rdata_len,
>>   				   XE_SYSCTRL_MB_DEFAULT_TIMEOUT_MS);
>>   	if (ret)
>> -		xe_err(xe, "sysctrl: Mailbox command failed: %pe\n", ERR_PTR(ret));
>> +		xe_log_err(xe, SYSCTRL, ret,
>> +			   "Mailbox command failed\n");
> ditto
ditto :)
>
>>   	kfree(mbox_cmd);
>>