[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: [PATCH] fbsplash: Fix mmap size and offset calculations
From:       Georges Savoundararadj <savoundg () gmail ! com>
Date:       2016-05-11 15:14:08
Message-ID: d80e90b9-22dd-4025-952d-d2a17646e7cc () gmail ! com
[Download RAW message or body]

On 05/11/2016 01:16 AM, Timo Teras wrote:
> On Tue, 10 May 2016 23:46:30 -0700
> Georges Savoundararadj <savoundg@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@gmail.com wrote:
>>>   
>>>> From: Georges Savoundararadj <savoundg@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.
OK, I understand it.
> Mapping more video memory is not harmful. It does not normally take
> memory more (in worst case the mmu page table can grow slightly).
OK
> So what is the benefit of such change? Is there some non-obvious bug it
> is meant to fix? Could you describe it.
>
It seems that there is no benefit of such change.

In the stable versions of busybox, I fall on a segfault because your 
patch was not applied and I fixed the bug that way.

Please disregard this patch.

Thanks,

Georges



_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]