Re: [lwip-devel] possible issue in sockets.c

[Sylvain Rochet]

Hi,

On Fri, Jan 24, 2014 at 02:00:15PM +0100, Albert Huitsing wrote:
> op 24-01-14 12:19, Simon Goldschmidt schreef:
> >Albert Huising wrote:
> >> you're saying that SYS_ARCH_PROTECT() should block all task
> >switching alltogether? Is that really the intention of
> >SYS_ARCH_PROTECT()?
> >Yes, it is. :-)
> hmm...
> 
> as far as I understand things correctly then (note I'm on ARM / FreeRTOS)
> 
> 1) SYS_ARCH_UNPROTECT() will call taskENTER_CRITICAL() and block
> IRQs -> blocking pre-emptive task switching. But this has no
> consequence for the SWI instruction on ARM:
> 2) sys_sem_signal() potentially calls portYIELD_WITHIN_API which is
> implemented by a 'SWI' instruction on ARM (see FreeRTOS's
> implementation of xSemaphoreGive / xQueueGenericSend)
> 3) the SWI instruction will NOT be blocked by blocking IRQs
> --> so within sys_sem_signal() the task switch will NOT be blocked,
> which violates lwip's expected behaviour
> 
> Does nobody have this problem but me? Maybe somebody else could shine a light 
> on it?
> 
> During a quick scan through the lwip code, I couldn't find the same
> situation again: every time SYS_ARCH_PROTECT() /
> SYS_ARCH_UNPROTECT() is called there is only plain vanilla code
> in-between (i.e.: no calls to RTOS functions) which strongs my
> belief that this *really* is a bug inside sockets.c only !
> 
> anyway the fix is simple enough: it's fixed when moving the
> last_select_cb_ctr = select_cb_ctr in front of the sys_sem_signal().
> I'll stick to that for now :-)
> 
> kindest regards,

I agree with Albert on this point, we really should, as much as 
possible, prevent calling functions that cause context switch from 
critical sections, especially if there is only one case in all the lwIP 
code and which appears to be easily fixable.

SYS_ARCH_PROTECT() and SYS_ARCH_UNPROTECT() is meant to be very fast, 
therefore most of embedded devices are only switching global interrupt 
in those callbacks, which seems to be the fastest way of handling 
critical sections.

However, adding the restriction that lwIP ports must check all of their 
RTOS entry points to prevent context switch inside critical section and 
therefore adding the necessary code in SYS_ARCH_UNPROTECT() to do a 
context switch seems like unecessary if we can easily prevent that to be 
required.

The documentation is rather unclear about what sys_arch_protect() should 
really do, it talks about a mutex *or* a semaphore *or* by disabling 
tasking depending on where you read the documentation (sys.h / 
sys_arch.txt), a mutex nor a semaphore never prevented task switching, 
they are actually task switching events ! :)

Considering the lack of documentation on this point, meaning there is a 
lot of system and ports in the wild with a critical section 
implementation without this requirement taken into consideration, I 
consider following the Albert proposal to be the best we can do.

Sylvain

signature.asc
Description: Digital signature