Re: [PATCH v3 5/6] drm/panthor: Add support for entering and exiting protected mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Polling for protected mode entry using `wait_event_timeout` without a wakeup source will unconditionally sleep for the full 500ms timeout.
- [High] Acknowledging a pending protected mode event during a slot reset does not clear the corresponding software state, causing a permanent malfunction for the group.
- [High] TOCTOU race in `panthor_gpu_protm_fault_pending` leads to missed faults and an infinite reset loop.
--

commit 134f4af77e10adece5a3a20d7839a25e0c7a720d
Author: Florent Tomasin <florent.tomasin@arm.com>

drm/panthor: Add support for entering and exiting protected mode

This commit modifies the Panthor driver to handle GPU hardware protected mode
enter and exit operations. It introduces the mechanisms needed for entering
and exiting protected mode, handling protected mode IRQs, scheduling
modifications, and GPU fault handling during protected execution.

> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> index 5f9f7a92c56a8..96770ce34da84 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
[ ... ]
> @@ -1476,6 +1481,91 @@ static void panthor_fw_ping_work(struct work_struct *work)
>  	}
>  }
>  
> +static bool wait_protm_enter(struct panthor_device *ptdev,
> +			     long long enter_count)
> +{
> +	return (panthor_gpu_status(ptdev) & GPU_STATUS_PROTM_ACTIVE) ||
> +	       (atomic64_read(&ptdev->protm.protm_exit_count) >= enter_count);
> +}
> +
> +int panthor_fw_protm_enter(struct panthor_device *ptdev)
> +{
[ ... ]
> +	/* Poll for the entry of protected mode.
> +	 * It is possible that GPU_STATUS_PROTM_ACTIVE is set and cleared
> +	 * before we check it below, so we must also check for GLB_PROTM_EXIT.
> +	 * GLB_PROTM_EXIT can not be checked directly, because this could also
> +	 * be handled and clear before we check below. We count number of
> +	 * protm enters and exits to safely handle that case.
> +	 */
> +	ret = wait_event_timeout(ptdev->fw->req_waitqueue,
> +				 wait_protm_enter(ptdev, enter_count),
> +				 msecs_to_jiffies(500));

[Severity: High]
Since GPU_STATUS_PROTM_ACTIVE is a hardware status bit and does not trigger a 
new interrupt when it becomes true, wouldn't this wait queue never be woken 
up? If the bit isn't set immediately on the first check in wait_protm_enter(), 
it seems like this will unconditionally sleep for the full 500ms duration 
before returning success, causing a half-second delay every time the GPU 
enters protected mode.

> +	if (!ret) {
> +		drm_err(&ptdev->base,
> +			"Wait for GPU protected mode enter timed out");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}

> diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
> index a383b04f101ed..27617f3a72394 100644
> --- a/drivers/gpu/drm/panthor/panthor_gpu.c
> +++ b/drivers/gpu/drm/panthor/panthor_gpu.c
[ ... ]
> @@ -123,6 +152,13 @@ static irqreturn_t panthor_gpu_irq_threaded_handler(int irq, void *data)
>  	return panthor_irq_default_threaded_handler(data, panthor_gpu_irq_handler);
>  }
>  
> +bool panthor_gpu_protm_fault_pending(struct panthor_device *ptdev)
> +{
> +	return atomic_read(&ptdev->gpu->protm_fault) ||
> +	       gpu_read(ptdev->gpu->irq.iomem, INT_RAWSTAT) &
> +		       GPU_IRQ_PROTM_FAULT;
> +}

[Severity: High]
Could there be a race condition here due to the short-circuit evaluation?

If CPU 0 evaluates the left side and reads the software flag 
ptdev->gpu->protm_fault as 0, and is then preempted, CPU 1 could execute the 
interrupt handler, set the software flag to 1, and clear the hardware 
register. 

When CPU 0 resumes to evaluate the right side, it would read the hardware 
register as 0 and return false. This would cause the fault to be missed 
entirely (e.g., in sched_process_protm_exit_event_locked()), potentially 
allowing a faulty protected mode group to restart and trigger an infinite 
reset loop.

> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 0c8ea07fc7b9d..1fe77e5c41995 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
[ ... ]
> @@ -1149,12 +1170,14 @@ cs_slot_reset_locked(struct panthor_device *ptdev, u32 csg_id, u32 cs_id)
>  	struct panthor_fw_cs_iface *cs_iface = panthor_fw_get_cs_iface(ptdev, csg_id, cs_id);
>  	struct panthor_group *group = ptdev->scheduler->csg_slots[csg_id].group;
>  	struct panthor_queue *queue = group->queues[cs_id];
> +	u32 val, mask;
>  
>  	lockdep_assert_held(&ptdev->scheduler->lock);
>  
> -	panthor_fw_update_reqs(cs_iface, req,
> -			       CS_STATE_STOP,
> -			       CS_STATE_MASK);
> +	val = CS_STATE_STOP | (cs_iface->output->ack & CS_PROTM_PENDING);
> +	mask = CS_STATE_MASK | CS_PROTM_PENDING;
> +
> +	panthor_fw_update_reqs(cs_iface, req, val, mask);

[Severity: High]
When acknowledging the pending CS_PROTM_PENDING hardware event here during a 
slot reset, the software state group->protm_pending_queues is not cleared.

Later, tick_ctx_handle_protm_group() only clears bits from 
group->protm_pending_queues if the hardware event is still pending via 
(ack ^ req) & CS_PROTM_PENDING. Since the hardware event was already cleared 
here, will the software bit remain permanently set, causing the group to 
perpetually request entry into protected mode?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911114014.79139-1-ketil.johnsen@arm.com?part=5