On Thu, Sep 3, 2026 at 11:48 AM <[email protected]> wrote: > > 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.
I was a bit curious how this even works on panfrost/panthor, which do something similar. But looks like they declare the variable as extern in a header, and doesn't throw a link error because the reference gets optimized out. BR, -R > > + 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
