Re: [PATCH v2 20/28] drm/panthor: Handle partition control INVALID_COMMAND interrupt

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] drm/panthor: unassigned current_aw in window_open() breaks
  recovery
- [Medium] drm/panthor: stale last_cmd on PART_STATE_RESET in
  yield/close helpers
--

commit 94f33b60c028012f4fc4c4edbb429977e08e2557
Author: Karunika Choo <karunika.choo@arm.com>

drm/panthor: Handle partition control INVALID_COMMAND interrupt

This commit tracks the last sent partition command and handles
PART_INVALID_COMMAND interrupts by retrying rejected commands or
requesting scheduler recovery when the partition is open for a different
access window.

> diff --git a/drivers/gpu/drm/panthor/arbitration/panthor_partition_control.c b/drivers/gpu/drm/panthor/arbitration/panthor_partition_control.c
> index d8fdafbf31864..3b472403f49d1 100644
> --- a/drivers/gpu/drm/panthor/arbitration/panthor_partition_control.c
> +++ b/drivers/gpu/drm/panthor/arbitration/panthor_partition_control.c
[ ... ]
> @@ -128,8 +158,8 @@ static int yield_now(struct panthor_partition_control *pc)
>  	if (partition_state_get(pc) != PART_STATE_WINDOW_OPEN)
>  		return -EAGAIN;
>  
> -	gpu_write(pc->iomem, AM_PART_COMMAND,
> -		  AM_PART_SET_COMMAND(PART_CMD_YIELD_NOW));
> +	partition_command_write(pc, AM_PART_SET_COMMAND(PART_CMD_YIELD_NOW),
> +				pc->current_aw);
>  
>  	return 0;
>  }
> @@ -144,8 +174,8 @@ static int yield_idle(struct panthor_partition_control *pc)
>  	if (partition_state_get(pc) != PART_STATE_WINDOW_OPEN)
>  		return -EAGAIN;
>  
> -	gpu_write(pc->iomem, AM_PART_COMMAND,
> -		  AM_PART_SET_COMMAND(PART_CMD_YIELD_IDLE));
> +	partition_command_write(pc, AM_PART_SET_COMMAND(PART_CMD_YIELD_IDLE),
> +				pc->current_aw);
>  
>  	return 0;
>  }
> @@ -162,8 +192,8 @@ static int window_close(struct panthor_partition_control *pc)
>  
>  	pc->closing = true;
>  
> -	gpu_write(pc->iomem, AM_PART_COMMAND,
> -		  AM_PART_SET_COMMAND(PART_CMD_CLOSE_WINDOW));
> +	partition_command_write(pc, AM_PART_SET_COMMAND(PART_CMD_CLOSE_WINDOW),
> +				pc->current_aw);
>  
>  	return 0;
>  }

[Severity: Medium]
Can a stale PART_CMD_OPEN_WINDOW in pc->last_cmd reopen the partition
after yield_now(), yield_idle(), or window_close() has already returned 0?

window_open() writes PART_CMD_OPEN_WINDOW, stores it in pc->last_cmd, and
returns 0 without waiting for AM_PART_STATE to leave PART_STATE_RESET.  If
PART_CMD_OPEN_WINDOW is rejected while a previous reset is still finishing
(or before the hardware leaves PART_STATE_RESET) and the scheduler then
calls yield_now(), yield_idle(), or window_close(), those helpers see
partition_state_get(pc) == PART_STATE_RESET and return 0 without clearing
pc->last_cmd or scheduling retry_work:

