[PATCH v4 0/2] staging: fbtft: cleanup fbtft_framebuffer_alloc()

[PATCH v4 0/2] staging: fbtft: cleanup fbtft_framebuffer_alloc()

[Abdun Nihaal]

Cleanup error handling in fbtft_framebuffer_alloc()

This patchset includes the revert commit for the v1 patch, and the
cleanup patch that is not yet applied.

I have not included the v3 patch ("staging: fbtft: fix potential memory 
leak in fbtft_framebuffer_alloc()") in this patchset, as it has been 
already applied on staging-testing

v4:
- Add a revert patch to remove v1 patch
- Not included the patch that is already applied on staging-testing
- Added Reviewed-by tags

v3:
- Remove a redundant check before calling kfree

v2:
- Change the earlier patch to also handle the error code returned by
  fb_deferred_io_init() and update Fixes tag to point to the commit that
  introduced the memory allocation (which leads to leak).
- Add second patch to make the error handling order symmetric to
  fbtft_framebuffer_release() and also remove managed allocation for
  txbuf as suggested by Andy and Dan.

Link to v3: https://lore.kernel.org/linux-staging/cover.1751207100.git.abdun.nihaal@gmail.com/
Link to v2: https://lore.kernel.org/linux-staging/cover.1751086324.git.abdun.nihaal@gmail.com/T/#md111471ddd69e6ddb0a6b98e565551ffbd791a34
Link to v1: https://lore.kernel.org/all/20250626172412.18355-1-abdun.nihaal@gmail.com/

Abdun Nihaal (2):
  Revert "staging: fbtft: fix potential memory leak in
    fbtft_framebuffer_alloc()"
  staging: fbtft: cleanup error handling in fbtft_framebuffer_alloc()

 drivers/staging/fbtft/fbtft-core.c | 32 +++++++++++++++---------------
 1 file changed, 16 insertions(+), 16 deletions(-)

-- 
2.43.0

[PATCH v4 1/2] Revert "staging: fbtft: fix potential memory leak in fbtft_framebuffer_alloc()"

[Abdun Nihaal]

This reverts commit eb2cb7dab60f ("staging: fbtft: fix potential memory
leak in fbtft_framebuffer_alloc()").

An updated patch has been added as commit 505bffe21233 ("staging:
fbtft: fix potential memory leak in fbtft_framebuffer_alloc()"),
and so reverting the old patch.

Signed-off-by: Abdun Nihaal <abdun.nihaal@gmail.com>
---
Newly added in v4.

 drivers/staging/fbtft/fbtft-core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index d920164e7710..8538b6bab6a5 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -695,7 +695,6 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 cleanup_deferred:
 	fb_deferred_io_cleanup(info);
 release_framebuf:
-	fb_deferred_io_cleanup(info);
 	framebuffer_release(info);
 
 alloc_fail:
-- 
2.43.0

Re: [PATCH v4 1/2] Revert "staging: fbtft: fix potential memory leak in fbtft_framebuffer_alloc()"

[Andy Shevchenko]

On Tue, Jul 01, 2025 at 03:10:22PM +0530, Abdun Nihaal wrote:
> This reverts commit eb2cb7dab60f ("staging: fbtft: fix potential memory
> leak in fbtft_framebuffer_alloc()").
> 
> An updated patch has been added as commit 505bffe21233 ("staging:
> fbtft: fix potential memory leak in fbtft_framebuffer_alloc()"),
> and so reverting the old patch.

Revert has its automatic line, please do not remove it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/2] Revert "staging: fbtft: fix potential memory leak in fbtft_framebuffer_alloc()"

[Dan Carpenter]

On Tue, Jul 01, 2025 at 04:48:45PM +0300, Andy Shevchenko wrote:
> On Tue, Jul 01, 2025 at 03:10:22PM +0530, Abdun Nihaal wrote:
> > This reverts commit eb2cb7dab60f ("staging: fbtft: fix potential memory
> > leak in fbtft_framebuffer_alloc()").
> > 
> > An updated patch has been added as commit 505bffe21233 ("staging:
> > fbtft: fix potential memory leak in fbtft_framebuffer_alloc()"),
> > and so reverting the old patch.
> 
> Revert has its automatic line, please do not remove it.

Why?

I hate the revert format.  It is from when git was invented in 2005.
It sets you up for failure.  These days we have so many other things
that we want in patches.

1) The subsystem prefix in the subject
2) The 12 character hashes
3) A proper commit message
4) A Fixes tag

The automated revert commit messages don't have any of that.  It's
always better to hand write them.

regards,
dan carpenter

Re: [PATCH v4 1/2] Revert "staging: fbtft: fix potential memory leak in fbtft_framebuffer_alloc()"

[Greg KH]

On Tue, Jul 01, 2025 at 05:16:07PM +0300, Dan Carpenter wrote:
> On Tue, Jul 01, 2025 at 04:48:45PM +0300, Andy Shevchenko wrote:
> > On Tue, Jul 01, 2025 at 03:10:22PM +0530, Abdun Nihaal wrote:
> > > This reverts commit eb2cb7dab60f ("staging: fbtft: fix potential memory
> > > leak in fbtft_framebuffer_alloc()").
> > > 
> > > An updated patch has been added as commit 505bffe21233 ("staging:
> > > fbtft: fix potential memory leak in fbtft_framebuffer_alloc()"),
> > > and so reverting the old patch.
> > 
> > Revert has its automatic line, please do not remove it.
> 
> Why?
> 
> I hate the revert format.  It is from when git was invented in 2005.
> It sets you up for failure.  These days we have so many other things
> that we want in patches.
> 
> 1) The subsystem prefix in the subject
> 2) The 12 character hashes
> 3) A proper commit message
> 4) A Fixes tag
> 
> The automated revert commit messages don't have any of that.  It's
> always better to hand write them.

