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). 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)? 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 > -- Matt Coster E: [email protected]
OpenPGP_signature.asc
Description: OpenPGP digital signature
