[PATCH v4 00/31] accel: ethosu: Another batch of fixes

[PATCH v4 00/31] accel: ethosu: Another batch of fixes

[Rob Herring (Arm)]

This is another series of fixes to the ethosu driver. It's mostly probe 
error paths and cmd stream validation fixes. It's a mixture of 
AI reported issues, different solutions to other posted issues and 
fixes[1] and my own fixes. The last 2 patches are additional operation 
support.

The series adds rejecting OFM tranpose in cmd stream, but then a later 
patch adds support for it. I did this so the rejection can be 
backported, but maybe supporting OFM transpose is small enough that it 
should just be backported too?

Sashiko keeps raising issues about IRQs, but the h/w only generates an 
interrupt if a job was submitted. The scenarios sashiko talks about are 
not possible.

Rob

[1] https://lore.kernel.org/all/20260717061145.1478139-1-zhaoguohan@kylinos.cn

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
Changes in v4:
- More patches added for existing issues raised by Sashiko in v3 and 
  local runs
- Combined the 2 patches for unknown cmds and unhandled parameters into  
  one patch 
- Link to v3: https://patch.msgid.link/20260908-ethosu-fixes-v3-0-490fe215286f@kernel.org

Changes in v3:
- Address Sashiko comments
- Another runtime-PM fix for driver remove()
- Add a fix for NHCWB16 bounds calculation
- Link to v2: https://patch.msgid.link/20260904-ethosu-fixes-v2-0-3767738756a4@kernel.org

Changes in v2:
- Dropped first 5 patches which are already applied
- Patches 11-17 and 19 are new patches with more cmd stream validation
- Add fix to diable clocks on PM setup failure
- Rework the runtime-PM to suspend after initialization and before 
  drm_dev_register(). This simplifies the error clean-up in probe.
- Handle quiescing jobs before scheduler teardown
- Rework the cmd stream register initialization tracking to use a bitmap 
  of registers instead of relying on initial state values.
- Handle U85 DMA 2D/3D modes correctly
- Link to v1: https://patch.msgid.link/20260827-ethosu-fixes-v1-0-346f9ea8791c@kernel.org

---
Rob Herring (Arm) (31):
      accel: ethosu: Suspend after initialization
      accel: ethosu: Ensure suspended on removal
      accel: ethosu: Fix probe error cleanup
      accel: ethosu: Disable clocks on PM setup failure
      accel: ethosu: Quiesce jobs before scheduler teardown
      accel: ethosu: Fix runtime PM get error handling
      accel: ethosu: Limit perfmon counters to UAPI
      accel: ethosu: Serialize job completion
      accel: ethosu: Guard operations after unplug
      accel: ethosu: Keep scheduler after unplug
      accel: ethosu: Clear SRAM before submission
      accel: ethosu: Validate timeout command offset
      accel: ethosu: Prevent command stream export
      accel: ethosu: Move DMA mode to src/dst struct
      accel: ethosu: Track command stream register setup
      accel: ethosu: Factor buffer bounds checks
      accel: ethosu: Fix NHCWB16 bounds calculation
      accel: ethosu: Validate secondary streams
      accel: ethosu: Reject unsupported commands and params
      accel: ethosu: Validate all feature map tiles
      accel: ethosu: Account for feature map element size
      accel: ethosu: Validate convolution parameter
      accel: ethosu: Account for kernel dilation in IFM size
      accel: ethosu: Validate accumulator input
      accel: ethosu: Restrict dynamic IFM2 weights
      accel: ethosu: Split U65 and U85 DMA length validation
      accel: ethosu: Validate OFM transpose
      accel: ethosu: Validate resize operations
      accel: ethosu: Ensure cmd stream is multiple of 4 bytes
      accel: ethosu: Require active weight streams setup
      accel: ethosu: Support U85 IFM upscaling

 drivers/accel/ethosu/ethosu_device.h  |  40 ++
 drivers/accel/ethosu/ethosu_drv.c     |  56 +-
 drivers/accel/ethosu/ethosu_drv.h     |   6 +
 drivers/accel/ethosu/ethosu_gem.c     | 951 +++++++++++++++++++++++++++++++---
 drivers/accel/ethosu/ethosu_job.c     | 236 +++++++--
 drivers/accel/ethosu/ethosu_job.h     |   5 +-
 drivers/accel/ethosu/ethosu_perfmon.c | 123 +++--
 7 files changed, 1236 insertions(+), 181 deletions(-)
---
base-commit: 9a96e169c9267af918d28fb291df948d83112807
change-id: 20260827-ethosu-fixes-ee58386e9000

Best regards,
--  
Rob Herring (Arm) <robh@kernel.org>

[PATCH v4 01/31] accel: ethosu: Suspend after initialization

[Rob Herring (Arm)]

The initial runtime-PM reference is held only while initializing the
NPU. Release it synchronously from ethosu_init() after the final
hardware access, before registering the DRM device.

This keeps the runtime-PM setup and initial reference handling together
and leaves the autosuspend configuration in place for subsequent jobs.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v2:
 - new patch
---
 drivers/accel/ethosu/ethosu_drv.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
index 4a24bbd2bb61..38af57807cee 100644
--- a/drivers/accel/ethosu/ethosu_drv.c
+++ b/drivers/accel/ethosu/ethosu_drv.c
@@ -338,6 +338,8 @@ static int ethosu_init(struct ethosu_device *ethosudev)
 		 ethosudev->npu_info.sram_size / 1024,
 		 ethosudev->npu_info.pmu_counters);
 
+	pm_runtime_put_sync_suspend(ethosudev->base.dev);
+
 	return 0;
 }
 
@@ -376,10 +378,6 @@ static int ethosu_probe(struct platform_device *pdev)
 		return ret;
 
 	ret = drm_dev_register(&ethosudev->base, 0);
-	if (ret)
-		pm_runtime_dont_use_autosuspend(ethosudev->base.dev);
-
-	pm_runtime_put_autosuspend(ethosudev->base.dev);
 	return ret;
 }
 

-- 
2.53.0

Re: [PATCH v4 01/31] accel: ethosu: Suspend after initialization

[Frank Li]

On Thu, Oct 29, 2026 at 08:35:48AM -0500, Rob Herring (Arm) wrote:
> The initial runtime-PM reference is held only while initializing the
> NPU. Release it synchronously from ethosu_init() after the final
> hardware access, before registering the DRM device.
>
> This keeps the runtime-PM setup and initial reference handling together
> and leaves the autosuspend configuration in place for subsequent jobs.
>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> v2:
>  - new patch
> ---
>  drivers/accel/ethosu/ethosu_drv.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
> index 4a24bbd2bb61..38af57807cee 100644
> --- a/drivers/accel/ethosu/ethosu_drv.c
> +++ b/drivers/accel/ethosu/ethosu_drv.c
> @@ -338,6 +338,8 @@ static int ethosu_init(struct ethosu_device *ethosudev)
>  		 ethosudev->npu_info.sram_size / 1024,
>  		 ethosudev->npu_info.pmu_counters);
>
> +	pm_runtime_put_sync_suspend(ethosudev->base.dev);
> +
>  	return 0;
>  }
>
> @@ -376,10 +378,6 @@ static int ethosu_probe(struct platform_device *pdev)
>  		return ret;
>
>  	ret = drm_dev_register(&ethosudev->base, 0);
> -	if (ret)
> -		pm_runtime_dont_use_autosuspend(ethosudev->base.dev);
> -
> -	pm_runtime_put_autosuspend(ethosudev->base.dev);
>  	return ret;
>  }
>
>
> --
> 2.53.0
>

[PATCH v4 02/31] accel: ethosu: Ensure suspended on removal

[Rob Herring (Arm)]

If device removal occurs before an auto-suspend timeout occurs, the
suspend is cancelled and the device will be left with clocks running.
Make the remove() callback check this and force a suspend if the device
is currently active.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Reported-by: sashiko-bot@kernel.org
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v4:
 - Use pm_runtime_barrier() and call ethosu_device_suspend() directly.
   This works in the case of !CONFIG_PM or power/control=on whereas
   pm_runtime_get_if_active() did not.

v3:
 - New patch
---
 drivers/accel/ethosu/ethosu_drv.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
index 38af57807cee..ae7ebf29cb2f 100644
--- a/drivers/accel/ethosu/ethosu_drv.c
+++ b/drivers/accel/ethosu/ethosu_drv.c
@@ -387,6 +387,11 @@ static void ethosu_remove(struct platform_device *pdev)
 
 	drm_dev_unregister(&ethosudev->base);
 	ethosu_job_fini(ethosudev);
+
+	pm_runtime_barrier(&pdev->dev);
+	if (!pm_runtime_status_suspended(&pdev->dev))
+		ethosu_device_suspend(&pdev->dev);
+
 	if (ethosudev->sram)
 		gen_pool_free(ethosudev->srampool, (unsigned long)ethosudev->sram,
 			      ethosudev->npu_info.sram_size);

-- 
2.53.0

Re: [PATCH v4 02/31] accel: ethosu: Ensure suspended on removal

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] accel: ethosu: double ethosu_device_suspend() call in ethosu_remove()
--

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

accel: ethosu: Ensure suspended on removal

This commit adds a runtime PM barrier and a direct call to
ethosu_device_suspend() in ethosu_remove() when the device is not runtime
suspended, aiming to ensure clocks are disabled on device removal.

> diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
> index 38af57807cee7..ae7ebf29cb2f6 100644
> --- a/drivers/accel/ethosu/ethosu_drv.c
> +++ b/drivers/accel/ethosu/ethosu_drv.c
> @@ -387,6 +387,11 @@ static void ethosu_remove(struct platform_device *pdev)
>  
>  	drm_dev_unregister(&ethosudev->base);
>  	ethosu_job_fini(ethosudev);
> +
> +	pm_runtime_barrier(&pdev->dev);
> +	if (!pm_runtime_status_suspended(&pdev->dev))
> +		ethosu_device_suspend(&pdev->dev);

[Severity: High]
Can calling ethosu_device_suspend() directly here while runtime PM is still
enabled and runtime_status is RPM_ACTIVE cause ethosu_device_suspend() to be
invoked a second time during devres teardown, underflowing the clock enable
and prepare counts?

In ethosu_init() in drivers/accel/ethosu/ethosu_drv.c, runtime PM is
configured with autosuspend and devres:

	pm_runtime_set_autosuspend_delay(ethosudev->base.dev, 50);
	pm_runtime_use_autosuspend(ethosudev->base.dev);
	ret = devm_pm_runtime_set_active_enabled(ethosudev->base.dev);

During driver unbind or device removal under CONFIG_PM=y with
power/control=auto, __device_release_driver() calls pm_runtime_get_sync() and
pm_runtime_put_sync() before invoking ethosu_remove(), which leaves the
device in RPM_ACTIVE with usage_count == 0 and the autosuspend timer armed.

In ethosu_remove(), pm_runtime_barrier() cancels the autosuspend timer, and
ethosu_device_suspend() then runs:

	clk_bulk_disable_unprepare(ethosudev->num_clks, ethosudev->clks);

However, runtime PM remains enabled (disable_depth == 0) and runtime_status
remains RPM_ACTIVE. Immediately after ethosu_remove() returns,
__device_release_driver() -> device_unbind_cleanup() -> devres_release_all()
runs pm_runtime_disable_action() in drivers/base/power/runtime.c:

static void pm_runtime_disable_action(void *data)
{
	pm_runtime_dont_use_autosuspend(data);
	pm_runtime_disable(data);
}

Because pm_runtime_dont_use_autosuspend() runs before pm_runtime_disable(),
it calls __pm_runtime_use_autosuspend(dev, false) -> update_autosuspend() ->
rpm_idle(dev, RPM_AUTO) -> rpm_suspend(dev, RPM_AUTO). Since disable_depth is
0, usage_count is 0, use_autosuspend is now false, and runtime_status is
still RPM_ACTIVE, rpm_suspend() synchronously invokes ethosu_device_suspend()
a second time, calling clk_bulk_disable_unprepare() on clocks that were
already disabled and unprepared.

Also, while runtime_status remains RPM_ACTIVE after ethosu_device_suspend()
has disabled the clocks, could an open DRM file descriptor closing or issuing
an ioctl concurrently (such as ethosu_postclose() ->
ethosu_perfmon_close_file() -> ethosu_perfmon_stop_locked() checking
pm_runtime_get_if_active(), or ethosu_ioctl_perfmon_get_values() calling
pm_runtime_resume_and_get()) see RPM_ACTIVE and access ethosu->pmu_regs with
the NPU clocks disabled?

Would calling pm_runtime_dont_use_autosuspend(&pdev->dev) and
pm_runtime_disable(&pdev->dev) before checking pm_runtime_status_suspended()
(and calling pm_runtime_set_suspended(&pdev->dev) after
ethosu_device_suspend()) avoid both issues?

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

[PATCH v4 03/31] accel: ethosu: Fix probe error cleanup

[Rob Herring (Arm)]

Once the job scheduler has been initialized, failures from ethosu_init()
or drm_dev_register() return from probe without tearing it down. The
registration failure also leaves the SRAM-pool allocation in use, because
the platform remove callback is not called after a failed probe.

Unwind the initialized resources on both paths. Also do not call
drm_sched_fini() after a failed drm_sched_init(): the scheduler initializer
already unwinds its partial setup, while drm_sched_fini() requires a
successfully initialized scheduler.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v2:
 - Adjust for previous patch reworking runtime-PM suspend
---
 drivers/accel/ethosu/ethosu_drv.c | 13 ++++++++++++-
 drivers/accel/ethosu/ethosu_job.c |  6 +-----
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
index ae7ebf29cb2f..9ee144c41550 100644
--- a/drivers/accel/ethosu/ethosu_drv.c
+++ b/drivers/accel/ethosu/ethosu_drv.c
@@ -375,9 +375,20 @@ static int ethosu_probe(struct platform_device *pdev)
 
 	ret = ethosu_init(ethosudev);
 	if (ret)
-		return ret;
+		goto err_job_fini;
 
 	ret = drm_dev_register(&ethosudev->base, 0);
+	if (ret)
+		goto err_sram_free;
+
+	return 0;
+
+err_sram_free:
+	if (ethosudev->sram)
+		gen_pool_free(ethosudev->srampool, (unsigned long)ethosudev->sram,
+			      ethosudev->npu_info.sram_size);
+err_job_fini:
+	ethosu_job_fini(ethosudev);
 	return ret;
 }
 
diff --git a/drivers/accel/ethosu/ethosu_job.c b/drivers/accel/ethosu/ethosu_job.c
index d656b3ec670e..b7faa715b47e 100644
--- a/drivers/accel/ethosu/ethosu_job.c
+++ b/drivers/accel/ethosu/ethosu_job.c
@@ -353,14 +353,10 @@ int ethosu_job_init(struct ethosu_device *edev)
 	ret = drm_sched_init(&edev->sched, &args);
 	if (ret) {
 		dev_err(dev, "Failed to create scheduler: %d\n", ret);
-		goto err_sched;
+		return ret;
 	}
 
 	return 0;
-
-err_sched:
-	drm_sched_fini(&edev->sched);
-	return ret;
 }
 
 void ethosu_job_fini(struct ethosu_device *dev)

-- 
2.53.0

Re: [PATCH v4 03/31] accel: ethosu: Fix probe error cleanup

[Frank Li]

On Thu, Oct 29, 2026 at 08:35:50AM -0500, Rob Herring (Arm) wrote:
> Once the job scheduler has been initialized, failures from ethosu_init()
> or drm_dev_register() return from probe without tearing it down. The
> registration failure also leaves the SRAM-pool allocation in use, because
> the platform remove callback is not called after a failed probe.
>
> Unwind the initialized resources on both paths. Also do not call
> drm_sched_fini() after a failed drm_sched_init(): the scheduler initializer
> already unwinds its partial setup, while drm_sched_fini() requires a
> successfully initialized scheduler.
>
> Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
> v2:
>  - Adjust for previous patch reworking runtime-PM suspend
> ---
>  drivers/accel/ethosu/ethosu_drv.c | 13 ++++++++++++-
>  drivers/accel/ethosu/ethosu_job.c |  6 +-----
>  2 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
> index ae7ebf29cb2f..9ee144c41550 100644
> --- a/drivers/accel/ethosu/ethosu_drv.c
> +++ b/drivers/accel/ethosu/ethosu_drv.c
> @@ -375,9 +375,20 @@ static int ethosu_probe(struct platform_device *pdev)
>
>  	ret = ethosu_init(ethosudev);
>  	if (ret)
> -		return ret;
> +		goto err_job_fini;
>
>  	ret = drm_dev_register(&ethosudev->base, 0);
> +	if (ret)
> +		goto err_sram_free;
> +
> +	return 0;
> +
> +err_sram_free:
> +	if (ethosudev->sram)
> +		gen_pool_free(ethosudev->srampool, (unsigned long)ethosudev->sram,
> +			      ethosudev->npu_info.sram_size);

Does worth add devm_gen_pool_dma_alloc()?

Frank

> +err_job_fini:
> +	ethosu_job_fini(ethosudev);
>  	return ret;
>  }
>
> diff --git a/drivers/accel/ethosu/ethosu_job.c b/drivers/accel/ethosu/ethosu_job.c
> index d656b3ec670e..b7faa715b47e 100644
> --- a/drivers/accel/ethosu/ethosu_job.c
> +++ b/drivers/accel/ethosu/ethosu_job.c
> @@ -353,14 +353,10 @@ int ethosu_job_init(struct ethosu_device *edev)
>  	ret = drm_sched_init(&edev->sched, &args);
>  	if (ret) {
>  		dev_err(dev, "Failed to create scheduler: %d\n", ret);
> -		goto err_sched;
> +		return ret;
>  	}
>
>  	return 0;
> -
> -err_sched:
> -	drm_sched_fini(&edev->sched);
> -	return ret;
>  }
>
>  void ethosu_job_fini(struct ethosu_device *dev)
>
> --
> 2.53.0
>

Re: [PATCH v4 03/31] accel: ethosu: Fix probe error cleanup

[Rob Herring]

On Thu, Oct 29, 2026 at 11:52:14AM -0400, Frank Li wrote:
> On Thu, Oct 29, 2026 at 08:35:50AM -0500, Rob Herring (Arm) wrote:
> > Once the job scheduler has been initialized, failures from ethosu_init()
> > or drm_dev_register() return from probe without tearing it down. The
> > registration failure also leaves the SRAM-pool allocation in use, because
> > the platform remove callback is not called after a failed probe.
> >
> > Unwind the initialized resources on both paths. Also do not call
> > drm_sched_fini() after a failed drm_sched_init(): the scheduler initializer
> > already unwinds its partial setup, while drm_sched_fini() requires a
> > successfully initialized scheduler.
> >
> > Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > ---
> > v2:
> >  - Adjust for previous patch reworking runtime-PM suspend
> > ---
> >  drivers/accel/ethosu/ethosu_drv.c | 13 ++++++++++++-
> >  drivers/accel/ethosu/ethosu_job.c |  6 +-----
> >  2 files changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
> > index ae7ebf29cb2f..9ee144c41550 100644
> > --- a/drivers/accel/ethosu/ethosu_drv.c
> > +++ b/drivers/accel/ethosu/ethosu_drv.c
> > @@ -375,9 +375,20 @@ static int ethosu_probe(struct platform_device *pdev)
> >
> >  	ret = ethosu_init(ethosudev);
> >  	if (ret)
> > -		return ret;
> > +		goto err_job_fini;
> >
> >  	ret = drm_dev_register(&ethosudev->base, 0);
> > +	if (ret)
> > +		goto err_sram_free;
> > +
> > +	return 0;
> > +
> > +err_sram_free:
> > +	if (ethosudev->sram)
> > +		gen_pool_free(ethosudev->srampool, (unsigned long)ethosudev->sram,
> > +			      ethosudev->npu_info.sram_size);
> 
> Does worth add devm_gen_pool_dma_alloc()?

We want this backported to stable, so I think we want it as-is and then 
perhaps later add a helper.

Rob

Re: [PATCH v4 03/31] accel: ethosu: Fix probe error cleanup

[Frank Li]

On Thu, Oct 29, 2026 at 11:00:58AM -0500, Rob Herring wrote:
> On Thu, Oct 29, 2026 at 11:52:14AM -0400, Frank Li wrote:
> > On Thu, Oct 29, 2026 at 08:35:50AM -0500, Rob Herring (Arm) wrote:
> > > Once the job scheduler has been initialized, failures from ethosu_init()
> > > or drm_dev_register() return from probe without tearing it down. The
> > > registration failure also leaves the SRAM-pool allocation in use, because
> > > the platform remove callback is not called after a failed probe.
> > >
> > > Unwind the initialized resources on both paths. Also do not call
> > > drm_sched_fini() after a failed drm_sched_init(): the scheduler initializer
> > > already unwinds its partial setup, while drm_sched_fini() requires a
> > > successfully initialized scheduler.
> > >
> > > Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > > ---
> > > v2:
> > >  - Adjust for previous patch reworking runtime-PM suspend
> > > ---
> > >  drivers/accel/ethosu/ethosu_drv.c | 13 ++++++++++++-
> > >  drivers/accel/ethosu/ethosu_job.c |  6 +-----
> > >  2 files changed, 13 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
> > > index ae7ebf29cb2f..9ee144c41550 100644
> > > --- a/drivers/accel/ethosu/ethosu_drv.c
> > > +++ b/drivers/accel/ethosu/ethosu_drv.c
> > > @@ -375,9 +375,20 @@ static int ethosu_probe(struct platform_device *pdev)
> > >
> > >  	ret = ethosu_init(ethosudev);
> > >  	if (ret)
> > > -		return ret;
> > > +		goto err_job_fini;
> > >
> > >  	ret = drm_dev_register(&ethosudev->base, 0);
> > > +	if (ret)
> > > +		goto err_sram_free;
> > > +
> > > +	return 0;
> > > +
> > > +err_sram_free:
> > > +	if (ethosudev->sram)
> > > +		gen_pool_free(ethosudev->srampool, (unsigned long)ethosudev->sram,
> > > +			      ethosudev->npu_info.sram_size);
> >
> > Does worth add devm_gen_pool_dma_alloc()?
>
> We want this backported to stable, so I think we want it as-is and then
> perhaps later add a helper.

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>
> Rob

[PATCH v4 04/31] accel: ethosu: Disable clocks on PM setup failure

[Rob Herring (Arm)]

ethosu_init() enables the clocks directly to reset and query the NPU
before runtime PM takes over. If runtime-PM setup fails, the error path
returns while those clocks remain enabled.

Disable the clocks before returning the setup error to balance the
initial direct resume.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v2:
 - new patch
---
 drivers/accel/ethosu/ethosu_drv.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
index 9ee144c41550..1a65e9eef562 100644
--- a/drivers/accel/ethosu/ethosu_drv.c
+++ b/drivers/accel/ethosu/ethosu_drv.c
@@ -311,8 +311,10 @@ static int ethosu_init(struct ethosu_device *ethosudev)
 	pm_runtime_set_autosuspend_delay(ethosudev->base.dev, 50);
 	pm_runtime_use_autosuspend(ethosudev->base.dev);
 	ret = devm_pm_runtime_set_active_enabled(ethosudev->base.dev);
-	if (ret)
+	if (ret) {
+		ethosu_device_suspend(ethosudev->base.dev);
 		return ret;
+	}
 	pm_runtime_get_noresume(ethosudev->base.dev);
 
 	ethosudev->npu_info.id = id = readl_relaxed(ethosudev->regs + NPU_REG_ID);

-- 
2.53.0

Re: [PATCH v4 04/31] accel: ethosu: Disable clocks on PM setup failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] accel: ethosu: double clock disable in ethosu_init() on PM enable failure
--

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

accel: ethosu: Disable clocks on PM setup failure

This commit calls ethosu_device_suspend() on the error path when
devm_pm_runtime_set_active_enabled() fails in ethosu_init(), aiming to
disable the clocks that were enabled earlier by ethosu_device_resume().

> diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
> index 9ee144c41550c..1a65e9eef562f 100644
> --- a/drivers/accel/ethosu/ethosu_drv.c
> +++ b/drivers/accel/ethosu/ethosu_drv.c
> @@ -311,8 +311,10 @@ static int ethosu_init(struct ethosu_device *ethosudev)
>  	pm_runtime_set_autosuspend_delay(ethosudev->base.dev, 50);
>  	pm_runtime_use_autosuspend(ethosudev->base.dev);
>  	ret = devm_pm_runtime_set_active_enabled(ethosudev->base.dev);
> -	if (ret)
> +	if (ret) {
> +		ethosu_device_suspend(ethosudev->base.dev);
>  		return ret;
> +	}

[Severity: Medium]
Can this cause a double call to clk_bulk_disable_unprepare() on
ethosudev->clks if devm_pm_runtime_set_active_enabled() fails inside
devm_pm_runtime_enable()?

In drivers/accel/ethosu/ethosu_drv.c:ethosu_init(),
pm_runtime_use_autosuspend() is called and then
devm_pm_runtime_set_active_enabled() runs while dev->power.usage_count is
still 0, because pm_runtime_get_noresume() is only called after
devm_pm_runtime_set_active_enabled() succeeds.

Inside drivers/base/power/runtime.c:devm_pm_runtime_set_active_enabled():

int devm_pm_runtime_set_active_enabled(struct device *dev)
{
	int err;

	err = pm_runtime_set_active(dev);
	if (err)
		return err;

	err = devm_add_action_or_reset(dev, pm_runtime_set_suspended_action, dev);
	if (err)
		return err;

	return devm_pm_runtime_enable(dev);
}

And in drivers/base/power/runtime.c:devm_pm_runtime_enable():

static void pm_runtime_disable_action(void *data)
{
	pm_runtime_dont_use_autosuspend(data);
	pm_runtime_disable(data);
}

int devm_pm_runtime_enable(struct device *dev)
{
	pm_runtime_enable(dev);

	return devm_add_action_or_reset(dev, pm_runtime_disable_action, dev);
}

If the devm_add_action_or_reset() allocation in devm_pm_runtime_enable()
fails, it invokes pm_runtime_disable_action() while runtime PM is still
enabled, runtime_status is RPM_ACTIVE, and usage_count is 0.

pm_runtime_dont_use_autosuspend() clears use_autosuspend and calls
update_autosuspend() -> rpm_idle() -> rpm_suspend(), which synchronously
executes the runtime_suspend callback (ethosu_device_suspend()) and disables
ethosudev->clks. When devm_pm_runtime_set_active_enabled() then returns the
error to ethosu_init(), calling ethosu_device_suspend() unconditionally in
the error path will disable and unprepare ethosudev->clks a second time.