There are tools out there that expect the "traditional" format, so it's
good to keep them if at all possible.

But I agree, for this one it doesn't make sense, just do a fixup patch
on top of the current tree.  It's just a staging driver, not a big deal :)

thanks,

greg k-h

Re: [PATCH v4 1/2] Revert "staging: fbtft: fix potential memory leak in fbtft_framebuffer_alloc()"

[Dan Carpenter]

On Tue, Jul 01, 2025 at 03:10:22PM +0530, Abdun Nihaal wrote:
> This reverts commit eb2cb7dab60f ("staging: fbtft: fix potential memory
> leak in fbtft_framebuffer_alloc()").
> 
> An updated patch has been added as commit 505bffe21233 ("staging:
> fbtft: fix potential memory leak in fbtft_framebuffer_alloc()"),
> and so reverting the old patch.
> 
> Signed-off-by: Abdun Nihaal <abdun.nihaal@gmail.com>
> ---

This is the wrong approach.  The original patch was fine.  Just
write the next patches on top of that.

regards,
dan carpenter

[PATCH v4 2/2] staging: fbtft: cleanup error handling in fbtft_framebuffer_alloc()

[Abdun Nihaal]

The error handling in fbtft_framebuffer_alloc() mixes managed allocation
and plain allocation, and performs error handling in an order different
from the order in fbtft_framebuffer_release().

Fix them by moving vmem allocation closer to where it is used, and using
plain kzalloc() for txbuf allocation.

Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Suggested-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Abdun Nihaal <abdun.nihaal@gmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>
---
v3->v4:
- Added Reviewed-by tags

v2->v3: 
- Remove the if check before kfree of txbuf.buf, because it is zero
  initialized on allocation, and kfree is NULL aware.

Newly added in v2

 drivers/staging/fbtft/fbtft-core.c | 31 +++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index 8538b6bab6a5..9e7b84071174 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -568,18 +568,13 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 		height = display->height;
 	}
 
-	vmem_size = display->width * display->height * bpp / 8;
-	vmem = vzalloc(vmem_size);
-	if (!vmem)
-		goto alloc_fail;
-
 	fbdefio = devm_kzalloc(dev, sizeof(struct fb_deferred_io), GFP_KERNEL);
 	if (!fbdefio)
-		goto alloc_fail;
+		return NULL;
 
 	buf = devm_kzalloc(dev, 128, GFP_KERNEL);
 	if (!buf)
-		goto alloc_fail;
+		return NULL;
 
 	if (display->gamma_num && display->gamma_len) {
 		gamma_curves = devm_kcalloc(dev,
@@ -588,12 +583,17 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 					    sizeof(gamma_curves[0]),
 					    GFP_KERNEL);
 		if (!gamma_curves)
-			goto alloc_fail;
+			return NULL;
 	}
 
 	info = framebuffer_alloc(sizeof(struct fbtft_par), dev);
 	if (!info)
-		goto alloc_fail;
+		return NULL;
+
+	vmem_size = display->width * display->height * bpp / 8;
+	vmem = vzalloc(vmem_size);
+	if (!vmem)
+		goto release_framebuf;
 
 	info->screen_buffer = vmem;
 	info->fbops = &fbtft_ops;
@@ -613,7 +613,7 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 	info->fix.accel =          FB_ACCEL_NONE;
 	info->fix.smem_len =       vmem_size;
 	if (fb_deferred_io_init(info))
-		goto release_framebuf;
+		goto release_screen_buffer;
 
 	info->var.rotate =         pdata->rotate;
 	info->var.xres =           width;
@@ -668,7 +668,7 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 #endif
 
 	if (txbuflen > 0) {
-		txbuf = devm_kzalloc(par->info->device, txbuflen, GFP_KERNEL);
+		txbuf = kzalloc(txbuflen, GFP_KERNEL);
 		if (!txbuf)
 			goto cleanup_deferred;
 		par->txbuf.buf = txbuf;
@@ -694,12 +694,10 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 
 cleanup_deferred:
 	fb_deferred_io_cleanup(info);
+release_screen_buffer:
+	vfree(info->screen_buffer);
 release_framebuf:
 	framebuffer_release(info);
-
-alloc_fail:
-	vfree(vmem);
-
 	return NULL;
 }
 EXPORT_SYMBOL(fbtft_framebuffer_alloc);
@@ -712,6 +710,9 @@ EXPORT_SYMBOL(fbtft_framebuffer_alloc);
  */
 void fbtft_framebuffer_release(struct fb_info *info)
 {
+	struct fbtft_par *par = info->par;
+
+	kfree(par->txbuf.buf);
 	fb_deferred_io_cleanup(info);
 	vfree(info->screen_buffer);
 	framebuffer_release(info);
-- 
2.43.0