[PATCH] fbsplash: Fix mmap size and offset calculations

[Timo Teras]

On Tue, 10 May 2016 23:46:30 -0700
Georges Savoundararadj <savoundg at gmail.com> wrote:

> Hi Timo,
> 
> On 05/10/2016 11:23 PM, Timo Teras wrote:
> > On Tue, 10 May 2016 22:53:49 -0700
> > savoundg at gmail.com wrote:
> >  
> >> From: Georges Savoundararadj <savoundg at gmail.com>
> >>
> >> Before the commit 82c2fad, we were mapping the frame buffer device
> >> with the size: yres * line_length.
> >> This leads to a segmentation fault if the computed offset (yoffset
> >> * line_length + xoffset * bytes_per_pixel) is greater than the
> >> size.
> >>
> >> This commit maps the frame buffer device with the right offset
> >> avoiding the need to map to a larger size as done in commit 82c2fad
> >> (by using yres_virtual (if non-zero) instead of yres).  
> > Does this actually work?  
> Yes, it was working in my environment.
> >   mmap requires offset to be aligned by
> > PAGE_SIZE to work (it returns error otherwise), so even if it works
> > in your environment, I doubt this works with different combinations
> > of y-offset, line length and bytes per pixes.  
> You are right, the offset should be a multiple of the page size.
> I was wondering why the offset argument was not used instead of
> adding the offset to the base address G.addr.
> 
> We could align the offset to the page size and fix the size
> accordingly. What do you think?

If you just map the whole memory, you can keep the calculations in one
place. Splitting it to two place takes more code, is harder to
understand, and results in larger binary size.

Mapping more video memory is not harmful. It does not normally take
memory more (in worst case the mmu page table can grow slightly).

So what is the benefit of such change? Is there some non-obvious bug it
is meant to fix? Could you describe it.