Re: [PATCH v11 04/14] accel/rocket: let the core suspend after a reset

[sashiko-bot@kernel.org]

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

New issues:
- [High] Using asynchronous pm_runtime_put_autosuspend() instead of a synchronous suspend in the reset path prevents the required power domain cycle if the next job is submitted immediately.

Pre-existing issues:
- [High] The shared IRQ handler accesses hardware registers without verifying if the device is active, and the runtime suspend callback powers down hardware without synchronizing with in-flight shared IRQs.
- [High] Reference count leak of the IOMMU group on every job completion in rocket_job_next_locked().
- [High] Detaching the IOMMU before halting the hardware during reset causes SMMU translation faults if the hardware is still active.
--

commit b4995381e41020bdb1af33ec4a432209b68f8226
Author: Jiaxing Hu <gahing@gahingwoo.com>

accel/rocket: let the core suspend after a reset

This patch modifies rocket_reset to drop the PM reference using
pm_runtime_put_autosuspend, allowing the core to suspend and trigger
a bus interface reset upon resuming.

Link: https://lore.kernel.org/all/20260819073530.6087-1-royalnet026@gmail.com/

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index 3c0ed46050669..a89ab49e17e52 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -406,12 +406,12 @@ rocket_reset(struct rocket_core *core, struct drm_sched_job *bad)
>  
>  	/*
>  	 * No handler is running now, but we might still have stuck jobs. Let's
> -	 * make sure the PM counters stay balanced by manually calling
> -	 * pm_runtime_put_noidle().
> +	 * make sure the PM counters stay balanced by putting the reference the
> +	 * job took, and request idle while doing it so the core can suspend.
>  	 */
>  	scoped_guard(mutex, &core->job_lock) {
>  		if (core->in_flight_job)
> -			pm_runtime_put_noidle(core->dev);
> +			pm_runtime_put_autosuspend(core->dev);

[Severity: High]
Does using the asynchronous pm_runtime_put_autosuspend() here prevent the
required power domain cycle?

If a queued job is submitted immediately after reset, couldn't the autosuspend
timer be canceled before the core actually suspends?

rocket_reset() finishes by restarting the scheduler:

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

If the scheduler restarts and the next job runs immediately, it will take a
PM reference and cancel the autosuspend timer before the suspend happens.
Would using a synchronous suspend instead guarantee the hardware ordering
constraints are met?

>  
>  		iommu_detach_group(NULL, core->iommu_group);

[Severity: High]
This is a pre-existing issue, but does detaching the IOMMU here before halting
the hardware risk SMMU translation faults?

If a hung job leaves the hardware actively attempting DMA transactions,
detaching the IOMMU unmaps the device's DMA domain while the transactions are
still in flight.

The hardware is reset and halted after the IOMMU group is already detached:

    iommu_detach_group(NULL, core->iommu_group);
    ...
    /* Proceed with reset now. */
    rocket_core_reset(core);

Should the hardware be halted prior to revoking its DMA mappings?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does the shared IRQ handler
access hardware registers without verifying the device's PM state?

In rocket_job_irq_handler(), the handler reads a hardware register on a shared
IRQ line:

    u32 raw_status = rocket_pc_readl(core, INTERRUPT_RAW_STATUS);

If an interrupt fires for another device sharing the same IRQ line while the
NPU is runtime suspended, won't this read unclocked hardware registers and
potentially cause an asynchronous SError panic?

Additionally, in rocket_device_runtime_suspend(), the hardware is powered down
without synchronizing with in-flight shared IRQs:

    clk_bulk_disable_unprepare(rdev->cores[core].soc->num_clks, 
                               rdev->cores[core].clks);

Could this crash the kernel if the handler is executing mid-flight while the
clocks are disabled?

[Severity: High]
This is a pre-existing issue, but does rocket_job_next_locked() leak an IOMMU
group reference on every normal job completion?

The group reference count is incremented but not decremented when detaching:

    iommu_detach_group(NULL, iommu_group_get(core->dev));

Because jobs execute continuously, won't this eventually saturate the refcount
and permanently leak the IOMMU group object on teardown?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831081956.84871-1-gahing@gahingwoo.com?part=4