Re: [PATCH v2 17/28] drm/panthor: Add arbitration scheduler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] drm/panthor: infinite reset loop in arb_sched_reset_work() on
  close failure
- [High] drm/panthor: dropped GPU request in
  panthor_arbitration_sched_on_request()
- [Medium] drm/panthor: unbounded retry loop in arb_sched_retry_work()
  on -EAGAIN
- [Medium] drm/panthor: timer re-armed by flush_workqueue() during
  scheduler stop
--

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

drm/panthor: Add arbitration scheduler

This commit adds a per-partition arbitration scheduler that queues GPU
requests from access windows, grants access in turn, and uses hrtimers and
workqueues to manage timeslices, yields, force-closes, and error recovery.

> diff --git a/drivers/gpu/drm/panthor/arbitration/panthor_arbitration_sched.c b/drivers/gpu/drm/panthor/arbitration/panthor_arbitration_sched.c
> new file mode 100644
> index 0000000000000..23768739e9ef5
> --- /dev/null
> +++ b/drivers/gpu/drm/panthor/arbitration/panthor_arbitration_sched.c
[ ... ]
> +static int __arb_sched_close_locked(struct panthor_arbitration_sched *sched)
> +{
> +	int ret;
> +
> +	lockdep_assert_held(&sched->lock);
> +
> +	ret = panthor_arbitration_on_close(to_adev(sched), sched->id);
> +	if (ret == -EAGAIN) {
> +		arb_sched_queue_retry_work(sched, PANTHOR_ARB_WORK_CLOSE);
> +	} else if (ret < 0) {
> +		dev_err(sched->dev, "%s: Failed to close AW%u. %d", sched->name,
> +			sched->active_aw_id, ret);
> +
> +		arb_sched_queue_work(sched, &sched->reset_work);
> +	}
> +
> +	return ret;
> +}
> +
> +static void arb_sched_close_locked(struct panthor_arbitration_sched *sched)
> +{
> +	int ret;
> +
> +	lockdep_assert_held(&sched->lock);
> +
> +	ret = __arb_sched_close_locked(sched);
> +	if (!ret) {
> +		sched->close_timer.epoch = sched->epoch;
> +		hrtimer_start(&sched->close_timer.timer,
> +			      ms_to_ktime(yield_timeout), HRTIMER_MODE_REL);
> +	}
> +}
[ ... ]
> +static void arb_sched_reset_work(struct work_struct *work)
> +{
> +	struct panthor_arbitration_work *w = to_work(work);
> +	struct panthor_arbitration_sched *sched =
> +		container_of(w, struct panthor_arbitration_sched, reset_work);
> +
> +	guard(spinlock_irqsave)(&sched->lock);
> +	if (sched->epoch != w->epoch)
> +		return;
> +
> +	if (sched->active_aw_id != w->aw_id)
> +		return;
> +
> +	dev_warn(sched->dev, "%s: scheduler reset. active_aw=%u",
> +			sched->name, sched->active_aw_id);
> +
> +	arb_sched_force_close_locked(sched);
> +}

[Severity: High]
Can arb_sched_reset_work() enter an infinite reset loop when closing an
access window fails or times out?

__arb_sched_close_locked() and panthor_arbitration_sched_on_close_timeout()
queue sched->reset_work to recover from close errors and close timeouts.
However, arb_sched_reset_work() does not retire the lease or advance
sched->epoch (note that arb_sched_on_stopped_locked() has an unused forward
declaration at the top of the file); it only calls
arb_sched_force_close_locked() -> arb_sched_close_locked().

If panthor_arbitration_on_close() returns a negative error other than
-EAGAIN, __arb_sched_close_locked() immediately re-queues sched->reset_work
in a tight loop on sched->wq, hanging flush_workqueue() during suspend or
teardown.  If panthor_arbitration_on_close() returns 0 when close_timer has
already timed out, arb_sched_close_locked() simply re-arms close_timer,
which expires and queues sched->reset_work again, wedging the scheduler in
ARB_SCHED_PHASE_LOST and starving all queued access windows.

