Thu, Jun 26, 2025 at 11:11:39PM +0300, Dan Carpenter kirjoitti: > 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:
... > > > 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. Ah, you have a point. Yes, the moving vmem allocation will solve the ordering issue. -- With Best Regards, Andy Shevchenko
