Re: [PATCH v4 10/31] accel: ethosu: Keep scheduler after unplug

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] accel: ethosu: race with work_tdr and pending_list leak in ethosu_job_stop()
- [High] accel: ethosu: double clock disable and RPM imbalance in ethosu_remove()
- [Medium] accel: ethosu: premature entity kill on file close in ethosu_job_close()
--

commit b4de18ee902b7d8141192389e4617bb0996bcadc
Author: Rob Herring (Arm) <robh@kernel.org>

accel: ethosu: Keep scheduler after unplug

This commit defers scheduler teardown until the final DRM device reference is
released and tracks open scheduling entities so device removal can stop each
entity and cancel the in-flight job while leaving the scheduler alive for
postclose handling.

> diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
> index 91b2c2ab0a03e..ee50e7db36ad1 100644
> --- a/drivers/accel/ethosu/ethosu_drv.c
> +++ b/drivers/accel/ethosu/ethosu_drv.c
[ ... ]
> @@ -389,20 +389,18 @@ static int ethosu_probe(struct platform_device *pdev)
[ ... ]
>  static void ethosu_remove(struct platform_device *pdev)
>  {
>  	struct ethosu_device *ethosudev = dev_get_drvdata(&pdev->dev);
> +	bool clocks_disabled;
>  
>  	drm_dev_unplug(&ethosudev->base);
> -	ethosu_job_fini(ethosudev);
> -
>  	pm_runtime_barrier(&pdev->dev);
> -	if (!pm_runtime_status_suspended(&pdev->dev))
> +	clocks_disabled = ethosu_job_stop(ethosudev);
> +	if (!clocks_disabled && !pm_runtime_status_suspended(&pdev->dev))
>  		ethosu_device_suspend(&pdev->dev);

[Severity: High]
Can calling ethosu_device_suspend() directly here (or inside ethosu_job_stop()
when ethosu_device_reset() fails) cause clk_bulk_disable_unprepare() to run a
second time during devres teardown?

In ethosu_init(), the driver enables autosuspend and registers runtime PM
cleanup via devm_pm_runtime_set_active_enabled().

In ethosu_remove(), pm_runtime_barrier() is called before ethosu_job_stop().
Inside ethosu_job_stop(), ethosu_perfmon_stop_locked() calls pm_runtime_put(),
and flushing work_free_job runs ethosu_job_cleanup() ->
pm_runtime_put_autosuspend(), which drops usage_count and arms the autosuspend
timer while runtime_status remains RPM_ACTIVE.

When ethosu_device_suspend() is then called directly, it disables and
unprepares the clocks without updating runtime_status to RPM_SUSPENDED or
disabling runtime PM. When ethosu_remove() returns and devres_release_all()
runs pm_runtime_disable_action() -> pm_runtime_dont_use_autosuspend() ->
rpm_idle() -> rpm_suspend() (or if the autosuspend timer fires first),
runtime PM invokes ethosu_device_suspend() again on already-disabled clocks.

In addition, for queued entity jobs killed via drm_sched_entity_kill() in
ethosu_job_stop(), cleanup runs asynchronously via
drm_sched_entity_kill_jobs_work(). Because ethosu_job_stop() does not wait for
those killed jobs (such as jobs waiting on external dependencies kept alive via
keep_device), won't they retain their pm_runtime_resume_and_get() usage_count
references across ethosu_remove() and later call pm_runtime_put_autosuspend()
in ethosu_job_cleanup() after the device has been unbound?

> diff --git a/drivers/accel/ethosu/ethosu_job.c b/drivers/accel/ethosu/ethosu_job.c
> index 8ffdc83d06f98..ef95a8abd61e2 100644
> --- a/drivers/accel/ethosu/ethosu_job.c
> +++ b/drivers/accel/ethosu/ethosu_job.c
[ ... ]
> @@ -388,45 +421,76 @@ int ethosu_job_init(struct ethosu_device *edev)
[ ... ]
> -void ethosu_job_fini(struct ethosu_device *dev)
> +bool ethosu_job_stop(struct ethosu_device *dev)
>  {
> +	struct ethosu_file_priv *priv;
> +	struct ethosu_job *job;
> +	bool clocks_disabled = false;
> +
> +	guard(mutex)(&dev->sched_lock);
>  	drm_sched_wqueue_stop(&dev->sched);
> +	disable_irq(dev->irq);
>  	cancel_delayed_work_sync(&dev->sched.work_tdr);
>  
> -	if (READ_ONCE(dev->in_flight_job)) {
> -		WRITE_ONCE(dev->in_flight_job, NULL);
> -		ethosu_device_reset(dev);
> +	scoped_guard(mutex, &dev->perfmon_state.lock)
> +		ethosu_perfmon_stop_locked(dev, dev->perfmon_state.active, false);
> +
> +	list_for_each_entry(priv, &dev->sched_entities, sched_entity_list)
> +		drm_sched_entity_kill(&priv->sched_entity);
> +
> +	job = ethosu_job_clear_in_flight(dev);
> +	if (job && ethosu_device_reset(dev)) {
> +		ethosu_device_suspend(dev->base.dev);
> +		clocks_disabled = true;
>  	}
>  
> -	synchronize_irq(dev->irq);
> -	drm_sched_fini(&dev->sched);
> +	if (job)
> +		ethosu_job_cancel(&job->base);
> +
> +	/* Let drm-sched release the cancelled in-flight job. */
> +	drm_sched_wqueue_start(&dev->sched);
> +	flush_work(&dev->sched.work_free_job);
> +	drm_sched_wqueue_stop(&dev->sched);
> +
> +	return clocks_disabled;
>  }

[Severity: High]
What happens in ethosu_job_stop() if work_tdr (drm_sched_job_timedout() ->
ethosu_job_timedout()) is already running or has previously failed recovery?

First, drm_sched_wqueue_stop() and disable_irq() are called before
cancel_delayed_work_sync(&dev->sched.work_tdr) and before killing entities with
drm_sched_entity_kill(). Since ethosu_job_timedout() does not take
dev->sched_lock:

drivers/accel/ethosu/ethosu_job.c:ethosu_job_timedout() {
	...
	drm_sched_stop(&dev->sched, bad);

	ethosu_job_clear_in_flight(dev);

	/* Proceed with reset now. */
	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;
	}

	/* Drain a pending completion IRQ before a replacement job can run. */
	synchronize_irq(dev->irq);

	/* Restart the scheduler */
	drm_sched_start(&dev->sched, 0);
	...
}

an in-progress ethosu_job_timedout() can finish recovery while
ethosu_job_stop() is waiting in cancel_delayed_work_sync(&dev->sched.work_tdr)
and call drm_sched_start() -> drm_sched_wqueue_start() (as well as
drm_sched_start_timeout_unlocked() in drm_sched_job_timedout()). When
cancel_delayed_work_sync() returns, sched->pause_submit is false and
work_run_job is queued while dev->sched_entities are not yet killed. Can
drm_sched_run_job_work() then pop a job from an entity before
drm_sched_entity_kill() reaches it, submit it to hardware via ethosu_job_run()
with IRQs already disabled, and re-arm work_tdr after
cancel_delayed_work_sync() has completed?

Second, if a prior timeout recovery in ethosu_job_timedout() failed at
pm_runtime_force_suspend() or pm_runtime_force_resume() and returned
DRM_GPU_SCHED_STAT_ENODEV, drm_sched_stop() has already placed the timed-out
job back onto dev->sched.pending_list and detached its parent fence callback,
while ethosu_job_clear_in_flight() cleared dev->in_flight_job to NULL.

Because ethosu_job_push() now takes a drm_dev_get() and try_module_get()
reference on every pushed job until ethosu_job_cleanup(), and drm_sched_fini()
(which calls drm_sched_cancel_remaining_jobs()) is deferred to the drmm action
ethosu_job_fini(), ethosu_job_stop() sees ethosu_job_clear_in_flight(dev) ==
NULL and never cancels the job left on dev->sched.pending_list. Doesn't this
create a reference cycle where the unsignaled job on pending_list (and any
entity jobs chained to entity->last_scheduled in drm_sched_entity_kill()) is
never freed, leaking the drm_device, THIS_MODULE, GEM objects, and runtime PM
references?

>  
>  int ethosu_job_open(struct ethosu_file_priv *ethosu_priv)
>  {
>  	struct ethosu_device *dev = ethosu_priv->edev;
>  	struct drm_gpu_scheduler *sched = &dev->sched;
> +	int ret;
>  
> -	return drm_sched_entity_init(&ethosu_priv->sched_entity,
> -				     DRM_SCHED_PRIORITY_NORMAL,
> -				     &sched, 1, NULL);
> +	guard(mutex)(&dev->sched_lock);
> +	ret = drm_sched_entity_init(&ethosu_priv->sched_entity,
> +				    DRM_SCHED_PRIORITY_NORMAL,
> +				    &sched, 1, NULL);
> +	if (!ret)
> +		list_add(&ethosu_priv->sched_entity_list, &dev->sched_entities);
> +
> +	return ret;
>  }
>  
>  void ethosu_job_close(struct ethosu_file_priv *ethosu_priv)
>  {
> +	struct ethosu_device *dev = ethosu_priv->edev;
>  	struct drm_sched_entity *entity = &ethosu_priv->sched_entity;
>  
> +	scoped_guard(mutex, &dev->sched_lock) {
> +		drm_sched_entity_kill(entity);
> +		list_del(&ethosu_priv->sched_entity_list);
> +	}
>  	drm_sched_entity_destroy(entity);
>  }

[Severity: Medium]
Should ethosu_job_close() call drm_sched_entity_kill() unconditionally before
drm_sched_entity_destroy()?

During normal operation when the device has not been unplugged,
drm_sched_entity_destroy() calls drm_sched_entity_flush() to allow queued jobs
in entity->job_queue to be scheduled before drm_sched_entity_fini() kills the
entity.

Because the scheduler has credit_limit = 1, if userspace submits multiple jobs
or a job waiting on dependencies and then closes the DRM file descriptor,
calling drm_sched_entity_kill() first immediately sets entity->stopped = true
and aborts all unscheduled jobs in entity->job_queue with -ESRCH instead of
flushing them in drm_sched_entity_destroy().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-ethosu-fixes-v4-0-4aa25e22578e@kernel.org?part=10