Re: [PATCH 4/4] drm/imagination: Access FW initialised state with READ/WRITE_ONCE

[Brajesh Gupta <Brajesh.Gupta@imgtec.com>]

On Mon, 2026-05-18 at 15:02 +0000, Matt Coster wrote:
> Hi Brajesh,
> 
> On 12/05/2026 07:47, Brajesh Gupta wrote:
> > Update FW initialised state shared resource access with READ/WRITE_ONCE
> > to prevent any complier optimization and ensure atomicity of operation.
> 
> We're not trying to prevent _any_ compiler optimisations, there are
> specific ones that READ_ONCE() prevents. Can you please include an
> explanation of exactly what we're trying to avoid here so future readers
> can understand the motivation of this change?
> 
> My understanding is that (for instance in one case) it's to prevent the
> compiler from assuming it only needs to read the value of
> fw_dev->initialised once outside the loop instead of on every iteration
> (grep "merge successive loads" in [1] for details).
> 
Updated. Please check.

> One further thought after skimming[1]: do we actually need to use
> READ/WRITE_ONCE() on _every_ read/write of ->initialised? Or can/should
> we just use it in critical cases (like the loop body example mentioned
> above)?
> 
Agree that we don't need READ/WRITE_ONCE for every use but kept it to make
shared variable and synchronisation explicit.

