Re: [PATCH v2 04/14] drm/msm: order the display snapshot sources around its worker

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm: unsynchronized fault handler update in msm_mmu_set_fault_handler()
--

commit 9dc038a2fc1d880292f387fc89c8359b97d9cf93
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/msm: order the display snapshot sources around its worker

This commit reorders the registration and teardown of the display IOMMU
fault handler and display snapshot worker in msm_drm_kms_init() and
msm_drm_kms_uninit() so that snapshot sources are only active while the
KMS and its dump worker are initialized.

> diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
> index f3e39c3907a93..a94d2888a21e5 100644
> --- a/drivers/gpu/drm/msm/msm_kms.c
> +++ b/drivers/gpu/drm/msm/msm_kms.c
[ ... ]
> @@ -245,6 +241,8 @@ void msm_drm_kms_uninit(struct device *dev)
>  
>  	BUG_ON(!kms);
>  
> +	msm_mmu_set_fault_handler(to_msm_vm(kms->vm)->mmu, kms, NULL);
> +
>  	/* We must cancel and cleanup any pending vblank enable/disable
>  	 * work before msm_irq_uninstall() to avoid work re-enabling an
>  	 * irq after uninstall has disabled it.
> @@ -256,12 +254,12 @@ void msm_drm_kms_uninit(struct device *dev)
>  
>  	drm_kms_helper_poll_fini(ddev);
>  
> -	msm_disp_snapshot_destroy(ddev);
> -
>  	pm_runtime_get_sync(dev);
>  	msm_irq_uninstall(ddev);
>  	pm_runtime_put_sync(dev);
>  
> +	msm_disp_snapshot_destroy(ddev);
> +
>  	if (kms && kms->funcs)
>  		kms->funcs->destroy(kms);
>  }
> @@ -339,6 +337,8 @@ int msm_drm_kms_init(struct device *dev, const struct drm_driver *drv)
>  
>  	drm_mode_config_reset(ddev);
>  
> +	msm_mmu_set_fault_handler(to_msm_vm(kms->vm)->mmu, kms, msm_kms_fault_handler);

[Severity: High]
Can msm_mmu_set_fault_handler() in drivers/gpu/drm/msm/msm_kms.c race with
an in-flight display IOMMU fault during msm_drm_kms_init() or
msm_drm_kms_uninit()?

In drivers/gpu/drm/msm/msm_mmu.h, msm_mmu_set_fault_handler() performs
plain unlocked stores:

static inline void msm_mmu_set_fault_handler(struct msm_mmu *mmu, void *arg,
		int (*handler)(void *arg, unsigned long iova, int flags, void *data))
{
	mmu->arg = arg;
	mmu->handler = handler;
}

And in drivers/gpu/drm/msm/msm_iommu.c, msm_disp_fault_handler() reads and
calls those fields without locking or memory barriers:

static int msm_disp_fault_handler(struct iommu_domain *domain, struct device *dev,
				  unsigned long iova, int flags, void *arg)
{
	struct msm_iommu *iommu = arg;

	if (iommu->base.handler)
		return iommu->base.handler(iommu->base.arg, iova, flags, NULL);

	return -ENOSYS;
}

During msm_drm_kms_uninit(), msm_irq_uninstall() only frees the display
controller interrupt (kms->irq), while the SMMU context fault interrupt
remains active until kms->funcs->destroy(kms) detaches the IOMMU at the end
of msm_drm_kms_uninit().

If msm_disp_fault_handler() is already executing on another CPU when
msm_drm_kms_uninit() clears mmu->handler to NULL, nothing waits for
msm_disp_fault_handler() -> msm_kms_fault_handler() to finish before
msm_disp_snapshot_destroy() calls kthread_destroy_worker(kms->dump_worker).
Could an in-flight msm_kms_fault_handler() then call
msm_disp_snapshot_state() and queue kms->dump_work onto the freed
kms->dump_worker, or could a compiler reload of iommu->base.handler in
msm_disp_fault_handler() call a NULL function pointer?

Similarly, during msm_drm_kms_init(), msm_kms_init_vm() ->
msm_iommu_disp_new() attaches the IOMMU domain before
msm_mmu_set_fault_handler() is called. Without release/acquire ordering
when publishing mmu->arg and mmu->handler, could a concurrent fault on
weakly ordered architectures observe a non-NULL iommu->base.handler while
iommu->base.arg is still NULL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-msm-kms-destroy-fixes-v2-0-83727c122de7@oss.qualcomm.com?part=4