Closed snobu closed 9 years ago
Gadgetoid
commented
9 years ago Awesome detective work there! Hopefully I can come back to Unicorn HAT and have a bit of a dig with this in mind- but I'm definitely no C developer either.
Thanks for sharing your findings.
snobu
commented
9 years ago Running valgrind on the "commented out" version:
root@raspberrypi /tmp/unicorn-hat/c/unicorn # valgrind --tool=memcheck --leak-check=yes ./unicorn
==5680== Memcheck, a memory error detector
==5680== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al.
==5680== Using Valgrind-3.7.0 and LibVEX; rerun with -h for copyright info
==5680== Command: ./unicorn
==5680==
==5680== Warning: bad signal number 0 in sigaction()
==5680== Warning: ignored attempt to set SIGKILL handler in sigaction();
==5680== the SIGKILL signal is uncatchable
==5680== Warning: ignored attempt to set SIGSTOP handler in sigaction();
==5680== the SIGSTOP signal is uncatchable
Requesting 4096 bytes
0000: 0x00000024
0004: 0x80000000
0008: 0x0003000c
000c: 0x0000000c
0010: 0x80000004
0014: 0x0000002f
0018: 0x00001000
001c: 0x00000004
0020: 0x00000000
0000: 0x0000001c
0004: 0x80000000
0008: 0x0003000d
000c: 0x00000004
0010: 0x80000004
0014: 0xfd7a8000
0018: 0x00000000
base=0x3d7a8000, mem=0x4023000
base=0x3f007000, mem=0x4024000
base=0x3f20c000, mem=0x484d000
base=0x3f200000, mem=0x484e000
base=0x3f101000, mem=0x484f000
Calling unmap_registers() from ws2811_fini()
ws2811.c -- Now inside unmap_registers(): calling unmapmem on device->pwm
mailbox.c -- Calling unmapmem(0x484d000, 40)
ws2811.c -- This line printed after unmapmem(device->pwm). unmapmem() went well.
ws2811.c -- Now inside unmap_registers(): calling unmapmem on device->gpio
mailbox.c -- Calling unmapmem(0x484e000, 180)
ws2811.c -- This is after unmapmem(device->gpio). unmapmem() went well.
Done calling unmap_registers()
Calling ws2811_cleanup() from ws2811_fini()
mailbox.c -- Calling unmapmem(0x4023000, 4096)
0000: 0x0000001c
0004: 0x80000000
0008: 0x0003000e
000c: 0x00000004
0010: 0x80000004
0014: 0x00000000
0018: 0x00000000
0000: 0x0000001c
0004: 0x80000000
0008: 0x0003000f
000c: 0x00000004
0010: 0x80000004
0014: 0x00000000
0018: 0x00000000
Done calling ws2811_cleanup()
==5680==
==5680== HEAP SUMMARY:
==5680== in use at exit: 0 bytes in 0 blocks
==5680== total heap usage: 7 allocs, 7 frees, 1,728 bytes allocated
==5680==
==5680== All heap blocks were freed -- no leaks are possible
==5680==
==5680== For counts of detected and suppressed errors, rerun with: -v
==5680== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 21 from 6)And on the "vanilla" library (with the broken unmaps left in):
root@raspberrypi /tmp/unicorn-hat/c/unicorn # clear; valgrind --tool=memcheck --leak-check=yes ./unicorn
==5796== Memcheck, a memory error detector
==5796== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al.
==5796== Using Valgrind-3.7.0 and LibVEX; rerun with -h for copyright info
==5796== Command: ./unicorn
==5796==
==5796== Warning: bad signal number 0 in sigaction()
==5796== Warning: ignored attempt to set SIGKILL handler in sigaction();
==5796== the SIGKILL signal is uncatchable
==5796== Warning: ignored attempt to set SIGSTOP handler in sigaction();
==5796== the SIGSTOP signal is uncatchable
Requesting 4096 bytes
0000: 0x00000024
0004: 0x80000000
0008: 0x0003000c
000c: 0x0000000c
0010: 0x80000004
0014: 0x0000002f
0018: 0x00001000
001c: 0x00000004
0020: 0x00000000
0000: 0x0000001c
0004: 0x80000000
0008: 0x0003000d
000c: 0x00000004
0010: 0x80000004
0014: 0xfd7a8000
0018: 0x00000000
base=0x3d7a8000, mem=0x4023000
base=0x3f007000, mem=0x4024000
base=0x3f20c000, mem=0x484d000
base=0x3f200000, mem=0x484e000
base=0x3f101000, mem=0x484f000
Calling unmap_registers() from ws2811_fini()
ws2811.c -- Now inside unmap_registers(): calling unmapmem on device->dma
mailbox.c -- Calling unmapmem(0x4024500, 36)
mailbox.c -- unmapmem(): munmap error -1
==5796==
==5796== HEAP SUMMARY:
==5796== in use at exit: 288 bytes in 3 blocks
==5796== total heap usage: 7 allocs, 4 frees, 1,728 bytes allocated
==5796==
==5796== LEAK SUMMARY:
==5796== definitely lost: 0 bytes in 0 blocks
==5796== indirectly lost: 0 bytes in 0 blocks
==5796== possibly lost: 0 bytes in 0 blocks
==5796== still reachable: 288 bytes in 3 blocks
==5796== suppressed: 0 bytes in 0 blocks
==5796== Reachable blocks (those to which a pointer was found) are not shown.
==5796== To see them, rerun with: --leak-check=full --show-reachable=yes
==5796==
==5796== For counts of detected and suppressed errors, rerun with: -v
==5796== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 21 from 6)So i guess we have a cleaner exit without those two unmaps because we actually reach ws2811_cleanup().
euang
commented
9 years ago The Pi's memory is being file mapped in the mapmem function in mailbox.c The mmap function must be passed in a starting point in the file that is being mapped as a multiple of page size. For the registers that you notice are offset, their location in memory isn't a multiple of page size. Therefore the nearest multiple before them is used as starting point. That's what this bit does in mapmem
unsigned offset = base % PAGE_SIZE;
base = base - offset;
...
void *mem = mmap(
0,
size,
PROT_READ|PROT_WRITE,
MAP_SHARED/*|MAP_FIXED*/,
mem_fd,
base);This means that memory address returned by mmap is now pointing to before the location you want, so you have to offset the returned location to get where you want to be.
return (char *)mem + offset;The problem then comes when you try to unmap the memory, because munmap has to be passed in the address it allocated you from mmap, but in the case of the offset registers the pointers are pointing to the allocated address plus the offset. To fix it you have to reverse the page alignment calculation to get the correct pointer address to unmap. See corrected code below:
void *unmapmem(void *addr, unsigned size)
{
unsigned offset = (unsigned)addr % PAGE_SIZE;
addr = addr - offset;
int s = munmap(addr, size);
if (s != 0) {
printf("munmap error %d\n", s);
exit (-1);
}
return NULL;
}Saw your pull request! Thank you. Going to have to give it a try before I merge, but it's much appreciated!
Gadgetoid
commented
9 years ago Fixed in https://github.com/pimoroni/unicorn-hat/commit/4234c4959f4ea4ca27115835a778033e6f5328a0 thanks to @euang
I own a Raspberry Pi 2 Model B. This is about the notorious munmap error -1.
First an strace reveals that munmap() is called on device->dma, with a size of 36 (it's 36 every time).
Let's take the last two lines, from the last run:
munmap is always called at whatever we mapped at + 0x00000500. I can't figure out where this offset is coming from.
Now, i've put in some debug messages in ws2811.c and mailbox.c (also turned on the already existing #define DEBUG).
Let's compile and run.
It's exiting here
unmapmem((void *)device->dma, sizeof(dma_t));, just as we saw on strace.Alright, let's see what happens if we skip unmap on device->dma altogether.
Compile and run.
Now we're exiting at device->cm_pwm, which also fails to call the right memory location and has a (wrong) offset of (always) a0.
Well.. it has come to this so why not skip that too.
Compile and run, now with both device->dma and device->cm_pwm unmaps commented out.
Ha. We've reached
ws2811_cleanup().We have a clean exit. Well... clean but with garbage left in memory.Now, i clearly do not know enough C to figure this out in a sensible amount of time, so that's why this issue here. If someone finds out what's with those offsets.. needless to say i'm burning here to understand this one. All my debugging here happened on a Pi2 Model B.
Thank you.
Towards a happier Hat! --Adrian.