PPP Deadlock

[Jonathan Larmour]

John Paul King wrote:
> There is a deadlock wherein if cyg_pppd_main exits before 
> cyg_ppp_tx_thread starts, cyg_ppp_up or cyg_ppp_tx_thread will stall.

Thanks for the patch. Although I'm not intimately familiar with this code, 
it seems to me that the logic of the expressions is the opposite sense from 
what they should be. See specific comments below.

> Index: net/ppp/current/src/sys-ecos.c
> ===================================================================
> RCS file: /cvs/ecos/ecos-opt/net/net/ppp/current/src/sys-ecos.c,v
> retrieving revision 1.10
> diff -u -5 -p -r1.10 sys-ecos.c
> --- net/ppp/current/src/sys-ecos.c	23 Oct 2005 20:46:20 -0000	1.10
> +++ net/ppp/current/src/sys-ecos.c	24 Jun 2006 23:17:15 -0000
> @@ -556,11 +556,11 @@ static void cyg_ppp_tx_thread(CYG_ADDRWO
>  {
>      ppp_tty.tx_thread_running = true;
>  
>      // Wait for the PPPD thread to get going and start the PPP
>      // initialization phase.
> -    while(phase == PHASE_DEAD )
> +    while((phase == PHASE_DEAD) && ppp_tty.pppd_thread_running)
>          cyg_thread_delay(100);

Shouldn't this delay be while the pppd_thread is *not* running?

>      // Now loop until the link goes back down.
>      while( phase != PHASE_DEAD )
>      {
> @@ -1700,25 +1700,26 @@ externC cyg_ppp_handle_t cyg_ppp_up( con
>  
>      strncpy( devnam, devnam_arg, PATH_MAX );
>  
>      ppp_tty.options = options;
>  
> +    cyg_semaphore_init( &ppp_tty.tx_sem,  0 );
> +
>      // Start the PPPD thread
>      cyg_thread_create(CYGNUM_PPP_PPPD_THREAD_PRIORITY,
>                        cyg_pppd_main,
>                        (CYG_ADDRWORD)&ppp_tty,
>                        "PPPD",
>                        &cyg_pppd_stack[0],
>                        CYGNUM_PPP_PPPD_THREAD_STACK_SIZE,
>                        &ppp_tty.pppd_thread,
>                        &cyg_pppd_thread_obj
>              );
> +    ppp_tty.pppd_thread_running = true;

This seems unnecessary since it is set in sys_init() anyway.

>      cyg_thread_resume(ppp_tty.pppd_thread);
>  
>      // Start the TX thread
> -    cyg_semaphore_init( &ppp_tty.tx_sem,  0 );
> -    
>      cyg_thread_create(CYGNUM_PPP_PPPD_THREAD_PRIORITY+1,
>                        cyg_ppp_tx_thread,
>                        (CYG_ADDRWORD)&ppp_tty,
>                        "PPP Tx Thread",
>                        &cyg_ppp_tx_thread_stack[0],
> @@ -1728,11 +1729,11 @@ externC cyg_ppp_handle_t cyg_ppp_up( con
>              );
>      cyg_thread_resume(ppp_tty.tx_thread);
>  
>      // Wait for the PPPD thread to get going and start the PPP
>      // initialization phase.
> -    while(phase == PHASE_DEAD )
> +    while((phase == PHASE_DEAD) && ppp_tty.pppd_thread_running)
>          cyg_thread_delay(100);

Again, shouldn't this only be delaying as long as the pppd_thread is *not* 
running?

Moving the semaphore init makes sense though. The pppd thread could be a 
higher priority than the thread calling cyg_ppp_up. For now, that is the 
only change I've checked in.

Thanks,

Jifl
-- 
eCosCentric    http://www.eCosCentric.com/    The eCos and RedBoot experts
------["The best things in life aren't things."]------      Opinions==mine