> Cheers,
> Matt
> 
> [1]: https://www.kernel.org/doc/html/latest/core-api/wrappers/memory-barriers.html
> 
> > 
> > Signed-off-by: Brajesh Gupta <brajesh.gupta@imgtec.com>
> > ---
> >  drivers/gpu/drm/imagination/pvr_device.c |  2 +-
> >  drivers/gpu/drm/imagination/pvr_fw.c     |  4 ++--
> >  drivers/gpu/drm/imagination/pvr_mmu.c    |  2 +-
> >  drivers/gpu/drm/imagination/pvr_power.c  | 10 +++++-----
> >  4 files changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
> > index 49696101b547..2691ef9af0ca 100644
> > --- a/drivers/gpu/drm/imagination/pvr_device.c
> > +++ b/drivers/gpu/drm/imagination/pvr_device.c
> > @@ -213,7 +213,7 @@ static irqreturn_t pvr_device_irq_thread_handler(int irq, void *data)
> >         while (pvr_fw_irq_pending(pvr_dev)) {
> >                 pvr_fw_irq_clear(pvr_dev);
> > 
> > -               if (pvr_dev->fw_dev.initialised) {
> > +               if (READ_ONCE(pvr_dev->fw_dev.initialised)) {
> >                         pvr_fwccb_process(pvr_dev);
> >                         pvr_kccb_wake_up_waiters(pvr_dev);
> >                         pvr_device_process_active_queues(pvr_dev);
> > diff --git a/drivers/gpu/drm/imagination/pvr_fw.c b/drivers/gpu/drm/imagination/pvr_fw.c
> > index b8ad3f1d222c..850a3ec8e775 100644
> > --- a/drivers/gpu/drm/imagination/pvr_fw.c
> > +++ b/drivers/gpu/drm/imagination/pvr_fw.c
> > @@ -1004,7 +1004,7 @@ pvr_fw_init(struct pvr_device *pvr_dev)
> >                 goto err_fw_stop;
> >         }
> > 
> > -       fw_dev->initialised = true;
> > +       WRITE_ONCE(fw_dev->initialised, true);
> > 
> >         return 0;
> > 
> > @@ -1044,7 +1044,7 @@ pvr_fw_fini(struct pvr_device *pvr_dev)
> >  {
> >         struct pvr_fw_device *fw_dev = &pvr_dev->fw_dev;
> > 
> > -       fw_dev->initialised = false;
> > +       WRITE_ONCE(fw_dev->initialised, false);
> > 
> >         pvr_fw_destroy_structures(pvr_dev);
> >         pvr_fw_object_unmap_and_destroy(pvr_dev->kccb.rtn_obj);
> > diff --git a/drivers/gpu/drm/imagination/pvr_mmu.c b/drivers/gpu/drm/imagination/pvr_mmu.c
> > index e9fefcc4e234..3cac482e1034 100644
> > --- a/drivers/gpu/drm/imagination/pvr_mmu.c
> > +++ b/drivers/gpu/drm/imagination/pvr_mmu.c
> > @@ -134,7 +134,7 @@ int pvr_mmu_flush_exec(struct pvr_device *pvr_dev, bool wait)
> >                 return -EIO;
> > 
> >         /* Can't flush MMU if the firmware hasn't been initialised yet. */
> > -       if (!pvr_dev->fw_dev.initialised)
> > +       if (!READ_ONCE(pvr_dev->fw_dev.initialised))
> >                 goto err_drm_dev_exit;
> > 
> >         cmd_mmu_cache_data->cache_flags =
> > diff --git a/drivers/gpu/drm/imagination/pvr_power.c b/drivers/gpu/drm/imagination/pvr_power.c
> > index a73a6815306b..0ed9e7be604b 100644
> > --- a/drivers/gpu/drm/imagination/pvr_power.c
> > +++ b/drivers/gpu/drm/imagination/pvr_power.c
> > @@ -216,7 +216,7 @@ pvr_watchdog_worker(struct work_struct *work)
> >         if (pm_runtime_get_if_in_use(from_pvr_device(pvr_dev)->dev) <= 0)
> >                 goto out_requeue;
> > 
> > -       if (!pvr_dev->fw_dev.initialised)
> > +       if (!READ_ONCE(pvr_dev->fw_dev.initialised))
> >                 goto out_pm_runtime_put;
> > 
> >         stalled = pvr_watchdog_kccb_stalled(pvr_dev);
> > @@ -378,7 +378,7 @@ pvr_power_device_suspend(struct device *dev)
> >         if (!drm_dev_enter(drm_dev, &idx))
> >                 return -EIO;
> > 
> > -       if (pvr_dev->fw_dev.initialised) {
> > +       if (READ_ONCE(pvr_dev->fw_dev.initialised)) {
> >                 err = pvr_power_fw_disable(pvr_dev, false);
> >                 if (err)
> >                         goto err_drm_dev_exit;
> > @@ -408,7 +408,7 @@ pvr_power_device_resume(struct device *dev)
> >         if (err)
> >                 goto err_drm_dev_exit;
> > 
> > -       if (pvr_dev->fw_dev.initialised) {
> > +       if (READ_ONCE(pvr_dev->fw_dev.initialised)) {
> >                 err = pvr_power_fw_enable(pvr_dev);
> >                 if (err)
> >                         goto err_power_off;
> > @@ -548,7 +548,7 @@ pvr_power_reset(struct pvr_device *pvr_dev, bool hard_reset)
> >                 err = pvr_power_fw_disable(pvr_dev, hard_reset, false);
> >                 if (!err) {
> >                         if (hard_reset) {
> > -                               pvr_dev->fw_dev.initialised = false;
> > +                               WRITE_ONCE(pvr_dev->fw_dev.initialised, false);
> >                                 WARN_ON(pvr_power_device_suspend(from_pvr_device(pvr_dev)->dev));
> > 
> >                                 err = pvr_fw_hard_reset(pvr_dev);
> > @@ -556,7 +556,7 @@ pvr_power_reset(struct pvr_device *pvr_dev, bool hard_reset)
> >                                         goto err_device_lost;
> > 
> >                                 err = pvr_power_device_resume(from_pvr_device(pvr_dev)->dev);
> > -                               pvr_dev->fw_dev.initialised = true;
> > +                               WRITE_ONCE(pvr_dev->fw_dev.initialised, true);
> >                                 if (err)
> >                                         goto err_device_lost;
> >                         } else {
> > 
> > --
> > 2.43.0
> > 
> 
>