Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] accel: ethosu: broken reset error path in ethosu_job_timedout() - [High] accel: ethosu: synchronize_irq() after clock disable in ethosu_job_timedout() - [Medium] accel: ethosu: race in ethosu_job_timedout() clears next job in_flight_job - [Low] accel: ethosu: missing job_lock member in struct ethosu_device --
commit a2791ceca00bb04bd66b56abb41eced27fba55c3 Author: Rob Herring (Arm) <[email protected]> accel: ethosu: Serialize job completion This commit adds spinlock helpers around in_flight_job updates and drains pending completion IRQs during timeout recovery so a late completion cannot overwrite the tracking pointer of a newly scheduled job. > diff --git a/drivers/accel/ethosu/ethosu_device.h > b/drivers/accel/ethosu/ethosu_device.h > index 1eca8590e68d4..51d5bc5d6137d 100644 > --- a/drivers/accel/ethosu/ethosu_device.h > +++ b/drivers/accel/ethosu/ethosu_device.h > @@ -7,6 +7,7 @@ > #include <linux/bitfield.h> > #include <linux/bits.h> > #include <linux/mutex.h> > +#include <linux/spinlock.h> > #include <linux/types.h> > > #include <drm/drm_device.h> [Severity: Low] Does this commit fail to build when CONFIG_DRM_ACCEL_ARM_ETHOSU is enabled? This patch includes linux/spinlock.h in drivers/accel/ethosu/ethosu_device.h and uses dev->job_lock in ethosu_job_set_in_flight(), ethosu_job_clear_in_flight(), and ethosu_job_init(), but struct ethosu_device does not declare job_lock yet: drivers/accel/ethosu/ethosu_device.h: struct ethosu_job *in_flight_job; /* For dma_fence */ spinlock_t fence_lock; The spinlock_t job_lock member declaration appears to have been placed in a later commit in the series, commit b4de18ee902b ("accel: ethosu: Keep scheduler after unplug"), which breaks compilation and git bisectability at this commit. Should the job_lock declaration in struct ethosu_device be moved into this patch? > diff --git a/drivers/accel/ethosu/ethosu_job.c > b/drivers/accel/ethosu/ethosu_job.c > index d418ae7961cff..f22cacb19e3dc 100644 > --- a/drivers/accel/ethosu/ethosu_job.c > +++ b/drivers/accel/ethosu/ethosu_job.c > @@ -42,6 +42,25 @@ static const struct dma_fence_ops ethosu_fence_ops = { > .get_timeline_name = ethosu_fence_get_timeline_name, > }; > > +static void ethosu_job_set_in_flight(struct ethosu_device *dev, > + struct ethosu_job *job) > +{ > + guard(spinlock)(&dev->job_lock); > + dev->in_flight_job = job; > +} > + > +static struct ethosu_job * > +ethosu_job_clear_in_flight(struct ethosu_device *dev) > +{ > + struct ethosu_job *job; > + > + guard(spinlock)(&dev->job_lock); > + job = dev->in_flight_job; > + dev->in_flight_job = NULL; > + > + return job; > +} [ ... ] > @@ -311,11 +329,19 @@ static enum drm_gpu_sched_stat > ethosu_job_timedout(struct drm_sched_job *bad) > > drm_sched_stop(&dev->sched, bad); > > - WRITE_ONCE(dev->in_flight_job, NULL); > + ethosu_job_clear_in_flight(dev); [Severity: Medium] Can ethosu_job_timedout() still clear the next job's in_flight_job pointer and reset the NPU while a newly submitted job is running? When work_tdr calls drm_sched_job_timedout(), the timed-out job (bad) is removed from sched->pending_list before ethosu_job_timedout(bad) is invoked, while sched->submit_wq remains active until drm_sched_stop() is called: ethosu_job_timedout() cmdaddr = readl_relaxed(dev->regs + NPU_REG_QREAD); running = FIELD_GET(STATUS_STATE_RUNNING, ...); if (running) readl_relaxed_poll_timeout(...); dev_err(...); drm_sched_stop(&dev->sched, bad); ethosu_job_clear_in_flight(dev); If bad completes just before NPU_REG_STATUS is read (so running is false) or right after readl_relaxed_poll_timeout() expires while dev_err() is running, ethosu_job_irq_handler_thread()->ethosu_job_handle_irq() clears dev->in_flight_job and signals bad->done_fence. Signaling bad->done_fence runs drm_sched_job_done_cb()->drm_sched_job_done(), which decrements sched->credit_count and queues work_free_job and drm_sched_run_job_work(). Because drm_sched_stop() has not been called yet, drm_sched_run_job_work()->ethosu_job_run() can set dev->in_flight_job to the next job and submit it to the hardware. When ethosu_job_timedout() then calls drm_sched_stop(&dev->sched, bad) and ethosu_job_clear_in_flight(dev), wouldn't it unconditionally clear the new job's dev->in_flight_job pointer, reset the NPU underneath it, and cancel the new job during drm_sched_start(&dev->sched, 0)? > /* Proceed with reset now. */ > - pm_runtime_force_suspend(dev->base.dev); > - pm_runtime_force_resume(dev->base.dev); > + ret = pm_runtime_force_suspend(dev->base.dev); > + if (!ret) > + ret = pm_runtime_force_resume(dev->base.dev); > + if (ret) { > + dev_err(dev->base.dev, "Failed to recover NPU: %d\n", ret); > + return DRM_GPU_SCHED_STAT_ENODEV; > + } [Severity: High] What happens on this error path if CONFIG_PM_SLEEP is disabled, or if pm_runtime_force_suspend() or pm_runtime_force_resume() fails? First, DRM_ACCEL_ARM_ETHOSU does not depend on CONFIG_PM_SLEEP. When CONFIG_PM_SLEEP is disabled, pm_runtime_force_resume() in include/linux/pm_runtime.h is a stub that unconditionally returns -ENXIO: static inline int pm_runtime_force_suspend(struct device *dev) { return 0; } static inline int pm_runtime_force_resume(struct device *dev) { return -ENXIO; } When CONFIG_PM=y and CONFIG_PM_SLEEP=n, pm_runtime_force_suspend() disables runtime PM and calls ethosu_device_suspend() to turn off the clocks, and then pm_runtime_force_resume() returns -ENXIO, causing every timeout recovery to fail with the NPU left suspended. Second, before this check, ethosu_job_timedout() calls drm_sched_stop(&dev->sched, bad), which removes &bad->cb from bad->s_fence->parent (job->done_fence) and sets bad->s_fence->parent = NULL. Returning DRM_GPU_SCHED_STAT_ENODEV here skips drm_sched_start(&dev->sched, 0), which would otherwise see s_job->s_fence->parent == NULL and call drm_sched_job_done(s_job, -ECANCELED) to signal bad->s_fence->finished (job->inference_done_fence). Even during subsequent teardown in ethosu_job_fini()->drm_sched_fini(), ethosu_job_cancel() only signals job->done_fence: static void ethosu_job_cancel(struct drm_sched_job *sched_job) { struct ethosu_job *job = to_ethosu_job(sched_job); if (dma_fence_was_initialized(job->done_fence) && !dma_fence_is_signaled(job->done_fence)) { dma_fence_set_error(job->done_fence, -ECANCELED); dma_fence_signal(job->done_fence); } } Because drm_sched_stop() already detached &bad->cb from job->done_fence, signaling job->done_fence will not signal bad->s_fence->finished. Wouldn't that leave bad->s_fence->finished permanently unsignaled, hanging callers in ethosu_ioctl_bo_wait() and drm_sched_entity_kill() and triggering a warning in dma_fence_release() when ethosu_job_free() frees the job? > + > + /* Drain a pending completion IRQ before a replacement job can run. */ > + synchronize_irq(dev->irq); [Severity: High] Is calling synchronize_irq(dev->irq) after pm_runtime_force_suspend() safe if an IRQ is already in flight? Both ethosu_job_irq_handler() and ethosu_job_irq_handler_thread()->ethosu_job_handle_irq() read NPU_REG_STATUS from dev->regs: static void ethosu_job_handle_irq(struct ethosu_device *dev) { u32 status = readl_relaxed(dev->regs + NPU_REG_STATUS); struct ethosu_job *job; if (status & (STATUS_BUS_STATUS | STATUS_CMD_PARSE_ERR)) { dev_err(dev->base.dev, "Error IRQ - %x\n", status); drm_sched_fault(&dev->sched); return; } In ethosu_job_timedout(), pm_runtime_force_suspend(dev->base.dev) runs ethosu_device_suspend()->clk_bulk_disable_unprepare() before synchronize_irq(dev->irq) is called (and synchronize_irq() is skipped entirely if the suspend/resume check returns early). If an interrupt fires concurrently with ethosu_job_timedout(), could ethosu_job_irq_handler() or ethosu_job_handle_irq() read NPU_REG_STATUS while the NPU clocks are disabled during the window between pm_runtime_force_suspend() and pm_runtime_force_resume(), or call drm_sched_fault(&dev->sched) after drm_sched_stop() cancelled sched->work_tdr? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
