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

From: Brajesh Gupta

Date: Tue May 19 2026 - 04:36:01 EST

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@xxxxxxxxxx>
> > ---
> > 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
> >
>
>