[Bugfix rtems-lwip 4/4] Fix crash due to link_detect_thread
Bernd Moessner
berndmoessner80 at gmail.com
Tue Apr 2 07:51:32 UTC 2024
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.moore at 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 <berndmoessner80 at 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 at rtems.org
>> http://lists.rtems.org/mailman/listinfo/devel
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20240402/c63bc88e/attachment-0001.htm>
More information about the devel
mailing list