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) <[email protected]> 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(ðosudev->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/[email protected]?part=2
