I'd have to see what you're describing. If you'd like, I can reformulate the patch as well.
Kinsey On Tue, Apr 2, 2024 at 2:51 AM Bernd Moessner <berndmoessne...@gmail.com> wrote: > Is it ok to implement it using a single ifdef __rtems__ else ? The driver > is already a ifdef - hell and it will become even less readable when I add > even more of them. > > Kinsey Moore <kinsey.mo...@oarcorp.com> schrieb am Mo., 1. Apr. 2024, > 16:39: > >> The intent of this patch is fine, but changes should be purely additive >> and properly wrapped in the __rtems__ check. The addition of thread startup >> at the end should also be wrapped in !NO_SYS. >> >> Kinsey >> >> On Sun, Mar 31, 2024 at 5:49 PM Bernd Moessner <berndmoessne...@gmail.com> >> wrote: >> >>> Within xemac_add, the link_detect_thread is set up before the ethernet >>> interface is configured and all data structures have been set up >>> correctly. >>> The steps within xemac_add are basically: >>> 1) Set up link_detect_thread >>> 2) Initialize the interface >>> 3) Set up the link speed / start autonegotiation >>> 4) When autonegotiate has completed, set up the data structures >>> >>> The issue is here that, for example if no cable is connected, the >>> autonegotiate sequence will suspend the calling task. As soon as >>> this happens, the link_detect_thread gets into running state and >>> dereferences the data structures that havent been set up. Unfortunatelly, >>> it isnt as easy as checking the pointer to the relevant data structure >>> for being NULL within the link_detect_thread. Not going into details, >>> the pointer (netif->state) isnt NULL in this moment of time, but it >>> has to point to something completely different. >>> >>> It seems to me that the easiest solution to this problem is to reorder >>> the sequence within xemac_add. Initialize the interface and set up the >>> data structures, then create the link_detect_thread which requires all of >>> this. After applying this patch, the sequence becomes: >>> >>> 1) Initialize the interface >>> 2) Set up the link speed / start autonegotiation >>> 3) When autonegotiate has completed, set up the data structures >>> 4) Set up link_detect_thread >>> --- >>> .../src/contrib/ports/xilinx/netif/xadapter.c | 32 ++++++++++++++----- >>> 1 file changed, 24 insertions(+), 8 deletions(-) >>> >>> diff --git >>> a/embeddedsw/ThirdParty/sw_services/lwip211/src/contrib/ports/xilinx/netif/xadapter.c >>> b/embeddedsw/ThirdParty/sw_services/lwip211/src/contrib/ports/xilinx/netif/xadapter.c >>> index 93ff148..79a10c1 100644 >>> --- >>> a/embeddedsw/ThirdParty/sw_services/lwip211/src/contrib/ports/xilinx/netif/xadapter.c >>> +++ >>> b/embeddedsw/ThirdParty/sw_services/lwip211/src/contrib/ports/xilinx/netif/xadapter.c >>> @@ -126,8 +126,9 @@ xemac_add(struct netif *netif, >>> UINTPTR mac_baseaddr) >>> { >>> int i; >>> + struct netif * ret = NULL; >>> >>> -#if !NO_SYS >>> +#if !NO_SYS && !defined(__rtems__) >>> /* Start thread to detect link periodically for Hot Plug >>> autodetect */ >>> sys_thread_new("link_detect_thread", link_detect_thread, netif, >>> THREAD_STACKSIZE, tskIDLE_PRIORITY); >>> @@ -142,7 +143,7 @@ xemac_add(struct netif *netif, >>> switch (find_mac_type(mac_baseaddr)) { >>> case xemac_type_xps_emaclite: >>> #ifdef XLWIP_CONFIG_INCLUDE_EMACLITE >>> - return netif_add(netif, ipaddr, netmask, >>> gw, >>> + ret = netif_add(netif, ipaddr, netmask, >>> gw, >>> (void*)mac_baseaddr, >>> xemacliteif_init, >>> #if NO_SYS >>> @@ -151,12 +152,14 @@ xemac_add(struct netif *netif, >>> tcpip_input >>> #endif >>> ); >>> + break; >>> #else >>> - return NULL; >>> + ret = NULL; >>> + break; >>> #endif >>> case xemac_type_axi_ethernet: >>> #ifdef XLWIP_CONFIG_INCLUDE_AXI_ETHERNET >>> - return netif_add(netif, ipaddr, netmask, >>> gw, >>> + ret = netif_add(netif, ipaddr, netmask, >>> gw, >>> (void*)mac_baseaddr, >>> xaxiemacif_init, >>> #if NO_SYS >>> @@ -165,16 +168,18 @@ xemac_add(struct netif *netif, >>> tcpip_input >>> #endif >>> ); >>> + break; >>> #else >>> - return NULL; >>> + ret = NULL; >>> + break; >>> #endif >>> #if defined (__arm__) || defined (__aarch64__) >>> case xemac_type_emacps: >>> #ifdef XLWIP_CONFIG_INCLUDE_GEM >>> #ifndef __rtems__ >>> - return netif_add(netif, ipaddr, netmask, >>> gw, >>> + ret = netif_add(netif, ipaddr, netmask, >>> gw, >>> #else /* __rtems__ */ >>> - return netif_add( netif, >>> + ret = netif_add( netif, >>> (const ip4_addr_t *) >>> ipaddr, >>> (const ip4_addr_t *) >>> netmask, >>> (const ip4_addr_t *) gw, >>> @@ -188,7 +193,10 @@ xemac_add(struct netif *netif, >>> #endif >>> >>> ); >>> + break; >>> #endif >>> + ret = NULL; >>> + break; >>> #endif >>> default: >>> #ifndef __rtems__ >>> @@ -199,8 +207,16 @@ xemac_add(struct netif *netif, >>> mac_baseaddr); >>> xil_printf("\r\n"); >>> #endif >>> - return NULL; >>> + ret = NULL; >>> } >>> + >>> +#if defined(__rtems__) >>> + /* Start thread to detect link periodically for Hot Plug >>> autodetect */ >>> + sys_thread_new("link_detect_thread", link_detect_thread, netif, >>> + THREAD_STACKSIZE, tskIDLE_PRIORITY); >>> +#endif >>> + >>> + return ret; >>> } >>> >>> #if !NO_SYS >>> -- >>> 2.34.1 >>> >>> _______________________________________________ >>> devel mailing list >>> devel@rtems.org >>> http://lists.rtems.org/mailman/listinfo/devel >>> >> _______________________________________________ > devel mailing list > devel@rtems.org > http://lists.rtems.org/mailman/listinfo/devel
_______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel