rocket_job_handle_irq() writes OPERATION_ENABLE and INTERRUPT_CLEAR before
taking job_lock, while rocket_job_hw_submit() writes OPERATION_ENABLE from
inside it. The two can therefore race: a completion being handled on one
core can write its zero after a submit on the same core has written its
one, and stop a task that has only just started.
Nothing in tree hits this often, because the interrupt is the only
completion path and it does not overlap its own submit, but the ordering is
wrong on its own terms.
To be exact about what the lock does and does not buy: a mutex gives mutual
exclusion, not ordering, so it does not by itself stop a zero from landing
after a one. What keeps the ordinary path safe is that the handler signals
the job's done fence before the scheduler can issue the next one. The
reason the writes belong inside the guard is that stopping the block and
deciding what to start next have to be one step, which they were not.
Move both writes inside the existing scoped_guard() rather than adding a
second critical section, so stopping the block and deciding what to start
next are one atomic step.
Fixes: 0810d5ad88a1 ("accel/rocket: Add job submission IOCTL")
Signed-off-by: Jiaxing Hu <[email protected]>
Tested-by: Igor Paunovic <[email protected]> # RK3588, three cores, induced
reset, JOB_TIMEOUT_MS=2
---
drivers/accel/rocket/rocket_job.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/accel/rocket/rocket_job.c
b/drivers/accel/rocket/rocket_job.c
index f40435505..575945015 100644
--- a/drivers/accel/rocket/rocket_job.c
+++ b/drivers/accel/rocket/rocket_job.c
@@ -345,10 +345,15 @@ static void rocket_job_handle_irq(struct rocket_core
*core)
{
pm_runtime_mark_last_busy(core->dev);
- rocket_pc_writel(core, OPERATION_ENABLE, 0x0);
- rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff);
+ scoped_guard(mutex, &core->job_lock) {
+ /*
+ * Stopping the block belongs under the lock. hw_submit() writes
+ * OPERATION_ENABLE too, and outside the lock this zero can land
+ * after that one and stop a task that has only just started.
+ */
+ rocket_pc_writel(core, OPERATION_ENABLE, 0x0);
+ rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff);
- scoped_guard(mutex, &core->job_lock)
if (core->in_flight_job) {
if (core->in_flight_job->next_task_idx <
core->in_flight_job->task_count) {
rocket_job_hw_submit(core, core->in_flight_job);
@@ -360,6 +365,7 @@ static void rocket_job_handle_irq(struct rocket_core *core)
pm_runtime_put_autosuspend(core->dev);
core->in_flight_job = NULL;
}
+ }
}
static void
--
2.43.0
