From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: accel: ethosu: Remove redundant job_lock
Date: Mon, 18 May 2026 16:46:57 +1000
Message-ID: <review-patch1-20260516144623.2582427-2-mcanal@igalia.com>
In-Reply-To: <20260516144623.2582427-2-mcanal@igalia.com>
References: <20260516144623.2582427-2-mcanal@igalia.com> <20260516144623.2582427-2-mcanal@igalia.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Commit message quality**: Excellent. The three-scenario analysis is accur=
ate and demonstrates deep understanding of the DRM scheduler's guarantees a=
nd the concurrency model.

**Header change** (`ethosu_device.h`):
```c
-	/* For in_flight_job and ethosu_job_hw_submit() */
-	struct mutex job_lock;
```
Clean removal. The `in_flight_job` field remains but is now protected by `W=
RITE_ONCE`/`READ_ONCE` + scheduler serialization rather than the mutex.

**`ethosu_job_run()`** (lines 197=E2=80=93198):
```c
	WRITE_ONCE(dev->in_flight_job, job);
	ethosu_job_hw_submit(dev, job);
```
Correct. `WRITE_ONCE` prevents the compiler from tearing or eliding the sto=
re. The subsequent `writel()` in `ethosu_job_hw_submit()` provides a write =
memory barrier, ensuring the store to `in_flight_job` is globally visible b=
efore the HW processes the command and raises an interrupt.

**`ethosu_job_handle_irq()`** (lines 214=E2=80=93218):
```c
	job =3D READ_ONCE(dev->in_flight_job);
	if (job) {
		WRITE_ONCE(dev->in_flight_job, NULL);
		dma_fence_signal(job->done_fence);
	}
```
This is the most critical change. Two things to verify:

1. **Ordering of clear-before-signal**: The `WRITE_ONCE(NULL)` must be visi=
ble to other CPUs before `dma_fence_signal()` wakes the scheduler. This hol=
ds because `dma_fence_signal()` internally acquires a spinlock (`spin_lock_=
irqsave`), which acts as an acquire barrier =E2=80=94 all preceding stores =
(including the NULL write) are committed before the lock is acquired and be=
fore any waiter is woken. So `run_job(next)` on another CPU will observe `i=
n_flight_job =3D=3D NULL`. **Correct.**

2. **Race with `timedout_job()`**: If `timedout_job()` concurrently clears =
`in_flight_job`, the IRQ handler's local `job` copy (captured by `READ_ONCE=
`) remains valid because the job isn't freed until the scheduler calls `fre=
e_job`, which can't happen until after `done_fence` is signaled or the sche=
duler reprocesses the timed-out job. No use-after-free risk. **Correct.**

**`ethosu_job_timedout()`** (line 273):
```c
	WRITE_ONCE(dev->in_flight_job, NULL);
```
Called after `drm_sched_stop()`, which synchronizes with the scheduler's wo=
rkqueue but not with the threaded IRQ handler. The `WRITE_ONCE` ensures the=
 compiler emits a single atomic store. If the IRQ handler has already captu=
red the old pointer via `READ_ONCE`, it operates on its local copy safely (=
see scenario 3 in the commit message). **Correct.**

**`ethosu_job_init()`** (lines 303=E2=80=93305):
```c
-	ret =3D devm_mutex_init(dev, &edev->job_lock);
-	if (ret)
-		return ret;
```
Clean removal of initialization code.

**Minor observation**: It might be worth adding a brief comment on `in_flig=
ht_job` in the struct definition noting the access protocol (e.g., `/* Writ=
ten with WRITE_ONCE by run_job/timedout_job, read with READ_ONCE by IRQ han=
dler */`), since the locking is now implicit rather than explicit. But this=
 is optional and a matter of taste =E2=80=94 the commit message serves as t=
he permanent record.

**No bugs found. Reviewed-by worthy.**

---
Generated by Claude Code Patch Reviewer