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 <[email protected]> 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-83727c122...@oss.qualcomm.com?part=4
