[Bugfix rtems-lwip 4/4] Fix crash due to link_detect_thread
Kinsey Moore
kinsey.moore at oarcorp.com
Tue Apr 2 14:04:31 UTC 2024
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 <berndmoessner80 at 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.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
>>>
>> _______________________________________________
> 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/3d73ca7f/attachment-0001.htm>
More information about the devel
mailing list