Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] panthor: false success returned when hardware waits are interrupted by AW loss - [High] panthor: firmware events dropped or IRQ livelock on AW loss - [High] panthor: threads block for full timeout duration on AW loss due to missing wakeups --
commit 4e7abd3f16aae357d6bd8c91a0c0d4c8218b1fcc Author: Karunika Choo <[email protected]> drm/panthor: Tolerate access-window loss during HW waits The patch handles access window (AW) loss during hardware waits to prevent false hardware timeouts or recoveries. It uses panthor_aw_has_gpu_access to suppress timeout handling once access is revoked. > diff --git a/drivers/gpu/drm/panthor/panthor_fw.c > b/drivers/gpu/drm/panthor/panthor_fw.c > index 1449d9a66302e..b4c234f3d3ea4 100644 > --- a/drivers/gpu/drm/panthor/panthor_fw.c > +++ b/drivers/gpu/drm/panthor/panthor_fw.c > @@ -1140,9 +1140,10 @@ static int panthor_fw_start(struct panthor_device > *ptdev) > gpu_write(fw->iomem, MCU_CONTROL, MCU_CONTROL_AUTO); > > if (!wait_event_timeout(ptdev->fw->req_waitqueue, > - ptdev->fw->booted, > + (ptdev->fw->booted || > + !panthor_aw_has_gpu_access(ptdev)), > msecs_to_jiffies(1000))) { > - if (!ptdev->fw->booted && > + if (!ptdev->fw->booted && panthor_aw_has_gpu_access(ptdev) && > !(gpu_read(fw->irq.iomem, INT_STAT) & JOB_INT_GLOBAL_IF)) > timedout = true; > } [Severity: High] Does this code silently fail to boot but return success? If the AW is lost during the wait, panthor_aw_has_gpu_access() returns false, preventing timedout from being set to true. As a result, panthor_fw_start() returns success even though the firmware is not actually booted. Additionally, because there is no wakeup mechanism triggered upon AW loss, will this thread stall in wait_event_timeout() for the full 1000ms duration when AW access is revoked? [ ... ] > diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c > b/drivers/gpu/drm/panthor/panthor_gpu.c > index e6e970e9b59d3..25fe7c77f8763 100644 > --- a/drivers/gpu/drm/panthor/panthor_gpu.c > +++ b/drivers/gpu/drm/panthor/panthor_gpu.c > @@ -376,7 +377,8 @@ int panthor_gpu_flush_caches(struct panthor_device *ptdev, > msecs_to_jiffies(100))) { > scoped_guard(spinlock, &ptdev->gpu->reqs_lock) { > if ((ptdev->gpu->pending_reqs & > GPU_IRQ_CLEAN_CACHES_COMPLETED) != 0 && > - !(gpu_read(gpu->irq.iomem, INT_RAWSTAT) & > GPU_IRQ_CLEAN_CACHES_COMPLETED)) > + !(gpu_read(gpu->irq.iomem, INT_RAWSTAT) & > + GPU_IRQ_CLEAN_CACHES_COMPLETED) && > panthor_aw_has_gpu_access(ptdev)) > ret = -ETIMEDOUT; > else > ptdev->gpu->pending_reqs &= > ~GPU_IRQ_CLEAN_CACHES_COMPLETED; [Severity: High] Can this deceive callers into assuming caches were successfully flushed? If AW is lost, the timeout error is suppressed and the pending request flag is cleared, causing panthor_gpu_flush_caches() to return 0. This could cause divergence if callers proceed assuming the flush completed in the hardware. Also, wait_event_timeout() lacks an AW check in the condition and has no associated wakeup on AW loss, which will cause a 100ms stall. [ ... ] > diff --git a/drivers/gpu/drm/panthor/panthor_pwr.c > b/drivers/gpu/drm/panthor/panthor_pwr.c > index cc2e01595f23b..2a0feb0b39ece 100644 > --- a/drivers/gpu/drm/panthor/panthor_pwr.c > +++ b/drivers/gpu/drm/panthor/panthor_pwr.c > @@ -122,7 +123,8 @@ static int panthor_pwr_reset(struct panthor_device > *ptdev, u32 reset_cmd) > msecs_to_jiffies(PWR_RESET_TIMEOUT_MS))) { > guard(spinlock_irqsave)(&ptdev->pwr->reqs_lock); > > - if (reset_pending(ptdev) && !reset_irq_raised(ptdev)) { > + if (reset_pending(ptdev) && !reset_irq_raised(ptdev) && > + panthor_aw_has_gpu_access(ptdev)) { > drm_err(&ptdev->base, "RESET timed out (0x%x)", > reset_cmd); > return -ETIMEDOUT; > } [Severity: High] Is the thread blocked for the full PWR_RESET_TIMEOUT_MS when AW is lost? The wait_event_timeout() lacks an AW check in its condition, and without a corresponding wakeup mechanism for AW loss, it appears this will stall the thread unnecessarily in panthor_pwr_reset(). [ ... ] > @@ -266,9 +268,14 @@ static int panthor_pwr_domain_transition(struct > panthor_device *ptdev, u32 cmd, > > panthor_pwr_write_command(ptdev, pwr_cmd, mask); > > - ret = gpu_read64_poll_timeout(pwr->iomem, ready_reg, val, (mask & val) > == expected_val, > + ret = gpu_read64_poll_timeout(pwr->iomem, ready_reg, val, > + ((mask & val) == expected_val || > + !panthor_aw_has_gpu_access(ptdev)), > 100, timeout_us); > if (ret) { > + if (!panthor_aw_has_gpu_access(ptdev)) > + return 0; > + [Severity: High] Does returning success here break the power domain transition sequence? If the AW arbiter revokes access before the transition finishes, this will return 0 even though the hardware power domain transition failed to complete. [ ... ] > diff --git a/drivers/gpu/drm/panthor/panthor_sched.c > b/drivers/gpu/drm/panthor/panthor_sched.c > index 60b2417deb81b..e13553ec97ec9 100644 > --- a/drivers/gpu/drm/panthor/panthor_sched.c > +++ b/drivers/gpu/drm/panthor/panthor_sched.c > @@ -1886,6 +1886,9 @@ void panthor_sched_report_fw_events(struct > panthor_device *ptdev, u32 events) > > guard(spinlock)(&ptdev->scheduler->events_lock); > > + if (!panthor_aw_has_gpu_access(ptdev)) > + return; > + [Severity: High] Could this early return cause an IRQ livelock or dropped firmware events? If a hardware interrupt occurs and panthor_job_irq_handler() successfully clears the interrupt from the hardware, but then AW access is lost, this returns early. The cleared events would be permanently dropped. If AW access is lost before the hardware is cleared in the IRQ handler, the interrupt remains asserted but this function ignores it, which may cause the threaded IRQ handler to enter an infinite loop. [ ... ] > @@ -2059,6 +2062,8 @@ static int csgs_upd_ctx_apply_locked(struct > panthor_device *ptdev, > csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id); > > ret = panthor_fw_csg_wait_acks(ptdev, csg_id, req_mask, &acked, > 100); > + if (ret && !panthor_aw_has_gpu_access(ptdev)) > + ret = 0; > [Severity: High] Does this override the timeout error with success, causing the scheduler to erroneously assume the firmware processed the CSG state change? Also, panthor_fw_csg_wait_acks() relies on panthor_fw_wait_acks() which uses wait_event_timeout(). Since this is called in a loop while holding the scheduler lock, will the lack of AW wakeups cause compounded delays (N * 100ms) that block the entire scheduler? [ ... ] Regarding panthor_job_irq_handler() in drivers/gpu/drm/panthor/panthor_fw.c: static void panthor_job_irq_handler(struct panthor_irq *pirq, u32 status) { ... if (!ptdev->fw->booted && (status & JOB_INT_GLOBAL_IF)) ptdev->fw->booted = true; ... } [Severity: Medium] This is a pre-existing issue, but does this logic spuriously set the firmware state to booted on an AW loss? When AW access is lost, the MMIO read for the interrupt status will return 0xFFFFFFFF, which has JOB_INT_GLOBAL_IF set. This would incorrectly flag the firmware as booted during early execution. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=25