drivers/gpu/drm/panthor/arbitration/panthor_partition_control.c:yield_now() {
	guard(spinlock_irqsave)(&pc->lock);

	if (partition_state_get(pc) == PART_STATE_RESET)
		return 0;

When partition_handle_invalid_cmd() later runs for the rejected open
command, pc->last_cmd.cmd is still PART_CMD_OPEN_WINDOW and the partition
is not in PART_STATE_WINDOW_OPEN, so it re-issues PART_CMD_OPEN_WINDOW and
opens the access window after the yield or close already succeeded.

> @@ -188,8 +218,8 @@ static int window_open(struct panthor_partition_control *pc, u8 aw_id)
>  		return -EBUSY;
>  	}
>  
> -	gpu_write(pc->iomem, AM_PART_COMMAND,
> -		  AM_PART_SET_COMMAND(PART_CMD_OPEN_WINDOW) | AM_PART_SET_WINDOW(aw_id));
> +	partition_command_write(pc, (AM_PART_SET_COMMAND(PART_CMD_OPEN_WINDOW) |
> +				     AM_PART_SET_WINDOW(aw_id)), aw_id);
>  
>  	pc->current_aw = aw_id;
>  
> @@ -201,10 +231,16 @@ static void partition_handle_reset_done(struct panthor_partition_control *pc)
>  	bool notify_stopped = false;
>  	int aw_id;
>  
> +	gpu_write(pc->iomem, AM_PART_IRQ_CLEAR, PART_RESET_DONE);
> +
>  	scoped_guard(spinlock_irqsave, &pc->lock) {
>  		aw_id = pc->current_aw;
>  		pc->current_aw = AW_UNASSIGNED;
>  
> +		/* invalidate last command */
> +		pc->last_cmd.aw_id = AW_UNASSIGNED;
> +		pc->last_cmd.cmd = 0;
> +
>  		/* RESET_DONE from CLOSE_WINDOW */
>  		if (pc->closing)
>  			pc->closing = false;
> @@ -216,6 +252,55 @@ static void partition_handle_reset_done(struct panthor_partition_control *pc)
>  		panthor_arbitration_on_stopped(dev_get_drvdata(pc->dev), aw_id);
>  }
>  
> +static void partition_handle_invalid_cmd(struct panthor_partition_control *pc)
> +{
> +	bool on_err = false;
> +	int aw_id;
> +
> +	gpu_write(pc->iomem, AM_PART_IRQ_CLEAR, PART_INVALID_COMMAND);
> +
> +	scoped_guard(spinlock_irqsave, &pc->lock) {
> +		if (!pc->last_cmd.cmd)
> +			return;
> +
> +		if (pc->last_cmd.aw_id < 0)
> +			return;

[Severity: Medium]
Does leaving pc->current_aw as AW_UNASSIGNED (-1) when window_open() finds
the partition in PART_STATE_WINDOW_OPEN prevent command retries and
scheduler recovery?

When window_open() sees PART_STATE_WINDOW_OPEN, it returns 0 (if
cur_aw == aw_id) or -EBUSY (if cur_aw != aw_id) before setting
pc->current_aw = aw_id:

drivers/gpu/drm/panthor/arbitration/panthor_partition_control.c:window_open() {
	if (partition_state_get(pc) == PART_STATE_WINDOW_OPEN) {
		u8 cur_aw = partition_aw_get(pc);

		if (cur_aw == aw_id)
			return 0;

		dev_warn(pc->dev,
			 "%s: OPEN_WINDOW(%u): Already opened for aw=%u, state=%u. Closing...",
			 pc->name, aw_id, cur_aw, partition_state_get(pc));

		return -EBUSY;
	}

On -EBUSY, arb_sched_grant_locked() queues sched->reset_work, which calls
arb_sched_force_close_locked() -> arb_sched_close_locked() ->
window_close().  Because pc->current_aw is still -1, window_close() (as
well as yield_now() and yield_idle() when cur_aw == aw_id) records
pc->last_cmd.aw_id = -1.  If that command raises PART_INVALID_COMMAND,
partition_handle_invalid_cmd() aborts at pc->last_cmd.aw_id < 0 without
retrying it.  Furthermore, when PART_RESET_DONE later fires,
partition_handle_reset_done() sees aw_id < 0 and skips
panthor_arbitration_on_stopped(), leaving the scheduler stuck in
ARB_SCHED_PHASE_LOST.

In addition, panthor_partition_control_term() calls
partition_irq_suspend() before window_close() and partition_state_wait(),
which masks PART_INVALID_COMMAND so a rejected close command cannot be
retried during teardown.

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