Re: [PATCH 1/2] drm/msm: Enable THP for GEM buffers

[sashiko-bot@kernel.org]

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 <robin.clark@oss.qualcomm.com>

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/20260903183725.15835-1-robin.clark@oss.qualcomm.com?part=1