Would calling pm_runtime_get_noresume() before
devm_pm_runtime_set_active_enabled() (and pairing it with
pm_runtime_put_noidle() on error) prevent runtime PM from suspending the
device during failure cleanup?

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

[PATCH v4 05/31] accel: ethosu: Quiesce jobs before scheduler teardown

[Rob Herring (Arm)]

An NPU job can complete while driver removal tears down the scheduler. Its
IRQ handler could then access scheduler state after it has been
destroyed.

Stop scheduler submission and timeout work, reset the NPU, and
synchronize its IRQ before finalizing the scheduler. Add a cancel_job
callback so drm_sched_fini() signals queued jobs with -ECANCELED; their
runtime-PM references are then released during normal job cleanup.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v3:
 - Add dma_fence_was_initialized() check in job cancel (sashiko)
v2:
 - new patch
---
 drivers/accel/ethosu/ethosu_drv.c |  4 ++--
 drivers/accel/ethosu/ethosu_drv.h |  2 ++
 drivers/accel/ethosu/ethosu_job.c | 23 ++++++++++++++++++++++-
 3 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
index 1a65e9eef562..6875ed17a004 100644
--- a/drivers/accel/ethosu/ethosu_drv.c
+++ b/drivers/accel/ethosu/ethosu_drv.c
@@ -216,7 +216,7 @@ static const struct drm_driver ethosu_drm_driver = {
 #define U85_MEM_ATTR0_CFG	0x00000000
 #define U85_MEM_ATTR2_CFG	0x000000b7
 
-static int ethosu_reset(struct ethosu_device *ethosudev)
+int ethosu_device_reset(struct ethosu_device *ethosudev)
 {
 	int ret;
 	u32 reg;
@@ -263,7 +263,7 @@ static int ethosu_device_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	ret = ethosu_reset(ethosudev);
+	ret = ethosu_device_reset(ethosudev);
 	if (!ret)
 		return 0;
 
diff --git a/drivers/accel/ethosu/ethosu_drv.h b/drivers/accel/ethosu/ethosu_drv.h
index 2193bc51d425..f59c845c758b 100644
--- a/drivers/accel/ethosu/ethosu_drv.h
+++ b/drivers/accel/ethosu/ethosu_drv.h
@@ -11,6 +11,8 @@ struct ethosu_device;
 struct drm_device;
 struct drm_file;
 
+int ethosu_device_reset(struct ethosu_device *ethosudev);
+
 struct ethosu_file_priv {
 	struct ethosu_device *edev;
 	struct drm_sched_entity sched_entity;
diff --git a/drivers/accel/ethosu/ethosu_job.c b/drivers/accel/ethosu/ethosu_job.c
index b7faa715b47e..d418ae7961cf 100644
--- a/drivers/accel/ethosu/ethosu_job.c
+++ b/drivers/accel/ethosu/ethosu_job.c
@@ -189,6 +189,17 @@ static void ethosu_job_free(struct drm_sched_job *sched_job)
 	ethosu_job_put(job);
 }
 
+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);
+	}
+}
+
 static void
 ethosu_switch_perfmon(struct ethosu_device *ethosu, struct ethosu_job *job)
 {
@@ -315,7 +326,8 @@ static enum drm_gpu_sched_stat ethosu_job_timedout(struct drm_sched_job *bad)
 static const struct drm_sched_backend_ops ethosu_sched_ops = {
 	.run_job = ethosu_job_run,
 	.timedout_job = ethosu_job_timedout,
-	.free_job = ethosu_job_free
+	.free_job = ethosu_job_free,
+	.cancel_job = ethosu_job_cancel,
 };
 
 int ethosu_job_init(struct ethosu_device *edev)
@@ -361,6 +373,15 @@ int ethosu_job_init(struct ethosu_device *edev)
 
 void ethosu_job_fini(struct ethosu_device *dev)
 {
+	drm_sched_wqueue_stop(&dev->sched);
+	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);
+	}
+
+	synchronize_irq(dev->irq);
 	drm_sched_fini(&dev->sched);
 }
 

-- 
2.53.0

Re: [PATCH v4 05/31] accel: ethosu: Quiesce jobs before scheduler teardown

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] accel: ethosu: wrong TDR and IRQ teardown order in ethosu_job_fini()
--

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

accel: ethosu: Quiesce jobs before scheduler teardown

This commit stops the scheduler workqueue and timeout work, resets the NPU if
a job is in flight, and synchronizes the IRQ before calling drm_sched_fini().
It also adds a cancel_job callback so queued jobs are signaled with
-ECANCELED during scheduler teardown.

> diff --git a/drivers/accel/ethosu/ethosu_job.c b/drivers/accel/ethosu/ethosu_job.c
> index b7faa715b47e4..d418ae7961cff 100644
> --- a/drivers/accel/ethosu/ethosu_job.c
> +++ b/drivers/accel/ethosu/ethosu_job.c

[ ... ]

> @@ -361,6 +373,15 @@ int ethosu_job_init(struct ethosu_device *edev)
>  
>  void ethosu_job_fini(struct ethosu_device *dev)
>  {
> +	drm_sched_wqueue_stop(&dev->sched);
> +	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);
> +	}
> +
> +	synchronize_irq(dev->irq);
>  	drm_sched_fini(&dev->sched);
>  }

[Severity: High]
Could the ordering of drm_sched_wqueue_stop(), cancel_delayed_work_sync(), and
IRQ synchronization here allow work_run_job or work_tdr to be re-queued during
teardown?

First, drm_sched_wqueue_stop() is called before canceling dev->sched.work_tdr.
If dev->sched.work_tdr is already executing ethosu_job_timedout() when
ethosu_job_fini() starts, cancel_delayed_work_sync() waits for it to finish:

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

	WRITE_ONCE(dev->in_flight_job, NULL);

	/* Proceed with reset now. */
	pm_runtime_force_suspend(dev->base.dev);
	pm_runtime_force_resume(dev->base.dev);

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

	return DRM_GPU_SCHED_STAT_RESET;
}

At the end of ethosu_job_timedout(), drm_sched_start() calls
drm_sched_wqueue_start(), which clears sched->pause_submit and queues
work_run_job and work_free_job on sched->submit_wq. Once
cancel_delayed_work_sync() returns, work_run_job can pop a queued job and call
ethosu_job_run() -> ethosu_job_hw_submit(), setting dev->in_flight_job,
submitting a new job to the NPU, and re-arming work_tdr via
drm_sched_job_begin() concurrently with ethosu_device_reset() and
synchronize_irq().

Second, cancel_delayed_work_sync(&dev->sched.work_tdr) runs before
ethosu_device_reset() and synchronize_irq(dev->irq). Before the NPU is reset
and the IRQ is synchronized, ethosu_job_irq_handler_thread() ->
ethosu_job_handle_irq() can still run and handle an error interrupt without
checking dev->in_flight_job:

drivers/accel/ethosu/ethosu_job.c:ethosu_job_handle_irq() {
	...
	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;
	}
	...
}

Calling drm_sched_fault() schedules dev->sched.work_tdr again after
cancel_delayed_work_sync() has already returned. That allows
ethosu_job_timedout() to run concurrently with ethosu_device_reset() (where
pm_runtime_force_suspend() disables the NPU clocks while ethosu_device_reset()
polls MMIO registers and clears SRAM with memset_io()) as well as concurrently
with drm_sched_fini().

