mailbox.c mapmem needs to adjust size to allow for offset in call to mmap

[steverpalmer]

Hi,
I, like many others, have copied mailbox.c into my own projects to allow access to various features. The function mapmem correctly adjusts the value of base (by an amount offset) downwards to put it onto a page boundary. It also correctly adjusts the address returned by the same offset upwards. However, it should correspondingly adjust the value of size (upwards) so that the memory returned will include the memory identified in the original call parameters. In short, after base = base - offset;, there should be a size = size + offset;
Also, similar adjustments are required in the function unmapmem.
Thanks,
Steve

[popcornmix]

I assume you are talking about mailbox.c in hello_fft?
I think for hello_fft the offset will always be zero.

Looking at the code the unmap won't work when offset is not zero (the address to pass to unmap will not be as expected).

I think this can only be solved by changing the API.
E.g. return address is actual mmap-ed buffer (4K aligned) and a separate offset is returned.

Or, make mapmem only accept 4K aligned addresses - return fail when offset != 0.
The calling code can always handle the offset for non-aligned blocks.

[steverpalmer]

Yes, I do mean mailbox.c in hello_fft.
Yes, I think that in hello_fft the offset will always be zero.
I tweaked unmapmem as follows:

   const intptr_t offset = (intptr_t)addr % PAGE_SIZE;
   addr = (char *)addr - offset;
   size = size + offset;
   int s = munmap(addr, size);

(ugly, but I think it will work as the "reverse" of what is done in mapmem (including my size adjustment)).
Or the API could be changed as you suggest. In any case, although not apparent in hello_fft, I would still suggest that mapmem is wrong to adjust the base without adjusting size. It feels like it goes half way towards providing a feature.
Or perhaps there is some other file/module/library that I should be using to do this kind of mapping?
Cheers,
Steve

[popcornmix]

Yes, I think that code is safe. The value returned from mmap will be 4K aligned, so the offset can be extracted from that.

As the current code looks like it handles non-zero offsets but doesn't then I'm happy to fix it up with your suggested change. It may save someone else some head scratching.

Added to internal repo - will appear in firmware/userland on next firmware build.

[JamesH65]

Fixed.