Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] THP initialization (`msm_gem_thp_init`) occurs after the DRM device is exposed to userspace via `drm_dev_register`, creating a data race and allowing premature access. - [Low] The module parameter `transparent_hugepage` is defined inside an `#ifdef CONFIG_TRANSPARENT_HUGEPAGE` block but accessed unconditionally in `msm_gem_thp_init()`, causing a compilation error when the config is disabled. --
commit 46b2b6fe3a31a0213c502585828f1edd5794c935 Author: Rob Clark <[email protected]> drm/msm: Enable THP for GEM buffers This commit introduces a dedicated tmpfs mount point for Transparent Hugepage support in MSM GEM buffers to improve page allocation performance. > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > index db1b655dd055b..33b9f495af020 100644 > --- a/drivers/gpu/drm/msm/msm_drv.c > +++ b/drivers/gpu/drm/msm/msm_drv.c [ ... ] > @@ -58,9 +58,29 @@ static bool separate_gpu_kms; > MODULE_PARM_DESC(separate_gpu_drm, "Use separate DRM device for the GPU > (0=single DRM device for both GPU and display (default), 1=two DRM devices)"); > module_param(separate_gpu_kms, bool, 0400); > > +#ifdef CONFIG_TRANSPARENT_HUGEPAGE > +static bool transparent_hugepage = true; > +module_param(transparent_hugepage, bool, 0400); > +MODULE_PARM_DESC(transparent_hugepage, "Use a dedicated tmpfs mount point > with Transparent Hugepage enabled (true = default)"); > +#endif > + > DECLARE_FAULT_ATTR(fail_gem_alloc); > DECLARE_FAULT_ATTR(fail_gem_iova); > > +static void msm_gem_thp_init(struct drm_device *dev) > +{ > + int err; > + > + if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) && !transparent_hugepage) [Severity: Low] Will this cause a build failure when CONFIG_TRANSPARENT_HUGEPAGE is disabled? The transparent_hugepage module parameter is only defined inside the ifdef block just above, but the C compiler will still evaluate it unconditionally here in msm_gem_thp_init(), resulting in an undeclared identifier error. > + return; > + > + err = drm_gem_huge_mnt_create(dev, "within_size"); > + if (drm_gem_get_huge_mnt(dev)) > + drm_info(dev, "Using Transparent Hugepage\n"); > + else if (err) > + drm_warn(dev, "Can't use Transparent Hugepage (%d)\n", err); > +} [ ... ] > @@ -178,6 +198,8 @@ static int msm_drm_init(struct device *dev, const struct > drm_driver *drv, > if (priv->kms_init) > msm_drm_kms_post_init(dev); > > + msm_gem_thp_init(ddev); [Severity: High] Is it safe to initialize the THP mount point here? Looking earlier in msm_drm_init(), drm_dev_register(ddev, 0) is called before this point. Because the device node is already exposed to userspace, a process could open it immediately after registration. If userspace starts allocating GEM objects before msm_gem_thp_init() completes, could this race condition cause the allocations to silently fall back to non-THP shmem, or potentially lead to a crash if a partially written pointer is read? > + > return 0; > > err_msm_uninit: -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
