Re: [PATCH] drm/i915/gt: fix refcount underflow in intel_engine_park_heartbeat

[Andi Shyti <andi.shyti@kernel.org>]

Hi Sebastian,

...

> Fix this by replacing the non-atomic pointer read + separate clear with
> xchg() in both racing paths. xchg() is a single indivisible hardware
> instruction that atomically reads the old pointer and writes NULL. This
> guarantees only one of the two concurrent callers obtains the non-NULL
> pointer and performs the put, the other gets NULL and skips it.
> 
> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/15880
> Fixes: 058179e72e09 ("drm/i915/gt: Replace hangcheck by heartbeats")

Cc: <stable@vger.kernel.org> # v5.5+

> Signed-off-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
> ---
>  .../gpu/drm/i915/gt/intel_engine_heartbeat.c  | 26 +++++++++++++------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> index b279878dca29..a3830627ef81 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> @@ -148,10 +148,12 @@ static void heartbeat(struct work_struct *wrk)
>  	/* Just in case everything has gone horribly wrong, give it a kick */
>  	intel_engine_flush_submission(engine);
>  
> -	rq = engine->heartbeat.systole;
> -	if (rq && i915_request_completed(rq)) {
> -		i915_request_put(rq);
> -		engine->heartbeat.systole = NULL;
> +	rq = xchg(&engine->heartbeat.systole, NULL);
> +	if (rq) {
> +		if (i915_request_completed(rq))
> +			i915_request_put(rq);
> +		else
> +			engine->heartbeat.systole = rq;

Well spotted, Sebastian!

>  	}
>  
>  	if (!intel_engine_pm_get_if_awake(engine))
> @@ -232,8 +234,11 @@ static void heartbeat(struct work_struct *wrk)
>  unlock:
>  	mutex_unlock(&ce->timeline->mutex);
>  out:
> -	if (!engine->i915->params.enable_hangcheck || !next_heartbeat(engine))
> -		i915_request_put(fetch_and_zero(&engine->heartbeat.systole));
> +	if (!engine->i915->params.enable_hangcheck || !next_heartbeat(engine)) {
> +		rq = xchg(&engine->heartbeat.systole, NULL);
> +		if (rq)
> +			i915_request_put(rq);
> +	}
>  	intel_engine_pm_put(engine);
>  }
>  
> @@ -247,8 +252,13 @@ void intel_engine_unpark_heartbeat(struct intel_engine_cs *engine)
>  
>  void intel_engine_park_heartbeat(struct intel_engine_cs *engine)
>  {
> -	if (cancel_delayed_work(&engine->heartbeat.work))
> -		i915_request_put(fetch_and_zero(&engine->heartbeat.systole));
> +	struct i915_request *rq;

nit: this should go inside the if statement.

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Nice patch, Sebastian. Two very little nitpicks that I can take
care of before merging.

Thank you,
Andi

> +	if (cancel_delayed_work(&engine->heartbeat.work)) {
> +		rq = xchg(&engine->heartbeat.systole, NULL);
> +		if (rq)
> +			i915_request_put(rq);
> +	}
>  }
>  
>  void intel_gt_unpark_heartbeats(struct intel_gt *gt)
> -- 
> 2.52.0
>