Re: [PATCH v2 6/6] drm/msm: Make msm_framebuffer_init() an internal interface again

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing cleanup in the error path of msm_framebuffer_prepare() leading to resource leaks and potential hardware faults.
--

commit 9c0f26778b6b3f337240eba05217d26a3010e217
Author: Thomas Zimmermann <tzimmermann@suse.de>

drm/msm: Make msm_framebuffer_init() an internal interface again

The patch makes msm_framebuffer_init() a static function since it is only
called from msm_framebuffer_create() within the same source file.

[ ... ]

> diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c
> index dec550e6cf750..60c108d35d2a1 100644
> --- a/drivers/gpu/drm/msm/msm_fb.c
> +++ b/drivers/gpu/drm/msm/msm_fb.c
> @@ -134,10 +134,10 @@ const struct msm_format *msm_framebuffer_format(struct drm_framebuffer *fb)
>  	return msm_fb->format;
>  }
>  
> -struct drm_framebuffer *msm_framebuffer_init(struct drm_device *dev,
> -					     const struct drm_format_info *info,
> -					     const struct drm_mode_fb_cmd2 *mode_cmd,
> -					     struct drm_gem_object **bos)
> +static struct drm_framebuffer *
> +msm_framebuffer_init(struct drm_device *dev, const struct drm_format_info *info,
> +		     const struct drm_mode_fb_cmd2 *mode_cmd,
> +		     struct drm_gem_object **bos)

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this file,
I noticed a missing cleanup in the error path of msm_framebuffer_prepare()
that can lead to resource leaks and potential regressions.

When msm_framebuffer_prepare() successfully pins some planes but fails on a
subsequent plane, it returns early:

drivers/gpu/drm/msm/msm_fb.c:msm_framebuffer_prepare() {
	...
	for (i = 0; i < n; i++) {
		msm_gem_vma_get(fb->obj[i]);
		ret = msm_gem_get_and_pin_iova(fb->obj[i], vm, &msm_fb->iova[i]);
		drm_dbg_state(fb->dev, "FB[%u]: iova[%d]: %08llx (%d)\n",
			      fb->base.id, i, msm_fb->iova[i], ret);
		if (ret)
			return ret;
	}
	...
}

Does returning immediately here fail to roll back the earlier pinned planes
and vma_get references? 

Furthermore, DRM core's drm_atomic_helper_prepare_planes() skips calling
cleanup_fb() for the plane that failed its prepare_fb(). Since
msm_fb->prepare_count is incremented at the start of the function but not
decremented on this error path, will a subsequent attempt to use this
framebuffer evaluate atomic_inc_return(&msm_fb->prepare_count) > 1 as true?

If it falsely returns success without pinning the remaining planes, could
this result in an IOMMU fault or memory corruption during scanout when the
hardware attempts to access unmapped addresses?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618141249.151338-1-tzimmermann@suse.de?part=6