drivers: net: ot: Provide structure instance for HDLC RCP context

[lmajewski]

Without this patch the hdlc_iface_init() function assigns to ot_hdlc_rcp_context structure pointer (*ctx) value of 0, as in the NET_DEVICE_DT_INST_DEFINE() preprocessor macro the 'data' field is set to NULL.

Afterwards, the ctx->iface is set to iface address passed to the function (as well as the ctx->ot_context is set).
Writing those values to address 0x0 is catastrophic to for example mimxrt1020, which uses ITCM memory (mapped from 0x0) to store flash handling functions, as those are used to XIP code directly from SPI NOR memory (as mximxrt1020 doesn't have internal flash).

In this particular case - the flash_flexspi_nor_erase() function is mapped (i.e. relocated) to ITCM's 0x0 address. Overwriting first 8 bytes of it causes the SoC to enter "Precise data bus error" exception.

The fix is to define the static instance of struct ot_hdlc_rcp_context and pass its address to the NET_DEVICE_DT_INST_DEFINE() macro. As a result its storage is now in RAM, not ITCM.

This issue has been discovered on UART based HDLC RCP communication, but as it also may be problematic on the NXP driver, this patch fixes it too.

[decsny]

@xavraz

[lmajewski]

Gentle ping on this issue ....

[mmahadevan108]

@JA-NXP @xavraz can you help take a look at the PR.

[xavraz]

Hello @lmajewski,
Please, find here, the call stack during the initialisation step :
hdlc_iface_init@0x18014690 (<user_path>\zephyr_downstream\zephyr\drivers\hdlc_rcp_if\hdlc_rcp_if_nxp.c:55)
init_iface@0x1800ef9e (<user_path>\zephyr_downstream\zephyr\subsys\net\ip\net_if.c:446)
net_if_init@0x1800ef9e (<user_path>\zephyr_downstream\zephyr\subsys\net\ip\net_if.c:6162)
init_rx_queues@0x1800c482 (<user_path>\zephyr_downstream\zephyr\subsys\net\ip\net_core.c:585)
net_init@0x1800c482 (<user_path>\zephyr_downstream\zephyr\subsys\net\ip\net_core.c:641)
z_sys_init_run_level@0x1802c0c0 (<user_path>\zephyr_downstream\zephyr\kernel\init.c:371)
bg_thread_main@0x1802c0e0 (<user_path>\zephyr_downstream\zephyr\kernel\init.c:519)
z_thread_entry@0x18005c74 (<user_path>\zephyr_downstream\zephyr\lib\os\thread_entry.c:48)
arch_switch_to_main_thread@0x1800ac9a (<user_path>\zephyr_downstream\zephyr\arch\arm\core\cortex_m\thread.c:575)

There is no problem with the NET_DEVICE_DT_INST_DEFINE macro and the "No context data" field set to NULL as you can see with the previous call stack.

I can understand the static ot_hdlc_rcp_ctx variable instantiation can change the mapping. But the ot_hdlc_rcp_ctx variable is not used in the hdlc_iface_init() function.

Why the following code is not done ?
*struct ot_hdlc_rcp_context ctx = net_if_get_device(iface)->data;
replaced by
*ot_hdlc_rcp_ctx = net_if_get_device(iface)->data;

Thanks

[lmajewski]

@xavraz - Thanks for your input.

There is no problem with the NET_DEVICE_DT_INST_DEFINE macro and the "No context data" field set to NULL as you can see with the previous call stack.

IMHO the issue is that you currently have a dereference to the area of memory starting from 0x0, which in case MIMXRT1020 is a valid one and has flash driver code necessary for XIP.

As both hdlc_rcp_if_uart.c and hdlc_rcp_if_nxp.c define struct ot_hdlc_rcp_context structure - it shall be passed as "context data" to NET_DEVICE_DT_INST_DEFINE() macro.

I can understand the static ot_hdlc_rcp_ctx variable instantiation can change the mapping.

There is no instantiation - without this patch the struct ot_hdlc_rcp_context structure is "laid" on 0x0 address and then in hdlc_iface_init() the call to ctx->ot_context = net_if_l2_data(iface); overwrites 4 bytes at 0x0 memory, which on MIMXRT1020 has code for flash erasing function.

But the ot_hdlc_rcp_ctx variable is not used in the hdlc_iface_init() function.

In the hdlc_iface_init() function the pointer (*ctx) to static struct ot_hdlc_rcp_context is used. And for the NET_DEVICE_DT_INST_DEFINE() definition the pointer to instantiated static struct ot_hdlc_rcp_context - in this case pointer to ot_hdlc_rcp_ctx.

Why the following code is not done ?
*struct ot_hdlc_rcp_context ctx = net_if_get_device(iface)->data;

It shall be (as it is now): struct ot_hdlc_rcp_context *ctx = net_if_get_device(iface)->data;

replaced by
*ot_hdlc_rcp_ctx = net_if_get_device(iface)->data;

Is the *ot_hdlc_rcp_ctx valid ?

With the struct ot_hdlc_rcp_context *ctx = net_if_get_device(iface)->data; I've followed Zephyr's paradigm to use pointer - very often - named as ctx. This is also the code used without this patch.

I think that this approach is correct.

[xavraz]

@lmajewski ,
Thanks for the explanation. The previous /* No context data */ field shall be initialized to the &ot_hdlc_rcp_ctx, you are right.

It would be better to change the comment /* No context data */, now.

Proposal :
/* driver context structure address */

[JA-NXP]

Comment should be updated /* No context data */