Re: [PATCH v3 02/22] accel: ethosu: Ensure suspended on removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The patch evaluates the return value of `pm_runtime_get_if_active()` as a boolean, which fails to handle the `-EINVAL` error case and causes an unbalanced usage counter decrement when Runtime PM is disabled.

Pre-existing issues:
- [High] In `ethosu_perfmon_stop_locked()`, the return value of `pm_runtime_get_if_active()` is checked with a boolean NOT operator, misinterpreting the `-EINVAL` error as success, leading to unpowered hardware access and an unbalanced PM usage counter.
- [High] The IRQ handler accesses hardware registers without checking the device's runtime PM state, risking a system crash if clocks are disabled.
--

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

accel: ethosu: Ensure suspended on removal

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

> diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
> --- a/drivers/accel/ethosu/ethosu_drv.c
> +++ b/drivers/accel/ethosu/ethosu_drv.c
> @@ -387,6 +387,10 @@ static void ethosu_remove(struct platform_device *pdev)
>  
>  	drm_dev_unregister(&ethosudev->base);
>  	ethosu_job_fini(ethosudev);
> +
> +	if (pm_runtime_get_if_active(&pdev->dev))
> +		pm_runtime_put_sync_suspend(&pdev->dev);

[Severity: High]
Does evaluating the return value of pm_runtime_get_if_active() as a boolean
fail to handle the -EINVAL error case?

If Runtime PM is disabled, pm_runtime_get_if_active() returns -EINVAL, which
evaluates to true here. Could this cause an unbalanced usage counter decrement
that corrupts the device PM state, and potentially power off the device
prematurely?


While reviewing this, I noticed a couple of related pre-existing issues in
the subsystem.

In drivers/accel/ethosu/ethosu_perfmon.c:ethosu_perfmon_stop_locked():

> 	if (!pm_runtime_get_if_active(ethosu->base.dev)) {
> 		ethosu->perfmon_state.active = NULL;
> 		return;
> 	}

[Severity: High]
This is a pre-existing issue, but does using a boolean NOT operator on the
return value of pm_runtime_get_if_active() misinterpret the -EINVAL error
as success?

If Runtime PM is disabled, -EINVAL evaluates as non-zero (so !(-EINVAL) is
false), bypassing this abort condition. Could this lead to reading PMU
registers without clocks, causing an asynchronous external abort (bus fault)
that crashes the system, as well as dropping a PM reference that was never
acquired?


In drivers/accel/ethosu/ethosu_job.c:ethosu_job_irq_handler():

> static irqreturn_t ethosu_job_irq_handler(int irq, void *data)
> {
> 	struct ethosu_device *dev = data;
> 	u32 status = readl_relaxed(dev->regs + NPU_REG_STATUS);

[Severity: High]
This is a pre-existing issue, but does this IRQ handler access hardware
registers without checking the device's runtime PM state?

Any spurious or delayed interrupt firing during the suspended state could
trigger this path. Does unconditionally reading NPU_REG_STATUS upon entry
risk a bus fault and kernel panic if the device clocks are disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-ethosu-fixes-v3-0-490fe215286f@kernel.org?part=2