> +static void arb_sched_retry_work(struct work_struct *work)
> +{
> +	struct panthor_arbitration_work *w = to_work(work);
> +	struct panthor_arbitration_sched *sched =
> +		container_of(w, struct panthor_arbitration_sched, retry_work);
> +	DECLARE_BITMAP(work_tmp, PANTHOR_ARB_WORK_COUNT);
> +
> +	guard(spinlock_irqsave)(&sched->lock);
> +	if (bitmap_empty(sched->pending_retry_work, PANTHOR_ARB_WORK_COUNT))
> +		return;
> +
> +	bitmap_copy(work_tmp, sched->pending_retry_work, PANTHOR_ARB_WORK_COUNT);
> +	bitmap_zero(sched->pending_retry_work, PANTHOR_ARB_WORK_COUNT);
> +
> +	if (sched->epoch != w->epoch || sched->active_aw_id != w->aw_id)
> +		return;
> +
> +	switch (sched->phase) {
> +	case ARB_SCHED_PHASE_STOPPING:
> +		if (test_bit(PANTHOR_ARB_WORK_STOP, work_tmp))
> +			arb_sched_stop_locked(sched);
> +		break;
> +	case ARB_SCHED_PHASE_CLOSING:
> +	case ARB_SCHED_PHASE_LOST:
> +		if (test_bit(PANTHOR_ARB_WORK_CLOSE, work_tmp))
> +			arb_sched_close_locked(sched);
> +		break;
> +	default:
> +		break;
> +	}
> +}

[Severity: Medium]
Can arb_sched_retry_work() spin indefinitely on sched->wq if
panthor_arbitration_on_stop() or panthor_arbitration_on_close() continues to
return -EAGAIN?

When __arb_sched_stop_locked() or __arb_sched_close_locked() gets -EAGAIN,
stop_timer and close_timer are not started, and sched->retry_work is queued
immediately with no delay or retry limit.  If the partition remains in a
state that returns -EAGAIN, arb_sched_retry_work() clears pending_retry_work
and re-queues itself in an unthrottled busy loop with no running timer to
force recovery, which also prevents flush_workqueue(sched->wq) from
completing in panthor_arbitration_sched_stop().

[ ... ]
> +int panthor_arbitration_sched_on_request(struct panthor_arbitration_sched *sched, u8 aw_id)
> +{
> +	guard(spinlock_irqsave)(&sched->lock);
> +
> +	if (aw_id >= AM_ARB_MAX_AW_COUNT)
> +		return -EINVAL;
> +
> +	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;
> +	}

[Severity: High]
Does panthor_arbitration_sched_on_request() drop valid GPU requests that
arrive while the active access window is closing?

When the active access window yields via panthor_arbitration_sched_on_idle()
(or is force-closed), sched->phase moves to ARB_SCHED_PHASE_CLOSING (or
ARB_SCHED_PHASE_LOST) while sched->active_aw_id remains set until the
partition reset completes and calls panthor_arbitration_sched_on_stopped().
Because the duplicate check only exempts ARB_SCHED_PHASE_STOPPING, a new
request from that access window during ARB_SCHED_PHASE_CLOSING or
ARB_SCHED_PHASE_LOST is rejected with -EEXIST and never queued, leaving the
access window unscheduled once panthor_arbitration_sched_on_stopped() clears
active_aw_id.

[ ... ]
> +int panthor_arbitration_sched_stop(struct panthor_arbitration_sched *sched)
> +{
> +	int ret;
> +
> +	arb_sched_request_stop(sched);
> +
> +	/*
> +	 * Provisionally waiting for up to 2x yield_timeout, but path may
> +	 * include time taken for panthor_arbitration_on_grant() to complete
> +	 * if sched_request_stop() is called while in the GRANTING state.
> +	 *
> +	 * Realistically, if granting takes more than yield_timeout, HW is in
> +	 * bad state either way. Best continue and stop the scheduler.
> +	 */
> +	ret = arb_sched_wait_phase(sched, ARB_SCHED_PHASE_IDLE,
> +				   panthor_arbitration_stop_timeout(sched));
> +	if (ret) {
> +		/*
> +		 * TODO: handle the timeout here by escalating the error when
> +		 * reset escalation is supported.
> +		 */
> +		ret = arb_sched_force_close(sched);
> +	}
> +
> +	/* Should no longer have any further activity. Cancel timer. */
> +	hrtimer_cancel(&sched->grant_timer.timer);
> +	hrtimer_cancel(&sched->stop_timer.timer);
> +	hrtimer_cancel(&sched->close_timer.timer);
> +	flush_workqueue(sched->wq);

[Severity: Medium]
Can flush_workqueue(sched->wq) re-arm stop_timer or close_timer after the
timers have already been cancelled?

hrtimer_cancel() runs before flush_workqueue(sched->wq).  If
sched->reset_work or sched->retry_work is pending (for example, queued by
panthor_arbitration_sched_on_close_timeout() during arb_sched_force_close()),
flushing sched->wq runs arb_sched_reset_work() or arb_sched_retry_work(),
which calls arb_sched_close_locked() or arb_sched_stop_locked() and starts
close_timer or stop_timer again, leaving an active hrtimer after
panthor_arbitration_sched_stop() returns.

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