On Thu, Jun 26, 2025 at 08:50:43PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 26, 2025 at 10:54:10PM +0530, Abdun Nihaal wrote:
> > In the error paths after fb_info structure is successfully allocated,
> > the memory allocated in fb_deferred_io_init() for info->pagerefs is not
> > freed. Fix that by adding the cleanup function on the error path.
>
> Thanks for the report and the fix! My comments below.
>
> ...
>
> > release_framebuf:
> > + fb_deferred_io_cleanup(info);
> > framebuffer_release(info);
>
> While the fix sounds good, there are still problems in the driver in this
> area:
>
> 1) managed resources allocation is mixed up with plain allocations
> (as you discovery hints);
>
> 2) the order in fbtft_framebuffer_release() is asymmetrical to what
> we have in fbtft_framebuffer_alloc().
>
> I would recommend to study this code a bit more and provide the following
> patches as a result:
>
> 1) fixing the order in fbtft_framebuffer_release();
>
> 2) moving vmem allocation closer to when it's needed, i.e. just after
> successful allocation of the info; at the same time move txbuf allocation
> from managed to unmanaged (drop devm, add respective kfree() calls where
> it's required);
>
Symetrical in this sense means that the cleanup in
fbtft_framebuffer_release() and in fbtft_framebuffer_alloc() are
similar:
fb_deferred_io_cleanup();
vfree();
framebuffer_release();
I feel like number 1 and 2 are sort of opposite approaches to making the
order symmetrical. #1 is changing fbtft_framebuffer_release() and #2 is
changing fbtft_framebuffer_alloc(). #2 is the less awkward approach.
> 3) this patch.
>
> All three should have the respective Fixes tags and hence may be backported.
Changing the order isn't a bug fix so it wouldn't get a Fixes tag.
I agree with Andy that the code isn't beautiful. But I think it's
easier to just fix the bug, and do the cleanup later as an optional
patch 2/2. I would also have been fine with a larger patch that does
the cleanup and the bug fix in one patch but I think other people
won't like that.
Diff below. Except, oops, this doesn't compile. Change the other
goto alloc_fail places to "return NULL;" I guess that means you
get authorship credit if you fix that.
So if you want you could resend your patch and you could send these
changes I've suggested as a patch 2/2 and then I think everyone will
be happy.
regards,
dan carpenter
diff --git a/drivers/staging/fbtft/fbtft-core.c
b/drivers/staging/fbtft/fbtft-core.c
index da9c64152a60..abfd7b1157df 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -568,11 +568,6 @@ 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;
@@ -595,6 +590,11 @@ struct fb_info *fbtft_framebuffer_alloc(struct
fbtft_display *display,
if (!info)
goto alloc_fail;
+ 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;
info->fbdefio = fbdefio;
@@ -652,7 +652,7 @@ struct fb_info *fbtft_framebuffer_alloc(struct
fbtft_display *display,
if (par->gamma.curves && gamma) {
if (fbtft_gamma_parse_str(par, par->gamma.curves, gamma,
strlen(gamma)))
- goto release_framebuf;
+ goto cleanup_deferred;
}
/* Transmit buffer */
@@ -669,7 +669,7 @@ struct fb_info *fbtft_framebuffer_alloc(struct
fbtft_display *display,
if (txbuflen > 0) {
txbuf = devm_kzalloc(par->info->device, txbuflen, GFP_KERNEL);
if (!txbuf)
- goto release_framebuf;
+ goto cleanup_deferred;
par->txbuf.buf = txbuf;
par->txbuf.len = txbuflen;
}
@@ -691,12 +691,12 @@ struct fb_info *fbtft_framebuffer_alloc(struct
fbtft_display *display,
return info;
+cleanup_deferred:
+ fb_deferred_io_cleanup(info);
+ vfree(info->screen_buffer);
release_framebuf:
framebuffer_release(info);
-alloc_fail:
- vfree(vmem);
-
return NULL;
}
EXPORT_SYMBOL(fbtft_framebuffer_alloc);
