Claude review: accel/amdxdna: Improve tracing for job lifecycle and mailbox RX worker

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Removal of timeout tracepoint (aie2_ctx.c, deletion at line ~437):**

The patch removes the trace from `aie2_sched_job_timedout`:
```c
-	trace_xdna_job(sched_job, hwctx->name, "job timedout", job->seq);
```
This is a regression in observability. The timeout path is arguably the *most* important place to have a tracepoint — it fires when something has gone wrong. The commit message says nothing about why this was removed. If it was removed because the new signature requires an opcode and the author didn't want to handle it here, the same `job->drv_cmd ? job->drv_cmd->opcode : DEFAULT_IO` pattern used everywhere else should be applied here too. This should be restored.

**Also in that hunk — context mismatch with upstream:**

The patch context shows:
```c
 	guard(mutex)(&xdna->dev_lock);
```
But upstream has `mutex_lock(&xdna->dev_lock)` followed by explicit `mutex_unlock` (aie2_ctx.c:419–434). This means the patch was authored against a different tree version and will not apply cleanly to drm-next. The author should rebase.

**DEFAULT_IO enum value (amdxdna_ctx.h):**

```c
 enum amdxdna_job_opcode {
+	DEFAULT_IO,
 	SYNC_DEBUG_BO,
```

Adding `DEFAULT_IO` as the first enumerant (value 0) shifts all existing enum values (`SYNC_DEBUG_BO` becomes 1, etc.). If any code or firmware interface depends on the numeric values of `SYNC_DEBUG_BO`, `ATTACH_DEBUG_BO`, or `DETACH_DEBUG_BO`, this is a breaking change. Checking `aie2_sched_job_run` in the existing code shows these opcodes are matched in a switch statement (lines 358–368), so the driver-side should be fine since it uses the symbolic names, but this deserves explicit confirmation that firmware doesn't depend on the numeric values. If it's purely driver-internal, it's fine but a comment noting that would help.

More substantively, the name `DEFAULT_IO` is vague. It's used as a fallback when `job->drv_cmd` is NULL, meaning it's a regular user command (not a driver internal command). Something like `NORMAL_EXEC` or `NO_DRV_CMD` would be clearer about what it represents.

**Repeated ternary pattern (aie2_ctx.c):**

The expression `job->drv_cmd ? job->drv_cmd->opcode : DEFAULT_IO` appears 4 times:
```c
job->drv_cmd ? job->drv_cmd->opcode : DEFAULT_IO  // signaling fence
job->drv_cmd ? job->drv_cmd->opcode : DEFAULT_IO  // job run
job->drv_cmd ? job->drv_cmd->opcode : DEFAULT_IO  // sent to device
job->drv_cmd ? job->drv_cmd->opcode : DEFAULT_IO  // job free
```
This is a minor style nit, but a small static inline helper like `amdxdna_job_opcode(job)` would reduce the repetition and make it harder to get wrong if the fallback logic ever changes.

**Stray blank line addition (aie2_ctx.c, line ~66):**

```c
 	job = container_of(ref, struct amdxdna_sched_job, refcnt);
+
 	amdxdna_sched_job_cleanup(job);
```

This is a whitespace-only change unrelated to tracing and shouldn't be part of this patch (or mentioned in the commit message if intentional).

**Stray blank line removal (amdxdna_ctx.c, line ~517):**

```c
-
 	job->hwctx = hwctx;
```

Similarly, this cosmetic fix is unrelated to the tracing changes.

**New trace in submit ioctl (amdxdna_ctx.c):**

```c
+	trace_amdxdna_debug_point(current->comm, args->type, "job received");
+
 	switch (args->type) {
```

This is fine and useful — it traces before the switch dispatch, capturing the process name and command type. The `amdxdna_debug_point` prototype is `(const char *name, u64 number, const char *str)`, so `args->type` (a `u32`) will be implicitly widened to `u64`, which is correct.

**Trace header changes (amdxdna.h) — TP_printk format fix:**

The format string change from `%lld` to `%llu` for `fence_seqno` and `seq`:
```c
-	    TP_printk("fence=(context:%llu, seqno:%lld), %s seq#:%lld %s",
+	    TP_printk("fence=(context:%llu, seqno:%llu), %s seq#:%llu %s, op=%u",
```
This is a good fix — both `fence_seqno` and `seq` are `u64` fields, so `%llu` is correct. The prior `%lld` was technically wrong for unsigned values.

**Event class refactoring (amdxdna.h):**

The refactoring of `mbox_irq_handle` from a standalone `TRACE_EVENT` to a `DECLARE_EVENT_CLASS`/`DEFINE_EVENT` pair with `xdna_mbox_name_id` is the correct pattern for sharing a trace event structure between multiple events (`mbox_irq_handle` and the new `mbox_rx_worker`). This is clean.

**Mailbox RX worker trace (amdxdna_mailbox.c):**

```c
+	trace_mbox_rx_worker(MAILBOX_NAME, mb_chann->msix_irq);
```

This is straightforward and useful for debugging mailbox activity.

---

**Summary of requested changes:**
1. **Restore** the `trace_xdna_job` in `aie2_sched_job_timedout` (with the new opcode argument).
2. **Rebase** against current drm-next — the `guard(mutex)` context doesn't match.
3. Consider renaming `DEFAULT_IO` to something more descriptive, and confirm the enum value shift doesn't affect firmware ABI.
4. Drop the unrelated whitespace changes, or mention them in the commit message.

---
Generated by Claude Code Patch Reviewer