[PATCH 1/2] drm/v3d: Disable preemption while updating GPU stats

[Maíra Canal]

Hi Tvrtko,

On 8/12/24 06:12, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
> 
> We forgot to disable preemption around the write_seqcount_begin/end() pair
> while updating GPU stats:
> 
>    [ ] WARNING: CPU: 2 PID: 12 at include/linux/seqlock.h:221 __seqprop_assert.isra.0+0x128/0x150 [v3d]
>    [ ] Workqueue: v3d_bin drm_sched_run_job_work [gpu_sched]
>   <...snip...>
>    [ ] Call trace:
>    [ ]  __seqprop_assert.isra.0+0x128/0x150 [v3d]
>    [ ]  v3d_job_start_stats.isra.0+0x90/0x218 [v3d]
>    [ ]  v3d_bin_job_run+0x23c/0x388 [v3d]
>    [ ]  drm_sched_run_job_work+0x520/0x6d0 [gpu_sched]
>    [ ]  process_one_work+0x62c/0xb48
>    [ ]  worker_thread+0x468/0x5b0
>    [ ]  kthread+0x1c4/0x1e0
>    [ ]  ret_from_fork+0x10/0x20
> 
> Fix it.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
> Fixes: 6abe93b621ab ("drm/v3d: Fix race-condition between sysfs/fdinfo and interrupt handler")
> Cc: Maíra Canal <mcanal at igalia.com>
> Cc: <stable at vger.kernel.org> # v6.10+
> ---
>   drivers/gpu/drm/v3d/v3d_sched.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
> index 42d4f4a2dba2..cc2e5a89467b 100644
> --- a/drivers/gpu/drm/v3d/v3d_sched.c
> +++ b/drivers/gpu/drm/v3d/v3d_sched.c
> @@ -136,6 +136,8 @@ v3d_job_start_stats(struct v3d_job *job, enum v3d_queue queue)
>   	struct v3d_stats *local_stats = &file->stats[queue];
>   	u64 now = local_clock();
>   
> +	preempt_disable();
> +
>   	write_seqcount_begin(&local_stats->lock);
>   	local_stats->start_ns = now;
>   	write_seqcount_end(&local_stats->lock);
> @@ -143,6 +145,8 @@ v3d_job_start_stats(struct v3d_job *job, enum v3d_queue queue)
>   	write_seqcount_begin(&global_stats->lock);
>   	global_stats->start_ns = now;
>   	write_seqcount_end(&global_stats->lock);
> +
> +	preempt_enable();
>   }
>   
>   static void
> @@ -164,8 +168,10 @@ v3d_job_update_stats(struct v3d_job *job, enum v3d_queue queue)
>   	struct v3d_stats *local_stats = &file->stats[queue];
>   	u64 now = local_clock();
>   
> +	preempt_disable();
>   	v3d_stats_update(local_stats, now);
>   	v3d_stats_update(global_stats, now);
> +	preempt_enable();

Although `v3d_stats_update()` is only used here, shouldn't we move 
`preempt_disable()` and `preempt_enable()` inside of the
`v3d_stats_update()` function? This way, we can guarantee that any
future uses will disable preemption.

With that, you can add my:

Acked-by: Maíra Canal <mcanal at igalia.com>

Best Regards,
- Maíra

>   }
>   
>   static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job)