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 <[email protected]> > > --- > > 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 > > > >
