I recently pulled a NXP crash fix for their ethernet driver which will
requeue a pbuf to the ethernet driver rather than sending it to the
lwip stack if it can't allocate a new pbuf to keep the ethernet
hardware primed with available packet buffers. While recently
reviewing this code I noticed that the full size of the pbuf wasn't
used on this re-queueing operation but the size of the last received
packet. I now reset the pbuf size back to its originally allocated
size before doing this requeue operation.
Previously the packet_rx() function would wait on the RxSem and when
signalled it would process all available inbound packets. This used to
cause no problem but once the thread synchronization was turned
on via SYS_LIGHTWEIGHT_PROT, the semaphore actually started to overflow
its maximum token count of 65535. This caused the mbed_die() flashing
LEDs of death. The old code was really breaking the producer/consumer
pattern that I typically see with a semaphore since the consumer was
written to consume more than 1 produced object per semaphore wait.
Before the thread synchronization was enabled, the packet_rx() thread
could use a single time slice to process all of these packets and then
loop back around a few more times to decrement the semaphore count
while skipping the packet processing since it had all been done.
Now the packet processing code would cause the thread to give up its
time slice as it hit newly enabled critical sections. In the end it
was possible for the code to leak 2 semaphore signals for every 1 by
which the thread was awaken. After about 10 seconds of load, this
would cause a leak of 65535 signals.
NOTE: Two potential issues with this change:
1) The LPC_EMAC->RxConsumeIndex != LPC_EMAC->RxProduceIndex check was
removed from packet_rx(). I believe that this is Ok since the same
condition is later checked in lpc_low_level_input() anyway so it
won't now try to process more packets than what exist.
2) What if ENET_IRQHandler(void) ends up not signalling the RxSem for
every packet received? When would that happen? I could see it
happening if the ethernet hardware would try to pend more than 1
interrupt when the priority was too elevated to process the
pending requests. Putting the consumer loop back in packet_rx()
and using a Signal instead of a Semaphore might be a better
solution?
This option actually enables the use of the lwip_sys_mutex for
protecting concurrent access to such important lwIP resources as:
select_cb_list (this is the one which orig flagged problem)
sockets array
mem stats (if enabled)
heap (if LWIP_ALLOW_MEM_FREE_FROM_OTHER_CONTEXT was non-zero)
memp pool allocs/frees
netif->loop_last pbuf linked list
pbuf reference counts
...
I first noticed this issue when I hit a crash while slamming the net
stack with a large number of TCP packets (I was actually sending 1k
data buffers from the TCPEchoServer mbed sample.) It crashed in the
last line of this code snippet from event_callback:
for (scb = select_cb_list; scb != NULL; scb = scb->next) {
if (scb->sem_signalled == 0) {
It was crashing because scb had an invalid address so it generated a
bus fault. I figured that memory was either corrupted or there was
some kind of concurrency issue. In trying to determine which, I wanted
to walk through the select_cb_list linked list and see where it was
corrupted:
(gdb) p scb
$1 = (struct lwip_select_cb *) 0x85100080
(gdb) p select_cb_list
$2 = (struct lwip_select_cb *) 0x0
That was interesting, the head of the linked list was now NULL but it
must have had a non-NULL value when this loop started running or we
would have never gotten to the point where we hit this crash.
This was starting to look like a concurrency issue since the linked
list was modified out from underneath this thread. Looking through the
source code for this function, I saw use of macros like
SYS_ARCH_PROTECT and SYS_ARCH_UNPROTECT which looked like they should
be providing the thead synchronization. I disassembled the
event_callback() function in the debugger and saw no signs of the
usage of synchronizition APIs that I expected. A search
through the code for the definition of these SYS_ARCH_UN/PROTECT
macros led me to discovering that they were actually ignored unless an
implementation defined them itself (the mbed version doesn't do so) or
the SYS_LIGHTWEIGHT_PROT macro is set to non-zero (the mbed version
didn't do this either). Flipping the SYS_LIGHTWEIGHT_PROT macro on in
lwipopts.h fixed the crash I kept hitting, increased the size of the
code a bit, and unfortunately slows things down a bit since it now
actually serializes access to these data structures by making calls
to the RTOS sync APIs.
After making my previous commit to completely disable LWIP_ASSERT
macro invocations, I ended up with a warning in pbuf.c where an
err variable was set but only checked for success in an assert. I
added a "(void)err;" reference to silence this warning.
I was doing some debugging that had me looking at the disassembly of
lpc_rx_queue() from within the debugger. I was looking for the call to
pbuf_alloc() that we see in the following code snippet:
p = pbuf_alloc(PBUF_RAW, (u16_t) EMAC_ETH_MAX_FLEN, PBUF_RAM);
if (p == NULL) {
LWIP_DEBUGF(UDP_LPC_EMAC | LWIP_DBG_TRACE,
("lpc_rx_queue: could not allocate RX pbuf (free desc=%d)\n",
lpc_enetif->rx_free_descs));
return queued;
}
/* pbufs allocated from the RAM pool should be non-chained. */
LWIP_ASSERT("lpc_rx_queue: pbuf is not contiguous (chained)",
pbuf_clen(p) <= 1);
When I was looking through the disassembly for this code I noticed a
call to pbuf_clen() in the actual machine code.
=> 0x0000bab0 <+24>: bl 0x44c0 <pbuf_clen>
0x0000bab4 <+28>: ldr r3, [r4, #112] ; 0x70
0x0000bab6 <+30>: ldrh.w r12, [r5, #10]
0x0000baba <+34>: add.w r2, r3, #9
0x0000babe <+38>: add.w r0, r12, #4294967295 ; 0xffffffff
The only call to pbuf_clean made from this function is made from
within the LWIP_ASSERT. When I looked more closely at how this macro
was defined, I saw that the mbed version of the stack had disabled the
LWIP_PLATFORM_ASSERT macro when LWIP_DEBUG was false which means that
no action will be taken if the assert is false but it still allows the
LWIP_ASSERT macro to potentially evaluate the assert expression.
Defining the LWIP_NOASSERT macro will fully disable the LWIP_ASSERT
macro.
I saw one of my TCP/IP samples shrink about 0.5K when I made this
change.
In gcc4mbed, I have been running with "-Wall -Wextra" and then
disabling a couple of noisy warnings that result. In particular, I
disable the unused-parameter and missing-field-initializers warnings.
The first commonly goes off for implementation of virtual methods or
other overridable functions where not all parameters are required for
every override. I don't find the second warning to be all that useful
anyway since missing structure field initializers will be set to 0
according to the C language specification. The RTOS code uses this
language feature and I see no reason that it shouldn't :)
The following line in USBHAL_KL25Z.cpp would generate a warning in GCC
because of a potential operator precendence issue:
return((USB0->FRMNUML | (USB0->FRMNUMH << 8) & 0x07FF));
This would have been interpreted as:
return((USB0->FRMNUML | ((USB0->FRMNUMH << 8) & 0x07FF)));
since & has higher precedence than |
I switched it to be:
return((USB0->FRMNUML | (USB0->FRMNUMH << 8)) & 0x07FF);
Since it makes more sense to & with 0x7FF after having merged the lower
and upper bytes together rather than just the upper byte. It should
have resulted in the same value either way.
The SDFileSystem class contained a few routines which compared a signed
integer loop index to an unsigned integer length/size. I switched the
loop index to be uint32_t as well.
From Adam Green, regarding using -fno-delete-null-pointer-checks:
"I would argue that on Cortex-M processors, it is more dangerous to not
have it. The compiler can actually generate incorrect code because it is
making an incorrect assumption (that reads from a NULL pointer will throw
an exception.) The GCC for ARM developers should actually never enable
the delete-null-pointer-checks optimization for Cortex-M processors.
There is a comment in the GCC manual that indicates, "Some targets,
especially embedded ones, disable this option [delete-null-pointer-checks]
at all levels." Not having this flag is pretty risky on the current
versions of GCC_ARM. Just to clarify, this flag doesn't enable an
optimization...it disables an unsafe optimization."
The code in netif_set_ipaddr would read the memory pointed to by its
ipaddr parameter, even if it was NULL on this line:
if ((ip_addr_cmp(ipaddr, &(netif->ip_addr))) == 0) {
On the Cortex-M3, it is typically OK to read from address 0 so this
code will actually compare the reset stack pointer value to the
current value in netif->ip_addr.
Later in the code, this same pointer will be used for a second read:
ip_addr_set(&(netif->ip_addr), ipaddr);
The ip_addr_set call will first check to see if the ipaddr is NULL and
if so, treats it like IP_ADDR_ANY (4 bytes of 0).
/** Safely copy one IP address to another (src may be NULL) */
#define ip_addr_set(dest, src) ((dest)->addr = \
((src) == NULL ? 0 : \
(src)->addr))
The issue here is that when GCC optimizes this code, it assumes that
the first dereference of ipaddr would have thrown an invalid memory
access exception and execution would never make it to this second
dereference. Therefore it optimizes out the NULL check in ip_addr_set.
The -fno-delete-null-pointer-checks will disable this optimization and
is a good thing to use with GCC in general on Cortex-M parts. I will
let the mbed guys make that change to their build system.
I have however corrected the code so that the intent of how to handle a
NULL ipaddr is more obvious and gets rid of the potential NULL
dereference.
By the way, this bug caused connect() to fail in obtaining an
address from DHCP. If I recall correctly from when I first debugged
this issue (late last year), I actually saw the initial value of the
stack pointer being used in the DHCP request as an IP address which
caused it to be rejected.
Peter's and my changes to LPC1768.ld ended up adding the same AHBSRAM0
and AHBSRAM1 section clauses to the script twice. I removed one copy.
I also pulled Peter's define of the ETHMEM_SECTION macro up into the
previous nested #if so that the preprocessor wouldn't spit out a
redefined macro warning.
I verified that building the code clean before and after these changes
still results in the same .bin file but now without warnings and/or
duplicate code.
Adam Green
Bogdan Marinescu
ytsuboi
Emilio Monti
Toyomasa Watarai
Bogdan Marinescu
Terry Guo