Note that while a later commit in the series ("accel: ethosu: Keep scheduler
after unplug") replaces this with ethosu_job_stop() and calls
disable_irq(dev->irq) before cancel_delayed_work_sync(&dev->sched.work_tdr),
it still calls drm_sched_wqueue_stop(&dev->sched) before
cancel_delayed_work_sync(&dev->sched.work_tdr), which leaves the race where an
in-progress ethosu_job_timedout() calls drm_sched_start() and restarts
work_run_job and work_free_job.

Should the IRQ be disabled or synchronized before canceling work_tdr, and
should drm_sched_wqueue_stop() be called after cancel_delayed_work_sync() (or
after preventing ethosu_job_timedout() from calling drm_sched_start() during
teardown)?

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

[PATCH v4 06/31] accel: ethosu: Fix runtime PM get error handling

[Rob Herring (Arm)]

pm_runtime_get_if_active() can return an error including for !CONFIG_PM.
Do not treat that as an active runtime-PM reference.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v4:
 - new patch
---
 drivers/accel/ethosu/ethosu_perfmon.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/accel/ethosu/ethosu_perfmon.c b/drivers/accel/ethosu/ethosu_perfmon.c
index 26f625374f9d..0ae2df3ceac8 100644
--- a/drivers/accel/ethosu/ethosu_perfmon.c
+++ b/drivers/accel/ethosu/ethosu_perfmon.c
@@ -71,7 +71,8 @@ void ethosu_perfmon_stop_locked(struct ethosu_device *ethosu, struct ethosu_perf
 
 	ncounters = perfmon->ncounters - 1;
 
-	if (!pm_runtime_get_if_active(ethosu->base.dev)) {
+	if (IS_ENABLED(CONFIG_PM) &&
+	    pm_runtime_get_if_active(ethosu->base.dev) <= 0) {
 		ethosu->perfmon_state.active = NULL;
 		return;
 	}

-- 
2.53.0

Re: [PATCH v4 06/31] accel: ethosu: Fix runtime PM get error handling

[Frank Li]

On Thu, Oct 29, 2026 at 08:35:53AM -0500, Rob Herring (Arm) wrote:
> pm_runtime_get_if_active() can return an error including for !CONFIG_PM.
> Do not treat that as an active runtime-PM reference.
>
> Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
> v4:
>  - new patch
> ---
>  drivers/accel/ethosu/ethosu_perfmon.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/accel/ethosu/ethosu_perfmon.c b/drivers/accel/ethosu/ethosu_perfmon.c
> index 26f625374f9d..0ae2df3ceac8 100644
> --- a/drivers/accel/ethosu/ethosu_perfmon.c
> +++ b/drivers/accel/ethosu/ethosu_perfmon.c
> @@ -71,7 +71,8 @@ void ethosu_perfmon_stop_locked(struct ethosu_device *ethosu, struct ethosu_perf
>
>  	ncounters = perfmon->ncounters - 1;
>
> -	if (!pm_runtime_get_if_active(ethosu->base.dev)) {

pm_runtime_get_if_active() return 1 when active
			   return 0 when suspend
			   return -EINVAL when !CONFIG_PM

!pm_runtime_get_if_active() shoud be correct if !CONFIG_PM. when !CONFIG_PM
we treat all devices is actived.

I think stub pm_runtime_get_if_active() for !CONFIG_PM return -EINVAL is
wrong.

It should be similar with stub

static inline int __pm_runtime_resume(struct device *dev, int rpmflags)
{
	return 1;
}

I will create patch change -EINVAL to 1.

Frank

> +	if (IS_ENABLED(CONFIG_PM) &&
> +	    pm_runtime_get_if_active(ethosu->base.dev) <= 0) {
>  		ethosu->perfmon_state.active = NULL;
>  		return;
>  	}
>
> --
> 2.53.0
>

Re: [PATCH v4 06/31] accel: ethosu: Fix runtime PM get error handling

[Frank Li]

On Thu, Oct 29, 2026 at 08:35:53AM -0500, Rob Herring (Arm) wrote:
> pm_runtime_get_if_active() can return an error including for !CONFIG_PM.
> Do not treat that as an active runtime-PM reference.
>
> Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
> v4:
>  - new patch
> ---
>  drivers/accel/ethosu/ethosu_perfmon.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/accel/ethosu/ethosu_perfmon.c b/drivers/accel/ethosu/ethosu_perfmon.c
> index 26f625374f9d..0ae2df3ceac8 100644
> --- a/drivers/accel/ethosu/ethosu_perfmon.c
> +++ b/drivers/accel/ethosu/ethosu_perfmon.c
> @@ -71,7 +71,8 @@ void ethosu_perfmon_stop_locked(struct ethosu_device *ethosu, struct ethosu_perf
>
>  	ncounters = perfmon->ncounters - 1;
>
> -	if (!pm_runtime_get_if_active(ethosu->base.dev)) {

This should be correct. I just post
https://lore.kernel.org/imx/20261029163044.3062958-1-Frank.Li@oss.nxp.com/T/#u

belong to Pattern A

Frank

> +	if (IS_ENABLED(CONFIG_PM) &&
> +	    pm_runtime_get_if_active(ethosu->base.dev) <= 0) {
>  		ethosu->perfmon_state.active = NULL;
>  		return;
>  	}
>
> --
> 2.53.0
>

[PATCH v4 07/31] accel: ethosu: Limit perfmon counters to UAPI

[Rob Herring (Arm)]

The perfmon creation UAPI has a fixed eight-entry counter array, but
only the hardware counter count limits ncounters. A device reporting more
than eight counters would allow out-of-bounds accesses to the request and
perfmon counter arrays.

Reject counts exceeding the UAPI array bound.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v4:
 - New patch
---
 drivers/accel/ethosu/ethosu_perfmon.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_perfmon.c b/drivers/accel/ethosu/ethosu_perfmon.c
index 0ae2df3ceac8..2a9b3cc1447b 100644
--- a/drivers/accel/ethosu/ethosu_perfmon.c
+++ b/drivers/accel/ethosu/ethosu_perfmon.c
@@ -165,8 +165,9 @@ int ethosu_ioctl_perfmon_create(struct drm_device *dev, void *data,
 	int ret;
 	u32 id;
 
-	/* Number of monitored counters cannot exceed HW limits. */
-	if (req->ncounters > ethosu->npu_info.pmu_counters)
+	/* Number of monitored counters cannot exceed HW or UAPI limits. */
+	if (req->ncounters > ethosu->npu_info.pmu_counters ||
+	    req->ncounters > DRM_ETHOSU_MAX_PERF_EVENT_COUNTERS)
 		return -EINVAL;
 
 	/* Make sure all counters are valid. */

-- 
2.53.0

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

[Rob Herring (Arm)]

The timeout worker and threaded IRQ handler can both clear
in_flight_job. If a new job is scheduled in between those,
in_flight_job could be overwritten when the IRQ thread resumes.
Serialize these updates so a late completion cannot clear the next job's
tracking pointer.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v4:
 - New patch
---
 drivers/accel/ethosu/ethosu_device.h |  1 +
 drivers/accel/ethosu/ethosu_job.c    | 43 +++++++++++++++++++++++++++++-------
 2 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_device.h b/drivers/accel/ethosu/ethosu_device.h
index 1eca8590e68d..51d5bc5d6137 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>
diff --git a/drivers/accel/ethosu/ethosu_job.c b/drivers/accel/ethosu/ethosu_job.c
index d418ae7961cf..f22cacb19e3d 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;
+}
+
 static void ethosu_job_hw_submit(struct ethosu_device *dev, struct ethosu_job *job)
 {
 	struct drm_gem_dma_object *cmd_bo = to_drm_gem_dma_obj(job->cmd_bo);
@@ -235,7 +254,7 @@ static struct dma_fence *ethosu_job_run(struct drm_sched_job *sched_job)
 
 	ethosu_switch_perfmon(dev, job);
 
-	WRITE_ONCE(dev->in_flight_job, job);
+	ethosu_job_set_in_flight(dev, job);
 	ethosu_job_hw_submit(dev, job);
 
 	return fence;
@@ -252,11 +271,9 @@ static void ethosu_job_handle_irq(struct ethosu_device *dev)
 		return;
 	}
 
-	job = READ_ONCE(dev->in_flight_job);
-	if (job) {
-		WRITE_ONCE(dev->in_flight_job, NULL);
+	job = ethosu_job_clear_in_flight(dev);
+	if (job)
 		dma_fence_signal(job->done_fence);
-	}
 }
 
 static irqreturn_t ethosu_job_irq_handler_thread(int irq, void *data)
@@ -285,6 +302,7 @@ static enum drm_gpu_sched_stat ethosu_job_timedout(struct drm_sched_job *bad)
 	struct ethosu_job *job = to_ethosu_job(bad);
 	struct ethosu_device *dev = job->dev;
 	bool running;
+	int ret;
 	u32 *bocmds = to_drm_gem_dma_obj(job->cmd_bo)->vaddr;
 	u32 cmdaddr;
 
@@ -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);
 
 	/* 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;
+	}
+
+	/* Drain a pending completion IRQ before a replacement job can run. */
+	synchronize_irq(dev->irq);
 
 	/* Restart the scheduler */
 	drm_sched_start(&dev->sched, 0);
@@ -344,6 +370,7 @@ int ethosu_job_init(struct ethosu_device *edev)
 	int ret;
 
 	spin_lock_init(&edev->fence_lock);
+	spin_lock_init(&edev->job_lock);
 	ret = devm_mutex_init(dev, &edev->sched_lock);
 	if (ret)
 		return ret;

-- 
2.53.0

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

[sashiko-bot]

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

[PATCH v4 09/31] accel: ethosu: Guard operations after unplug

[Rob Herring (Arm)]

drm_dev_is_unplugged() only checks the device state and can race with
removal. Use drm_dev_enter() around open and all private operations which
access driver state, and use drm_dev_unplug() during removal to wait for
them to complete.

Drop the drm_dev_enter() from the BO wait ioctl as that could
unnecessarily delay stopping the scheduler by waiting on a BO fence
signal timeout.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v4:
 - New patch
---
 drivers/accel/ethosu/ethosu_drv.c     |  28 ++++----
 drivers/accel/ethosu/ethosu_job.c     |  21 ++++--
 drivers/accel/ethosu/ethosu_perfmon.c | 117 ++++++++++++++++++++++------------
 3 files changed, 107 insertions(+), 59 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
index 6875ed17a004..91b2c2ab0a03 100644
--- a/drivers/accel/ethosu/ethosu_drv.c
+++ b/drivers/accel/ethosu/ethosu_drv.c
@@ -80,19 +80,12 @@ static int ethosu_ioctl_bo_wait(struct drm_device *ddev, void *data,
 				struct drm_file *file)
 {
 	struct drm_ethosu_bo_wait *args = data;
-	int cookie, ret;
 	unsigned long timeout = drm_timeout_abs_to_jiffies(args->timeout_ns);
 
 	if (args->pad)
 		return -EINVAL;
 
-	if (!drm_dev_enter(ddev, &cookie))
-		return -ENODEV;
-
-	ret = drm_gem_dma_resv_wait(file, args->handle, true, timeout);
-
-	drm_dev_exit(cookie);
-	return ret;
+	return drm_gem_dma_resv_wait(file, args->handle, true, timeout);
 }
 
 static int ethosu_ioctl_bo_mmap_offset(struct drm_device *ddev, void *data,
@@ -139,27 +132,34 @@ static int ethosu_ioctl_cmdstream_bo_create(struct drm_device *ddev, void *data,
 
 static int ethosu_open(struct drm_device *ddev, struct drm_file *file)
 {
-	int ret = 0;
+	struct ethosu_file_priv __free(kfree) * priv = NULL;
+	int cookie, ret;
 
 	if (!try_module_get(THIS_MODULE))
 		return -EINVAL;
+	if (!drm_dev_enter(ddev, &cookie)) {
+		module_put(THIS_MODULE);
+		return -ENODEV;
+	}
 
-	struct ethosu_file_priv __free(kfree) *priv = kzalloc_obj(*priv);
+	priv = kzalloc_obj(*priv);
 	if (!priv) {
 		ret = -ENOMEM;
-		goto err_put_mod;
+		goto err_dev_exit;
 	}
 	priv->edev = to_ethosu_device(ddev);
 
 	ret = ethosu_job_open(priv);
 	if (ret)
-		goto err_put_mod;
+		goto err_dev_exit;
 
 	ethosu_perfmon_open_file(priv);
 	file->driver_priv = no_free_ptr(priv);
+	drm_dev_exit(cookie);
 	return 0;
 
-err_put_mod:
+err_dev_exit:
+	drm_dev_exit(cookie);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -398,7 +398,7 @@ static void ethosu_remove(struct platform_device *pdev)
 {
 	struct ethosu_device *ethosudev = dev_get_drvdata(&pdev->dev);
 
-	drm_dev_unregister(&ethosudev->base);
+	drm_dev_unplug(&ethosudev->base);
 	ethosu_job_fini(ethosudev);
 
 	pm_runtime_barrier(&pdev->dev);
diff --git a/drivers/accel/ethosu/ethosu_job.c b/drivers/accel/ethosu/ethosu_job.c
index f22cacb19e3d..8ffdc83d06f9 100644
--- a/drivers/accel/ethosu/ethosu_job.c
+++ b/drivers/accel/ethosu/ethosu_job.c
@@ -10,6 +10,7 @@
 #include <linux/pm_runtime.h>
 
 #include <drm/drm_file.h>
+#include <drm/drm_drv.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_gem_dma_helper.h>
 #include <drm/drm_print.h>
@@ -557,26 +558,34 @@ static int ethosu_ioctl_submit_job(struct drm_device *dev, struct drm_file *file
 int ethosu_ioctl_submit(struct drm_device *dev, void *data, struct drm_file *file)
 {
 	struct drm_ethosu_submit *args = data;
-	int ret = 0;
+	int cookie, ret = 0;
 	unsigned int i = 0;
 
+	if (!drm_dev_enter(dev, &cookie))
+		return -ENODEV;
+
 	struct drm_ethosu_job __free(kvfree) *jobs =
 		kvmalloc_objs(*jobs, args->job_count);
-	if (!jobs)
-		return -ENOMEM;
+	if (!jobs) {
+		ret = -ENOMEM;
+		goto out_dev_exit;
+	}
 
 	if (copy_from_user(jobs,
 			   (void __user *)(uintptr_t)args->jobs,
 			   args->job_count * sizeof(*jobs))) {
 		drm_dbg(dev, "Failed to copy incoming job array\n");
-		return -EFAULT;
+		ret = -EFAULT;
+		goto out_dev_exit;
 	}
 
 	for (i = 0; i < args->job_count; i++) {
 		ret = ethosu_ioctl_submit_job(dev, file, &jobs[i], args->perfmon_id);
 		if (ret)
-			return ret;
+			goto out_dev_exit;
 	}
 
-	return 0;
+out_dev_exit:
+	drm_dev_exit(cookie);
+	return ret;
 }
diff --git a/drivers/accel/ethosu/ethosu_perfmon.c b/drivers/accel/ethosu/ethosu_perfmon.c
index 2a9b3cc1447b..8d3635ba3347 100644
--- a/drivers/accel/ethosu/ethosu_perfmon.c
+++ b/drivers/accel/ethosu/ethosu_perfmon.c
@@ -10,6 +10,7 @@
 #include <linux/uaccess.h>
 
 #include <drm/drm_file.h>
+#include <drm/drm_drv.h>
 #include <drm/drm_ioctl.h>
 
 #include <uapi/drm/ethosu_accel.h>
@@ -162,27 +163,36 @@ int ethosu_ioctl_perfmon_create(struct drm_device *dev, void *data,
 	struct ethosu_device *ethosu = to_ethosu_device(dev);
 	struct ethosu_perfmon *perfmon;
 	unsigned int i, event_max;
-	int ret;
+	int cookie, ret;
 	u32 id;
 
+	if (!drm_dev_enter(dev, &cookie))
+		return -ENODEV;
+
 	/* Number of monitored counters cannot exceed HW or UAPI limits. */
 	if (req->ncounters > ethosu->npu_info.pmu_counters ||
-	    req->ncounters > DRM_ETHOSU_MAX_PERF_EVENT_COUNTERS)
-		return -EINVAL;
+	    req->ncounters > DRM_ETHOSU_MAX_PERF_EVENT_COUNTERS) {
+		ret = -EINVAL;
+		goto out_dev_exit;
+	}
 
 	/* Make sure all counters are valid. */
 	event_max = ethosu_is_u65(ethosu) ? 433 : 671;
 	for (i = 0; i < req->ncounters; i++) {
-		if (req->counters[i] > event_max)
-			return -EINVAL;
+		if (req->counters[i] > event_max) {
+			ret = -EINVAL;
+			goto out_dev_exit;
+		}
 	}
 
 	/* Add 1 more counter for cycle counter */
 	req->ncounters++;
 
 	perfmon = kzalloc_flex(*perfmon, values, req->ncounters);
-	if (!perfmon)
-		return -ENOMEM;
+	if (!perfmon) {
+		ret = -ENOMEM;
+		goto out_dev_exit;
+	}
 
 	for (i = 0; i < req->ncounters - 1; i++)
 		perfmon->counters[i] = req->counters[i];
@@ -196,12 +206,14 @@ int ethosu_ioctl_perfmon_create(struct drm_device *dev, void *data,
 
 	if (ret < 0) {
 		kfree(perfmon);
-		return ret;
+		goto out_dev_exit;
 	}
 
 	req->id = id;
 
-	return 0;
+out_dev_exit:
+	drm_dev_exit(cookie);
+	return ret;
 }
 
 int ethosu_ioctl_perfmon_destroy(struct drm_device *dev, void *data,
@@ -210,14 +222,22 @@ int ethosu_ioctl_perfmon_destroy(struct drm_device *dev, void *data,
 	struct ethosu_file_priv *ethosu_priv = file_priv->driver_priv;
 	struct drm_ethosu_perfmon_destroy *req = data;
 	struct ethosu_perfmon *perfmon;
+	int cookie, ret = 0;
+
+	if (!drm_dev_enter(dev, &cookie))
+		return -ENODEV;
 
 	perfmon = xa_erase(&ethosu_priv->perfmons, req->id);
-	if (!perfmon)
-		return -EINVAL;
+	if (!perfmon) {
+		ret = -EINVAL;
+		goto out_dev_exit;
+	}
 
 	ethosu_perfmon_delete(ethosu_priv, perfmon);
 
-	return 0;
+out_dev_exit:
+	drm_dev_exit(cookie);
+	return ret;
 }
 
 int ethosu_ioctl_perfmon_get_values(struct drm_device *dev, void *data,
@@ -227,19 +247,26 @@ int ethosu_ioctl_perfmon_get_values(struct drm_device *dev, void *data,
 	struct ethosu_file_priv *ethosu_priv = file_priv->driver_priv;
 	struct drm_ethosu_perfmon_get_values *req = data;
 	struct ethosu_perfmon *perfmon;
-	int ret = 0;
+	int cookie, ret = 0;
 
-	if (req->pad != 0)
-		return -EINVAL;
+	if (!drm_dev_enter(dev, &cookie))
+		return -ENODEV;
+
+	if (req->pad != 0) {
+		ret = -EINVAL;
+		goto out_dev_exit;
+	}
 
 	perfmon = ethosu_perfmon_find(ethosu_priv, req->id);
-	if (!perfmon)
-		return -EINVAL;
+	if (!perfmon) {
+		ret = -EINVAL;
+		goto out_dev_exit;
+	}
 
 	ret = pm_runtime_resume_and_get(dev->dev);
 	if (ret) {
 		ethosu_perfmon_put(perfmon);
-		return ret;
+		goto out_dev_exit;
 	}
 	ethosu_perfmon_stop(ethosu, perfmon, true);
 
@@ -251,6 +278,8 @@ int ethosu_ioctl_perfmon_get_values(struct drm_device *dev, void *data,
 
 	ethosu_perfmon_put(perfmon);
 
+out_dev_exit:
+	drm_dev_exit(cookie);
 	return ret;
 }
 
@@ -260,44 +289,54 @@ int ethosu_ioctl_perfmon_set_global(struct drm_device *dev, void *data,
 	struct ethosu_file_priv *ethosu_priv = file_priv->driver_priv;
 	struct drm_ethosu_perfmon_set_global *req = data;
 	struct ethosu_device *ethosu = to_ethosu_device(dev);
-	struct ethosu_perfmon *perfmon;
+	struct ethosu_perfmon *old = NULL, *perfmon;
+	int cookie, ret = 0;
 
-	if (req->flags & ~DRM_ETHOSU_PERFMON_CLEAR_GLOBAL)
-		return -EINVAL;
+	if (!drm_dev_enter(dev, &cookie))
+		return -ENODEV;
+
+	if (req->flags & ~DRM_ETHOSU_PERFMON_CLEAR_GLOBAL) {
+		ret = -EINVAL;
+		goto out_dev_exit;
+	}
 
 	perfmon = ethosu_perfmon_find(ethosu_priv, req->id);
-	if (!perfmon)
-		return -EINVAL;
+	if (!perfmon) {
+		ret = -EINVAL;
+		goto out_dev_exit;
+	}
 
 	/* If the request is to clear the global performance monitor */
 	if (req->flags & DRM_ETHOSU_PERFMON_CLEAR_GLOBAL) {
-		struct ethosu_perfmon *old;
-
 		scoped_guard(mutex, &ethosu->perfmon_state.lock) {
 			old = ethosu->global_perfmon;
-			if (!old) {
-				ethosu_perfmon_put(perfmon);
-				return -EINVAL;
+			if (old) {
+				ethosu->global_perfmon = NULL;
+				ethosu_perfmon_stop_locked(ethosu, old, true);
 			}
+		}
 
-			ethosu->global_perfmon = NULL;
-			ethosu_perfmon_stop_locked(ethosu, old, true);
+		ethosu_perfmon_put(perfmon);
+		if (!old) {
+			ret = -EINVAL;
+			goto out_dev_exit;
 		}
 
 		ethosu_perfmon_put(old);
-		ethosu_perfmon_put(perfmon);
 
-		return 0;
+		goto out_dev_exit;
 	}
 
 	scoped_guard(mutex, &ethosu->perfmon_state.lock) {
-		if (ethosu->perfmon_state.active || ethosu->global_perfmon) {
-			ethosu_perfmon_put(perfmon);
-			return -EBUSY;
-		}
-
-		ethosu->global_perfmon = perfmon;
+		if (ethosu->perfmon_state.active || ethosu->global_perfmon)
+			ret = -EBUSY;
+		else
+			ethosu->global_perfmon = perfmon;
 	}
+	if (ret)
+		ethosu_perfmon_put(perfmon);
 
-	return 0;
+out_dev_exit:
+	drm_dev_exit(cookie);
+	return ret;
 }

-- 
2.53.0

Re: [PATCH v4 09/31] accel: ethosu: Guard operations after unplug

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] accel: ethosu: mixing __free() and scoped_guard() with goto cleanup labels
--

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

accel: ethosu: Guard operations after unplug

This commit guards ethosu device file open and private ioctl operations
with drm_dev_enter() and drm_dev_exit(), and switches device removal from
drm_dev_unregister() to drm_dev_unplug(). It also drops drm_dev_enter()
from the BO wait ioctl so fence waits do not delay scheduler teardown.

> diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
> index 6875ed17a004d..91b2c2ab0a03e 100644
> --- a/drivers/accel/ethosu/ethosu_drv.c
> +++ b/drivers/accel/ethosu/ethosu_drv.c
[ ... ]
> @@ -139,27 +132,34 @@ static int ethosu_ioctl_cmdstream_bo_create(struct drm_device *ddev, void *data,
>  
>  static int ethosu_open(struct drm_device *ddev, struct drm_file *file)
>  {
> -	int ret = 0;
> +	struct ethosu_file_priv __free(kfree) * priv = NULL;
> +	int cookie, ret;
>  
>  	if (!try_module_get(THIS_MODULE))
>  		return -EINVAL;
> +	if (!drm_dev_enter(ddev, &cookie)) {
> +		module_put(THIS_MODULE);
> +		return -ENODEV;
> +	}
>  
> -	struct ethosu_file_priv __free(kfree) *priv = kzalloc_obj(*priv);
> +	priv = kzalloc_obj(*priv);
>  	if (!priv) {
>  		ret = -ENOMEM;
> -		goto err_put_mod;
> +		goto err_dev_exit;
>  	}
>  	priv->edev = to_ethosu_device(ddev);
>  
>  	ret = ethosu_job_open(priv);
>  	if (ret)
> -		goto err_put_mod;
> +		goto err_dev_exit;
>  
>  	ethosu_perfmon_open_file(priv);
>  	file->driver_priv = no_free_ptr(priv);
> +	drm_dev_exit(cookie);
>  	return 0;
>  
> -err_put_mod:
> +err_dev_exit:
> +	drm_dev_exit(cookie);
>  	module_put(THIS_MODULE);
>  	return ret;
>  }

[Severity: Medium]
In drivers/accel/ethosu/ethosu_drv.c, ethosu_open() declares priv with
__free(kfree) = NULL at the top of the function before try_module_get() and
drm_dev_enter(), and then mixes __free(kfree) with goto err_dev_exit when
kzalloc_obj() or ethosu_job_open() fails.

Because scope-based cleanup runs when the function scope exits after the
err_dev_exit label executes, drm_dev_exit(cookie) and
module_put(THIS_MODULE) will run before kfree(priv), reversing the reverse
acquisition (lifo) teardown order and mixing goto error handling with
cleanup.h helpers.

Should ethosu_open() avoid mixing __free(kfree) and goto labels so that
priv is freed before exiting the drm device section and dropping the module
reference?

> diff --git a/drivers/accel/ethosu/ethosu_job.c b/drivers/accel/ethosu/ethosu_job.c
> index f22cacb19e3dc..8ffdc83d06f98 100644
> --- a/drivers/accel/ethosu/ethosu_job.c
> +++ b/drivers/accel/ethosu/ethosu_job.c
[ ... ]
> @@ -557,26 +558,34 @@ static int ethosu_ioctl_submit_job(struct drm_device *dev, struct drm_file *file
>  int ethosu_ioctl_submit(struct drm_device *dev, void *data, struct drm_file *file)
>  {
>  	struct drm_ethosu_submit *args = data;
> -	int ret = 0;
> +	int cookie, ret = 0;
>  	unsigned int i = 0;
>  
> +	if (!drm_dev_enter(dev, &cookie))
> +		return -ENODEV;
> +
>  	struct drm_ethosu_job __free(kvfree) *jobs =
>  		kvmalloc_objs(*jobs, args->job_count);
> -	if (!jobs)
> -		return -ENOMEM;
> +	if (!jobs) {
> +		ret = -ENOMEM;
> +		goto out_dev_exit;
> +	}
>  
>  	if (copy_from_user(jobs,
>  			   (void __user *)(uintptr_t)args->jobs,
>  			   args->job_count * sizeof(*jobs))) {
>  		drm_dbg(dev, "Failed to copy incoming job array\n");
> -		ret = -EFAULT;
> +		ret = -EFAULT;
> +		goto out_dev_exit;
>  	}
>  
>  	for (i = 0; i < args->job_count; i++) {
>  		ret = ethosu_ioctl_submit_job(dev, file, &jobs[i], args->perfmon_id);
>  		if (ret)
> -			return ret;
> +			goto out_dev_exit;
>  	}
>  
> -	return 0;
> +out_dev_exit:
> +	drm_dev_exit(cookie);
> +	return ret;
>  }
> diff --git a/drivers/accel/ethosu/ethosu_perfmon.c b/drivers/accel/ethosu/ethosu_perfmon.c
> index 2a9b3cc1447bd..8d3635ba3347b 100644
> --- a/drivers/accel/ethosu/ethosu_perfmon.c
> +++ b/drivers/accel/ethosu/ethosu_perfmon.c
[ ... ]
> @@ -260,44 +289,54 @@ int ethosu_ioctl_perfmon_set_global(struct drm_device *dev, void *data,
>  	struct ethosu_file_priv *ethosu_priv = file_priv->driver_priv;
>  	struct drm_ethosu_perfmon_set_global *req = data;
>  	struct ethosu_device *ethosu = to_ethosu_device(dev);
> -	struct ethosu_perfmon *perfmon;
> +	struct ethosu_perfmon *old = NULL, *perfmon;
> +	int cookie, ret = 0;
>  
> -	if (req->flags & ~DRM_ETHOSU_PERFMON_CLEAR_GLOBAL)
> -		return -EINVAL;
> +	if (!drm_dev_enter(dev, &cookie))
> +		return -ENODEV;
> +
> +	if (req->flags & ~DRM_ETHOSU_PERFMON_CLEAR_GLOBAL) {
> +		ret = -EINVAL;
> +		goto out_dev_exit;
> +	}
>  
>  	perfmon = ethosu_perfmon_find(ethosu_priv, req->id);
> -	if (!perfmon)
> -		return -EINVAL;
> +	if (!perfmon) {
> +		ret = -EINVAL;
> +		goto out_dev_exit;
> +	}
>  
>  	/* If the request is to clear the global performance monitor */
>  	if (req->flags & DRM_ETHOSU_PERFMON_CLEAR_GLOBAL) {
> -		struct ethosu_perfmon *old;
> -
>  		scoped_guard(mutex, &ethosu->perfmon_state.lock) {
>  			old = ethosu->global_perfmon;
> -			if (!old) {
> -				ethosu_perfmon_put(perfmon);
> -				return -EINVAL;
> +			if (old) {
> +				ethosu->global_perfmon = NULL;
> +				ethosu_perfmon_stop_locked(ethosu, old, true);
>  			}
> +		}
>  
> -			ethosu->global_perfmon = NULL;
> -			ethosu_perfmon_stop_locked(ethosu, old, true);
> +		ethosu_perfmon_put(perfmon);
> +		if (!old) {
> +			ret = -EINVAL;
> +			goto out_dev_exit;
>  		}
>  
>  		ethosu_perfmon_put(old);
> -		ethosu_perfmon_put(perfmon);
>  
> -		return 0;
> +		goto out_dev_exit;
>  	}
>  
>  	scoped_guard(mutex, &ethosu->perfmon_state.lock) {
> -		if (ethosu->perfmon_state.active || ethosu->global_perfmon) {
> -			ethosu_perfmon_put(perfmon);
> -			return -EBUSY;
> -		}
> -
> -		ethosu->global_perfmon = perfmon;
> +		if (ethosu->perfmon_state.active || ethosu->global_perfmon)
> +			ret = -EBUSY;
> +		else
> +			ethosu->global_perfmon = perfmon;
>  	}
> +	if (ret)
> +		ethosu_perfmon_put(perfmon);
>  
> -	return 0;
> +out_dev_exit:
> +	drm_dev_exit(cookie);
> +	return ret;
>  }

[Severity: Medium]
Similarly, in drivers/accel/ethosu/ethosu_job.c, ethosu_ioctl_submit()
mixes a mid-function __free(kvfree) declaration for jobs with goto
out_dev_exit, which causes drm_dev_exit(cookie) to run before kvfree(jobs).
In drivers/accel/ethosu/ethosu_perfmon.c, ethosu_ioctl_perfmon_set_global()
also mixes scoped_guard(mutex, &ethosu->perfmon_state.lock) with goto
out_dev_exit labels.

Per include/linux/cleanup.h, goto-based cleanup and scope-based cleanup
helpers should not be mixed within the same function. Could these functions
either use scope-based cleanup consistently or use explicit cleanup along
the goto paths?

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

Re: [PATCH v4 09/31] accel: ethosu: Guard operations after unplug

[Rob Herring]

On Thu, Oct 29, 2026 at 02:26:01PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] accel: ethosu: mixing __free() and scoped_guard() with goto cleanup labels
> --
> 
> commit cbd771f7e8b7995eaa7aedcd81a7cde35a4b6ada
> Author: Rob Herring (Arm) <robh@kernel.org>
> 
> accel: ethosu: Guard operations after unplug
> 
> This commit guards ethosu device file open and private ioctl operations
> with drm_dev_enter() and drm_dev_exit(), and switches device removal from
> drm_dev_unregister() to drm_dev_unplug(). It also drops drm_dev_enter()
> from the BO wait ioctl so fence waits do not delay scheduler teardown.
> 
> > diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
> > index 6875ed17a004d..91b2c2ab0a03e 100644
> > --- a/drivers/accel/ethosu/ethosu_drv.c
> > +++ b/drivers/accel/ethosu/ethosu_drv.c
> [ ... ]
> > @@ -139,27 +132,34 @@ static int ethosu_ioctl_cmdstream_bo_create(struct drm_device *ddev, void *data,
> >  
> >  static int ethosu_open(struct drm_device *ddev, struct drm_file *file)
> >  {
> > -	int ret = 0;
> > +	struct ethosu_file_priv __free(kfree) * priv = NULL;
> > +	int cookie, ret;
> >  
> >  	if (!try_module_get(THIS_MODULE))
> >  		return -EINVAL;
> > +	if (!drm_dev_enter(ddev, &cookie)) {
> > +		module_put(THIS_MODULE);
> > +		return -ENODEV;
> > +	}
> >  
> > -	struct ethosu_file_priv __free(kfree) *priv = kzalloc_obj(*priv);
> > +	priv = kzalloc_obj(*priv);
> >  	if (!priv) {
> >  		ret = -ENOMEM;
> > -		goto err_put_mod;
> > +		goto err_dev_exit;
> >  	}
> >  	priv->edev = to_ethosu_device(ddev);
> >  
> >  	ret = ethosu_job_open(priv);
> >  	if (ret)
> > -		goto err_put_mod;
> > +		goto err_dev_exit;
> >  
> >  	ethosu_perfmon_open_file(priv);
> >  	file->driver_priv = no_free_ptr(priv);
> > +	drm_dev_exit(cookie);
> >  	return 0;
> >  
> > -err_put_mod:
> > +err_dev_exit:
> > +	drm_dev_exit(cookie);
> >  	module_put(THIS_MODULE);
> >  	return ret;
> >  }
> 
> [Severity: Medium]
> In drivers/accel/ethosu/ethosu_drv.c, ethosu_open() declares priv with
> __free(kfree) = NULL at the top of the function before try_module_get() and
> drm_dev_enter(), and then mixes __free(kfree) with goto err_dev_exit when
> kzalloc_obj() or ethosu_job_open() fails.
> 
> Because scope-based cleanup runs when the function scope exits after the
> err_dev_exit label executes, drm_dev_exit(cookie) and
> module_put(THIS_MODULE) will run before kfree(priv), reversing the reverse
> acquisition (lifo) teardown order and mixing goto error handling with
> cleanup.h helpers.
> 
> Should ethosu_open() avoid mixing __free(kfree) and goto labels so that
> priv is freed before exiting the drm device section and dropping the module
> reference?

No! There's no such requirement that everything uses scoped cleanups or 
nothing does. And delaying a free is not an issue.

Rob

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

[Rob Herring (Arm)]

Open DRM files retain scheduling entities after the device is unbound.
Defer scheduler teardown until the final DRM device reference is released.

Track open scheduling entities so removal can stop each entity and signal
the active job. Then leave the scheduler alive for postclose handling.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v4:
 - New patch
---
 drivers/accel/ethosu/ethosu_device.h |  4 ++
 drivers/accel/ethosu/ethosu_drv.c    | 12 ++---
 drivers/accel/ethosu/ethosu_drv.h    |  4 ++
 drivers/accel/ethosu/ethosu_job.c    | 98 +++++++++++++++++++++++++++++-------
 drivers/accel/ethosu/ethosu_job.h    |  5 +-
 5 files changed, 98 insertions(+), 25 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_device.h b/drivers/accel/ethosu/ethosu_device.h
index 51d5bc5d6137..1a90d0ebe5d9 100644
--- a/drivers/accel/ethosu/ethosu_device.h
+++ b/drivers/accel/ethosu/ethosu_device.h
@@ -6,6 +6,7 @@
 
 #include <linux/bitfield.h>
 #include <linux/bits.h>
+#include <linux/list.h>
 #include <linux/mutex.h>
 #include <linux/spinlock.h>
 #include <linux/types.h>
@@ -197,6 +198,8 @@ struct ethosu_device {
 	struct drm_ethosu_npu_info npu_info;
 
 	struct ethosu_job *in_flight_job;
+	/* Protects @in_flight_job. */
+	spinlock_t job_lock;
 
 	/* For dma_fence */
 	spinlock_t fence_lock;
@@ -204,6 +207,7 @@ struct ethosu_device {
 	struct drm_gpu_scheduler sched;
 	/* For ethosu_job_do_push() */
 	struct mutex sched_lock;
+	struct list_head sched_entities;
 	u64 fence_context;
 	u64 emit_seqno;
 
diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
index 91b2c2ab0a03..ee50e7db36ad 100644
--- a/drivers/accel/ethosu/ethosu_drv.c
+++ b/drivers/accel/ethosu/ethosu_drv.c
@@ -271,7 +271,7 @@ static int ethosu_device_resume(struct device *dev)
 	return ret;
 }
 
-static int ethosu_device_suspend(struct device *dev)
+int ethosu_device_suspend(struct device *dev)
 {
 	struct ethosu_device *ethosudev = dev_get_drvdata(dev);
 
@@ -377,7 +377,7 @@ static int ethosu_probe(struct platform_device *pdev)
 
 	ret = ethosu_init(ethosudev);
 	if (ret)
-		goto err_job_fini;
+		return ret;
 
 	ret = drm_dev_register(&ethosudev->base, 0);
 	if (ret)
@@ -389,20 +389,18 @@ static int ethosu_probe(struct platform_device *pdev)
 	if (ethosudev->sram)
 		gen_pool_free(ethosudev->srampool, (unsigned long)ethosudev->sram,
 			      ethosudev->npu_info.sram_size);
-err_job_fini:
-	ethosu_job_fini(ethosudev);
 	return ret;
 }
 
 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);
 
 	if (ethosudev->sram)
diff --git a/drivers/accel/ethosu/ethosu_drv.h b/drivers/accel/ethosu/ethosu_drv.h
index f59c845c758b..6d4ef8ce9378 100644
--- a/drivers/accel/ethosu/ethosu_drv.h
+++ b/drivers/accel/ethosu/ethosu_drv.h
@@ -4,18 +4,22 @@
 #define __ETHOSU_DRV_H__
 
 #include <linux/mutex.h>
+#include <linux/list.h>
 #include <linux/xarray.h>
 #include <drm/gpu_scheduler.h>
 
 struct ethosu_device;
+struct device;
 struct drm_device;
 struct drm_file;
 
 int ethosu_device_reset(struct ethosu_device *ethosudev);
+int ethosu_device_suspend(struct device *dev);
 
 struct ethosu_file_priv {
 	struct ethosu_device *edev;
 	struct drm_sched_entity sched_entity;
+	struct list_head sched_entity_list;
 	struct xarray perfmons;
 };
 
diff --git a/drivers/accel/ethosu/ethosu_job.c b/drivers/accel/ethosu/ethosu_job.c
index 8ffdc83d06f9..ef95a8abd61e 100644
--- a/drivers/accel/ethosu/ethosu_job.c
+++ b/drivers/accel/ethosu/ethosu_job.c
@@ -6,6 +6,7 @@
 #include <linux/genalloc.h>
 #include <linux/interrupt.h>
 #include <linux/iopoll.h>
+#include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 
@@ -13,6 +14,7 @@
 #include <drm/drm_drv.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_gem_dma_helper.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_print.h>
 #include <drm/ethosu_accel.h>
 
@@ -149,6 +151,15 @@ static int ethosu_job_push(struct ethosu_job *job)
 
 	ret = pm_runtime_resume_and_get(job->dev->base.dev);
 	if (!ret) {
+		if (!try_module_get(THIS_MODULE)) {
+			pm_runtime_put_autosuspend(job->dev->base.dev);
+			ret = -ENODEV;
+			goto out;
+		}
+
+		drm_dev_get(&job->dev->base);
+		job->keep_device = true;
+
 		guard(mutex)(&job->dev->sched_lock);
 
 		drm_sched_job_arm(&job->base);
@@ -188,12 +199,19 @@ static void ethosu_job_cleanup(struct kref *ref)
 {
 	struct ethosu_job *job = container_of(ref, struct ethosu_job,
 						refcount);
+	struct drm_device *ddev = &job->dev->base;
+	bool keep_device = job->keep_device;
 
 	pm_runtime_put_autosuspend(job->dev->base.dev);
 
 	dma_fence_put(job->inference_done_fence);
 
 	ethosu_job_err_cleanup(job);
+
+	if (keep_device) {
+		drm_dev_put(ddev);
+		module_put(THIS_MODULE);
+	}
 }
 
 static void ethosu_job_put(struct ethosu_job *job)
@@ -357,6 +375,8 @@ static const struct drm_sched_backend_ops ethosu_sched_ops = {
 	.cancel_job = ethosu_job_cancel,
 };
 
+static void ethosu_job_fini(struct drm_device *ddev, void *data);
+
 int ethosu_job_init(struct ethosu_device *edev)
 {
 	struct device *dev = edev->base.dev;
@@ -372,7 +392,20 @@ int ethosu_job_init(struct ethosu_device *edev)
 
 	spin_lock_init(&edev->fence_lock);
 	spin_lock_init(&edev->job_lock);
-	ret = devm_mutex_init(dev, &edev->sched_lock);
+	ret = drmm_mutex_init(&edev->base, &edev->sched_lock);
+	if (ret)
+		return ret;
+	INIT_LIST_HEAD(&edev->sched_entities);
+
+	edev->fence_context = dma_fence_context_alloc(1);
+
+	ret = drm_sched_init(&edev->sched, &args);
+	if (ret) {
+		dev_err(dev, "Failed to create scheduler: %d\n", ret);
+		return ret;
+	}
+
+	ret = drmm_add_action_or_reset(&edev->base, ethosu_job_fini, edev);
 	if (ret)
 		return ret;
 
@@ -388,45 +421,76 @@ int ethosu_job_init(struct ethosu_device *edev)
 	if (ret)
 		return ret;
 
-	edev->fence_context = dma_fence_context_alloc(1);
+	return 0;
+}
 
-	ret = drm_sched_init(&edev->sched, &args);
-	if (ret) {
-		dev_err(dev, "Failed to create scheduler: %d\n", ret);
-		return ret;
-	}
+static void ethosu_job_fini(struct drm_device *ddev, void *data)
+{
+	struct ethosu_device *dev = to_ethosu_device(ddev);
 
-	return 0;
+	(void)data;
+	drm_sched_fini(&dev->sched);
 }
 
-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;
 }
 
 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);
 }
 
diff --git a/drivers/accel/ethosu/ethosu_job.h b/drivers/accel/ethosu/ethosu_job.h
index 8988edd00eed..6d4622dfac43 100644
--- a/drivers/accel/ethosu/ethosu_job.h
+++ b/drivers/accel/ethosu/ethosu_job.h
@@ -29,13 +29,16 @@ struct ethosu_job {
 	/* Fence to be signaled by IRQ handler when the job is complete. */
 	struct dma_fence *done_fence;
 
+	/* Keep driver state alive while a killed job waits on dependencies. */
+	bool keep_device;
+
 	struct kref refcount;
 };
 
 int ethosu_ioctl_submit(struct drm_device *dev, void *data, struct drm_file *file);
 
 int ethosu_job_init(struct ethosu_device *dev);
-void ethosu_job_fini(struct ethosu_device *dev);
+bool ethosu_job_stop(struct ethosu_device *dev);
 int ethosu_job_open(struct ethosu_file_priv *ethosu_priv);
 void ethosu_job_close(struct ethosu_file_priv *ethosu_priv);
 

-- 
2.53.0

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

[sashiko-bot]

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

[PATCH v4 11/31] accel: ethosu: Clear SRAM before submission

[Rob Herring (Arm)]

SRAM contents persist between jobs including different users. Clear the
SRAM before programming the SRAM region to prevent one client observing
another client's data.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v4:
 - New patch
---
 drivers/accel/ethosu/ethosu_device.h |  2 ++
 drivers/accel/ethosu/ethosu_job.c    | 47 +++++++++++++++++++++++++++++++-----
 2 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_device.h b/drivers/accel/ethosu/ethosu_device.h
index 1a90d0ebe5d9..af1489fe08c4 100644
--- a/drivers/accel/ethosu/ethosu_device.h
+++ b/drivers/accel/ethosu/ethosu_device.h
@@ -205,6 +205,8 @@ struct ethosu_device {
 	spinlock_t fence_lock;
 
 	struct drm_gpu_scheduler sched;
+	/* Scheduler entity that last used SRAM. */
+	struct drm_sched_entity *sram_entity;
 	/* For ethosu_job_do_push() */
 	struct mutex sched_lock;
 	struct list_head sched_entities;
diff --git a/drivers/accel/ethosu/ethosu_job.c b/drivers/accel/ethosu/ethosu_job.c
index ef95a8abd61e..3976115902e8 100644
--- a/drivers/accel/ethosu/ethosu_job.c
+++ b/drivers/accel/ethosu/ethosu_job.c
@@ -45,11 +45,27 @@ 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)
+static u32 ethosu_job_sram_size(struct ethosu_job *job)
 {
+	struct ethosu_validated_cmdstream_info *cmd_info =
+		to_ethosu_bo(job->cmd_bo)->info;
+
+	return cmd_info->region_size[ETHOSU_SRAM_REGION];
+}
+
+static bool ethosu_job_set_in_flight(struct ethosu_device *dev,
+				     struct ethosu_job *job, u32 sram_size)
+{
+	bool clear_sram = false;
+
 	guard(spinlock)(&dev->job_lock);
+	if (sram_size && dev->sram_entity != job->base.entity) {
+		dev->sram_entity = job->base.entity;
+		clear_sram = true;
+	}
 	dev->in_flight_job = job;
+
+	return clear_sram;
 }
 
 static struct ethosu_job *
@@ -64,7 +80,8 @@ ethosu_job_clear_in_flight(struct ethosu_device *dev)
 	return job;
 }
 
-static void ethosu_job_hw_submit(struct ethosu_device *dev, struct ethosu_job *job)
+static void ethosu_job_hw_submit(struct ethosu_device *dev, struct ethosu_job *job,
+				 u32 sram_size, bool clear_sram)
 {
 	struct drm_gem_dma_object *cmd_bo = to_drm_gem_dma_obj(job->cmd_bo);
 	struct ethosu_validated_cmdstream_info *cmd_info = to_ethosu_bo(job->cmd_bo)->info;
@@ -79,7 +96,12 @@ static void ethosu_job_hw_submit(struct ethosu_device *dev, struct ethosu_job *j
 		dev_dbg(dev->base.dev, "Region %d base addr = %pad\n", region, &bo->dma_addr);
 	}
 
-	if (job->sram_size) {
+	if (sram_size) {
+		if (clear_sram) {
+			memset_io(dev->sram, 0, dev->npu_info.sram_size);
+			/* Order the SRAM clear before starting the NPU. */
+			wmb();
+		}
 		writel_relaxed(lower_32_bits(dev->sramphys),
 			       dev->regs + NPU_REG_BASEP(ETHOSU_SRAM_REGION));
 		writel_relaxed(upper_32_bits(dev->sramphys),
@@ -263,6 +285,8 @@ static struct dma_fence *ethosu_job_run(struct drm_sched_job *sched_job)
 	struct ethosu_job *job = to_ethosu_job(sched_job);
 	struct ethosu_device *dev = job->dev;
 	struct dma_fence *fence = job->done_fence;
+	u32 sram_size;
+	bool clear_sram;
 
 	if (unlikely(job->base.s_fence->finished.error))
 		return NULL;
@@ -273,8 +297,9 @@ static struct dma_fence *ethosu_job_run(struct drm_sched_job *sched_job)
 
 	ethosu_switch_perfmon(dev, job);
 
-	ethosu_job_set_in_flight(dev, job);
-	ethosu_job_hw_submit(dev, job);
+	sram_size = ethosu_job_sram_size(job);
+	clear_sram = ethosu_job_set_in_flight(dev, job, sram_size);
+	ethosu_job_hw_submit(dev, job, sram_size, clear_sram);
 
 	return fence;
 }
@@ -482,12 +507,22 @@ int ethosu_job_open(struct ethosu_file_priv *ethosu_priv)
 	return ret;
 }
 
+static void ethosu_job_clear_sram_entity(struct ethosu_device *dev,
+					 struct drm_sched_entity *entity)
+{
+	guard(spinlock)(&dev->job_lock);
+	if (dev->sram_entity == entity) {
+		dev->sram_entity = NULL;
+	}
+}
+
 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) {
+		ethosu_job_clear_sram_entity(dev, entity);
 		drm_sched_entity_kill(entity);
 		list_del(&ethosu_priv->sched_entity_list);
 	}

-- 
2.53.0

Re: [PATCH v4 11/31] accel: ethosu: Clear SRAM before submission

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] accel: ethosu: ethosu_job_sram_size() clobbers region 2 GEM BO with SRAM addr
- [High] accel: ethosu: job->base.entity is always NULL in ethosu_job_set_in_flight()
- [Medium] accel: ethosu: ethosu_job_close() clears sram_entity before killing entity
--

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

accel: ethosu: Clear SRAM before submission

This patch tracks the scheduler entity that last used the on-chip SRAM and
clears the SRAM before submitting a job from a different entity.

> diff --git a/drivers/accel/ethosu/ethosu_job.c b/drivers/accel/ethosu/ethosu_job.c
> index ef95a8abd61e2..3976115902e83 100644
> --- a/drivers/accel/ethosu/ethosu_job.c
> +++ b/drivers/accel/ethosu/ethosu_job.c
> @@ -45,11 +45,27 @@ 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)
> +static u32 ethosu_job_sram_size(struct ethosu_job *job)
>  {
> +	struct ethosu_validated_cmdstream_info *cmd_info =
> +		to_ethosu_bo(job->cmd_bo)->info;
> +
> +	return cmd_info->region_size[ETHOSU_SRAM_REGION];
> +}
> +
> +static bool ethosu_job_set_in_flight(struct ethosu_device *dev,
> +				     struct ethosu_job *job, u32 sram_size)
> +{
> +	bool clear_sram = false;
> +
>  	guard(spinlock)(&dev->job_lock);
> +	if (sram_size && dev->sram_entity != job->base.entity) {
> +		dev->sram_entity = job->base.entity;
> +		clear_sram = true;
> +	}

[Severity: High]
Isn't job->base.entity always NULL when ethosu_job_set_in_flight() is called
from ethosu_job_run()?

In the DRM scheduler, drm_sched_run_job_work() pops the job via
drm_sched_entity_pop_job() before invoking sched->ops->run_job(sched_job):

drivers/gpu/drm/scheduler/sched_entity.c:drm_sched_entity_pop_job() {
	...
	/* Jobs and entities might have different lifecycles. Since we're
	 * removing the job from the entities queue, set the jobs entity pointer
	 * to NULL to prevent any future access of the entity through this job.
	 */
	sched_job->entity = NULL;

	return sched_job;
}

Because job->base.entity is set to NULL before ethosu_job_run() runs, and
dev->sram_entity starts at NULL, won't dev->sram_entity != job->base.entity
always evaluate to false (NULL != NULL), leaving clear_sram false so that
memset_io() in ethosu_job_hw_submit() never executes?

Would tracking job->base.s_fence->drm_client_id (or
job->base.s_fence->scheduled.context) instead of job->base.entity avoid this?

>  	dev->in_flight_job = job;
> +
> +	return clear_sram;
>  }
>  
>  static struct ethosu_job *
> @@ -64,7 +80,8 @@ ethosu_job_clear_in_flight(struct ethosu_device *dev)
>  	return job;
>  }
>  
> -static void ethosu_job_hw_submit(struct ethosu_device *dev, struct ethosu_job *job)
> +static void ethosu_job_hw_submit(struct ethosu_device *dev, struct ethosu_job *job,
> +				 u32 sram_size, bool clear_sram)
>  {
>  	struct drm_gem_dma_object *cmd_bo = to_drm_gem_dma_obj(job->cmd_bo);
>  	struct ethosu_validated_cmdstream_info *cmd_info = to_ethosu_bo(job->cmd_bo)->info;
> @@ -79,7 +96,12 @@ static void ethosu_job_hw_submit(struct ethosu_device *dev, struct ethosu_job *j
>  		dev_dbg(dev->base.dev, "Region %d base addr = %pad\n", region, &bo->dma_addr);
>  	}
>  
> -	if (job->sram_size) {
> +	if (sram_size) {
> +		if (clear_sram) {
> +			memset_io(dev->sram, 0, dev->npu_info.sram_size);
> +			/* Order the SRAM clear before starting the NPU. */
> +			wmb();
> +		}
>  		writel_relaxed(lower_32_bits(dev->sramphys),
>  			       dev->regs + NPU_REG_BASEP(ETHOSU_SRAM_REGION));
>  		writel_relaxed(upper_32_bits(dev->sramphys),

[Severity: Critical]
When a job backs region 2 (ETHOSU_SRAM_REGION) with a GEM BO and sets
job->sram_size to 0, won't replacing the job->sram_size check with
ethosu_job_sram_size() overwrite the region 2 GEM BO DMA address with
dev->sramphys?

In ethosu_ioctl_submit_job(), region 2 is only reserved for on-chip SRAM when
job->sram_size is non-zero:

	/* BO region 2 is reserved if SRAM is used */
	if (job->region_bo_handles[ETHOSU_SRAM_REGION] && job->sram_size)
		return -EINVAL;

	if (edev->npu_info.sram_size < job->sram_size)
		return -EINVAL;

When job->sram_size is 0 and job->region_bo_handles[ETHOSU_SRAM_REGION] is
non-zero, ethosu_ioctl_submit_job() only validates
cmd_info->region_size[ETHOSU_SRAM_REGION] against gem->size, not against
edev->npu_info.sram_size.

Since ethosu_job_sram_size() unconditionally returns
cmd_info->region_size[ETHOSU_SRAM_REGION] (truncated to u32) without checking
job->sram_size, ethosu_job_hw_submit() will clobber
NPU_REG_BASEP(ETHOSU_SRAM_REGION) and NPU_REG_BASEP_HI(ETHOSU_SRAM_REGION)
with dev->sramphys, causing the NPU to perform unchecked DMA beyond the
bounds of on-chip SRAM (or at physical address 0 when no SRAM is configured).

Should this continue checking job->sram_size instead of
cmd_info->region_size[ETHOSU_SRAM_REGION]?

[ ... ]

> @@ -482,12 +507,22 @@ int ethosu_job_open(struct ethosu_file_priv *ethosu_priv)
>  	return ret;
>  }
>  
> +static void ethosu_job_clear_sram_entity(struct ethosu_device *dev,
> +					 struct drm_sched_entity *entity)
> +{
> +	guard(spinlock)(&dev->job_lock);
> +	if (dev->sram_entity == entity) {
> +		dev->sram_entity = NULL;
> +	}
> +}
> +
>  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) {
> +		ethosu_job_clear_sram_entity(dev, entity);
>  		drm_sched_entity_kill(entity);
>  		list_del(&ethosu_priv->sched_entity_list);
>  	}

[Severity: Medium]
Can ethosu_job_clear_sram_entity() race with ethosu_job_run() if it is called
before drm_sched_entity_kill()?

drm_sched_run_job_work() runs on the scheduler workqueue without holding
dev->sched_lock, invokes ethosu_job_run() -> ethosu_job_set_in_flight(), and
only signals entity->entity_idle after run_job() returns.

If ethosu_job_clear_sram_entity() runs before drm_sched_entity_kill() stops
the entity and waits on entity->entity_idle, a concurrent ethosu_job_run() can
repopulate dev->sram_entity right before ethosu_priv is freed in
ethosu_postclose(). If a subsequent open() reallocates ethosu_file_priv at the
same address, could the stale dev->sram_entity pointer match the new entity
and skip clearing SRAM?

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

[PATCH v4 12/31] accel: ethosu: Validate timeout command offset

[Rob Herring (Arm)]

QREAD is supplied by the NPU. Check that its command-stream offset is
word aligned and within the validated command stream before using it to
index the command buffer in the timeout diagnostic.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v4:
 - New patch
---
 drivers/accel/ethosu/ethosu_job.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_job.c b/drivers/accel/ethosu/ethosu_job.c
index 3976115902e8..77218bb17006 100644
--- a/drivers/accel/ethosu/ethosu_job.c
+++ b/drivers/accel/ethosu/ethosu_job.c
@@ -345,6 +345,8 @@ static enum drm_gpu_sched_stat ethosu_job_timedout(struct drm_sched_job *bad)
 {
 	struct ethosu_job *job = to_ethosu_job(bad);
 	struct ethosu_device *dev = job->dev;
+	struct ethosu_validated_cmdstream_info *cmd_info =
+		to_ethosu_bo(job->cmd_bo)->info;
 	bool running;
 	int ret;
 	u32 *bocmds = to_drm_gem_dma_obj(job->cmd_bo)->vaddr;
@@ -367,9 +369,15 @@ static enum drm_gpu_sched_stat ethosu_job_timedout(struct drm_sched_job *bad)
 			return DRM_GPU_SCHED_STAT_NO_HANG;
 	}
 
-	dev_err(dev->base.dev, "NPU sched timed out: NPU %s, cmdstream offset 0x%x: 0x%x\n",
-		running ? "running" : "stopped",
-		cmdaddr, bocmds[cmdaddr / 4]);
+	if (!(cmdaddr & 3) && cmdaddr < cmd_info->cmd_size)
+		dev_err(dev->base.dev,
+			"NPU sched timed out: NPU %s, cmdstream offset 0x%x: 0x%x\n",
+			running ? "running" : "stopped", cmdaddr,
+			bocmds[cmdaddr / 4]);
+	else
+		dev_err(dev->base.dev,
+			"NPU sched timed out: NPU %s, invalid cmdstream offset 0x%x\n",
+			running ? "running" : "stopped", cmdaddr);
 
 	drm_sched_stop(&dev->sched, bad);
 

-- 
2.53.0

[PATCH v4 13/31] accel: ethosu: Prevent command stream export

[Rob Herring (Arm)]

Command stream BOs are validated at creation time, but the generic PRIME
export path can expose their backing memory to a DMA-BUF importer. The
importer can then modify the command stream before the NPU executes it.

Reject PRIME export for validated command stream BOs while retaining
generic export for data BOs.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v3:
 - new patch
---
 drivers/accel/ethosu/ethosu_gem.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index 9afe2549ec84..c046aee42687 100644
--- a/drivers/accel/ethosu/ethosu_gem.c
+++ b/drivers/accel/ethosu/ethosu_gem.c
@@ -6,6 +6,7 @@
 #include <linux/slab.h>
 
 #include <drm/ethosu_accel.h>
+#include <drm/drm_prime.h>
 
 #include "ethosu_device.h"
 #include "ethosu_gem.h"
@@ -30,9 +31,18 @@ static int ethosu_gem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vm
 	return drm_gem_dma_object_mmap(obj, vma);
 }
 
+static struct dma_buf *ethosu_gem_export(struct drm_gem_object *obj, int flags)
+{
+	if (to_ethosu_bo(obj)->info)
+		return ERR_PTR(-EPERM);
+
+	return drm_gem_prime_export(obj, flags);
+}
+
 static const struct drm_gem_object_funcs ethosu_gem_funcs = {
 	.free = ethosu_gem_free_object,
 	.print_info = drm_gem_dma_object_print_info,
+	.export = ethosu_gem_export,
 	.get_sg_table = drm_gem_dma_object_get_sg_table,
 	.vmap = drm_gem_dma_object_vmap,
 	.mmap = ethosu_gem_mmap,

-- 
2.53.0

[PATCH v4 14/31] accel: ethosu: Move DMA mode to src/dst struct

[Rob Herring (Arm)]

The DMA mode setting is independent for source and destination, so it
should be part of the src/dst struct dma rather than the global DMA
state.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v2:
 - new patch
---
 drivers/accel/ethosu/ethosu_gem.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index c046aee42687..2b9c98251c94 100644
--- a/drivers/accel/ethosu/ethosu_gem.c
+++ b/drivers/accel/ethosu/ethosu_gem.c
@@ -110,6 +110,7 @@ int ethosu_gem_create_with_handle(struct drm_file *file,
 
 struct dma {
 	s8 region;
+	s8 mode;
 	u64 len;
 	u64 offset;
 	s64 stride[2];
@@ -118,7 +119,6 @@ struct dma {
 struct dma_state {
 	u16 size0;
 	u16 size1;
-	s8 mode;
 	struct dma src;
 	struct dma dst;
 };
@@ -171,7 +171,7 @@ static u64 cmd_to_addr(u32 *cmd)
 static u64 dma_length(struct ethosu_validated_cmdstream_info *info,
 		      struct dma_state *dma_st, struct dma *dma)
 {
-	s8 mode = dma_st->mode;
+	s8 mode = dma->mode;
 	u64 len = dma->len;
 
 	if (len == U64_MAX)
@@ -664,13 +664,14 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 				st.dma.src.region = -1;
 			else
 				st.dma.src.region = param & 0x7;
-			st.dma.mode = (param >> 9) & 0x3;
+			st.dma.src.mode = (param >> 9) & 0x3;
 			break;
 		case NPU_SET_DMA0_DST_REGION:
 			if (param & 0x100)
 				st.dma.dst.region = -1;
 			else
 				st.dma.dst.region = param & 0x7;
+			st.dma.dst.mode = (param >> 9) & 0x3;
 			break;
 		case NPU_SET_DMA0_SIZE0:
 			st.dma.size0 = param;

-- 
2.53.0

[PATCH v4 15/31] accel: ethosu: Track command stream register setup

[Rob Herring (Arm)]

The current method of tracking command stream state initializing state
tracking to illegal values and assuming unaccessed registers are 0 is
proving inadequate with additional validation. Instead, track all the
registers in a bitmap as the register address space is fairly small.

CMD1 opcodes overlap CMD0 after bit 14 is stripped, so maintain a
separate bitmap for each bank.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v4:
 - Use NPU_CMD_CTRL_CMD1 define

v3:
 - Fix return value in dma_params_valid()

v2:
 - new patch
---
 drivers/accel/ethosu/ethosu_device.h |   2 +
 drivers/accel/ethosu/ethosu_gem.c    | 271 ++++++++++++++++++++++++++++++-----
 2 files changed, 237 insertions(+), 36 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_device.h b/drivers/accel/ethosu/ethosu_device.h
index af1489fe08c4..8405e69acb0c 100644
--- a/drivers/accel/ethosu/ethosu_device.h
+++ b/drivers/accel/ethosu/ethosu_device.h
@@ -88,6 +88,8 @@ struct gen_pool;
 #define PMU_EV_TYPE_CYCLES	0x11
 #define PMU_EV_TYPE_IDLE	0x20
 
+#define NPU_CMD_CTRL_CMD1		BIT(14)
+
 enum ethosu_cmds {
 	NPU_OP_STOP = 0x0,
 	NPU_OP_CONV = 0x2,
diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index 2b9c98251c94..75f6901d19e5 100644
--- a/drivers/accel/ethosu/ethosu_gem.c
+++ b/drivers/accel/ethosu/ethosu_gem.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only or MIT
 /* Copyright 2025 Arm, Ltd. */
 
+#include <linux/bitmap.h>
 #include <linux/err.h>
 #include <linux/overflow.h>
 #include <linux/slab.h>
@@ -148,7 +149,12 @@ struct feat_matrix {
 	u8 pad_right;
 };
 
+#define NPU_CMD0_REGS	0x200
+#define NPU_CMD1_REGS	0x100
+
 struct cmd_state {
+	DECLARE_BITMAP(cmd0, NPU_CMD0_REGS);
+	DECLARE_BITMAP(cmd1, NPU_CMD1_REGS);
 	struct dma_state dma;
 	struct buffer scale[2];
 	struct buffer weight[4];
@@ -159,8 +165,29 @@ struct cmd_state {
 
 static void cmd_state_init(struct cmd_state *st)
 {
-	/* Initialize to all 1s to detect missing setup */
-	memset(st, 0xff, sizeof(*st));
+	memset(st, 0, sizeof(*st));
+}
+
+static void cmd_state_set_reg(struct cmd_state *st, u16 cmd)
+{
+	u16 reg = cmd & ~NPU_CMD_CTRL_CMD1;
+
+	if (cmd & NPU_CMD_CTRL_CMD1) {
+		if (reg < NPU_CMD1_REGS)
+			__set_bit(reg, st->cmd1);
+	} else if (reg < NPU_CMD0_REGS) {
+		__set_bit(reg, st->cmd0);
+	}
+}
+
+static bool cmd_state_reg_is_set(struct cmd_state *st, u16 cmd)
+{
+	u16 reg = cmd & ~NPU_CMD_CTRL_CMD1;
+
+	if (cmd & NPU_CMD_CTRL_CMD1)
+		return reg < NPU_CMD1_REGS && test_bit(reg, st->cmd1);
+
+	return reg < NPU_CMD0_REGS && test_bit(reg, st->cmd0);
 }
 
 static u64 cmd_to_addr(u32 *cmd)
@@ -168,13 +195,54 @@ static u64 cmd_to_addr(u32 *cmd)
 	return (((u64)cmd[0] & 0xff0000) << 16) | cmd[1];
 }
 
-static u64 dma_length(struct ethosu_validated_cmdstream_info *info,
-		      struct dma_state *dma_st, struct dma *dma)
+static bool dma_use_src_stride(struct ethosu_device *edev,
+			       const struct dma_state *dma_st, const struct dma *dma)
+{
+	return ethosu_is_u65(edev) || dma == &dma_st->src;
+}
+
+static bool dma_params_valid(struct ethosu_device *edev, struct cmd_state *st,
+			     const struct dma_state *dma_st,
+			     const struct dma *dma,
+			     u16 region_cmd, u16 addr_cmd)
+{
+	s8 mode = dma->mode;
+
+	if (!cmd_state_reg_is_set(st, region_cmd) ||
+	    !cmd_state_reg_is_set(st, addr_cmd) ||
+	    !cmd_state_reg_is_set(st, NPU_SET_DMA0_LEN) || mode < 0 || mode > 2)
+		return false;
+
+	if (mode >= 1 &&
+	    !cmd_state_reg_is_set(st, dma_use_src_stride(edev, dma_st, dma) ?
+				  NPU_SET_DMA0_SRC_STRIDE0 :
+				  NPU_SET_DMA0_DST_STRIDE0))
+		return false;
+	if (mode == 2 &&
+	    !cmd_state_reg_is_set(st, dma_use_src_stride(edev, dma_st, dma) ?
+				  NPU_SET_DMA0_SRC_STRIDE1 :
+				  NPU_SET_DMA0_DST_STRIDE1))
+		return false;
+
+	if (mode >= 1 &&
+	    (!cmd_state_reg_is_set(st, NPU_SET_DMA0_SIZE0) || !dma_st->size0))
+		return false;
+	if (mode == 2 &&
+	    (!cmd_state_reg_is_set(st, NPU_SET_DMA0_SIZE1) || !dma_st->size1))
+		return false;
+
+	return true;
+}
+
+static u64 dma_length(struct ethosu_device *edev,
+		      struct ethosu_validated_cmdstream_info *info,
+		      struct cmd_state *st, struct dma_state *dma_st,
+		      struct dma *dma, u16 region_cmd, u16 addr_cmd)
 {
 	s8 mode = dma->mode;
 	u64 len = dma->len;
 
-	if (len == U64_MAX)
+	if (!dma_params_valid(edev, st, dma_st, dma, region_cmd, addr_cmd))
 		return U64_MAX;
 
 	if (mode >= 1) {
@@ -209,17 +277,98 @@ static bool feat_matrix_chained(struct ethosu_device *edev, struct feat_matrix *
 	return !ethosu_is_u65(edev) && storage == 2;
 }
 
+enum feat_matrix_type {
+	FEAT_MATRIX_IFM,
+	FEAT_MATRIX_OFM,
+	FEAT_MATRIX_IFM2,
+};
+
+static u16 feat_matrix_base_cmd(enum feat_matrix_type type)
+{
+	switch (type) {
+	case FEAT_MATRIX_IFM:
+		return NPU_SET_IFM_BASE0;
+	case FEAT_MATRIX_OFM:
+		return NPU_SET_OFM_BASE0;
+	case FEAT_MATRIX_IFM2:
+		return NPU_SET_IFM2_BASE0;
+	}
+
+	return 0;
+}
+
+static int feat_matrix_validate(struct ethosu_device *edev,
+				struct cmd_state *st, struct feat_matrix *fm,
+				enum feat_matrix_type type)
+{
+	u32 format;
+	u16 stride_cmd;
+
+	switch (type) {
+	case FEAT_MATRIX_IFM:
+		if (!cmd_state_reg_is_set(st, NPU_SET_IFM_REGION) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_IFM_PRECISION) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_IFM_DEPTH_M1))
+			return -EINVAL;
+		if (feat_matrix_chained(edev, fm))
+			return 0;
+		if (!cmd_state_reg_is_set(st, NPU_SET_IFM_WIDTH0_M1) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_IFM_HEIGHT0_M1) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_IFM_HEIGHT1_M1) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_IFM_STRIDE_Y))
+			return -EINVAL;
+		break;
+	case FEAT_MATRIX_OFM:
+		if (!cmd_state_reg_is_set(st, NPU_SET_OFM_REGION) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_OFM_PRECISION) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_OFM_DEPTH_M1) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_OFM_WIDTH_M1) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_OFM_HEIGHT_M1))
+			return -EINVAL;
+		if (feat_matrix_chained(edev, fm))
+			return 0;
+		if (!cmd_state_reg_is_set(st, NPU_SET_OFM_WIDTH0_M1) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_OFM_HEIGHT0_M1) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_OFM_HEIGHT1_M1) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_OFM_STRIDE_Y))
+			return -EINVAL;
+		break;
+	case FEAT_MATRIX_IFM2:
+		if (!cmd_state_reg_is_set(st, NPU_SET_IFM2_REGION) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_IFM2_PRECISION))
+			return -EINVAL;
+		if (feat_matrix_chained(edev, fm))
+			return 0;
+		if (!cmd_state_reg_is_set(st, NPU_SET_IFM2_WIDTH0_M1) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_IFM2_HEIGHT0_M1) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_IFM2_HEIGHT1_M1) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_IFM2_STRIDE_Y))
+			return -EINVAL;
+		break;
+	}
+
+	format = (fm->precision >> 6) & 0x3;
+	stride_cmd = feat_matrix_base_cmd(type) + (format ? 6 : 4);
+	if (!cmd_state_reg_is_set(st, stride_cmd))
+		return -EINVAL;
+
+	return 0;
+}
 static u64 feat_matrix_length(struct ethosu_device *edev,
 			      struct ethosu_validated_cmdstream_info *info,
-			      struct feat_matrix *fm,
+			      struct cmd_state *st, struct feat_matrix *fm,
+			      enum feat_matrix_type type,
 			      u32 x, u32 y, u32 c, bool ofm)
 {
 	u32 element_size, storage = ethosu_is_u65(edev) ? 0 : fm->precision >> 14;
 	int tile = 0;
 	u64 addr;
+	u64 offset;
 
 	if (fm->region < 0)
 		return U64_MAX;
+	if (feat_matrix_validate(edev, st, fm, type))
+		return U64_MAX;
 
 	if (feat_matrix_chained(edev, fm))
 		return 0;
@@ -247,24 +396,39 @@ static u64 feat_matrix_length(struct ethosu_device *edev,
 	default:
 		return U64_MAX;
 	}
-	if (fm->base[tile] == U64_MAX)
+	if (!cmd_state_reg_is_set(st, feat_matrix_base_cmd(type) + tile))
 		return U64_MAX;
 
-	addr = fm->base[tile] + y * fm->stride_y;
+	if (check_mul_overflow(y, (u64)fm->stride_y, &offset) ||
+	    check_add_overflow(fm->base[tile], offset, &addr))
+		return U64_MAX;
 
 	switch ((fm->precision >> 6) & 0x3) { // format
 	case 0: //nhwc:
 		element_size = BIT((fm->precision >> (ofm ? 1 : 2)) & 0x3);
-		addr += x * fm->stride_x + c * element_size;
+		if (check_mul_overflow(x, (u64)fm->stride_x, &offset) ||
+		    check_add_overflow(addr, offset, &addr) ||
+		    check_mul_overflow(c, element_size, &offset) ||
+		    check_add_overflow(addr, offset, &addr))
+			return U64_MAX;
 		break;
 	case 1: //nhcwb16:
 		element_size = BIT((fm->precision >> (ofm ? 1 : 2)) & 0x3);
 
-		addr += (c / 16) * fm->stride_c + (16 * x + (c & 0xf)) * element_size;
+		if (check_mul_overflow(c / 16, (u64)fm->stride_c, &offset) ||
+		    check_add_overflow(addr, offset, &addr) ||
+		    check_mul_overflow(16 * x + (c & 0xf), element_size, &offset) ||
+		    check_add_overflow(addr, offset, &addr))
+			return U64_MAX;
 		break;
+	default:
+		return U64_MAX;
 	}
 
-	info->region_size[fm->region] = max(info->region_size[fm->region], addr + 1);
+	if (check_add_overflow(addr, 1ULL, &offset))
+		return U64_MAX;
+
+	info->region_size[fm->region] = max(info->region_size[fm->region], offset);
 
 	return addr;
 }
@@ -278,7 +442,13 @@ static int calc_sizes(struct drm_device *ddev,
 	u64 len;
 
 	if (ifm) {
-		if (st->ifm.stride_kernel == U16_MAX)
+		if (!cmd_state_reg_is_set(st, NPU_SET_KERNEL_WIDTH_M1) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_KERNEL_HEIGHT_M1) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_KERNEL_STRIDE) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_IFM_PAD_TOP) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_IFM_PAD_LEFT) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_IFM_PAD_RIGHT) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_IFM_PAD_BOTTOM))
 			return -EINVAL;
 		u32 stride_y = ((st->ifm.stride_kernel >> 8) & 0x2) +
 			((st->ifm.stride_kernel >> 1) & 0x1) + 1;
@@ -292,8 +462,9 @@ static int calc_sizes(struct drm_device *ddev,
 		if (ifm_height < 0 || ifm_width < 0)
 			return -EINVAL;
 
-		len = feat_matrix_length(edev, info, &st->ifm, ifm_width,
-					 ifm_height, st->ifm.depth, false);
+		len = feat_matrix_length(edev, info, st, &st->ifm,
+					 FEAT_MATRIX_IFM, ifm_width, ifm_height,
+					 st->ifm.depth, false);
 		dev_dbg(ddev->dev, "op %d: IFM:%d:0x%llx-0x%llx\n",
 			op, st->ifm.region, st->ifm.base[0], len);
 		if (len == U64_MAX)
@@ -301,8 +472,9 @@ static int calc_sizes(struct drm_device *ddev,
 	}
 
 	if (ifm2) {
-		len = feat_matrix_length(edev, info, &st->ifm2, st->ifm.depth,
-					 0, st->ofm.depth, false);
+		len = feat_matrix_length(edev, info, st, &st->ifm2,
+					 FEAT_MATRIX_IFM2, st->ifm.depth, 0,
+					 st->ofm.depth, false);
 		dev_dbg(ddev->dev, "op %d: IFM2:%d:0x%llx-0x%llx\n",
 			op, st->ifm2.region, st->ifm2.base[0], len);
 		if (len == U64_MAX)
@@ -313,8 +485,9 @@ static int calc_sizes(struct drm_device *ddev,
 		dev_dbg(ddev->dev, "op %d: W:%d:0x%llx-0x%llx\n",
 			op, st->weight[0].region, st->weight[0].base,
 			st->weight[0].base + st->weight[0].length - 1);
-		if (st->weight[0].region < 0 || st->weight[0].base == U64_MAX ||
-		    st->weight[0].length == U32_MAX)
+		if (!cmd_state_reg_is_set(st, NPU_SET_WEIGHT_REGION) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_WEIGHT_BASE) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_WEIGHT_LENGTH))
 			return -EINVAL;
 		info->region_size[st->weight[0].region] =
 			max(info->region_size[st->weight[0].region],
@@ -325,16 +498,18 @@ static int calc_sizes(struct drm_device *ddev,
 		dev_dbg(ddev->dev, "op %d: S:%d:0x%llx-0x%llx\n",
 			op, st->scale[0].region, st->scale[0].base,
 			st->scale[0].base + st->scale[0].length - 1);
-		if (st->scale[0].region < 0 || st->scale[0].base == U64_MAX ||
-		    st->scale[0].length == U32_MAX)
+		if (!cmd_state_reg_is_set(st, NPU_SET_SCALE_REGION) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_SCALE_BASE) ||
+		    !cmd_state_reg_is_set(st, NPU_SET_SCALE_LENGTH))
 			return -EINVAL;
 		info->region_size[st->scale[0].region] =
 			max(info->region_size[st->scale[0].region],
 			    st->scale[0].base + st->scale[0].length);
 	}
 
-	len = feat_matrix_length(edev, info, &st->ofm, st->ofm.width,
-				 st->ofm.height[2], st->ofm.depth, true);
+	len = feat_matrix_length(edev, info, st, &st->ofm, FEAT_MATRIX_OFM,
+				 st->ofm.width, st->ofm.height[2], st->ofm.depth,
+				 true);
 	dev_dbg(ddev->dev, "op %d: OFM:%d:0x%llx-0x%llx\n",
 		op, st->ofm.region, st->ofm.base[0], len);
 	if (len == U64_MAX)
@@ -359,8 +534,8 @@ static int calc_sizes_elemwise(struct drm_device *ddev,
 		width = st->ifm.broadcast & 0x2 ? 0 : st->ofm.width;
 		depth = st->ifm.broadcast & 0x4 ? 0 : st->ofm.depth;
 
-		len = feat_matrix_length(edev, info, &st->ifm, width,
-					 height, depth, false);
+		len = feat_matrix_length(edev, info, st, &st->ifm,
+					 FEAT_MATRIX_IFM, width, height, depth, false);
 		dev_dbg(ddev->dev, "op %d: IFM:%d:0x%llx-0x%llx\n",
 			op, st->ifm.region, st->ifm.base[0], len);
 		if (len == U64_MAX)
@@ -372,16 +547,17 @@ static int calc_sizes_elemwise(struct drm_device *ddev,
 		width = st->ifm2.broadcast & 0x2 ? 0 : st->ofm.width;
 		depth = st->ifm2.broadcast & 0x4 ? 0 : st->ofm.depth;
 
-		len = feat_matrix_length(edev, info, &st->ifm2, width,
-					 height, depth, false);
+		len = feat_matrix_length(edev, info, st, &st->ifm2,
+					 FEAT_MATRIX_IFM2, width, height, depth, false);
 		dev_dbg(ddev->dev, "op %d: IFM2:%d:0x%llx-0x%llx\n",
 			op, st->ifm2.region, st->ifm2.base[0], len);
 		if (len == U64_MAX)
 			return -EINVAL;
 	}
 
-	len = feat_matrix_length(edev, info, &st->ofm, st->ofm.width,
-				 st->ofm.height[2], st->ofm.depth, true);
+	len = feat_matrix_length(edev, info, st, &st->ofm, FEAT_MATRIX_OFM,
+				 st->ofm.width, st->ofm.height[2], st->ofm.depth,
+				 true);
 	dev_dbg(ddev->dev, "op %d: OFM:%d:0x%llx-0x%llx\n",
 		op, st->ofm.region, st->ofm.base[0], len);
 	if (len == U64_MAX)
@@ -425,7 +601,7 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 		cmd = cmds[0];
 		param = cmds[0] >> 16;
 
-		if (cmd & 0x4000) {
+		if (cmd & NPU_CMD_CTRL_CMD1) {
 			if (get_user(cmds[1], ucmds++))
 				return -EFAULT;
 
@@ -436,6 +612,8 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 			addr = cmd_to_addr(cmds);
 		}
 
+		cmd_state_set_reg(&st, cmd);
+
 		switch (cmd) {
 		case NPU_OP_STOP:
 			if (i != size / 4 - 1)
@@ -443,8 +621,10 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 			ends_with_stop = true;
 			break;
 		case NPU_OP_DMA_START:
-			srclen = dma_length(info, &st.dma, &st.dma.src);
-			dstlen = dma_length(info, &st.dma, &st.dma.dst);
+			srclen = dma_length(edev, info, &st, &st.dma, &st.dma.src,
+					    NPU_SET_DMA0_SRC_REGION, NPU_SET_DMA0_SRC);
+			dstlen = dma_length(edev, info, &st, &st.dma, &st.dma.dst,
+					    NPU_SET_DMA0_DST_REGION, NPU_SET_DMA0_DST);
 			if (srclen == U64_MAX || dstlen == U64_MAX)
 				return -EINVAL;
 
@@ -455,16 +635,28 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 				st.dma.dst.region, st.dma.dst.offset, dstlen);
 			break;
 		case NPU_OP_CONV:
-		case NPU_OP_DEPTHWISE:
 			use_ifm2 = param & 0x1;  // weights_ifm2
+			if (!cmd_state_reg_is_set(&st, NPU_SET_OFM_PRECISION))
+				return -EINVAL;
 			use_scale = !(st.ofm.precision & 0x100);
 			ret = calc_sizes(ddev, info, cmd, &st, true, use_ifm2,
 					 !use_ifm2, use_scale);
 			if (ret)
 				return ret;
 			break;
+		case NPU_OP_DEPTHWISE:
+			if (!cmd_state_reg_is_set(&st, NPU_SET_OFM_PRECISION))
+				return -EINVAL;
+			use_scale = !(st.ofm.precision & 0x100);
+			ret = calc_sizes(ddev, info, cmd, &st, true, false, true,
+					 use_scale);
+			if (ret)
+				return ret;
+			break;
 		case NPU_OP_POOL:
 			use_ifm = param != 0x4;  // pooling mode
+			if (!cmd_state_reg_is_set(&st, NPU_SET_OFM_PRECISION))
+				return -EINVAL;
 			use_scale = !(st.ofm.precision & 0x100);
 			ret = calc_sizes(ddev, info, cmd, &st, use_ifm, false,
 					 false, use_scale);
@@ -472,11 +664,18 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 				return ret;
 			break;
 		case NPU_OP_ELEMENTWISE:
-			use_scale = ethosu_is_u65(edev) ?
+			if (!ethosu_is_u65(edev) &&
+			    !cmd_state_reg_is_set(&st, NPU_SET_IFM_BROADCAST))
+				return -EINVAL;
+			use_ifm2 = (param != 5) && (param != 6) &&
+				(param != 7) && (param != 0x24);
+			if (use_ifm2 &&
+			    !cmd_state_reg_is_set(&st, NPU_SET_IFM2_BROADCAST))
+				return -EINVAL;
+			use_scale = use_ifm2 && (ethosu_is_u65(edev) ?
 				    (st.ifm2.broadcast & 0x80) :
-				    (st.ifm2.broadcast == 8);
-			use_ifm2 = !(use_scale || (param == 5) ||
-				(param == 6) || (param == 7) || (param == 0x24));
+				    (st.ifm2.broadcast == 8));
+			use_ifm2 = use_ifm2 && !use_scale;
 			use_ifm = st.ifm.broadcast != 8;
 			ret = calc_sizes_elemwise(ddev, info, cmd, &st, use_ifm, use_ifm2);
 			if (ret)

-- 
2.53.0

[PATCH v4 16/31] accel: ethosu: Factor buffer bounds checks

[Rob Herring (Arm)]

Move the repeated command-stream buffer range validation into a
helper in preparation for validating all weight and scale streams.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v2:
 - Adjust due to previous patch
---
 drivers/accel/ethosu/ethosu_gem.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index 75f6901d19e5..24f88696c18b 100644
--- a/drivers/accel/ethosu/ethosu_gem.c
+++ b/drivers/accel/ethosu/ethosu_gem.c
@@ -433,6 +433,25 @@ static u64 feat_matrix_length(struct ethosu_device *edev,
 	return addr;
 }
 
+static int buffer_size(struct ethosu_validated_cmdstream_info *info,
+		       struct cmd_state *st, struct buffer *buf, s8 region,
+		       u16 region_cmd, u16 base_cmd, u16 length_cmd)
+{
+	u64 end;
+
+	if (region < 0 || !cmd_state_reg_is_set(st, region_cmd) ||
+	    !cmd_state_reg_is_set(st, base_cmd) ||
+	    !cmd_state_reg_is_set(st, length_cmd))
+		return -EINVAL;
+
+	if (check_add_overflow(buf->base, (u64)buf->length, &end))
+		return -EINVAL;
+
+	info->region_size[region] = max(info->region_size[region], end);
+
+	return 0;
+}
+
 static int calc_sizes(struct drm_device *ddev,
 		      struct ethosu_validated_cmdstream_info *info,
 		      u16 op, struct cmd_state *st,
@@ -485,26 +504,20 @@ static int calc_sizes(struct drm_device *ddev,
 		dev_dbg(ddev->dev, "op %d: W:%d:0x%llx-0x%llx\n",
 			op, st->weight[0].region, st->weight[0].base,
 			st->weight[0].base + st->weight[0].length - 1);
-		if (!cmd_state_reg_is_set(st, NPU_SET_WEIGHT_REGION) ||
-		    !cmd_state_reg_is_set(st, NPU_SET_WEIGHT_BASE) ||
-		    !cmd_state_reg_is_set(st, NPU_SET_WEIGHT_LENGTH))
+		if (buffer_size(info, st, &st->weight[0], st->weight[0].region,
+				NPU_SET_WEIGHT_REGION, NPU_SET_WEIGHT_BASE,
+				NPU_SET_WEIGHT_LENGTH))
 			return -EINVAL;
-		info->region_size[st->weight[0].region] =
-			max(info->region_size[st->weight[0].region],
-			    st->weight[0].base + st->weight[0].length);
 	}
 
 	if (scale) {
 		dev_dbg(ddev->dev, "op %d: S:%d:0x%llx-0x%llx\n",
 			op, st->scale[0].region, st->scale[0].base,
 			st->scale[0].base + st->scale[0].length - 1);
-		if (!cmd_state_reg_is_set(st, NPU_SET_SCALE_REGION) ||
-		    !cmd_state_reg_is_set(st, NPU_SET_SCALE_BASE) ||
-		    !cmd_state_reg_is_set(st, NPU_SET_SCALE_LENGTH))
+		if (buffer_size(info, st, &st->scale[0], st->scale[0].region,
+				NPU_SET_SCALE_REGION, NPU_SET_SCALE_BASE,
+				NPU_SET_SCALE_LENGTH))
 			return -EINVAL;
-		info->region_size[st->scale[0].region] =
-			max(info->region_size[st->scale[0].region],
-			    st->scale[0].base + st->scale[0].length);
 	}
 
 	len = feat_matrix_length(edev, info, st, &st->ofm, FEAT_MATRIX_OFM,

-- 
2.53.0

[PATCH v4 17/31] accel: ethosu: Fix NHCWB16 bounds calculation

[Rob Herring (Arm)]

The Ethos-U NPU inherently processes NHCWB16 data in 16-channel bricks.
The NHCWB16 address calculation uses the final channel of the last
accessed brick. If the channel depth modulo 16 is less than 15, the
hardware will still read or write the full 16-element brick.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Reported-by: sashiko-bot@kernel.org
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v3:
 - new patch
---
 drivers/accel/ethosu/ethosu_gem.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index 24f88696c18b..67c7cbbd39b8 100644
--- a/drivers/accel/ethosu/ethosu_gem.c
+++ b/drivers/accel/ethosu/ethosu_gem.c
@@ -415,9 +415,10 @@ static u64 feat_matrix_length(struct ethosu_device *edev,
 	case 1: //nhcwb16:
 		element_size = BIT((fm->precision >> (ofm ? 1 : 2)) & 0x3);
 
+		/* NHCWB16 accesses the complete final channel brick. */
 		if (check_mul_overflow(c / 16, (u64)fm->stride_c, &offset) ||
 		    check_add_overflow(addr, offset, &addr) ||
-		    check_mul_overflow(16 * x + (c & 0xf), element_size, &offset) ||
+		    check_mul_overflow(16 * x + 15, element_size, &offset) ||
 		    check_add_overflow(addr, offset, &addr))
 			return U64_MAX;
 		break;

-- 
2.53.0

[PATCH v4 18/31] accel: ethosu: Validate secondary streams

[Rob Herring (Arm)]

The command-stream validator records the additional U65 scale and
weight stream addresses and the U85 weight decoder addresses, but only
checked stream 0 against its region buffer.

Check every configured secondary stream against the matching weight or
scale region before accepting a kernel operation.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v2:
 - Adjust for register state tracking
---
 drivers/accel/ethosu/ethosu_gem.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index 67c7cbbd39b8..af8a0c45039b 100644
--- a/drivers/accel/ethosu/ethosu_gem.c
+++ b/drivers/accel/ethosu/ethosu_gem.c
@@ -436,13 +436,17 @@ static u64 feat_matrix_length(struct ethosu_device *edev,
 
 static int buffer_size(struct ethosu_validated_cmdstream_info *info,
 		       struct cmd_state *st, struct buffer *buf, s8 region,
-		       u16 region_cmd, u16 base_cmd, u16 length_cmd)
+		       u16 region_cmd, u16 base_cmd, u16 length_cmd, bool optional)
 {
 	u64 end;
+	bool base_set = cmd_state_reg_is_set(st, base_cmd);
+	bool length_set = cmd_state_reg_is_set(st, length_cmd);
+
+	if (optional && !base_set && !length_set)
+		return 0;
 
 	if (region < 0 || !cmd_state_reg_is_set(st, region_cmd) ||
-	    !cmd_state_reg_is_set(st, base_cmd) ||
-	    !cmd_state_reg_is_set(st, length_cmd))
+	    !base_set || !length_set)
 		return -EINVAL;
 
 	if (check_add_overflow(buf->base, (u64)buf->length, &end))
@@ -507,7 +511,20 @@ static int calc_sizes(struct drm_device *ddev,
 			st->weight[0].base + st->weight[0].length - 1);
 		if (buffer_size(info, st, &st->weight[0], st->weight[0].region,
 				NPU_SET_WEIGHT_REGION, NPU_SET_WEIGHT_BASE,
-				NPU_SET_WEIGHT_LENGTH))
+				NPU_SET_WEIGHT_LENGTH, false))
+			return -EINVAL;
+
+		if (buffer_size(info, st, &st->weight[1], st->weight[0].region,
+				NPU_SET_WEIGHT_REGION, NPU_SET_WEIGHT1_BASE,
+				NPU_SET_WEIGHT1_LENGTH, true) ||
+		    buffer_size(info, st, &st->weight[3], st->weight[0].region,
+				NPU_SET_WEIGHT_REGION, NPU_SET_WEIGHT3_BASE,
+				NPU_SET_WEIGHT3_LENGTH, true))
+			return -EINVAL;
+		if (!ethosu_is_u65(edev) &&
+		    buffer_size(info, st, &st->weight[2], st->weight[0].region,
+				NPU_SET_WEIGHT_REGION, NPU_SET_WEIGHT2_BASE,
+				NPU_SET_WEIGHT2_LENGTH, true))
 			return -EINVAL;
 	}
 
@@ -517,7 +534,13 @@ static int calc_sizes(struct drm_device *ddev,
 			st->scale[0].base + st->scale[0].length - 1);
 		if (buffer_size(info, st, &st->scale[0], st->scale[0].region,
 				NPU_SET_SCALE_REGION, NPU_SET_SCALE_BASE,
-				NPU_SET_SCALE_LENGTH))
+				NPU_SET_SCALE_LENGTH, false))
+			return -EINVAL;
+
+		if (ethosu_is_u65(edev) &&
+		    buffer_size(info, st, &st->scale[1], st->scale[0].region,
+				NPU_SET_SCALE_REGION, NPU_SET_SCALE1_BASE,
+				NPU_SET_SCALE1_LENGTH, true))
 			return -EINVAL;
 	}
 

-- 
2.53.0

[PATCH v4 19/31] accel: ethosu: Reject unsupported commands and params

[Rob Herring (Arm)]

The command-stream validator does not model U85 branches, indexed DMA,
or OFM transposes. A branch can bypass the linear validation state,
indexed DMA accesses an unchecked index buffer, and a transpose changes
the feature-map address calculation.

Reject any unhandled operation commands or out of range registers, as
well as the reserved DMA stride mode and feature-map formats. Reject
command-stream IRQs because they can signal job completion before later
commands finish.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v4:
 - Combine "accel: ethosu: Reject unsupported commands" and "accel:
   ethosu: Reject reserved command encodings"
 - Move NPU_CMD_CTRL_CMD1 to "accel: ethosu: Track command stream register setup"
 - Rework checks for unknown op cmds and reserved bits

v2:
 - no changes
---
 drivers/accel/ethosu/ethosu_device.h |  4 ++++
 drivers/accel/ethosu/ethosu_gem.c    | 33 ++++++++++++++++++++++++++++++++-
 2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/accel/ethosu/ethosu_device.h b/drivers/accel/ethosu/ethosu_device.h
index 8405e69acb0c..86412c152f4e 100644
--- a/drivers/accel/ethosu/ethosu_device.h
+++ b/drivers/accel/ethosu/ethosu_device.h
@@ -88,6 +88,8 @@ struct gen_pool;
 #define PMU_EV_TYPE_CYCLES	0x11
 #define PMU_EV_TYPE_IDLE	0x20
 
+#define NPU_DMA_REGION_INDEX_MODE	BIT(11)
+
 #define NPU_CMD_CTRL_CMD1		BIT(14)
 
 enum ethosu_cmds {
@@ -98,6 +100,8 @@ enum ethosu_cmds {
 	NPU_OP_ELEMENTWISE = 0x6,
 	NPU_OP_RESIZE = 0x7,	// U85 only
 	NPU_OP_DMA_START = 0x10,
+	NPU_OP_DMA_WAIT = 0x11,
+	NPU_OP_KERNEL_WAIT = 0x12,
 	NPU_SET_IFM_PAD_TOP = 0x100,
 	NPU_SET_IFM_PAD_LEFT = 0x101,
 	NPU_SET_IFM_PAD_RIGHT = 0x102,
diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index af8a0c45039b..74ef86848a2d 100644
--- a/drivers/accel/ethosu/ethosu_gem.c
+++ b/drivers/accel/ethosu/ethosu_gem.c
@@ -190,6 +190,16 @@ static bool cmd_state_reg_is_set(struct cmd_state *st, u16 cmd)
 	return reg < NPU_CMD0_REGS && test_bit(reg, st->cmd0);
 }
 
+static bool cmd_is_register(u16 cmd)
+{
+	u16 reg = cmd & ~NPU_CMD_CTRL_CMD1;
+
+	if (cmd & NPU_CMD_CTRL_CMD1)
+		return reg < NPU_CMD1_REGS;
+
+	return cmd >= 0x100 && cmd < NPU_CMD0_REGS;
+}
+
 static u64 cmd_to_addr(u32 *cmd)
 {
 	return (((u64)cmd[0] & 0xff0000) << 16) | cmd[1];
@@ -652,6 +662,9 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 		cmd_state_set_reg(&st, cmd);
 
 		switch (cmd) {
+		case NPU_OP_DMA_WAIT:
+		case NPU_OP_KERNEL_WAIT:
+			break;
 		case NPU_OP_STOP:
 			if (i != size / 4 - 1)
 				return -EINVAL;
@@ -745,6 +758,8 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 			st.ifm.depth = param;
 			break;
 		case NPU_SET_IFM_PRECISION:
+			if (((param >> 6) & 0x3) > 1)
+				return -EINVAL;
 			st.ifm.precision = param;
 			break;
 		case NPU_SET_IFM_BROADCAST:
@@ -788,6 +803,10 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 			st.ofm.depth = param;
 			break;
 		case NPU_SET_OFM_PRECISION:
+			if (((param >> 6) & 0x3) > 1)
+				return -EINVAL;
+			if (!ethosu_is_u65(edev) && (param & GENMASK(13, 11)))
+				return -EINVAL;
 			st.ofm.precision = param;
 			break;
 		case NPU_SET_OFM_REGION:
@@ -822,6 +841,8 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 			st.ifm2.broadcast = param;
 			break;
 		case NPU_SET_IFM2_PRECISION:
+			if (((param >> 6) & 0x3) > 1)
+				return -EINVAL;
 			st.ifm2.precision = param;
 			break;
 		case NPU_SET_IFM2_REGION:
@@ -896,18 +917,26 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 			break;
 
 		case NPU_SET_DMA0_SRC_REGION:
+			if (param & NPU_DMA_REGION_INDEX_MODE)
+				return -EINVAL;
 			if (param & 0x100)
 				st.dma.src.region = -1;
 			else
 				st.dma.src.region = param & 0x7;
 			st.dma.src.mode = (param >> 9) & 0x3;
+			if (st.dma.src.mode == 3)
+				return -EINVAL;
 			break;
 		case NPU_SET_DMA0_DST_REGION:
+			if (param & NPU_DMA_REGION_INDEX_MODE)
+				return -EINVAL;
 			if (param & 0x100)
 				st.dma.dst.region = -1;
 			else
 				st.dma.dst.region = param & 0x7;
 			st.dma.dst.mode = (param >> 9) & 0x3;
+			if (st.dma.dst.mode == 3)
+				return -EINVAL;
 			break;
 		case NPU_SET_DMA0_SIZE0:
 			st.dma.size0 = param;
@@ -937,7 +966,9 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 			st.dma.src.len = st.dma.dst.len = addr;
 			break;
 		default:
-			break;
+			if (cmd_is_register(cmd))
+				break;
+			return -EINVAL;
 		}
 	}
 

-- 
2.53.0

[PATCH v4 20/31] accel: ethosu: Validate all feature map tiles

[Rob Herring (Arm)]

The command-stream validator checked only the final feature-map
coordinate. For tiled tensors, this can leave an earlier tile base
address unchecked even though the operation accesses it.

Check the final coordinate of every tile touched by an operation. Also
treat U65 feature maps as 2x2 tiled: its precision rounding bits are not
the U85 storage encoding.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v4:
 - NHCWB16 feature maps may be vertically tiled. Account for tile 0's
   maximum Y coordinate when calculating the feature map bounds.

v3:
 - Fix for storage mode 0 intermediate tile calculations

v2:
 - no changes
---
 drivers/accel/ethosu/ethosu_gem.c | 147 ++++++++++++++++++++++++++++++--------
 1 file changed, 119 insertions(+), 28 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index 74ef86848a2d..3333477bf2e5 100644
--- a/drivers/accel/ethosu/ethosu_gem.c
+++ b/drivers/accel/ethosu/ethosu_gem.c
@@ -444,6 +444,96 @@ static u64 feat_matrix_length(struct ethosu_device *edev,
 	return addr;
 }
 
+static int feat_matrix_check_location(struct ethosu_device *edev,
+				      struct ethosu_validated_cmdstream_info *info,
+				      struct cmd_state *st, struct feat_matrix *fm,
+				      enum feat_matrix_type type, u32 x, u32 y,
+				      u32 c, bool ofm, u64 *max_len)
+{
+	u64 len;
+
+	len = feat_matrix_length(edev, info, st, fm, type, x, y, c, ofm);
+	if (len == U64_MAX)
+		return -EINVAL;
+
+	*max_len = max(*max_len, len);
+	return 0;
+}
+
+static int feat_matrix_size(struct ethosu_device *edev,
+			    struct ethosu_validated_cmdstream_info *info,
+			    struct cmd_state *st, struct feat_matrix *fm,
+			    enum feat_matrix_type type,
+			    u32 x, u32 y, u32 c, bool ofm, u64 *max_len)
+{
+	u32 storage = ethosu_is_u65(edev) ? 0 : fm->precision >> 14;
+	int ret;
+
+	*max_len = 0;
+
+	if (storage == 0) {
+		ret = feat_matrix_check_location(edev, info, st, fm, type, 0, 0,
+						 c, ofm, max_len);
+		if (ret)
+			return ret;
+
+		ret = feat_matrix_check_location(edev, info, st, fm, type,
+						 min(x, (u32)fm->width0),
+						 min(y, (u32)fm->height[0]), c, ofm,
+						 max_len);
+		if (ret)
+			return ret;
+
+		if (fm->width0 < x) {
+			ret = feat_matrix_check_location(edev, info, st, fm, type, x,
+							 min(y, (u32)fm->height[1]), c,
+							 ofm, max_len);
+			if (ret)
+				return ret;
+		}
+
+		if (fm->height[0] < y) {
+			ret = feat_matrix_check_location(edev, info, st, fm, type,
+							 min(x, (u32)fm->width0), y, c,
+							 ofm, max_len);
+			if (ret)
+				return ret;
+		}
+
+		if (fm->width0 < x && fm->height[1] < y)
+			return feat_matrix_check_location(edev, info, st, fm, type,
+							  x, y, c, ofm, max_len);
+
+		return 0;
+	}
+
+	if (storage == 1) {
+		ret = feat_matrix_check_location(edev, info, st, fm, type, x,
+						 min3(y, (u32)fm->height[0],
+						      (u32)fm->height[1]), c, ofm,
+						 max_len);
+		if (ret)
+			return ret;
+		if (fm->height[0] < fm->height[1] && fm->height[1] <= y) {
+			ret = feat_matrix_check_location(edev, info, st, fm, type, x,
+							 fm->height[1], c, ofm,
+							 max_len);
+			if (ret)
+				return ret;
+		}
+		if (fm->height[1] < y) {
+			ret = feat_matrix_check_location(edev, info, st, fm, type, x,
+							 fm->height[1] + 1, c, ofm,
+							 max_len);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return feat_matrix_check_location(edev, info, st, fm, type, x, y, c, ofm,
+					  max_len);
+}
+
 static int buffer_size(struct ethosu_validated_cmdstream_info *info,
 		       struct cmd_state *st, struct buffer *buf, s8 region,
 		       u16 region_cmd, u16 base_cmd, u16 length_cmd, bool optional)
@@ -474,6 +564,7 @@ static int calc_sizes(struct drm_device *ddev,
 {
 	struct ethosu_device *edev = to_ethosu_device(ddev);
 	u64 len;
+	int ret;
 
 	if (ifm) {
 		if (!cmd_state_reg_is_set(st, NPU_SET_KERNEL_WIDTH_M1) ||
@@ -496,23 +587,22 @@ static int calc_sizes(struct drm_device *ddev,
 		if (ifm_height < 0 || ifm_width < 0)
 			return -EINVAL;
 
-		len = feat_matrix_length(edev, info, st, &st->ifm,
-					 FEAT_MATRIX_IFM, ifm_width, ifm_height,
-					 st->ifm.depth, false);
+		ret = feat_matrix_size(edev, info, st, &st->ifm, FEAT_MATRIX_IFM,
+				       ifm_width, ifm_height, st->ifm.depth, false,
+				       &len);
 		dev_dbg(ddev->dev, "op %d: IFM:%d:0x%llx-0x%llx\n",
 			op, st->ifm.region, st->ifm.base[0], len);
-		if (len == U64_MAX)
-			return -EINVAL;
+		if (ret)
+			return ret;
 	}
 
 	if (ifm2) {
-		len = feat_matrix_length(edev, info, st, &st->ifm2,
-					 FEAT_MATRIX_IFM2, st->ifm.depth, 0,
-					 st->ofm.depth, false);
+		ret = feat_matrix_size(edev, info, st, &st->ifm2, FEAT_MATRIX_IFM2,
+				       st->ifm.depth, 0, st->ofm.depth, false, &len);
 		dev_dbg(ddev->dev, "op %d: IFM2:%d:0x%llx-0x%llx\n",
 			op, st->ifm2.region, st->ifm2.base[0], len);
-		if (len == U64_MAX)
-			return -EINVAL;
+		if (ret)
+			return ret;
 	}
 
 	if (weight) {
@@ -554,13 +644,13 @@ static int calc_sizes(struct drm_device *ddev,
 			return -EINVAL;
 	}
 
-	len = feat_matrix_length(edev, info, st, &st->ofm, FEAT_MATRIX_OFM,
-				 st->ofm.width, st->ofm.height[2], st->ofm.depth,
-				 true);
+	ret = feat_matrix_size(edev, info, st, &st->ofm, FEAT_MATRIX_OFM,
+			       st->ofm.width, st->ofm.height[2], st->ofm.depth,
+			       true, &len);
 	dev_dbg(ddev->dev, "op %d: OFM:%d:0x%llx-0x%llx\n",
 		op, st->ofm.region, st->ofm.base[0], len);
-	if (len == U64_MAX)
-		return -EINVAL;
+	if (ret)
+		return ret;
 	if (!feat_matrix_chained(edev, &st->ofm))
 		info->output_region[st->ofm.region] = true;
 
@@ -575,18 +665,19 @@ static int calc_sizes_elemwise(struct drm_device *ddev,
 	struct ethosu_device *edev = to_ethosu_device(ddev);
 	u32 height, width, depth;
 	u64 len;
+	int ret;
 
 	if (ifm) {
 		height = st->ifm.broadcast & 0x1 ? 0 : st->ofm.height[2];
 		width = st->ifm.broadcast & 0x2 ? 0 : st->ofm.width;
 		depth = st->ifm.broadcast & 0x4 ? 0 : st->ofm.depth;
 
-		len = feat_matrix_length(edev, info, st, &st->ifm,
-					 FEAT_MATRIX_IFM, width, height, depth, false);
+		ret = feat_matrix_size(edev, info, st, &st->ifm, FEAT_MATRIX_IFM,
+				       width, height, depth, false, &len);
 		dev_dbg(ddev->dev, "op %d: IFM:%d:0x%llx-0x%llx\n",
 			op, st->ifm.region, st->ifm.base[0], len);
-		if (len == U64_MAX)
-			return -EINVAL;
+		if (ret)
+			return ret;
 	}
 
 	if (ifm2) {
@@ -594,21 +685,21 @@ static int calc_sizes_elemwise(struct drm_device *ddev,
 		width = st->ifm2.broadcast & 0x2 ? 0 : st->ofm.width;
 		depth = st->ifm2.broadcast & 0x4 ? 0 : st->ofm.depth;
 
-		len = feat_matrix_length(edev, info, st, &st->ifm2,
-					 FEAT_MATRIX_IFM2, width, height, depth, false);
+		ret = feat_matrix_size(edev, info, st, &st->ifm2, FEAT_MATRIX_IFM2,
+				       width, height, depth, false, &len);
 		dev_dbg(ddev->dev, "op %d: IFM2:%d:0x%llx-0x%llx\n",
 			op, st->ifm2.region, st->ifm2.base[0], len);
-		if (len == U64_MAX)
-			return -EINVAL;
+		if (ret)
+			return ret;
 	}
 
-	len = feat_matrix_length(edev, info, st, &st->ofm, FEAT_MATRIX_OFM,
-				 st->ofm.width, st->ofm.height[2], st->ofm.depth,
-				 true);
+	ret = feat_matrix_size(edev, info, st, &st->ofm, FEAT_MATRIX_OFM,
+			       st->ofm.width, st->ofm.height[2], st->ofm.depth,
+			       true, &len);
 	dev_dbg(ddev->dev, "op %d: OFM:%d:0x%llx-0x%llx\n",
 		op, st->ofm.region, st->ofm.base[0], len);
-	if (len == U64_MAX)
-		return -EINVAL;
+	if (ret)
+		return ret;
 	if (!feat_matrix_chained(edev, &st->ofm))
 		info->output_region[st->ofm.region] = true;
 

-- 
2.53.0

[PATCH v4 21/31] accel: ethosu: Account for feature map element size

[Rob Herring (Arm)]

The feature map bounds calculation currently accounts for only one byte
of the final element. This can leave the tail of B16, B32, and B64
elements outside the validated buffer.

Use the feature map element size when calculating the final byte.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v2:
 - new patch
---
 drivers/accel/ethosu/ethosu_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index 3333477bf2e5..bf1489734769 100644
--- a/drivers/accel/ethosu/ethosu_gem.c
+++ b/drivers/accel/ethosu/ethosu_gem.c
@@ -436,7 +436,7 @@ static u64 feat_matrix_length(struct ethosu_device *edev,
 		return U64_MAX;
 	}
 
-	if (check_add_overflow(addr, 1ULL, &offset))
+	if (check_add_overflow(addr, (u64)element_size, &offset))
 		return U64_MAX;
 
 	info->region_size[fm->region] = max(info->region_size[fm->region], offset);

-- 
2.53.0

[PATCH v4 22/31] accel: ethosu: Validate convolution parameter

[Rob Herring (Arm)]

The U65 NPU_OP_CONV command has no parameter fields, but the
validator interpreted bit zero as the U85 weights_ifm2 field. A crafted
U65 stream could consequently make validation skip the weight buffer
that hardware accesses.

Require a zero parameter on U65 and reject the reserved U85 parameter
bits.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v2:
 - new patch
---
 drivers/accel/ethosu/ethosu_device.h | 1 +
 drivers/accel/ethosu/ethosu_gem.c    | 5 ++++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/accel/ethosu/ethosu_device.h b/drivers/accel/ethosu/ethosu_device.h
index 86412c152f4e..61dec8ec23ea 100644
--- a/drivers/accel/ethosu/ethosu_device.h
+++ b/drivers/accel/ethosu/ethosu_device.h
@@ -89,6 +89,7 @@ struct gen_pool;
 #define PMU_EV_TYPE_IDLE	0x20
 
 #define NPU_DMA_REGION_INDEX_MODE	BIT(11)
+#define NPU_OP_CONV_WEIGHTS_IFM2	BIT(0)
 
 #define NPU_CMD_CTRL_CMD1		BIT(14)
 
diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index bf1489734769..143021a3f9ed 100644
--- a/drivers/accel/ethosu/ethosu_gem.c
+++ b/drivers/accel/ethosu/ethosu_gem.c
@@ -776,7 +776,10 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 				st.dma.dst.region, st.dma.dst.offset, dstlen);
 			break;
 		case NPU_OP_CONV:
-			use_ifm2 = param & 0x1;  // weights_ifm2
+			if ((ethosu_is_u65(edev) && param) || (param & ~NPU_OP_CONV_WEIGHTS_IFM2))
+				return -EINVAL;
+
+			use_ifm2 = param & NPU_OP_CONV_WEIGHTS_IFM2;
 			if (!cmd_state_reg_is_set(&st, NPU_SET_OFM_PRECISION))
 				return -EINVAL;
 			use_scale = !(st.ofm.precision & 0x100);

-- 
2.53.0

[PATCH v4 23/31] accel: ethosu: Account for kernel dilation in IFM size

[Rob Herring (Arm)]

Kernel dilation increases the input feature-map area accessed by a
kernel operation. Include the x and y dilation settings when
calculating the required IFM bounds.

Without this, a command stream using x2 dilation can access IFM
memory past the range validated by the driver.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v2:
 - new patch
---
 drivers/accel/ethosu/ethosu_device.h |  2 ++
 drivers/accel/ethosu/ethosu_gem.c    | 10 ++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_device.h b/drivers/accel/ethosu/ethosu_device.h
index 61dec8ec23ea..80cba318f773 100644
--- a/drivers/accel/ethosu/ethosu_device.h
+++ b/drivers/accel/ethosu/ethosu_device.h
@@ -90,6 +90,8 @@ struct gen_pool;
 
 #define NPU_DMA_REGION_INDEX_MODE	BIT(11)
 #define NPU_OP_CONV_WEIGHTS_IFM2	BIT(0)
+#define NPU_KERNEL_DILATION_X		BIT(3)
+#define NPU_KERNEL_DILATION_Y		BIT(4)
 
 #define NPU_CMD_CTRL_CMD1		BIT(14)
 
diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index 143021a3f9ed..15a4cab1b286 100644
--- a/drivers/accel/ethosu/ethosu_gem.c
+++ b/drivers/accel/ethosu/ethosu_gem.c
@@ -579,10 +579,16 @@ static int calc_sizes(struct drm_device *ddev,
 			((st->ifm.stride_kernel >> 1) & 0x1) + 1;
 		u32 stride_x = ((st->ifm.stride_kernel >> 5) & 0x2) +
 			(st->ifm.stride_kernel & 0x1) + 1;
+		u32 dilation_y = 1 + !!(st->ifm.stride_kernel &
+					 NPU_KERNEL_DILATION_Y);
+		u32 dilation_x = 1 + !!(st->ifm.stride_kernel &
+					 NPU_KERNEL_DILATION_X);
 		s32 ifm_height = st->ofm.height[2] * stride_y +
-			st->ifm.height[2] - (st->ifm.pad_top + st->ifm.pad_bottom);
+			st->ifm.height[2] * dilation_y -
+			(st->ifm.pad_top + st->ifm.pad_bottom);
 		s32 ifm_width = st->ofm.width * stride_x +
-			st->ifm.width - (st->ifm.pad_left + st->ifm.pad_right);
+			st->ifm.width * dilation_x -
+			(st->ifm.pad_left + st->ifm.pad_right);
 
 		if (ifm_height < 0 || ifm_width < 0)
 			return -EINVAL;

-- 
2.53.0

[PATCH v4 24/31] accel: ethosu: Validate accumulator input

[Rob Herring (Arm)]

The U85 ACC_FORMAT command can select IFM2 as the accumulator
input. This is used by null-pool operations and can also be used
by convolution. Track this selection and validate the IFM2 feature
map against the OFM extent before submitting the operation.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v4:
 - Accumulator input should not be checked for elementwise ops

v2:
 - new patch
---
 drivers/accel/ethosu/ethosu_device.h |  4 ++++
 drivers/accel/ethosu/ethosu_gem.c    | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/drivers/accel/ethosu/ethosu_device.h b/drivers/accel/ethosu/ethosu_device.h
index 80cba318f773..d3df6720085d 100644
--- a/drivers/accel/ethosu/ethosu_device.h
+++ b/drivers/accel/ethosu/ethosu_device.h
@@ -127,6 +127,7 @@ enum ethosu_cmds {
 	NPU_SET_KERNEL_WIDTH_M1 = 0x120,
 	NPU_SET_KERNEL_HEIGHT_M1 = 0x121,
 	NPU_SET_KERNEL_STRIDE = 0x122,
+	NPU_SET_ACC_FORMAT = 0x124,
 	NPU_SET_WEIGHT_REGION = 0x128,
 	NPU_SET_SCALE_REGION = 0x129,
 	NPU_SET_DMA0_SRC_REGION = 0x130,
@@ -181,6 +182,9 @@ enum ethosu_cmds {
 	NPU_SET_WEIGHT3_LENGTH = 0x4095,
 };
 
+#define NPU_ACC_FORMAT_INPUT_MASK	GENMASK(5, 4)
+#define NPU_ACC_INPUT_IFM2		2
+
 #define ETHOSU_SRAM_REGION	2	/* Matching Vela compiler */
 
 struct ethosu_perfmon;
diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index 15a4cab1b286..406e1c07246d 100644
--- a/drivers/accel/ethosu/ethosu_gem.c
+++ b/drivers/accel/ethosu/ethosu_gem.c
@@ -155,6 +155,7 @@ struct feat_matrix {
 struct cmd_state {
 	DECLARE_BITMAP(cmd0, NPU_CMD0_REGS);
 	DECLARE_BITMAP(cmd1, NPU_CMD1_REGS);
+	bool acc_input_ifm2;
 	struct dma_state dma;
 	struct buffer scale[2];
 	struct buffer weight[4];
@@ -534,6 +535,28 @@ static int feat_matrix_size(struct ethosu_device *edev,
 					  max_len);
 }
 
+static int
+calc_acc_input_size(struct drm_device *ddev,
+		    struct ethosu_validated_cmdstream_info *info,
+		    struct cmd_state *st)
+{
+	struct ethosu_device *edev = to_ethosu_device(ddev);
+	u64 len;
+	int ret;
+
+	if (!st->acc_input_ifm2)
+		return 0;
+
+	/* The accumulator has one input value for each OFM element. */
+	ret = feat_matrix_size(edev, info, st, &st->ifm2,
+			       FEAT_MATRIX_IFM2, st->ofm.width,
+			       st->ofm.height[2], st->ofm.depth, false, &len);
+	dev_dbg(ddev->dev, "ACC IFM2:%d:0x%llx-0x%llx\n",
+		st->ifm2.region, st->ifm2.base[0], len);
+
+	return ret;
+}
+
 static int buffer_size(struct ethosu_validated_cmdstream_info *info,
 		       struct cmd_state *st, struct buffer *buf, s8 region,
 		       u16 region_cmd, u16 base_cmd, u16 length_cmd, bool optional)
@@ -655,6 +678,9 @@ static int calc_sizes(struct drm_device *ddev,
 			       true, &len);
 	dev_dbg(ddev->dev, "op %d: OFM:%d:0x%llx-0x%llx\n",
 		op, st->ofm.region, st->ofm.base[0], len);
+	if (ret)
+		return ret;
+	ret = calc_acc_input_size(ddev, info, st);
 	if (ret)
 		return ret;
 	if (!feat_matrix_chained(edev, &st->ofm))
@@ -842,6 +868,15 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 		case NPU_SET_KERNEL_STRIDE:
 			st.ifm.stride_kernel = param;
 			break;
+		case NPU_SET_ACC_FORMAT:
+			if (!ethosu_is_u65(edev)) {
+				u32 acc_input = FIELD_GET(NPU_ACC_FORMAT_INPUT_MASK, param);
+
+				if (acc_input > NPU_ACC_INPUT_IFM2)
+					return -EINVAL;
+				st.acc_input_ifm2 = acc_input == NPU_ACC_INPUT_IFM2;
+			}
+			break;
 		case NPU_SET_IFM_PAD_TOP:
 			st.ifm.pad_top = param & 0x7f;
 			break;

-- 
2.53.0

[PATCH v4 25/31] accel: ethosu: Restrict dynamic IFM2 weights

[Rob Herring (Arm)]

The validator bounds dynamic IFM2 weights as a 1x1 weight matrix, but
did not enforce the corresponding kernel shape. A larger crafted kernel
could make the NPU access beyond the validated IFM2 feature map.

Reject dynamic-weight convolutions whose kernel is not 1x1.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v2:
 - new patch
---
 drivers/accel/ethosu/ethosu_gem.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index 406e1c07246d..117046e5a61f 100644
--- a/drivers/accel/ethosu/ethosu_gem.c
+++ b/drivers/accel/ethosu/ethosu_gem.c
@@ -598,6 +598,9 @@ static int calc_sizes(struct drm_device *ddev,
 		    !cmd_state_reg_is_set(st, NPU_SET_IFM_PAD_RIGHT) ||
 		    !cmd_state_reg_is_set(st, NPU_SET_IFM_PAD_BOTTOM))
 			return -EINVAL;
+		/* Dynamic IFM2 weights are only supported for 1x1 convolutions. */
+		if (ifm2 && (st->ifm.width || st->ifm.height[2]))
+			return -EINVAL;
 		u32 stride_y = ((st->ifm.stride_kernel >> 8) & 0x2) +
 			((st->ifm.stride_kernel >> 1) & 0x1) + 1;
 		u32 stride_x = ((st->ifm.stride_kernel >> 5) & 0x2) +

-- 
2.53.0

[PATCH v4 26/31] accel: ethosu: Split U65 and U85 DMA length validation

[Rob Herring (Arm)]

Ethos-U65 and Ethos-U85 have slightly different DMA programming models.
The U65 has skip values added on to the size whereas U85 has signed
stride values. The U65 shares the skip values for source and destination
whereas the U85 has independent settings for source and destination.

The current validation only correctly handles U65 constraints and only
for source 2D/3D mode. Split the shared DMA length calculation into U65
and U85 specific versions adding U65 destination 2D/3D mode and U85
constraints.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v4:
 - Only account for a skip/stride between transfers and not at the end of
   last transfer

v3:
 - Fix U65 destination stride/skip handling

v2:
 - new patch
---
 drivers/accel/ethosu/ethosu_gem.c | 119 ++++++++++++++++++++++++++++++--------
 1 file changed, 96 insertions(+), 23 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index 117046e5a61f..04c0de8336cd 100644
--- a/drivers/accel/ethosu/ethosu_gem.c
+++ b/drivers/accel/ethosu/ethosu_gem.c
@@ -245,40 +245,97 @@ static bool dma_params_valid(struct ethosu_device *edev, struct cmd_state *st,
 	return true;
 }
 
-static u64 dma_length(struct ethosu_device *edev,
-		      struct ethosu_validated_cmdstream_info *info,
-		      struct cmd_state *st, struct dma_state *dma_st,
-		      struct dma *dma, u16 region_cmd, u16 addr_cmd)
+static u64 dma_length_finish(struct ethosu_validated_cmdstream_info *info,
+			     const struct dma *dma, u64 len)
+{
+	if (dma->region >= 0) {
+		u64 end;
+
+		if (check_add_overflow(len, dma->offset, &end))
+			return U64_MAX;
+		info->region_size[dma->region] =
+			max(info->region_size[dma->region], end);
+	}
+
+	return len;
+}
+
+static u64 dma_length_u65(struct ethosu_validated_cmdstream_info *info,
+			  struct dma_state *dma_st,
+			  struct dma *dma)
 {
 	s8 mode = dma->mode;
 	u64 len = dma->len;
+	u64 stride;
 
-	if (!dma_params_valid(edev, st, dma_st, dma, region_cmd, addr_cmd))
-		return U64_MAX;
+	if (mode >= 1) {
+		if (check_mul_overflow(len, (u64)dma_st->size0, &len) ||
+		    check_mul_overflow((u64)dma->stride[0],
+				       (u64)dma_st->size0 - 1, &stride) ||
+		    check_add_overflow(len, stride, &len))
+			return U64_MAX;
+	}
+	if (mode == 2) {
+		if (check_mul_overflow(len, (u64)dma_st->size1, &len) ||
+		    check_mul_overflow((u64)dma->stride[1],
+				       (u64)dma_st->size1 - 1, &stride) ||
+		    check_add_overflow(len, stride, &len))
+			return U64_MAX;
+	}
+
+	return dma_length_finish(info, dma, len);
+}
+
+static u64 dma_length_u85(struct ethosu_validated_cmdstream_info *info,
+			  struct dma_state *dma_st,
+			  struct dma *dma)
+{
+	s8 mode = dma->mode;
+	s64 min = 0;
+	u64 max = dma->len;
+	s64 stride;
 
 	if (mode >= 1) {
-		if (dma->stride[0] < 0 && (u64)(-dma->stride[0]) > len)
+		if (check_mul_overflow(dma->stride[0],
+				       (s64)dma_st->size0 - 1, &stride))
 			return U64_MAX;
-		len += dma->stride[0];
-		if (check_mul_overflow(len, (u64)dma_st->size0, &len))
+		if (stride < 0) {
+			if (check_add_overflow(min, stride, &min))
+				return U64_MAX;
+		} else if (check_add_overflow(max, (u64)stride, &max)) {
 			return U64_MAX;
+		}
 	}
 	if (mode == 2) {
-		if (dma->stride[1] < 0 && (u64)(-dma->stride[1]) > len)
+		if (check_mul_overflow(dma->stride[1],
+				       (s64)dma_st->size1 - 1, &stride))
 			return U64_MAX;
-		len += dma->stride[1];
-		if (check_mul_overflow(len, (u64)dma_st->size1, &len))
+		if (stride < 0) {
+			if (check_add_overflow(min, stride, &min))
+				return U64_MAX;
+		} else if (check_add_overflow(max, (u64)stride, &max)) {
 			return U64_MAX;
+		}
 	}
-	if (dma->region >= 0) {
-		u64 end;
 
-		if (check_add_overflow(len, dma->offset, &end))
-			return U64_MAX;
-		info->region_size[dma->region] = max(info->region_size[dma->region], end);
-	}
+	if (min < 0 && -(u64)min > dma->offset)
+		return U64_MAX;
 
-	return len;
+	return dma_length_finish(info, dma, max);
+}
+
+static u64 dma_length(struct ethosu_device *edev,
+		      struct ethosu_validated_cmdstream_info *info,
+		      struct cmd_state *st, struct dma_state *dma_st,
+		      struct dma *dma, u16 region_cmd, u16 addr_cmd)
+{
+	if (!dma_params_valid(edev, st, dma_st, dma, region_cmd, addr_cmd))
+		return U64_MAX;
+
+	if (ethosu_is_u65(edev))
+		return dma_length_u65(info, dma_st, dma);
+
+	return dma_length_u85(info, dma_st, dma);
 }
 
 static bool feat_matrix_chained(struct ethosu_device *edev, struct feat_matrix *fm)
@@ -1083,16 +1140,32 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 			st.dma.size1 = param;
 			break;
 		case NPU_SET_DMA0_SRC_STRIDE0:
-			st.dma.src.stride[0] = ((s64)addr << 24) >> 24;
+			if (ethosu_is_u65(edev)) {
+				st.dma.src.stride[0] = addr;
+				st.dma.dst.stride[0] = addr;
+			} else {
+				st.dma.src.stride[0] = sign_extend64(addr, 39);
+			}
 			break;
 		case NPU_SET_DMA0_SRC_STRIDE1:
-			st.dma.src.stride[1] = ((s64)addr << 24) >> 24;
+			if (ethosu_is_u65(edev)) {
+				st.dma.src.stride[1] = addr;
+				st.dma.dst.stride[1] = addr;
+			} else {
+				st.dma.src.stride[1] = sign_extend64(addr, 39);
+			}
 			break;
 		case NPU_SET_DMA0_DST_STRIDE0:
-			st.dma.dst.stride[0] = ((s64)addr << 24) >> 24;
+			if (!ethosu_is_u65(edev))
+				st.dma.dst.stride[0] = sign_extend64(addr, 39);
+			else
+				return -EINVAL;
 			break;
 		case NPU_SET_DMA0_DST_STRIDE1:
-			st.dma.dst.stride[1] = ((s64)addr << 24) >> 24;
+			if (!ethosu_is_u65(edev))
+				st.dma.dst.stride[1] = sign_extend64(addr, 39);
+			else
+				return -EINVAL;
 			break;
 		case NPU_SET_DMA0_SRC:
 			st.dma.src.offset = addr;

-- 
2.53.0

Re: [PATCH v4 26/31] accel: ethosu: Split U65 and U85 DMA length validation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] accel: ethosu: missing size0/size1 scaling for 1D side in mixed-mode DMA
--

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

accel: ethosu: Split U65 and U85 DMA length validation

This commit splits DMA length validation in the Ethos-U driver into separate
functions for U65 and U85 to account for their different stride and skip
models. It also updates the command stream validator to populate shared U65
strides and sign-extended U85 strides for source and destination DMA.

> diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
> index 117046e5a61f8..04c0de8336cd1 100644
> --- a/drivers/accel/ethosu/ethosu_gem.c
> +++ b/drivers/accel/ethosu/ethosu_gem.c
> @@ -245,40 +245,97 @@ static bool dma_params_valid(struct ethosu_device *edev, struct cmd_state *st,
>  	return true;
>  }
>  
> -static u64 dma_length(struct ethosu_device *edev,
> -		      struct ethosu_validated_cmdstream_info *info,
> -		      struct cmd_state *st, struct dma_state *dma_st,
> -		      struct dma *dma, u16 region_cmd, u16 addr_cmd)
> +static u64 dma_length_finish(struct ethosu_validated_cmdstream_info *info,
> +			     const struct dma *dma, u64 len)
> +{
> +	if (dma->region >= 0) {
> +		u64 end;
> +
> +		if (check_add_overflow(len, dma->offset, &end))
> +			return U64_MAX;
> +		info->region_size[dma->region] =
> +			max(info->region_size[dma->region], end);
> +	}
> +
> +	return len;
> +}
> +
> +static u64 dma_length_u65(struct ethosu_validated_cmdstream_info *info,
> +			  struct dma_state *dma_st,
> +			  struct dma *dma)
>  {
>  	s8 mode = dma->mode;
>  	u64 len = dma->len;
> +	u64 stride;
>  
> -	if (!dma_params_valid(edev, st, dma_st, dma, region_cmd, addr_cmd))
> -		return U64_MAX;
> +	if (mode >= 1) {
> +		if (check_mul_overflow(len, (u64)dma_st->size0, &len) ||
> +		    check_mul_overflow((u64)dma->stride[0],
> +				       (u64)dma_st->size0 - 1, &stride) ||
> +		    check_add_overflow(len, stride, &len))
> +			return U64_MAX;
> +	}
> +	if (mode == 2) {
> +		if (check_mul_overflow(len, (u64)dma_st->size1, &len) ||
> +		    check_mul_overflow((u64)dma->stride[1],
> +				       (u64)dma_st->size1 - 1, &stride) ||
> +		    check_add_overflow(len, stride, &len))
> +			return U64_MAX;
> +	}
> +
> +	return dma_length_finish(info, dma, len);
> +}
> +
> +static u64 dma_length_u85(struct ethosu_validated_cmdstream_info *info,
> +			  struct dma_state *dma_st,
> +			  struct dma *dma)
> +{
> +	s8 mode = dma->mode;
> +	s64 min = 0;
> +	u64 max = dma->len;
> +	s64 stride;
>  
>  	if (mode >= 1) {
> -		if (dma->stride[0] < 0 && (u64)(-dma->stride[0]) > len)
> +		if (check_mul_overflow(dma->stride[0],
> +				       (s64)dma_st->size0 - 1, &stride))
>  			return U64_MAX;
> -		len += dma->stride[0];
> -		if (check_mul_overflow(len, (u64)dma_st->size0, &len))
> +		if (stride < 0) {
> +			if (check_add_overflow(min, stride, &min))
> +				return U64_MAX;
> +		} else if (check_add_overflow(max, (u64)stride, &max)) {
>  			return U64_MAX;
> +		}
>  	}
>  	if (mode == 2) {
> -		if (dma->stride[1] < 0 && (u64)(-dma->stride[1]) > len)
> +		if (check_mul_overflow(dma->stride[1],
> +				       (s64)dma_st->size1 - 1, &stride))
>  			return U64_MAX;
> -		len += dma->stride[1];
> -		if (check_mul_overflow(len, (u64)dma_st->size1, &len))
> +		if (stride < 0) {
> +			if (check_add_overflow(min, stride, &min))
> +				return U64_MAX;
> +		} else if (check_add_overflow(max, (u64)stride, &max)) {
>  			return U64_MAX;
> +		}
>  	}
> -	if (dma->region >= 0) {
> -		u64 end;
>  
> -		if (check_add_overflow(len, dma->offset, &end))
> -			return U64_MAX;
> -		info->region_size[dma->region] = max(info->region_size[dma->region], end);
> -	}
> +	if (min < 0 && -(u64)min > dma->offset)
> +		return U64_MAX;
>  
> -	return len;
> +	return dma_length_finish(info, dma, max);
> +}

[Severity: High]
When ethosu_gem_cmdstream_copy_and_validate() handles NPU_OP_DMA_START in
drivers/accel/ethosu/ethosu_gem.c, it validates the source and destination
endpoints independently via dma_length():

		case NPU_OP_DMA_START:
			srclen = dma_length(edev, info, &st, &st.dma, &st.dma.src,
					    NPU_SET_DMA0_SRC_REGION, NPU_SET_DMA0_SRC);
			dstlen = dma_length(edev, info, &st, &st.dma, &st.dma.dst,
					    NPU_SET_DMA0_DST_REGION, NPU_SET_DMA0_DST);

What happens when a DMA operation uses mixed modes between source and
destination, such as copying between a 1D contiguous buffer (mode == 0) and
a 2D or 3D strided buffer (mode == 1 or 2)?

On U65 in particular, NPU_SET_DMA0_SRC_STRIDE0 and NPU_SET_DMA0_SRC_STRIDE1
are shared between source and destination, so copying between a contiguous
buffer and a strided buffer requires one side to use mode == 0 while the
other uses mode == 1 or 2.

Both dma_length_u65() and dma_length_u85() only inspect dma->mode of the
endpoint currently being evaluated. When dma->mode == 0 while the other
endpoint has mode >= 1, the 1D endpoint's length is left as dma->len (a
single 1D transfer) rather than being scaled by dma_st->size0 (and
dma_st->size1 for 3D) to cover all transfers performed by the DMA engine.

Could this under-compute info->region_size[dma->region] in
dma_length_finish() for the 1D endpoint and allow out-of-bounds DMA reads or
writes past the end of the backing GEM buffer object?

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

[PATCH v4 27/31] accel: ethosu: Validate OFM transpose

[Rob Herring (Arm)]

U85 OFM dimensions are specified before transposition, while
tile bases and strides address the transposed feature map. Permute
the output endpoint before validating its tile and stride accesses.

Allow the defined U85 transpose encodings and reject the two
reserved encodings.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v4:
 - Allow only HWC, HCW, and CHW output layouts for elementwise ops
 - Don't allow 64-bit output precision and chained output storage

v2:
 - no change
---
 drivers/accel/ethosu/ethosu_device.h |  1 +
 drivers/accel/ethosu/ethosu_gem.c    | 69 ++++++++++++++++++++++++++++++++++--
 2 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_device.h b/drivers/accel/ethosu/ethosu_device.h
index d3df6720085d..c3789474fa7b 100644
--- a/drivers/accel/ethosu/ethosu_device.h
+++ b/drivers/accel/ethosu/ethosu_device.h
@@ -92,6 +92,7 @@ struct gen_pool;
 #define NPU_OP_CONV_WEIGHTS_IFM2	BIT(0)
 #define NPU_KERNEL_DILATION_X		BIT(3)
 #define NPU_KERNEL_DILATION_Y		BIT(4)
+#define NPU_OFM_TRANSPOSE_MASK		GENMASK(13, 11)
 
 #define NPU_CMD_CTRL_CMD1		BIT(14)
 
diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index 04c0de8336cd..fe6bf1a2f8cb 100644
--- a/drivers/accel/ethosu/ethosu_gem.c
+++ b/drivers/accel/ethosu/ethosu_gem.c
@@ -422,6 +422,52 @@ static int feat_matrix_validate(struct ethosu_device *edev,
 
 	return 0;
 }
+
+static int feat_matrix_permute(struct ethosu_device *edev,
+			       struct feat_matrix *fm, u32 *x, u32 *y,
+			       u32 *c, bool ofm)
+{
+	u32 width = *x;
+	u32 height = *y;
+	u32 depth = *c;
+	u32 transpose;
+
+	if (ethosu_is_u65(edev) || !ofm)
+		return 0;
+
+	transpose = FIELD_GET(NPU_OFM_TRANSPOSE_MASK, fm->precision);
+
+	switch (transpose) {
+	case 0: /* HWC */
+		break;
+	case 1: /* WHC */
+		*x = height;
+		*y = width;
+		break;
+	case 2: /* HCW */
+		*x = depth;
+		*c = width;
+		break;
+	case 3: /* WCH */
+		*x = depth;
+		*y = width;
+		*c = height;
+		break;
+	case 6: /* CHW */
+		*x = height;
+		*y = depth;
+		*c = width;
+		break;
+	case 7: /* CWH */
+		*y = depth;
+		*c = height;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
 static u64 feat_matrix_length(struct ethosu_device *edev,
 			      struct ethosu_validated_cmdstream_info *info,
 			      struct cmd_state *st, struct feat_matrix *fm,
@@ -528,6 +574,9 @@ static int feat_matrix_size(struct ethosu_device *edev,
 	int ret;
 
 	*max_len = 0;
+	ret = feat_matrix_permute(edev, fm, &x, &y, &c, ofm);
+	if (ret)
+		return ret;
 
 	if (storage == 0) {
 		ret = feat_matrix_check_location(edev, info, st, fm, type, 0, 0,
@@ -900,6 +949,11 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 				return ret;
 			break;
 		case NPU_OP_ELEMENTWISE:
+			if (!ethosu_is_u65(edev) &&
+			    FIELD_GET(NPU_OFM_TRANSPOSE_MASK, st.ofm.precision) != 0 &&
+			    FIELD_GET(NPU_OFM_TRANSPOSE_MASK, st.ofm.precision) != 2 &&
+			    FIELD_GET(NPU_OFM_TRANSPOSE_MASK, st.ofm.precision) != 6)
+				return -EINVAL;
 			if (!ethosu_is_u65(edev) &&
 			    !cmd_state_reg_is_set(&st, NPU_SET_IFM_BROADCAST))
 				return -EINVAL;
@@ -1000,8 +1054,19 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 		case NPU_SET_OFM_PRECISION:
 			if (((param >> 6) & 0x3) > 1)
 				return -EINVAL;
-			if (!ethosu_is_u65(edev) && (param & GENMASK(13, 11)))
-				return -EINVAL;
+			if (!ethosu_is_u65(edev)) {
+				u32 transpose = FIELD_GET(NPU_OFM_TRANSPOSE_MASK, param);
+
+				switch (transpose) {
+				case 4:
+				case 5:
+					return -EINVAL;
+				}
+				if (transpose && ((param >> 1) & 0x3) == 3)
+					return -EINVAL;
+				if ((param >> 14) == 2 && transpose)
+					return -EINVAL;
+			}
 			st.ofm.precision = param;
 			break;
 		case NPU_SET_OFM_REGION:

-- 
2.53.0

Re: [PATCH v4 27/31] accel: ethosu: Validate OFM transpose

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] accel: ethosu: missing NHCWB16 format rejection when OFM transpose is enabled
--

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

accel: ethosu: Validate OFM transpose

This commit adds validation and dimension permutation for Ethos-U85 output
feature map (OFM) transposition in the command stream validator.

> diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
> index 04c0de8336cd1..fe6bf1a2f8cb2 100644
> --- a/drivers/accel/ethosu/ethosu_gem.c
> +++ b/drivers/accel/ethosu/ethosu_gem.c

[ ... ]

> @@ -1000,8 +1054,19 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
>  		case NPU_SET_OFM_PRECISION:
>  			if (((param >> 6) & 0x3) > 1)
>  				return -EINVAL;
> -			if (!ethosu_is_u65(edev) && (param & GENMASK(13, 11)))
> -				return -EINVAL;
> +			if (!ethosu_is_u65(edev)) {
> +				u32 transpose = FIELD_GET(NPU_OFM_TRANSPOSE_MASK, param);
> +
> +				switch (transpose) {
> +				case 4:
> +				case 5:
> +					return -EINVAL;
> +				}
> +				if (transpose && ((param >> 1) & 0x3) == 3)
> +					return -EINVAL;
> +				if ((param >> 14) == 2 && transpose)
> +					return -EINVAL;
> +			}
>  			st.ofm.precision = param;
>  			break;

[Severity: Critical]
Should ethosu_gem_cmdstream_copy_and_validate() also reject non-zero
transpose when the activation format in NPU_SET_OFM_PRECISION is NHCWB16
(((param >> 6) & 0x3) == 1)?

On Ethos-U85, OFM transposition is only valid with the linear NHWC format.
If a command stream specifies both NHCWB16 (((param >> 6) & 0x3) == 1) and
a non-zero transpose in NPU_SET_OFM_PRECISION:

1. feat_matrix_size() -> feat_matrix_permute() permutes x, y, and c into
   the transposed coordinates.

2. feat_matrix_length() -> feat_matrix_validate() checks:

	format = (fm->precision >> 6) & 0x3;
	stride_cmd = feat_matrix_base_cmd(type) + (format ? 6 : 4);
	if (!cmd_state_reg_is_set(st, stride_cmd))
		return -EINVAL;

   which only requires NPU_SET_OFM_STRIDE_C to be set when format is 1,
   leaving NPU_SET_OFM_STRIDE_X unvalidated.

3. feat_matrix_length() then computes the region size using the NHCWB16
   brick address calculation on the already-permuted x and c coordinates:

	case 1: //nhcwb16:
		element_size = BIT((fm->precision >> (ofm ? 1 : 2)) & 0x3);

		/* NHCWB16 accesses the complete final channel brick. */
		if (check_mul_overflow(c / 16, (u64)fm->stride_c, &offset) ||
		    check_add_overflow(addr, offset, &addr) ||
		    check_mul_overflow(16 * x + 15, element_size, &offset) ||
		    check_add_overflow(addr, offset, &addr))
			return U64_MAX;
		break;

Can this cause feat_matrix_length() to underestimate
info->region_size[fm->region] (for example, by permuting a large width into
c while fm->stride_c is 0) and allow out-of-bounds DMA writes past the end
of the GEM buffer?

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

[PATCH v4 28/31] accel: ethosu: Validate resize operations

[Rob Herring (Arm)]

Resize input coordinates are controlled by the scale, offset, and step
registers. Require those values to be explicitly programmed, validate
the scale and step relationships, and use a conservative coordinate
bound when validating the input feature map.

This prevents retained or malformed resize state from accessing beyond
the validated input feature-map buffer.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v3:
 - Fix width and height calculations to account for kernel size reported
   by sashiko

v2:
 - new patch
---
 drivers/accel/ethosu/ethosu_device.h |   8 ++
 drivers/accel/ethosu/ethosu_gem.c    | 161 ++++++++++++++++++++++++++++++++++-
 2 files changed, 168 insertions(+), 1 deletion(-)

diff --git a/drivers/accel/ethosu/ethosu_device.h b/drivers/accel/ethosu/ethosu_device.h
index c3789474fa7b..135c33f57045 100644
--- a/drivers/accel/ethosu/ethosu_device.h
+++ b/drivers/accel/ethosu/ethosu_device.h
@@ -121,6 +121,8 @@ enum ethosu_cmds {
 	NPU_SET_OFM_HEIGHT_M1 = 0x112,
 	NPU_SET_OFM_DEPTH_M1 = 0x113,
 	NPU_SET_OFM_PRECISION = 0x114,
+	NPU_SET_OFM_BLK_WIDTH_M1 = 0x115,
+	NPU_SET_OFM_BLK_HEIGHT_M1 = 0x116,
 	NPU_SET_OFM_WIDTH0_M1 = 0x11a,
 	NPU_SET_OFM_HEIGHT0_M1 = 0x11b,
 	NPU_SET_OFM_HEIGHT1_M1 = 0x11c,
@@ -131,6 +133,10 @@ enum ethosu_cmds {
 	NPU_SET_ACC_FORMAT = 0x124,
 	NPU_SET_WEIGHT_REGION = 0x128,
 	NPU_SET_SCALE_REGION = 0x129,
+	NPU_SET_RESIZE_X_SCALE_N_M1 = 0x12a,
+	NPU_SET_RESIZE_Y_SCALE_N_M1 = 0x12b,
+	NPU_SET_RESIZE_X_OFFSET = 0x12c,
+	NPU_SET_RESIZE_Y_OFFSET = 0x12d,
 	NPU_SET_DMA0_SRC_REGION = 0x130,
 	NPU_SET_DMA0_DST_REGION = 0x131,
 	NPU_SET_DMA0_SIZE0 = 0x132,
@@ -181,6 +187,8 @@ enum ethosu_cmds {
 	NPU_SET_WEIGHT2_LENGTH = 0x4093,
 	NPU_SET_WEIGHT3_BASE = 0x4094,
 	NPU_SET_WEIGHT3_LENGTH = 0x4095,
+	NPU_SET_RESIZE_X = 0x4096,
+	NPU_SET_RESIZE_Y = 0x4097,
 };
 
 #define NPU_ACC_FORMAT_INPUT_MASK	GENMASK(5, 4)
diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index fe6bf1a2f8cb..0b227664afc7 100644
--- a/drivers/accel/ethosu/ethosu_gem.c
+++ b/drivers/accel/ethosu/ethosu_gem.c
@@ -149,6 +149,15 @@ struct feat_matrix {
 	u8 pad_right;
 };
 
+struct resize_axis {
+	u16 scale_n;
+	s16 offset;
+	u16 one_step_int;
+	u16 one_step_mod;
+	u16 blk_step_int;
+	u16 blk_step_mod;
+};
+
 #define NPU_CMD0_REGS	0x200
 #define NPU_CMD1_REGS	0x100
 
@@ -162,6 +171,9 @@ struct cmd_state {
 	struct feat_matrix ofm;
 	struct feat_matrix ifm;
 	struct feat_matrix ifm2;
+	u16 ofm_blk_width;
+	u16 ofm_blk_height;
+	struct resize_axis resize[2];
 };
 
 static void cmd_state_init(struct cmd_state *st)
@@ -663,6 +675,98 @@ calc_acc_input_size(struct drm_device *ddev,
 	return ret;
 }
 
+static int resize_axis_size(struct cmd_state *st, int axis, u16 ofm_size,
+			    u16 ofm_blk_size, u32 *size)
+{
+	struct resize_axis *resize = &st->resize[axis];
+	u64 one_step, blk_step, coord;
+
+	if (resize->offset < -(s16)resize->scale_n ||
+	    resize->offset >= resize->scale_n ||
+	    resize->one_step_mod >= resize->scale_n ||
+	    resize->blk_step_mod >= resize->scale_n)
+		return -EINVAL;
+
+	one_step = resize->one_step_int * resize->scale_n +
+		resize->one_step_mod;
+	blk_step = resize->blk_step_int * resize->scale_n +
+		resize->blk_step_mod;
+	if (check_mul_overflow((u64)ofm_blk_size, one_step, &coord) ||
+	    blk_step != coord)
+		return -EINVAL;
+
+	if (check_mul_overflow((u64)ofm_size, one_step, &coord) ||
+	    check_add_overflow(coord, (u64)resize->scale_n - 1, &coord))
+		return -EINVAL;
+
+	coord = div_u64(coord, resize->scale_n);
+	if (coord >= U32_MAX)
+		return -EINVAL;
+
+	*size = coord + 1;
+	return 0;
+}
+
+
+static int calc_sizes_resize(struct drm_device *ddev,
+			     struct ethosu_validated_cmdstream_info *info,
+			     struct cmd_state *st)
+{
+	struct ethosu_device *edev = to_ethosu_device(ddev);
+	u32 ifm_width, ifm_height;
+	u64 len;
+	int ret;
+
+	if (!cmd_state_reg_is_set(st, NPU_SET_KERNEL_WIDTH_M1) ||
+	    !cmd_state_reg_is_set(st, NPU_SET_KERNEL_HEIGHT_M1) ||
+	    !cmd_state_reg_is_set(st, NPU_SET_OFM_BLK_WIDTH_M1) ||
+	    !cmd_state_reg_is_set(st, NPU_SET_OFM_BLK_HEIGHT_M1) ||
+	    !cmd_state_reg_is_set(st, NPU_SET_RESIZE_X_SCALE_N_M1) ||
+	    !cmd_state_reg_is_set(st, NPU_SET_RESIZE_Y_SCALE_N_M1) ||
+	    !cmd_state_reg_is_set(st, NPU_SET_RESIZE_X_OFFSET) ||
+	    !cmd_state_reg_is_set(st, NPU_SET_RESIZE_Y_OFFSET) ||
+	    !cmd_state_reg_is_set(st, NPU_SET_RESIZE_X) ||
+	    !cmd_state_reg_is_set(st, NPU_SET_RESIZE_Y))
+		return -EINVAL;
+
+	ret = resize_axis_size(st, 0, st->ofm.width, st->ofm_blk_width,
+			       &ifm_width);
+	if (ret)
+		return ret;
+	ret = resize_axis_size(st, 1, st->ofm.height[2], st->ofm_blk_height,
+			       &ifm_height);
+	if (ret)
+		return ret;
+
+	if (check_add_overflow(ifm_width, (u32)st->ifm.width, &ifm_width) ||
+	    check_add_overflow(ifm_height, (u32)st->ifm.height[2], &ifm_height))
+		return -EINVAL;
+
+	ret = feat_matrix_size(edev, info, st, &st->ifm, FEAT_MATRIX_IFM,
+			       ifm_width, ifm_height, st->ifm.depth, false, &len);
+	dev_dbg(ddev->dev, "op %d: IFM:%d:0x%llx-0x%llx\n", NPU_OP_RESIZE,
+		st->ifm.region, st->ifm.base[0], len);
+	if (ret)
+		return ret;
+
+	ret = feat_matrix_size(edev, info, st, &st->ofm, FEAT_MATRIX_OFM,
+			       st->ofm.width, st->ofm.height[2], st->ofm.depth,
+			       true, &len);
+	dev_dbg(ddev->dev, "op %d: OFM:%d:0x%llx-0x%llx\n", NPU_OP_RESIZE,
+		st->ofm.region, st->ofm.base[0], len);
+	if (ret)
+		return ret;
+
+	ret = calc_acc_input_size(ddev, info, st);
+	if (ret)
+		return ret;
+
+	if (!feat_matrix_chained(edev, &st->ofm))
+		info->output_region[st->ofm.region] = true;
+
+	return 0;
+}
+
 static int buffer_size(struct ethosu_validated_cmdstream_info *info,
 		       struct cmd_state *st, struct buffer *buf, s8 region,
 		       u16 region_cmd, u16 base_cmd, u16 length_cmd, bool optional)
@@ -972,7 +1076,12 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 				return ret;
 			break;
 		case NPU_OP_RESIZE: // U85 only
-			return -EINVAL;
+			if (ethosu_is_u65(edev) || param > 2)
+				return -EINVAL;
+			ret = calc_sizes_resize(ddev, info, &st);
+			if (ret)
+				return ret;
+			break;
 		case NPU_SET_KERNEL_WIDTH_M1:
 			st.ifm.width = param;
 			break;
@@ -1069,6 +1178,12 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 			}
 			st.ofm.precision = param;
 			break;
+		case NPU_SET_OFM_BLK_WIDTH_M1:
+			st.ofm_blk_width = param & 0x7f;
+			break;
+		case NPU_SET_OFM_BLK_HEIGHT_M1:
+			st.ofm_blk_height = param & 0x7f;
+			break;
 		case NPU_SET_OFM_REGION:
 			st.ofm.region = param & 0x7;
 			break;
@@ -1139,6 +1254,34 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 		case NPU_SET_SCALE_REGION:
 			st.scale[0].region = param & 0x7;
 			break;
+		case NPU_SET_RESIZE_X_SCALE_N_M1:
+			if (ethosu_is_u65(edev))
+				break;
+			if (param & GENMASK(15, 11))
+				return -EINVAL;
+			st.resize[0].scale_n = param + 1;
+			break;
+		case NPU_SET_RESIZE_Y_SCALE_N_M1:
+			if (ethosu_is_u65(edev))
+				break;
+			if (param & GENMASK(15, 11))
+				return -EINVAL;
+			st.resize[1].scale_n = param + 1;
+			break;
+		case NPU_SET_RESIZE_X_OFFSET:
+			if (ethosu_is_u65(edev))
+				break;
+			if (param & GENMASK(15, 12))
+				return -EINVAL;
+			st.resize[0].offset = sign_extend32(param, 11);
+			break;
+		case NPU_SET_RESIZE_Y_OFFSET:
+			if (ethosu_is_u65(edev))
+				break;
+			if (param & GENMASK(15, 12))
+				return -EINVAL;
+			st.resize[1].offset = sign_extend32(param, 11);
+			break;
 		case NPU_SET_WEIGHT_BASE:
 			st.weight[0].base = addr;
 			break;
@@ -1175,6 +1318,22 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 		case NPU_SET_WEIGHT3_LENGTH:
 			st.weight[3].length = cmds[1];
 			break;
+		case NPU_SET_RESIZE_X:
+		case NPU_SET_RESIZE_Y:
+			if (ethosu_is_u65(edev))
+				break;
+			if ((cmds[0] & BIT(31)) ||
+			    (cmds[1] & (GENMASK(31, 27) | GENMASK(15, 11))))
+				return -EINVAL;
+			st.resize[cmd - NPU_SET_RESIZE_X].one_step_int =
+				FIELD_GET(GENMASK(19, 16), cmds[0]);
+			st.resize[cmd - NPU_SET_RESIZE_X].blk_step_int =
+				FIELD_GET(GENMASK(30, 20), cmds[0]);
+			st.resize[cmd - NPU_SET_RESIZE_X].one_step_mod =
+				FIELD_GET(GENMASK(10, 0), cmds[1]);
+			st.resize[cmd - NPU_SET_RESIZE_X].blk_step_mod =
+				FIELD_GET(GENMASK(26, 16), cmds[1]);
+			break;
 
 		case NPU_SET_DMA0_SRC_REGION:
 			if (param & NPU_DMA_REGION_INDEX_MODE)

-- 
2.53.0

Re: [PATCH v4 28/31] accel: ethosu: Validate resize operations

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] accel: ethosu: off-by-one in OFM block size for resize_axis_size()
- [High] accel: ethosu: off-by-one IFM coordinate bound in resize_axis_size()
--

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

accel: ethosu: Validate resize operations

This commit adds command stream validation for Ethos-U85 NPU_OP_RESIZE
operations by tracking the resize scale, offset, step, and OFM block size
registers and validating the input feature map bounds.

> diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
> index fe6bf1a2f8cb2..0b227664afc7b 100644
> --- a/drivers/accel/ethosu/ethosu_gem.c
> +++ b/drivers/accel/ethosu/ethosu_gem.c

[ ... ]

> @@ -663,6 +675,98 @@ calc_acc_input_size(struct drm_device *ddev,
>  	return ret;
>  }
>  
> +static int resize_axis_size(struct cmd_state *st, int axis, u16 ofm_size,
> +			    u16 ofm_blk_size, u32 *size)
> +{
> +	struct resize_axis *resize = &st->resize[axis];
> +	u64 one_step, blk_step, coord;
> +
> +	if (resize->offset < -(s16)resize->scale_n ||
> +	    resize->offset >= resize->scale_n ||
> +	    resize->one_step_mod >= resize->scale_n ||
> +	    resize->blk_step_mod >= resize->scale_n)
> +		return -EINVAL;
> +
> +	one_step = resize->one_step_int * resize->scale_n +
> +		resize->one_step_mod;
> +	blk_step = resize->blk_step_int * resize->scale_n +
> +		resize->blk_step_mod;
> +	if (check_mul_overflow((u64)ofm_blk_size, one_step, &coord) ||
> +	    blk_step != coord)
> +		return -EINVAL;
> +
> +	if (check_mul_overflow((u64)ofm_size, one_step, &coord) ||
> +	    check_add_overflow(coord, (u64)resize->scale_n - 1, &coord))
> +		return -EINVAL;
> +
> +	coord = div_u64(coord, resize->scale_n);
> +	if (coord >= U32_MAX)
> +		return -EINVAL;
> +
> +	*size = coord + 1;
> +	return 0;
> +}
> +
> +
> +static int calc_sizes_resize(struct drm_device *ddev,
> +			     struct ethosu_validated_cmdstream_info *info,
> +			     struct cmd_state *st)
> +{
> +	struct ethosu_device *edev = to_ethosu_device(ddev);
> +	u32 ifm_width, ifm_height;
> +	u64 len;
> +	int ret;
> +
> +	if (!cmd_state_reg_is_set(st, NPU_SET_KERNEL_WIDTH_M1) ||
> +	    !cmd_state_reg_is_set(st, NPU_SET_KERNEL_HEIGHT_M1) ||
> +	    !cmd_state_reg_is_set(st, NPU_SET_OFM_BLK_WIDTH_M1) ||
> +	    !cmd_state_reg_is_set(st, NPU_SET_OFM_BLK_HEIGHT_M1) ||
> +	    !cmd_state_reg_is_set(st, NPU_SET_RESIZE_X_SCALE_N_M1) ||
> +	    !cmd_state_reg_is_set(st, NPU_SET_RESIZE_Y_SCALE_N_M1) ||
> +	    !cmd_state_reg_is_set(st, NPU_SET_RESIZE_X_OFFSET) ||
> +	    !cmd_state_reg_is_set(st, NPU_SET_RESIZE_Y_OFFSET) ||
> +	    !cmd_state_reg_is_set(st, NPU_SET_RESIZE_X) ||
> +	    !cmd_state_reg_is_set(st, NPU_SET_RESIZE_Y))
> +		return -EINVAL;
> +
> +	ret = resize_axis_size(st, 0, st->ofm.width, st->ofm_blk_width,
> +			       &ifm_width);
> +	if (ret)
> +		return ret;
> +	ret = resize_axis_size(st, 1, st->ofm.height[2], st->ofm_blk_height,
> +			       &ifm_height);
> +	if (ret)
> +		return ret;
> +
> +	if (check_add_overflow(ifm_width, (u32)st->ifm.width, &ifm_width) ||
> +	    check_add_overflow(ifm_height, (u32)st->ifm.height[2], &ifm_height))
> +		return -EINVAL;
> +
> +	ret = feat_matrix_size(edev, info, st, &st->ifm, FEAT_MATRIX_IFM,
> +			       ifm_width, ifm_height, st->ifm.depth, false, &len);

[Severity: High]
Does this overestimate ifm_width and ifm_height when passing them to
feat_matrix_size()?

In resize_axis_size(), ofm_size is already the 0-based maximum OFM coordinate
(st->ofm.width or st->ofm.height[2]), so coord after dividing by
resize->scale_n is a 0-based IFM coordinate. Setting *size = coord + 1
converts it to a 1-based size, and calc_sizes_resize() then adds st->ifm.width
(NPU_SET_KERNEL_WIDTH_M1) and st->ifm.height[2] (NPU_SET_KERNEL_HEIGHT_M1) on
top of that. In addition, resize_axis_size() adds resize->scale_n - 1 rather
than accounting for resize->offset, which can increase coord by another 1 when
resize->offset < resize->scale_n - 1.

However, feat_matrix_size() -> feat_matrix_check_location() ->
feat_matrix_length() expects 0-based maximum coordinates (x, y, c), matching
how st->ofm.width, st->ofm.height[2], and st->ifm.depth are passed:

drivers/accel/ethosu/ethosu_gem.c:feat_matrix_length() {
	...
	switch (storage) {
	case 0:
		if (x >= fm->width0 + 1) {
			x -= fm->width0 + 1;
			tile += 1;
		}
		if (y >= fm->height[tile] + 1) {
			y -= fm->height[tile] + 1;
			tile += 2;
		}
		break;
	...
}

When ifm_width or ifm_height exceeds the actual 0-based maximum coordinate by
1 or 2 on a single-tile IFM (where fm->width0 and fm->height[0] are set to the
IFM dimensions minus 1 and only NPU_SET_IFM_BASE0 is programmed), wouldn't
feat_matrix_length() advance to tiles 1..3 and fail validation with -EINVAL,
or require a larger IFM buffer than the tensor size?

[ ... ]

> @@ -1069,6 +1178,12 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
>  			}
>  			st.ofm.precision = param;
>  			break;
> +		case NPU_SET_OFM_BLK_WIDTH_M1:
> +			st.ofm_blk_width = param & 0x7f;
> +			break;
> +		case NPU_SET_OFM_BLK_HEIGHT_M1:
> +			st.ofm_blk_height = param & 0x7f;
> +			break;

[Severity: High]
Should st.ofm_blk_width and st.ofm_blk_height store (param & 0x7f) + 1 (or
should resize_axis_size() add 1 to ofm_blk_size)?

NPU_SET_OFM_BLK_WIDTH_M1 and NPU_SET_OFM_BLK_HEIGHT_M1 encode the block
dimensions minus 1, similar to NPU_SET_RESIZE_X_SCALE_N_M1 and
NPU_SET_RESIZE_Y_SCALE_N_M1 where param + 1 is stored.

In ethosu_gem_cmdstream_copy_and_validate() -> calc_sizes_resize() ->
resize_axis_size(), ofm_blk_size is multiplied directly by one_step:

	if (check_mul_overflow((u64)ofm_blk_size, one_step, &coord) ||
	    blk_step != coord)
		return -EINVAL;

With the unadjusted _M1 value, resize_axis_size() checks that blk_step equals
(block_size - 1) * one_step instead of block_size * one_step, which rejects
valid NPU_OP_RESIZE command streams with -EINVAL whenever one_step is
non-zero.

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

[PATCH v4 29/31] accel: ethosu: Ensure cmd stream is multiple of 4 bytes

[Rob Herring (Arm)]

The command-stream validator consumes 32-bit words, so reject empty or
unaligned streams at that boundary. This also prevents QSIZE from
receiving an unvalidated trailing byte.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v4:
 - New patch
---
 drivers/accel/ethosu/ethosu_gem.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index 0b227664afc7..1935440fb815 100644
--- a/drivers/accel/ethosu/ethosu_gem.c
+++ b/drivers/accel/ethosu/ethosu_gem.c
@@ -965,6 +965,8 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 
 	if (!info)
 		return -ENOMEM;
+	if (!size || size & 3)
+		return -EINVAL;
 	info->cmd_size = size;
 
 	cmd_state_init(&st);

-- 
2.53.0

[PATCH v4 30/31] accel: ethosu: Require active weight streams setup

[Rob Herring (Arm)]

A secondary weight or scale stream must be explicitly disabled or
configured when the hardware uses it. Otherwise, stale stream state from
a previous command stream can bypass buffer bounds validation.

Derive the active stream count from the NPU configuration on U85 and
from PARALLEL_MODE on U65. Require every active stream's length command,
while allowing a zero length to disable it.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v4:
 - New patch
---
 drivers/accel/ethosu/ethosu_device.h |  4 +++
 drivers/accel/ethosu/ethosu_gem.c    | 60 +++++++++++++++++++++++++++++++-----
 2 files changed, 57 insertions(+), 7 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_device.h b/drivers/accel/ethosu/ethosu_device.h
index 135c33f57045..f41a8e0e1e1b 100644
--- a/drivers/accel/ethosu/ethosu_device.h
+++ b/drivers/accel/ethosu/ethosu_device.h
@@ -63,6 +63,7 @@ struct gen_pool;
 
 #define CONFIG_MACS_PER_CC_MASK	GENMASK(3, 0)
 #define CONFIG_CMD_STREAM_VER_MASK	GENMASK(7, 4)
+#define CONFIG_NUM_WD_MASK		GENMASK(13, 12)
 
 #define STATUS_STATE_RUNNING	BIT(0)
 #define STATUS_IRQ_RAISED	BIT(1)
@@ -130,6 +131,7 @@ enum ethosu_cmds {
 	NPU_SET_KERNEL_WIDTH_M1 = 0x120,
 	NPU_SET_KERNEL_HEIGHT_M1 = 0x121,
 	NPU_SET_KERNEL_STRIDE = 0x122,
+	NPU_SET_PARALLEL_MODE = 0x123,	// U65 only
 	NPU_SET_ACC_FORMAT = 0x124,
 	NPU_SET_WEIGHT_REGION = 0x128,
 	NPU_SET_SCALE_REGION = 0x129,
@@ -193,6 +195,8 @@ enum ethosu_cmds {
 
 #define NPU_ACC_FORMAT_INPUT_MASK	GENMASK(5, 4)
 #define NPU_ACC_INPUT_IFM2		2
+#define NPU_MACS_PER_CC_256		8
+#define NPU_PARALLEL_MODE_DUAL_CORE_DEPTH	1
 
 #define ETHOSU_SRAM_REGION	2	/* Matching Vela compiler */
 
diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index 1935440fb815..21458aacfcad 100644
--- a/drivers/accel/ethosu/ethosu_gem.c
+++ b/drivers/accel/ethosu/ethosu_gem.c
@@ -165,6 +165,7 @@ struct cmd_state {
 	DECLARE_BITMAP(cmd0, NPU_CMD0_REGS);
 	DECLARE_BITMAP(cmd1, NPU_CMD1_REGS);
 	bool acc_input_ifm2;
+	u8 parallel_mode;
 	struct dma_state dma;
 	struct buffer scale[2];
 	struct buffer weight[4];
@@ -767,19 +768,45 @@ static int calc_sizes_resize(struct drm_device *ddev,
 	return 0;
 }
 
+static int weight_streams(struct ethosu_device *edev, struct cmd_state *st,
+			  u8 *streams)
+{
+	if (ethosu_is_u65(edev)) {
+		if (FIELD_GET(CONFIG_MACS_PER_CC_MASK, edev->npu_info.config) ==
+		    NPU_MACS_PER_CC_256) {
+			*streams = 1;
+			return 0;
+		}
+
+		if (!cmd_state_reg_is_set(st, NPU_SET_PARALLEL_MODE))
+			return -EINVAL;
+
+		*streams = st->parallel_mode + 1;
+	} else {
+		*streams = 1 << FIELD_GET(CONFIG_NUM_WD_MASK,
+					   edev->npu_info.config);
+	}
+
+	return 0;
+}
+
 static int buffer_size(struct ethosu_validated_cmdstream_info *info,
 		       struct cmd_state *st, struct buffer *buf, s8 region,
-		       u16 region_cmd, u16 base_cmd, u16 length_cmd, bool optional)
+		       u16 region_cmd, u16 base_cmd, u16 length_cmd, bool can_omit)
 {
 	u64 end;
 	bool base_set = cmd_state_reg_is_set(st, base_cmd);
 	bool length_set = cmd_state_reg_is_set(st, length_cmd);
 
-	if (optional && !base_set && !length_set)
+	if (can_omit && !length_set)
 		return 0;
 
 	if (region < 0 || !cmd_state_reg_is_set(st, region_cmd) ||
-	    !base_set || !length_set)
+	    !length_set)
+		return -EINVAL;
+	if (!buf->length)
+		return 0;
+	if (!base_set)
 		return -EINVAL;
 
 	if (check_add_overflow(buf->base, (u64)buf->length, &end))
@@ -797,6 +824,7 @@ static int calc_sizes(struct drm_device *ddev,
 {
 	struct ethosu_device *edev = to_ethosu_device(ddev);
 	u64 len;
+	u8 streams;
 	int ret;
 
 	if (ifm) {
@@ -848,6 +876,10 @@ static int calc_sizes(struct drm_device *ddev,
 	}
 
 	if (weight) {
+		ret = weight_streams(edev, st, &streams);
+		if (ret)
+			return ret;
+
 		dev_dbg(ddev->dev, "op %d: W:%d:0x%llx-0x%llx\n",
 			op, st->weight[0].region, st->weight[0].base,
 			st->weight[0].base + st->weight[0].length - 1);
@@ -858,19 +890,23 @@ static int calc_sizes(struct drm_device *ddev,
 
 		if (buffer_size(info, st, &st->weight[1], st->weight[0].region,
 				NPU_SET_WEIGHT_REGION, NPU_SET_WEIGHT1_BASE,
-				NPU_SET_WEIGHT1_LENGTH, true) ||
+				NPU_SET_WEIGHT1_LENGTH, streams < 2) ||
 		    buffer_size(info, st, &st->weight[3], st->weight[0].region,
 				NPU_SET_WEIGHT_REGION, NPU_SET_WEIGHT3_BASE,
-				NPU_SET_WEIGHT3_LENGTH, true))
+				NPU_SET_WEIGHT3_LENGTH, streams < 4))
 			return -EINVAL;
 		if (!ethosu_is_u65(edev) &&
 		    buffer_size(info, st, &st->weight[2], st->weight[0].region,
 				NPU_SET_WEIGHT_REGION, NPU_SET_WEIGHT2_BASE,
-				NPU_SET_WEIGHT2_LENGTH, true))
+				NPU_SET_WEIGHT2_LENGTH, streams < 3))
 			return -EINVAL;
 	}
 
 	if (scale) {
+		ret = weight_streams(edev, st, &streams);
+		if (ret)
+			return ret;
+
 		dev_dbg(ddev->dev, "op %d: S:%d:0x%llx-0x%llx\n",
 			op, st->scale[0].region, st->scale[0].base,
 			st->scale[0].base + st->scale[0].length - 1);
@@ -882,7 +918,7 @@ static int calc_sizes(struct drm_device *ddev,
 		if (ethosu_is_u65(edev) &&
 		    buffer_size(info, st, &st->scale[1], st->scale[0].region,
 				NPU_SET_SCALE_REGION, NPU_SET_SCALE1_BASE,
-				NPU_SET_SCALE1_LENGTH, true))
+				NPU_SET_SCALE1_LENGTH, streams < 2))
 			return -EINVAL;
 	}
 
@@ -1093,6 +1129,16 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 		case NPU_SET_KERNEL_STRIDE:
 			st.ifm.stride_kernel = param;
 			break;
+		case NPU_SET_PARALLEL_MODE:
+			if (!ethosu_is_u65(edev) ||
+			    param > NPU_PARALLEL_MODE_DUAL_CORE_DEPTH)
+				return -EINVAL;
+			if (FIELD_GET(CONFIG_MACS_PER_CC_MASK, edev->npu_info.config) ==
+			    NPU_MACS_PER_CC_256 &&
+			    param)
+				return -EINVAL;
+			st.parallel_mode = param;
+			break;
 		case NPU_SET_ACC_FORMAT:
 			if (!ethosu_is_u65(edev)) {
 				u32 acc_input = FIELD_GET(NPU_ACC_FORMAT_INPUT_MASK, param);

-- 
2.53.0

Re: [PATCH v4 30/31] accel: ethosu: Require active weight streams setup

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] accel: ethosu: skip inactive streams in buffer_size() when length was set
--

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

accel: ethosu: Require active weight streams setup

This commit requires every active weight or scale stream to be explicitly
configured or disabled with a zero length, deriving the active stream count
from the NPU configuration on U85 and from NPU_SET_PARALLEL_MODE on U65.

> diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
> index 1935440fb815b..21458aacfcad6 100644
> --- a/drivers/accel/ethosu/ethosu_gem.c
> +++ b/drivers/accel/ethosu/ethosu_gem.c

[ ... ]

> @@ -767,19 +768,45 @@ static int calc_sizes_resize(struct drm_device *ddev,
>  	return 0;
>  }
>  
> +static int weight_streams(struct ethosu_device *edev, struct cmd_state *st,
> +			  u8 *streams)
> +{
> +	if (ethosu_is_u65(edev)) {
> +		if (FIELD_GET(CONFIG_MACS_PER_CC_MASK, edev->npu_info.config) ==
> +		    NPU_MACS_PER_CC_256) {
> +			*streams = 1;
> +			return 0;
> +		}
> +
> +		if (!cmd_state_reg_is_set(st, NPU_SET_PARALLEL_MODE))
> +			return -EINVAL;
> +
> +		*streams = st->parallel_mode + 1;
> +	} else {
> +		*streams = 1 << FIELD_GET(CONFIG_NUM_WD_MASK,
> +					   edev->npu_info.config);
> +	}
> +
> +	return 0;
> +}
> +
>  static int buffer_size(struct ethosu_validated_cmdstream_info *info,
>  		       struct cmd_state *st, struct buffer *buf, s8 region,
> -		       u16 region_cmd, u16 base_cmd, u16 length_cmd, bool optional)
> +		       u16 region_cmd, u16 base_cmd, u16 length_cmd, bool can_omit)
>  {
>  	u64 end;
>  	bool base_set = cmd_state_reg_is_set(st, base_cmd);
>  	bool length_set = cmd_state_reg_is_set(st, length_cmd);
>  
> -	if (optional && !base_set && !length_set)
> +	if (can_omit && !length_set)
>  		return 0;

[Severity: Medium]
Should buffer_size() return 0 whenever can_omit is true, regardless of
whether length_set is true?

On Ethos-U65 with 512 MACs/cc, weight_streams() computes the active stream
count per operation from st->parallel_mode (*streams = st->parallel_mode + 1).

If an operation in a command stream runs with NPU_SET_PARALLEL_MODE set to 1
(streams == 2), it configures NPU_SET_WEIGHT1_BASE / NPU_SET_WEIGHT1_LENGTH
and NPU_SET_SCALE1_BASE / NPU_SET_SCALE1_LENGTH. In
ethosu_gem_cmdstream_copy_and_validate(), cmd_state_set_reg() sets the
corresponding bits in the cumulative st.cmd1 bitmap (which is never cleared
during the command stream) and populates st.weight[1] and st.scale[1].

If a subsequent operation in the same command stream switches to
NPU_SET_PARALLEL_MODE = 0 (streams == 1) and changes NPU_SET_WEIGHT_REGION or
NPU_SET_SCALE_REGION (for example, to SRAM or a smaller buffer), stream 1 is
inactive and calc_sizes() passes can_omit = (streams < 2) as true:

calc_sizes()
	buffer_size(info, st, &st->weight[1], st->weight[0].region,
		    NPU_SET_WEIGHT_REGION, NPU_SET_WEIGHT1_BASE,
		    NPU_SET_WEIGHT1_LENGTH, streams < 2)

	buffer_size(info, st, &st->scale[1], st->scale[0].region,
		    NPU_SET_SCALE_REGION, NPU_SET_SCALE1_BASE,
		    NPU_SET_SCALE1_LENGTH, streams < 2)

Because length_set remains true in st.cmd1 from the earlier operation,
buffer_size() will not return early at "if (can_omit && !length_set)". Will
this charge the stale st->weight[1] and st->scale[1] bounds against the new
operation's st->weight[0].region and st->scale[0].region, inflating
info->region_size[region] and causing valid job submissions to be rejected in
ethosu_ioctl_submit_job()?

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

[PATCH v4 31/31] accel: ethosu: Support U85 IFM upscaling

[Rob Herring (Arm)]

Track the U85 IFM upscaling mode while validating command streams and
adjust feature-map bounds for its 2x resampling modes.

Resize operations require no IFM upscaling, so reject a non-default
mode there.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v4:
 - New patch
---
 drivers/accel/ethosu/ethosu_device.h |  7 +++++++
 drivers/accel/ethosu/ethosu_gem.c    | 30 ++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/drivers/accel/ethosu/ethosu_device.h b/drivers/accel/ethosu/ethosu_device.h
index f41a8e0e1e1b..1fec33d9917c 100644
--- a/drivers/accel/ethosu/ethosu_device.h
+++ b/drivers/accel/ethosu/ethosu_device.h
@@ -97,6 +97,12 @@ struct gen_pool;
 
 #define NPU_CMD_CTRL_CMD1		BIT(14)
 
+enum ethosu_ifm_upscale_mode {
+	NPU_IFM_UPSCALE_NONE,
+	NPU_IFM_UPSCALE_NEAREST,
+	NPU_IFM_UPSCALE_TRANSPOSE,
+};
+
 enum ethosu_cmds {
 	NPU_OP_STOP = 0x0,
 	NPU_OP_CONV = 0x2,
@@ -113,6 +119,7 @@ enum ethosu_cmds {
 	NPU_SET_IFM_PAD_BOTTOM = 0x103,
 	NPU_SET_IFM_DEPTH_M1 = 0x104,
 	NPU_SET_IFM_PRECISION = 0x105,
+	NPU_SET_IFM_UPSCALE = 0x107, // U85 only
 	NPU_SET_IFM_BROADCAST = 0x108,
 	NPU_SET_IFM_WIDTH0_M1 = 0x10a,
 	NPU_SET_IFM_HEIGHT0_M1 = 0x10b,
diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index 21458aacfcad..d82c9540e82a 100644
--- a/drivers/accel/ethosu/ethosu_gem.c
+++ b/drivers/accel/ethosu/ethosu_gem.c
@@ -165,6 +165,7 @@ struct cmd_state {
 	DECLARE_BITMAP(cmd0, NPU_CMD0_REGS);
 	DECLARE_BITMAP(cmd1, NPU_CMD1_REGS);
 	bool acc_input_ifm2;
+	u8 ifm_upscale;
 	u8 parallel_mode;
 	struct dma_state dma;
 	struct buffer scale[2];
@@ -718,6 +719,11 @@ static int calc_sizes_resize(struct drm_device *ddev,
 	u64 len;
 	int ret;
 
+	if (st->ifm_upscale != NPU_IFM_UPSCALE_NONE ||
+	    feat_matrix_chained(edev, &st->ofm) ||
+	    FIELD_GET(NPU_OFM_TRANSPOSE_MASK, st->ofm.precision))
+		return -EINVAL;
+
 	if (!cmd_state_reg_is_set(st, NPU_SET_KERNEL_WIDTH_M1) ||
 	    !cmd_state_reg_is_set(st, NPU_SET_KERNEL_HEIGHT_M1) ||
 	    !cmd_state_reg_is_set(st, NPU_SET_OFM_BLK_WIDTH_M1) ||
@@ -790,6 +796,20 @@ static int weight_streams(struct ethosu_device *edev, struct cmd_state *st,
 	return 0;
 }
 
+static u32 ifm_upscale_coord(u32 coord, u8 mode)
+{
+	switch (mode) {
+	case NPU_IFM_UPSCALE_NONE:
+		return coord;
+	case NPU_IFM_UPSCALE_NEAREST:
+		return coord / 2;
+	case NPU_IFM_UPSCALE_TRANSPOSE:
+		return coord / 2;
+	}
+
+	return 0;
+}
+
 static int buffer_size(struct ethosu_validated_cmdstream_info *info,
 		       struct cmd_state *st, struct buffer *buf, s8 region,
 		       u16 region_cmd, u16 base_cmd, u16 length_cmd, bool can_omit)
@@ -856,6 +876,8 @@ static int calc_sizes(struct drm_device *ddev,
 
 		if (ifm_height < 0 || ifm_width < 0)
 			return -EINVAL;
+		ifm_height = ifm_upscale_coord(ifm_height, st->ifm_upscale);
+		ifm_width = ifm_upscale_coord(ifm_width, st->ifm_upscale);
 
 		ret = feat_matrix_size(edev, info, st, &st->ifm, FEAT_MATRIX_IFM,
 				       ifm_width, ifm_height, st->ifm.depth, false,
@@ -952,6 +974,8 @@ static int calc_sizes_elemwise(struct drm_device *ddev,
 		height = st->ifm.broadcast & 0x1 ? 0 : st->ofm.height[2];
 		width = st->ifm.broadcast & 0x2 ? 0 : st->ofm.width;
 		depth = st->ifm.broadcast & 0x4 ? 0 : st->ofm.depth;
+		height = ifm_upscale_coord(height, st->ifm_upscale);
+		width = ifm_upscale_coord(width, st->ifm_upscale);
 
 		ret = feat_matrix_size(edev, info, st, &st->ifm, FEAT_MATRIX_IFM,
 				       width, height, depth, false, &len);
@@ -1168,6 +1192,12 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 				return -EINVAL;
 			st.ifm.precision = param;
 			break;
+		case NPU_SET_IFM_UPSCALE:
+			if (ethosu_is_u65(edev) ||
+			    param > NPU_IFM_UPSCALE_TRANSPOSE)
+				return -EINVAL;
+			st.ifm_upscale = param;
+			break;
 		case NPU_SET_IFM_BROADCAST:
 			st.ifm.broadcast = param;
 			break;

-- 
2.53.0