From ee752ab1ceafd3aa64bf5f861a3d397351c9a12a Mon Sep 17 00:00:00 2001 From: Sylvain Rochet Date: Thu, 19 Mar 2015 21:41:36 +0100 Subject: [PATCH] PPP, PPPoS, renamed PPP_INPROC_MULTITHREADED to PPP_INPROC_IRQ_SAFE Follow-up of the #44565 bug fix, renamed the misnamed PPP_INPROC_MULTITHREADED to PPP_INPROC_IRQ_SAFE because it is IRQ safe but not thread safe. Updated PPP documentation which now clearly state when and how this feature can be used. --- doc/ppp.txt | 48 ++++++++++++++++------------- src/api/tcpip.c | 8 ++--- src/include/lwip/opt.h | 18 +++-------- src/include/lwip/tcpip.h | 8 ++--- src/include/netif/ppp/pppos.h | 8 ++--- src/netif/ppp/pppos.c | 57 +++++++++++++++-------------------- 6 files changed, 68 insertions(+), 79 deletions(-) diff --git a/doc/ppp.txt b/doc/ppp.txt index a3bd0efc..280aef3b 100644 --- a/doc/ppp.txt +++ b/doc/ppp.txt @@ -6,7 +6,7 @@ Table of Contents: 1 - Supported PPP protocols and features 2 - Raw API PPP example for all protocols -3 - PPPoS input path (raw API, thread safe API, TCPIP API) +3 - PPPoS input path (raw API, IRQ safe API, TCPIP API) 4 - Thread safe PPP API (PPPAPI) 5 - Upgrading from lwIP <= 1.4.x to lwIP >= 1.5.x @@ -298,32 +298,34 @@ ppp_free(ppp); -3 PPPoS input path (raw API, thread safe API, TCPIP API) -======================================================== +3 PPPoS input path (raw API, IRQ safe API, TCPIP API) +===================================================== PPPoS requires a serial I/O SIO port (see include/lwip/sio.h). -Received data on serial port should be sent to lwIP using the pppos_input() or -pppos_input_sys() functions. +Received data on serial port should be sent to lwIP using the pppos_input() +function or the pppos_input_sys() function. -If PPP_INPROC_MULTITHREADED is 0 (the default), pppos_input() is not thread safe -and then *MUST* only be called inside the lwIP context. You should use that if -you are calling pppos_input() from your main loop context when NO_SYS=1. +If NO_SYS is 1 and if PPP_INPROC_IRQ_SAFE is 0 (the default), pppos_input() +is not IRQ safe and then *MUST* only be called inside your main loop. -If PPP_INPROC_MULTITHREADED is 1, pppos_input() is thread safe and can be called -from a dedicated RX-thread or from interrupt context… *BUT* you should NEVER -call pppos_connect(), pppos_listen() and ppp_free() if pppos_input() can still -be running, doing this is NOT thread safe. You should also avoid calling -pppos_input() if PPPoS session is not started yet. +Whatever the NO_SYS value, if PPP_INPROC_IRQ_SAFE is 1, pppos_input() is IRQ +safe and can be safely called from an interrupt context, using that is going +to reduce your need of buffer if pppos_input() is called byte after byte in +your rx serial interrupt. -Using PPP_INPROC_MULTITHREADED is discouraged unless you really know what you -are doing, though it may greatly reduce your need of buffer if pppos_input() is -called byte after byte in your rx serial interrupt, your move ;-) +if NO_SYS is 0, the thread safe way outside an interrupt context is to use +the pppos_input_tcpip() function to pass input data to the lwIP core thread +using the TCPIP API. This is thread safe in all cases but you should avoid +passing data byte after byte because it uses heavy locking (mailbox) and it +allocates pbuf, better fill them ! + +if NO_SYS is 0 and if PPP_INPROC_IRQ_SAFE is 1, you may also use pppos_input() +from an RX thread, however pppos_input() is not thread safe by itself. You can +do that *BUT* you should NEVER call pppos_connect(), pppos_listen() and +ppp_free() if pppos_input() can still be running, doing this is NOT thread safe +at all. Using PPP_INPROC_IRQ_SAFE from an RX thread is discouraged unless you +really know what you are doing, your move ;-) -Anyway, if you are using an OS (NO_SYS=0) and if PPP_INPROC_MULTITHREADED is 0, -you can use the pppos_input_tcpip() function to pass input data to the lwIP -core thread. This is thread safe in all cases but you should avoid passing -data byte after byte because it uses heavy locking (mailbox) and it allocates -pbuf, better fill them ! /* * Fonction to call for received data @@ -386,6 +388,10 @@ from previous lwIP version is pretty easy: * PPP_INPROC_OWNTHREAD was broken by design and was removed, you have to create your own serial rx thread +* PPP_INPROC_MULTITHREADED option was misnamed and confusing and was renamed + PPP_INPROC_IRQ_SAFE, please read the "PPPoS input path" documentation above + because you might have been fooled by that + * If you used tcpip_callback_with_block() on ppp_ functions you may wish to use the PPPAPI API instead. diff --git a/src/api/tcpip.c b/src/api/tcpip.c index 63b4aeac..50110874 100644 --- a/src/api/tcpip.c +++ b/src/api/tcpip.c @@ -127,12 +127,12 @@ tcpip_thread(void *arg) break; #endif /* LWIP_TCPIP_CORE_LOCKING_INPUT */ -#if PPPOS_SUPPORT && !PPP_INPROC_MULTITHREADED +#if PPPOS_SUPPORT && !PPP_INPROC_IRQ_SAFE case TCPIP_MSG_INPKT_PPPOS: pppos_input_sys(msg->msg.inp.p, msg->msg.inp.netif); memp_free(MEMP_TCPIP_MSG_INPKT, msg); break; -#endif /* PPPOS_SUPPORT && !PPP_INPROC_MULTITHREADED */ +#endif /* PPPOS_SUPPORT && !PPP_INPROC_IRQ_SAFE */ #if LWIP_NETIF_API case TCPIP_MSG_NETIFAPI: @@ -232,7 +232,7 @@ tcpip_input(struct pbuf *p, struct netif *inp) #endif /* LWIP_TCPIP_CORE_LOCKING_INPUT */ } -#if PPPOS_SUPPORT && !PPP_INPROC_MULTITHREADED +#if PPPOS_SUPPORT && !PPP_INPROC_IRQ_SAFE /** * Pass a received packet to tcpip_thread for input processing * @@ -272,7 +272,7 @@ tcpip_pppos_input(struct pbuf *p, struct netif *inp) return ERR_OK; #endif /* LWIP_TCPIP_CORE_LOCKING_INPUT */ } -#endif /* PPPOS_SUPPORT && !PPP_INPROC_MULTITHREADED */ +#endif /* PPPOS_SUPPORT && !PPP_INPROC_IRQ_SAFE */ /** * Call a specific function in the thread context of diff --git a/src/include/lwip/opt.h b/src/include/lwip/opt.h index 13a961d2..391d677c 100644 --- a/src/include/lwip/opt.h +++ b/src/include/lwip/opt.h @@ -1916,22 +1916,12 @@ #if PPP_SUPPORT /** - * PPP_INPROC_MULTITHREADED==1 call ppp_input() using tcpip_callback(). + * PPP_INPROC_IRQ_SAFE==1 call pppos_input() using tcpip_callback(). * - * Set this to 0 in the following cases: - * - pppos_input() is called from the main loop and NO_SYS==1 - * - you are using tcpip_input() (NO_SYS==0) on PPP data input - * - * Otherwise, if pppos_input() is called outside lwIP context (IRQ) - * set this to 1. - * - * CAUTION: if set to 1, you should NEVER call pppos_connect(), pppos_listen() - * and ppp_free() if pppos_input() can still be running, doing this is not - * thread safe. You should also avoid calling pppos_input() if PPPoS session - * is not started yet. + * Please read the "PPPoS input path" chapter in the PPP documentation about this option. */ -#ifndef PPP_INPROC_MULTITHREADED -#define PPP_INPROC_MULTITHREADED 0 +#ifndef PPP_INPROC_IRQ_SAFE +#define PPP_INPROC_IRQ_SAFE 0 #endif /** diff --git a/src/include/lwip/tcpip.h b/src/include/lwip/tcpip.h index 0fd93322..c9b7ad3c 100644 --- a/src/include/lwip/tcpip.h +++ b/src/include/lwip/tcpip.h @@ -143,9 +143,9 @@ err_t tcpip_apimsg(struct api_msg *apimsg); err_t tcpip_input(struct pbuf *p, struct netif *inp); -#if PPPOS_SUPPORT && !PPP_INPROC_MULTITHREADED +#if PPPOS_SUPPORT && !PPP_INPROC_IRQ_SAFE err_t tcpip_pppos_input(struct pbuf *p, struct netif *inp); -#endif /* PPPOS_SUPPORT && !PPP_INPROC_MULTITHREADED */ +#endif /* PPPOS_SUPPORT && !PPP_INPROC_IRQ_SAFE */ #if LWIP_NETIF_API err_t tcpip_netifapi(struct netifapi_msg *netifapimsg); @@ -182,9 +182,9 @@ enum tcpip_msg_type { TCPIP_MSG_API, #endif /* LWIP_NETCONN || LWIP_SOCKET */ TCPIP_MSG_INPKT, -#if PPPOS_SUPPORT && !PPP_INPROC_MULTITHREADED +#if PPPOS_SUPPORT && !PPP_INPROC_IRQ_SAFE TCPIP_MSG_INPKT_PPPOS, -#endif /* PPPOS_SUPPORT && !PPP_INPROC_MULTITHREADED */ +#endif /* PPPOS_SUPPORT && !PPP_INPROC_IRQ_SAFE */ #if LWIP_NETIF_API TCPIP_MSG_NETIFAPI, #endif /* LWIP_NETIF_API */ diff --git a/src/include/netif/ppp/pppos.h b/src/include/netif/ppp/pppos.h index 761f7c65..dfd884d4 100644 --- a/src/include/netif/ppp/pppos.h +++ b/src/include/netif/ppp/pppos.h @@ -104,10 +104,10 @@ struct pppos_pcb_s { ppp_pcb *pppos_create(struct netif *pppif, sio_fd_t fd, ppp_link_status_cb_fn link_status_cb, void *ctx_cb); -#if !NO_SYS && !PPP_INPROC_MULTITHREADED +#if !NO_SYS && !PPP_INPROC_IRQ_SAFE /* Pass received raw characters to PPPoS to be decoded through lwIP TCPIP thread. */ err_t pppos_input_tcpip(ppp_pcb *ppp, u8_t *s, int l); -#endif /* !NO_SYS && !PPP_INPROC_MULTITHREADED */ +#endif /* !NO_SYS && !PPP_INPROC_IRQ_SAFE */ /* PPP over Serial: this is the input function to be called for received data. */ void pppos_input(ppp_pcb *ppp, u8_t* data, int len); @@ -117,9 +117,9 @@ void pppos_input(ppp_pcb *ppp, u8_t* data, int len); * Functions called from lwIP * DO NOT CALL FROM lwIP USER APPLICATION. */ -#if !NO_SYS && !PPP_INPROC_MULTITHREADED +#if !NO_SYS && !PPP_INPROC_IRQ_SAFE err_t pppos_input_sys(struct pbuf *p, struct netif *inp); -#endif /* !NO_SYS && !PPP_INPROC_MULTITHREADED */ +#endif /* !NO_SYS && !PPP_INPROC_IRQ_SAFE */ #endif /* PPPOS_H */ #endif /* PPP_SUPPORT && PPPOL2TP_SUPPORT */ diff --git a/src/netif/ppp/pppos.c b/src/netif/ppp/pppos.c index 499b8758..094582bd 100644 --- a/src/netif/ppp/pppos.c +++ b/src/netif/ppp/pppos.c @@ -70,9 +70,9 @@ static err_t pppos_netif_input(ppp_pcb *ppp, void *ctx, struct pbuf *p, u16_t pr #endif /* VJ_SUPPORT */ /* Prototypes for procedures local to this file. */ -#if PPP_INPROC_MULTITHREADED +#if PPP_INPROC_IRQ_SAFE static void pppos_input_callback(void *arg); -#endif /* PPP_INPROC_MULTITHREADED */ +#endif /* PPP_INPROC_IRQ_SAFE */ static void pppos_input_free_current_packet(pppos_pcb *pppos); static void pppos_input_drop(pppos_pcb *pppos); static err_t pppos_output_append(pppos_pcb *pppos, err_t err, struct pbuf *nb, u8_t c, u8_t accm, u16_t *fcs); @@ -167,7 +167,7 @@ ppp_get_fcs(u8_t byte) #define PPP_INITFCS 0xffff /* Initial FCS value */ #define PPP_GOODFCS 0xf0b8 /* Good final FCS value */ -#if PPP_INPROC_MULTITHREADED +#if PPP_INPROC_IRQ_SAFE #define PPPOS_DECL_PROTECT(lev) SYS_ARCH_DECL_PROTECT(lev) #define PPPOS_PROTECT(lev) SYS_ARCH_PROTECT(lev) #define PPPOS_UNPROTECT(lev) SYS_ARCH_UNPROTECT(lev) @@ -175,7 +175,7 @@ ppp_get_fcs(u8_t byte) #define PPPOS_DECL_PROTECT(lev) #define PPPOS_PROTECT(lev) #define PPPOS_UNPROTECT(lev) -#endif /* PPP_INPROC_MULTITHREADED */ +#endif /* PPP_INPROC_IRQ_SAFE */ /* @@ -340,10 +340,10 @@ pppos_connect(ppp_pcb *ppp, void *ctx) { pppos_pcb *pppos = (pppos_pcb *)ctx; -#if PPP_INPROC_MULTITHREADED +#if PPP_INPROC_IRQ_SAFE /* input pbuf left over from last session? */ pppos_input_free_current_packet(pppos); -#endif /* PPP_INPROC_MULTITHREADED */ +#endif /* PPP_INPROC_IRQ_SAFE */ ppp_clear(ppp); /* reset PPPoS control block to its initial state */ @@ -379,10 +379,10 @@ pppos_listen(ppp_pcb *ppp, void *ctx, struct ppp_addrs *addrs) #endif /* PPP_IPV4_SUPPORT */ lcp_options *lcp_wo; -#if PPP_INPROC_MULTITHREADED +#if PPP_INPROC_IRQ_SAFE /* input pbuf left over from last session? */ pppos_input_free_current_packet(pppos); -#endif /* PPP_INPROC_MULTITHREADED */ +#endif /* PPP_INPROC_IRQ_SAFE */ ppp_clear(ppp); /* reset PPPoS control block to its initial state */ @@ -442,14 +442,14 @@ pppos_disconnect(ppp_pcb *ppp, void *ctx) pppos->open = 0; PPPOS_UNPROTECT(lev); - /* If PPP_INPROC_MULTITHREADED is used we cannot call + /* If PPP_INPROC_IRQ_SAFE is used we cannot call * pppos_input_free_current_packet() here because - * rx thread might still call pppos_input(). + * rx IRQ might still call pppos_input(). */ -#if !PPP_INPROC_MULTITHREADED +#if !PPP_INPROC_IRQ_SAFE /* input pbuf left ? */ pppos_input_free_current_packet(pppos); -#endif /* !PPP_INPROC_MULTITHREADED */ +#endif /* !PPP_INPROC_IRQ_SAFE */ ppp_link_end(ppp); /* notify upper layers */ } @@ -460,16 +460,16 @@ pppos_destroy(ppp_pcb *ppp, void *ctx) pppos_pcb *pppos = (pppos_pcb *)ctx; LWIP_UNUSED_ARG(ppp); -#if PPP_INPROC_MULTITHREADED +#if PPP_INPROC_IRQ_SAFE /* input pbuf left ? */ pppos_input_free_current_packet(pppos); -#endif /* PPP_INPROC_MULTITHREADED */ +#endif /* PPP_INPROC_IRQ_SAFE */ memp_free(MEMP_PPPOS_PCB, pppos); return ERR_OK; } -#if !NO_SYS && !PPP_INPROC_MULTITHREADED +#if !NO_SYS && !PPP_INPROC_IRQ_SAFE /** Pass received raw characters to PPPoS to be decoded through lwIP TCPIP thread. * * @param pcb PPP descriptor index, returned by pppos_create() @@ -506,11 +506,11 @@ err_t pppos_input_sys(struct pbuf *p, struct netif *inp) { pbuf_free(p); return ERR_OK; } -#endif /* !NO_SYS && !PPP_INPROC_MULTITHREADED */ +#endif /* !NO_SYS && !PPP_INPROC_IRQ_SAFE */ /** PPPoS input helper struct, must be packed since it is stored * to pbuf->payload, which might be unaligned. */ -#if PPP_INPROC_MULTITHREADED +#if PPP_INPROC_IRQ_SAFE #ifdef PACK_STRUCT_USE_INCLUDES # include "arch/bpstruct.h" #endif @@ -522,16 +522,9 @@ PACK_STRUCT_END #ifdef PACK_STRUCT_USE_INCLUDES # include "arch/epstruct.h" #endif -#endif /* PPP_INPROC_MULTITHREADED */ +#endif /* PPP_INPROC_IRQ_SAFE */ /** Pass received raw characters to PPPoS to be decoded. - * - * This function is thread-safe if PPP_INPROC_MULTITHREADED is set to 1 - * and can be called from a dedicated RX-thread or from interrupt context - * BUT you should NEVER call pppos_connect(), pppos_listen() - * and ppp_free() if pppos_input() can still be running, doing this is not - * thread safe. You should also avoid calling pppos_input() if PPPoS session - * is not started yet. * * @param pcb PPP descriptor index, returned by pppos_create() * @param data received data @@ -618,16 +611,16 @@ pppos_input(ppp_pcb *ppp, u8_t *s, int l) /* hide the room for Ethernet forwarding header */ pbuf_header(inp, -(s16_t)PBUF_LINK_HLEN); #endif /* IP_FORWARD || LWIP_IPV6_FORWARD */ -#if PPP_INPROC_MULTITHREADED +#if PPP_INPROC_IRQ_SAFE if(tcpip_callback_with_block(pppos_input_callback, inp, 0) != ERR_OK) { PPPDEBUG(LOG_ERR, ("pppos_input[%d]: tcpip_callback() failed, dropping packet\n", ppp->netif->num)); pbuf_free(inp); LINK_STATS_INC(link.drop); snmp_inc_ifindiscards(ppp->netif); } -#else /* PPP_INPROC_MULTITHREADED */ +#else /* PPP_INPROC_IRQ_SAFE */ ppp_input(ppp, inp); -#endif /* PPP_INPROC_MULTITHREADED */ +#endif /* PPP_INPROC_IRQ_SAFE */ } /* Prepare for a new packet. */ @@ -740,11 +733,11 @@ pppos_input(ppp_pcb *ppp, u8_t *s, int l) } if (pppos->in_head == NULL) { u8_t *payload = ((u8_t*)next_pbuf->payload) + pbuf_alloc_len; -#if PPP_INPROC_MULTITHREADED +#if PPP_INPROC_IRQ_SAFE ((struct pppos_input_header*)payload)->ppp = ppp; payload += sizeof(struct pppos_input_header); next_pbuf->len += sizeof(struct pppos_input_header); -#endif /* PPP_INPROC_MULTITHREADED */ +#endif /* PPP_INPROC_IRQ_SAFE */ next_pbuf->len += sizeof(pppos->in_protocol); *(payload++) = pppos->in_protocol >> 8; *(payload) = pppos->in_protocol & 0xFF; @@ -767,7 +760,7 @@ pppos_input(ppp_pcb *ppp, u8_t *s, int l) magic_randomize(); } -#if PPP_INPROC_MULTITHREADED +#if PPP_INPROC_IRQ_SAFE /* PPPoS input callback using one input pointer */ static void pppos_input_callback(void *arg) { @@ -789,7 +782,7 @@ drop: snmp_inc_ifindiscards(ppp->netif); pbuf_free(pb); } -#endif /* PPP_INPROC_MULTITHREADED */ +#endif /* PPP_INPROC_IRQ_SAFE */ static void pppos_send_config(ppp_pcb *ppp, void *ctx, u32_t accm, int pcomp, int accomp)