Re: [PATCH v4 08/31] accel: ethosu: Serialize job completion

[sashiko-bot@kernel.org]

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) <robh@kernel.org>

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/20261029-ethosu-fixes-v4-0-4aa25e22578e@kernel.org?part=8