Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Data race on `ctx->reset_counter` when a context submits to multiple GPUs. --
--- Patch [2]: [PATCH v3 2/2] drm/etnaviv: Add GPU reset counters --- commit abb119dbbf6be05ed3e37091e5708fd834c6fb78 Author: Christian Gmeiner <[email protected]> drm/etnaviv: Add GPU reset counters This commit introduces two counters for tracking GPU resets: a per-GPU counter and a per-context counter, queryable via a new RESET_QUERY ioctl. Userspace can use these counters to determine if its context was guilty of a reset or just an innocent victim. > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c > b/drivers/gpu/drm/etnaviv/etnaviv_drv.c > index a27ed014fb4e7..5c50e5a244082 100644 > --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c > +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c [ ... ] > @@ -296,6 +296,32 @@ static int etnaviv_ioctl_get_param(struct drm_device > *dev, void *data, > return etnaviv_gpu_get_param(gpu, args->param, &args->value); > } > > +static int etnaviv_ioctl_reset_query(struct drm_device *dev, void *data, > + struct drm_file *file) > +{ > + struct etnaviv_drm_private *priv = dev->dev_private; > + struct etnaviv_file_private *ctx = file->driver_priv; > + struct drm_etnaviv_reset_query *args = data; > + struct etnaviv_gpu *gpu; > + > + if (args->flags) > + return -EINVAL; > + > + if (args->pipe >= ETNA_MAX_PIPES) > + return -EINVAL; > + > + gpu = priv->gpu[args->pipe]; > + if (!gpu) > + return -ENXIO; > + > + mutex_lock(&gpu->lock); > + args->global_reset_counter = gpu->reset_counter; > + args->context_reset_counter = ctx->reset_counter; [Severity: Medium] Will this read of ctx->reset_counter race with updates on other GPUs? Since ctx (struct etnaviv_file_private) can be shared across multiple GPU cores, reading it under a single gpu->lock leaves it exposed to concurrent updates from timeout handlers running on different GPUs. > + mutex_unlock(&gpu->lock); > + > + return 0; > +} [ ... ] > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h > b/drivers/gpu/drm/etnaviv/etnaviv_drv.h > index cba4323ae589c..9c348aa7f8d35 100644 > --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h > +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h > @@ -34,6 +34,7 @@ struct etnaviv_file_private { > int id; > struct etnaviv_iommu_context *mmu; > struct drm_sched_entity sched_entity[ETNA_MAX_PIPES]; > + u32 reset_counter; [Severity: Medium] Since this context counter is shared across multiple GPU cores and can be updated concurrently if jobs on different GPUs hang simultaneously, does this need to be an atomic_t? Using a plain u32 for concurrent accesses can lead to data races and lost updates. > }; [ ... ] > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c > b/drivers/gpu/drm/etnaviv/etnaviv_sched.c > index 139e6e38784b3..6e9b677122e3d 100644 > --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c > +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c > @@ -79,6 +79,11 @@ static enum drm_gpu_sched_stat > etnaviv_sched_timedout_job(struct drm_sched_job > if(sched_job) > drm_sched_increase_karma(sched_job); > > + mutex_lock(&gpu->lock); > + gpu->reset_counter++; > + submit->ctx->reset_counter++; [Severity: Medium] Could this increment of submit->ctx->reset_counter race with other timeout handlers? The gpu->lock only protects the specific GPU, but the ctx is shared across all pipes. If two GPUs hang simultaneously, two timeout handlers could run concurrently on different workqueues and race while incrementing this plain integer, causing lost updates. > + mutex_unlock(&gpu->lock); > + > /* get the GPU back into the init state */ > etnaviv_core_dump(submit); > etnaviv_gpu_recover_hang(submit); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
