Re: [PATCH v1 22/27] drm/panthor: Add access-window support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] drm/panthor: missing AW IRQ cleanup in panthor_device_init() error path
- [High] drm/panthor: background msg_retry_work runs concurrently with suspend
- [Medium] workqueue: 100% CPU busy loop in panthor_aw_msg_retry_work
- [Medium] drm/panthor: 5-second stall during device unplug due to disabled AW IRQs
- [Medium] drm/panthor: dropped AM messages cause 5-second timeout stalls
- [Low] drm/panthor: ignored -EOPNOTSUPP in AW handshake leads to unsupported protocol usage
--

commit 3a133f64881fd3095e93885e724b739d725e86af
Author: Karunika Choo <karunika.choo@arm.com>

drm/panthor: Add access-window support

Add the VM-side access-window component for v15 GPUs. The new code
handles AM message handshakes, requests GPU access from the arbiter,
waits for the window-open interrupt, and yields access on suspend.
Split panthor_hw_init into bind device and initialize gpu_info after
panthor_aw has ensured GPU access.

> diff --git a/drivers/gpu/drm/panthor/panthor_aw.c b/drivers/gpu/drm/panthor/panthor_aw.c
> index 0000000000000..848fd64eb51b5
> --- /dev/null
> +++ b/drivers/gpu/drm/panthor/panthor_aw.c
[ ... ]
> @@ -118,17 +118,16 @@ static void panthor_aw_msg_retry_work(struct work_struct *work)
>  	struct panthor_aw *aw =
>  		container_of(work, struct panthor_aw, msg_retry_work);
>  	struct panthor_am_msg *msg = &aw->msg;
>  	int ret;
>  
>  	ret = panthor_am_msg_retry(msg);
>  	if (ret == -EINVAL)
>  		drm_warn(&aw->ptdev->base, "Send FIFO unexpectedly empty");
>  
>  	if (ret == -EBUSY || ret == -EAGAIN)
>  		queue_work(aw->wq, &aw->msg_retry_work);

[Severity: Medium]
Will this cause a busy loop pegging a CPU core? If panthor_am_msg_retry() 
returns -EBUSY (because the hardware AM_OUTGOING_MESSAGE FIFO is not empty), 
the workqueue immediately re-queues itself without any delay. Should this use
queue_delayed_work() or an interrupt-driven mechanism to back off while
hardware processes the request?

