On Wed, Aug 26, 2026 at 04:56:02PM +0200, Boris Brezillon wrote: > Now that the reset logic has been reworked to use disable/enable_work(), > there's no need for panthor_device::pm::state to be an atomic. It can > simply be accessed under the same lock we use to touch MMIO mappings. > > While at it, rename the lock to make it clear it protects more than just > the MMIO logic, and transition locked sections to scoped_guard(). > > Signed-off-by: Boris Brezillon <[email protected]> > --- > drivers/gpu/drm/panthor/panthor_device.c | 97 > +++++++++++++++++--------------- > drivers/gpu/drm/panthor/panthor_device.h | 16 ++++-- > 2 files changed, 63 insertions(+), 50 deletions(-) > > diff --git a/drivers/gpu/drm/panthor/panthor_device.c > b/drivers/gpu/drm/panthor/panthor_device.c > index 2974f4bc0bb1..133e3895cd0a 100644 > --- a/drivers/gpu/drm/panthor/panthor_device.c > +++ b/drivers/gpu/drm/panthor/panthor_device.c > @@ -147,8 +147,10 @@ static void panthor_device_reset_work(struct work_struct > *work) > /* If the device is entering suspend, we don't reset. A slow reset will > * be forced at resume time instead. > */ > - if (atomic_read(&ptdev->pm.state) != PANTHOR_DEVICE_PM_STATE_ACTIVE) > - return; > + scoped_guard(mutex, &ptdev->pm.lock) { > + if (ptdev->pm.state != PANTHOR_DEVICE_PM_STATE_ACTIVE) > + return; > + } > > if (!drm_dev_enter(&ptdev->base, &cookie)) > return; > @@ -204,7 +206,7 @@ int panthor_device_init(struct panthor_device *ptdev) > if (ret) > return ret; > > - ret = drmm_mutex_init(&ptdev->base, &ptdev->pm.mmio_lock); > + ret = drmm_mutex_init(&ptdev->base, &ptdev->pm.lock); > if (ret) > return ret; > > @@ -213,7 +215,7 @@ int panthor_device_init(struct panthor_device *ptdev) > INIT_LIST_HEAD(&ptdev->gems.node); > #endif > > - atomic_set(&ptdev->pm.state, PANTHOR_DEVICE_PM_STATE_SUSPENDED); > + ptdev->pm.state = PANTHOR_DEVICE_PM_STATE_SUSPENDED; > p = alloc_page(GFP_KERNEL | __GFP_ZERO); > if (!p) > return -ENOMEM; > @@ -432,40 +434,39 @@ static vm_fault_t panthor_mmio_vm_fault(struct vm_fault > *vmf) > { > struct vm_area_struct *vma = vmf->vma; > struct panthor_device *ptdev = vma->vm_private_data; > - u64 offset = (u64)vma->vm_pgoff << PAGE_SHIFT; > - unsigned long pfn; > - pgprot_t pgprot; > vm_fault_t ret; > - bool active; > int cookie; > > if (!drm_dev_enter(&ptdev->base, &cookie)) > return VM_FAULT_SIGBUS; > > - mutex_lock(&ptdev->pm.mmio_lock); > - active = atomic_read(&ptdev->pm.state) == > PANTHOR_DEVICE_PM_STATE_ACTIVE; > + scoped_guard(mutex, &ptdev->pm.lock) { > + bool active = ptdev->pm.state == PANTHOR_DEVICE_PM_STATE_ACTIVE; > + u64 offset = (u64)vma->vm_pgoff << PAGE_SHIFT; > + unsigned long pfn; > + pgprot_t pgprot; > > - switch (offset) { > - case DRM_PANTHOR_USER_FLUSH_ID_MMIO_OFFSET: > + switch (offset) { > + case DRM_PANTHOR_USER_FLUSH_ID_MMIO_OFFSET: > + if (active) > + pfn = __phys_to_pfn(ptdev->phys_addr + > CSF_GPU_LATEST_FLUSH_ID); > + else > + pfn = page_to_pfn(ptdev->pm.dummy_latest_flush); > + break; > + > + default: > + ret = VM_FAULT_SIGBUS; > + goto out_dev_exit; > + } > + > + pgprot = vma->vm_page_prot; > if (active) > - pfn = __phys_to_pfn(ptdev->phys_addr + > CSF_GPU_LATEST_FLUSH_ID); > - else > - pfn = page_to_pfn(ptdev->pm.dummy_latest_flush); > - break; > + pgprot = pgprot_noncached(pgprot); > > - default: > - ret = VM_FAULT_SIGBUS; > - goto out_unlock; > + ret = vmf_insert_pfn_prot(vma, vmf->address, pfn, pgprot); > } > > - pgprot = vma->vm_page_prot; > - if (active) > - pgprot = pgprot_noncached(pgprot); > - > - ret = vmf_insert_pfn_prot(vma, vmf->address, pfn, pgprot); > - > -out_unlock: > - mutex_unlock(&ptdev->pm.mmio_lock); > +out_dev_exit: > drm_dev_exit(cookie); > return ret; > } > @@ -526,10 +527,13 @@ int panthor_device_resume(struct device *dev) > struct panthor_device *ptdev = dev_get_drvdata(dev); > int ret, cookie; > > - if (atomic_read(&ptdev->pm.state) != PANTHOR_DEVICE_PM_STATE_SUSPENDED) > - return -EINVAL; > + scoped_guard(mutex, &ptdev->pm.lock) { > + if (ptdev->pm.state != PANTHOR_DEVICE_PM_STATE_SUSPENDED) > + return -EINVAL; > + > + ptdev->pm.state = PANTHOR_DEVICE_PM_STATE_RESUMING; > + } > > - atomic_set(&ptdev->pm.state, PANTHOR_DEVICE_PM_STATE_RESUMING); > > ret = clk_prepare_enable(ptdev->clks.core); > if (ret) > @@ -574,11 +578,11 @@ int panthor_device_resume(struct device *dev) > * are removed and the real iomem mapping will be restored on next > * access. > */ > - mutex_lock(&ptdev->pm.mmio_lock); > + mutex_lock(&ptdev->pm.lock); > unmap_mapping_range(ptdev->base.anon_inode->i_mapping, > DRM_PANTHOR_USER_MMIO_OFFSET, 0, 1); > - atomic_set(&ptdev->pm.state, PANTHOR_DEVICE_PM_STATE_ACTIVE); > - mutex_unlock(&ptdev->pm.mmio_lock); > + ptdev->pm.state = PANTHOR_DEVICE_PM_STATE_ACTIVE; > + mutex_unlock(&ptdev->pm.lock);
Can this also use scoped_guard()? Otherwise, this looks good to me! Reviewed-by: Liviu Dudau <[email protected]> Best regards, Liviu > > /* Now that everything is resumed, we can re-enable the reset work. */ > enable_resets(ptdev); > @@ -595,7 +599,9 @@ int panthor_device_resume(struct device *dev) > clk_disable_unprepare(ptdev->clks.core); > > err_set_suspended: > - atomic_set(&ptdev->pm.state, PANTHOR_DEVICE_PM_STATE_SUSPENDED); > + scoped_guard(mutex, &ptdev->pm.lock) > + ptdev->pm.state = PANTHOR_DEVICE_PM_STATE_SUSPENDED; > + > atomic_set(&ptdev->pm.recovery_needed, 1); > return ret; > } > @@ -605,21 +611,21 @@ int panthor_device_suspend(struct device *dev) > struct panthor_device *ptdev = dev_get_drvdata(dev); > int cookie; > > - if (atomic_read(&ptdev->pm.state) != PANTHOR_DEVICE_PM_STATE_ACTIVE) > - return -EINVAL; > - > /* Clear all IOMEM mappings pointing to this device before we > * shutdown the power-domain and clocks. Failing to do that results > * in external aborts when the process accesses the iomem region. > * We change the state and call unmap_mapping_range() with the > - * mmio_lock held to make sure the vm_fault handler won't set up > + * lock held to make sure the vm_fault handler won't set up > * invalid mappings. > */ > - mutex_lock(&ptdev->pm.mmio_lock); > - atomic_set(&ptdev->pm.state, PANTHOR_DEVICE_PM_STATE_SUSPENDING); > - unmap_mapping_range(ptdev->base.anon_inode->i_mapping, > - DRM_PANTHOR_USER_MMIO_OFFSET, 0, 1); > - mutex_unlock(&ptdev->pm.mmio_lock); > + scoped_guard(mutex, &ptdev->pm.lock) { > + if (ptdev->pm.state != PANTHOR_DEVICE_PM_STATE_ACTIVE) > + return -EINVAL; > + > + ptdev->pm.state = PANTHOR_DEVICE_PM_STATE_SUSPENDING; > + unmap_mapping_range(ptdev->base.anon_inode->i_mapping, > + DRM_PANTHOR_USER_MMIO_OFFSET, 0, 1); > + } > > /* Make sure we're not interrupted by resets after that point > * until the GPU is resumed. > @@ -644,6 +650,9 @@ int panthor_device_suspend(struct device *dev) > clk_disable_unprepare(ptdev->clks.coregroup); > clk_disable_unprepare(ptdev->clks.stacks); > clk_disable_unprepare(ptdev->clks.core); > - atomic_set(&ptdev->pm.state, PANTHOR_DEVICE_PM_STATE_SUSPENDED); > + > + scoped_guard(mutex, &ptdev->pm.lock) > + ptdev->pm.state = PANTHOR_DEVICE_PM_STATE_SUSPENDED; > + > return 0; > } > diff --git a/drivers/gpu/drm/panthor/panthor_device.h > b/drivers/gpu/drm/panthor/panthor_device.h > index f55baa21b25e..217eec811bdb 100644 > --- a/drivers/gpu/drm/panthor/panthor_device.h > +++ b/drivers/gpu/drm/panthor/panthor_device.h > @@ -298,18 +298,22 @@ struct panthor_device { > > /** @pm: Power management related data. */ > struct { > - /** @state: Power state. */ > - atomic_t state; > + /** > + * @state: Power state. > + * > + * Must be accessed with the panthor_device::pm::lock held. > + */ > + enum panthor_device_pm_state state; > > /** > - * @mmio_lock: Lock protecting MMIO userspace CPU mappings. > + * @lock: Lock protecting PM related fields. > * > * This is needed to ensure we map the dummy IO pages when > * the device is being suspended, and the real IO pages when > - * the device is being resumed. We can't just do with the > - * state atomicity to deal with this race. > + * the device is being resumed. We can't just do with an > + * atomic state to deal with this race. > */ > - struct mutex mmio_lock; > + struct mutex lock; > > /** > * @dummy_latest_flush: Dummy LATEST_FLUSH page. > > -- > 2.55.0 > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯
