Re: [PATCH v2 18/28] drm/panthor: Route arbitration events

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/panthor: unhandled RESET and current_aw states wedge
  arbitration
- [Medium] drm/panthor: dropped GPU_REQUEST in
  panthor_arbitration_sched_on_request()
--

commit 48a85078af75aa6fff27b7d739d44d8c4f545271
Author: Karunika Choo <karunika.choo@arm.com>

drm/panthor: Route arbitration events

This commit wires arbitration events between resource-group message handling,
the arbitration scheduler, and partition control.

> diff --git a/drivers/gpu/drm/panthor/arbitration/panthor_partition_control.c b/drivers/gpu/drm/panthor/arbitration/panthor_partition_control.c
> index 330d0bf5fd375..d8fdafbf31864 100644
> --- a/drivers/gpu/drm/panthor/arbitration/panthor_partition_control.c
> +++ b/drivers/gpu/drm/panthor/arbitration/panthor_partition_control.c
> @@ -198,13 +198,22 @@ static int window_open(struct panthor_partition_control *pc, u8 aw_id)
>  
>  static void partition_handle_reset_done(struct panthor_partition_control *pc)
>  {
> +	bool notify_stopped = false;
> +	int aw_id;
> +
>  	scoped_guard(spinlock_irqsave, &pc->lock) {
> +		aw_id = pc->current_aw;
>  		pc->current_aw = AW_UNASSIGNED;
>  
>  		/* RESET_DONE from CLOSE_WINDOW */
>  		if (pc->closing)
>  			pc->closing = false;
> +		if (aw_id >= 0)
> +			notify_stopped = true;
>  	}
> +
> +	if (notify_stopped)
> +		panthor_arbitration_on_stopped(dev_get_drvdata(pc->dev), aw_id);
>  }

[Severity: High]
Can relying solely on partition_handle_reset_done() with pc->current_aw >= 0
drop partition commands or wedge the arbitration scheduler in an infinite
close loop?

First, window_open() performs a posted write of PART_CMD_OPEN_WINDOW, sets
pc->current_aw = aw_id, and returns 0 before AM_PART_STATE leaves
PART_STATE_RESET. If the scheduler immediately follows up with a stop,
yield, or close command (for example when sched->disabled is set in
arb_sched_grant_locked(), or when another request is queued), yield_now()
and window_close() see partition_state_get(pc) == PART_STATE_RESET and
return 0 without issuing a command or returning -EAGAIN:

drivers/gpu/drm/panthor/arbitration/panthor_partition_control.c:window_close() {
	...
	if (partition_state_get(pc) == PART_STATE_RESET)
		return 0;

	if (partition_state_get(pc) != PART_STATE_WINDOW_OPEN)
		return -EAGAIN;
	...
}

Because 0 is returned while pc->current_aw >= 0, the command is silently
dropped instead of being retried once the window finishes opening.

Second, if partition_state_get(pc) is already PART_STATE_WINDOW_OPEN on
entry to window_open(), it returns 0 (when cur_aw == aw_id) or -EBUSY (which
triggers reset_work -> arb_sched_force_close_locked()) before setting
pc->current_aw = aw_id. When PART_RESET_DONE later fires, pc->current_aw is
still AW_UNASSIGNED, so partition_handle_reset_done() skips
panthor_arbitration_on_stopped().

Third, whenever the hardware partition is already in PART_STATE_RESET while
sched->active_aw_id >= 0 (such as after a skipped
panthor_arbitration_on_stopped() notification), arb_sched_close_locked()
calls window_close(), gets 0 back without any PART_CMD_CLOSE_WINDOW or
stopped notification, and starts close_timer. When close_timer expires,
reset_work calls arb_sched_force_close_locked() -> arb_sched_close_locked(),
repeating indefinitely and wedging GPU scheduling across all access windows.

> diff --git a/drivers/gpu/drm/panthor/arbitration/panthor_resource_group.c b/drivers/gpu/drm/panthor/arbitration/panthor_resource_group.c
> index e413cfd9e6d25..26b22461f6d39 100644
> --- a/drivers/gpu/drm/panthor/arbitration/panthor_resource_group.c
> +++ b/drivers/gpu/drm/panthor/arbitration/panthor_resource_group.c
> @@ -163,10 +163,10 @@ static void rg_handle_message(struct panthor_resource_group *rg, u8 aw_id,
>  
>  	switch (msg_id) {
>  	case VM_ARB_GPU_REQUEST:
> -		/* TODO: on_request */
> +		panthor_arbitration_on_request(dev_get_drvdata(rg->dev), aw_id);
>  		break;
>  	case VM_ARB_GPU_STOPPED:
> -		/* TODO: on_idle */
> +		panthor_arbitration_on_idle(dev_get_drvdata(rg->dev), aw_id);
>  		break;

[Severity: Medium]
Can a VM_ARB_GPU_REQUEST sent right after VM_ARB_GPU_STOPPED be rejected
with -EEXIST and lost while the partition is still closing?

Receiving VM_ARB_GPU_STOPPED calls panthor_arbitration_sched_on_idle(),
which sets sched->phase = ARB_SCHED_PHASE_CLOSING and starts an asynchronous
PART_CMD_CLOSE_WINDOW while keeping sched->active_aw_id == aw_id until
PART_RESET_DONE fires on the partition control IRQ. Because the access
window receives WINDOW_IRQ_WINDOW_CLOSED as soon as the window closes (prior
to PART_RESET_DONE) and the resource group and partition control use
separate interrupts, the access window can send a new VM_ARB_GPU_REQUEST
while sched->phase is still ARB_SCHED_PHASE_CLOSING.

In panthor_arbitration_sched_on_request(), only ARB_SCHED_PHASE_STOPPING is
exempted when checking whether the active access window may re-queue itself:

drivers/gpu/drm/panthor/arbitration/panthor_arbitration_sched.c:panthor_arbitration_sched_on_request() {
	...
	if (test_bit(aw_id, sched->queued_mask) ||
	    (sched->active_aw_id == aw_id &&
	     sched->phase != ARB_SCHED_PHASE_STOPPING)) {
		dev_warn(sched->dev, "%s: AW%u already in request queue.",
			 sched->name, aw_id);
		return -EEXIST;
	}
	...
}

The new request is dropped, and once PART_RESET_DONE completes, the
scheduler becomes idle without granting the GPU back to the access window,
stalling it until its request timeout expires. Allowing re-queuing when
sched->phase == ARB_SCHED_PHASE_CLOSING as well would avoid dropping the
request.

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