>  }
>  
>  static void panthor_aw_send_msg(struct panthor_aw *aw, u64 message)
>  {
>  	struct panthor_device *ptdev = aw->ptdev;
>  	int ret;
>  
>  	ret = panthor_am_msg_send(&aw->msg, message);
>  	if (ret == -ENOSPC)
>  		drm_err(&ptdev->base, "Send FIFO is full");

[Severity: Medium]
Should we handle this error instead of silently swallowing it? If the FIFO is
full, the request is never buffered or dispatched, meaning callers will 
unnecessarily stall for 5 seconds waiting for a state transition response that
will never arrive.

>  
>  	if (ret == -EBUSY) {
>  		drm_dbg(&ptdev->base, "Pending messages, scheduling retry work");
>  		queue_work(aw->wq, &aw->msg_retry_work);
>  	}
>  }
>  
>  static void panthor_aw_handshake_handle(struct panthor_aw *aw, u64 message)
>  {
>  	struct panthor_device *ptdev = aw->ptdev;
>  	bool acked = AM_MSG_ACK_GET(message);
>  	u8 version = AM_MSG_VERSION_GET(message);
>  	int ret;
>  
>  	ret = panthor_am_msg_version_validate(&aw->msg, version);
>  	if (ret == -EOPNOTSUPP)
>  		drm_warn(&ptdev->base,
>  			 "Msg protocol version less than minimum supported (%u < %u)",
>  			 version, AM_MSG_MIN_SUPPORTED_VERSION);

[Severity: Low]
Is it safe to ignore the -EOPNOTSUPP error and continue? Operating with an
unsupported firmware protocol version may cause undefined behavior or firmware
crashes. Should the code explicitly fail the initialization rather than
assuming it is safe to interact with an incompatible Arbiter version?

>  
>  	if (!acked) {
>  		u64 reply = VM_ARB_INIT_MAKE(1, aw->msg.version);
>  
>  		panthor_aw_send_msg(aw, reply);
[ ... ]
> @@ -303,17 +302,15 @@ int panthor_aw_init(struct panthor_device *ptdev)
>  
>  	ret = panthor_request_aw_irq(ptdev, &aw->irq, irq, WINDOW_IRQ_MASK,
>  				     aw->iomem + WINDOW_INT_BASE);
>  	if (ret)
>  		return ret;
>  
>  	ret = panthor_aw_request(aw);
>  	if (ret)
>  		return ret;

[Severity: High]
Does this error path leak the active AW IRQ state? If panthor_aw_request() 
fails, we return without unwinding the active IRQ or pending work queue. 

In panthor_device_init(), if this function returns an error, we jump directly
to err_rpm_put, bypassing panthor_aw_unplug():

drivers/gpu/drm/panthor/panthor_device.c:panthor_device_init() {
    ...
	ret = panthor_aw_init(ptdev);
	if (ret)
		goto err_rpm_put;
    ...
}

Leaving a shared hardware IRQ unmasked while the device may be partially 
initialized or powered off could cause kernel panics, such as bus errors on 
MMIO access in the handler.

>  
>  	return 0;
>  }
>  
>  void panthor_aw_unplug(struct panthor_device *ptdev)
>  {
>  	struct panthor_aw *aw = ptdev->aw;
>  
>  	if (!aw)
>  		return;
>  
>  	disable_work_sync(&aw->msg_retry_work);
>  
>  	panthor_aw_irq_suspend(&aw->irq);

[Severity: Medium]
Will this sequence cause a 5-second stall during device unplug? 

In panthor_device_unplug(), panthor_aw_unplug() is called before
pm_runtime_put_sync_suspend():

drivers/gpu/drm/panthor/panthor_device.c:panthor_device_unplug() {
    ...
	panthor_aw_unplug(ptdev);

	pm_runtime_dont_use_autosuspend(ptdev->base.dev);
	pm_runtime_put_sync_suspend(ptdev->base.dev);
    ...
}

panthor_aw_unplug() disables and masks the AW IRQ. Then, the PM suspend path 
calls panthor_aw_suspend() which uses panthor_aw_state_wait() to wait for an 
interrupt-driven state transition to READY. Since the IRQ is masked before 
the suspend yield happens, the completion interrupt is never handled, 
guaranteeing a full 5-second timeout stall.

>  }
>  
>  int panthor_aw_resume(struct panthor_device *ptdev)
[ ... ]
> @@ -333,16 +332,15 @@ int panthor_aw_suspend(struct panthor_device *ptdev)
>  
>  	if (atomic_read(&aw->state) == PANTHOR_AW_STATE_READY)
>  		goto out_irq_suspend;
>  
>  	if (panthor_aw_state_try_set(aw, PANTHOR_AW_STATE_GPU_GRANTED,
>  		PANTHOR_AW_STATE_GPU_STOPPED))
>  		ret = panthor_aw_yield(aw);
>  	else
>  		ret = panthor_aw_state_wait(
>  			aw, PANTHOR_AW_STATE_READY,
>  			PANTHOR_AW_STATE_TRANSITION_TIMEOUT_MS);
>  
>  out_irq_suspend:
>  	panthor_aw_irq_suspend(&aw->irq);
>  	return ret;

[Severity: High]
Does this exit path omit flushing or canceling the background work? If a
message is sent just before or during device suspend, msg_retry_work could be
queued. 

Unlike in panthor_aw_unplug(), panthor_aw_suspend() does not call 
disable_work_sync(&aw->msg_retry_work) before the device transitions to a 
suspended state. Since aw->wq is not allocated with WQ_FREEZABLE, the work
can run after the device's clocks and power domains are disabled, and
accessing powered-off MMIO registers would cause a bus error (kernel panic).

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922204535.2850094-1-karunika.choo@arm.com?part=22