Re: [PATCH] dmaengine: imx-sdma: fix regression with uart scripts

From: Lucas Stach
Date: Thu Apr 07 2022 - 06:19:15 EST


Am Mittwoch, dem 06.04.2022 um 18:48 -0400 schrieb Kevin Groeneveld:
> Commit b98ce2f4e32b ("dmaengine: imx-sdma: add uart rom script") broke
> uart rx on imx5 when using sdma firmware from older Freescale 2.6.35
> kernel. In this case reading addr->uartXX_2_mcu_addr was going out of
> bounds of the firmware memory and corrupting the uart script addresses.
>
> Simply adding a bounds check before accessing addr->uartXX_2_mcu_addr
> does not work as the uartXX_2_mcu_addr members are now beyond the size
> of the older firmware and the uart addresses would never be populated
> in that case. There are other ways to fix this but overall the logic
> seems clearer to me to revert the uartXX_2_mcu_ram_addr structure
> entries back to uartXX_2_mcu_addr, change the newer entries to
> uartXX_2_mcu_rom_addr and update the logic accordingly.
>
> Fixes: b98ce2f4e32b ("dmaengine: imx-sdma: add uart rom script")
> Signed-off-by: Kevin Groeneveld <kgroeneveld@xxxxxxxxxxxx>

I clearly didn't think about this case when reviewing the breaking
change. The solution in this patch looks fine to me.

Reviewed-by: Lucas Stach <l.stach@xxxxxxxxxxxxxx>

> ---
> drivers/dma/imx-sdma.c | 28 ++++++++++++++--------------
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 70c0aa931ddf..b708d029b6e9 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -198,12 +198,12 @@ struct sdma_script_start_addrs {
> s32 per_2_firi_addr;
> s32 mcu_2_firi_addr;
> s32 uart_2_per_addr;
> - s32 uart_2_mcu_ram_addr;
> + s32 uart_2_mcu_addr;
> s32 per_2_app_addr;
> s32 mcu_2_app_addr;
> s32 per_2_per_addr;
> s32 uartsh_2_per_addr;
> - s32 uartsh_2_mcu_ram_addr;
> + s32 uartsh_2_mcu_addr;
> s32 per_2_shp_addr;
> s32 mcu_2_shp_addr;
> s32 ata_2_mcu_addr;
> @@ -232,8 +232,8 @@ struct sdma_script_start_addrs {
> s32 mcu_2_ecspi_addr;
> s32 mcu_2_sai_addr;
> s32 sai_2_mcu_addr;
> - s32 uart_2_mcu_addr;
> - s32 uartsh_2_mcu_addr;
> + s32 uart_2_mcu_rom_addr;
> + s32 uartsh_2_mcu_rom_addr;
> /* End of v3 array */
> s32 mcu_2_zqspi_addr;
> /* End of v4 array */
> @@ -1796,17 +1796,17 @@ static void sdma_add_scripts(struct sdma_engine *sdma,
> saddr_arr[i] = addr_arr[i];
>
> /*
> - * get uart_2_mcu_addr/uartsh_2_mcu_addr rom script specially because
> - * they are now replaced by uart_2_mcu_ram_addr/uartsh_2_mcu_ram_addr
> - * to be compatible with legacy freescale/nxp sdma firmware, and they
> - * are located in the bottom part of sdma_script_start_addrs which are
> - * beyond the SDMA_SCRIPT_ADDRS_ARRAY_SIZE_V1.
> + * For compatibility with NXP internal legacy kernel before 4.19 which
> + * is based on uart ram script and mainline kernel based on uart rom
> + * script, both uart ram/rom scripts are present in newer sdma
> + * firmware. Use the rom versions if they are present (V3 or newer).
> */
> - if (addr->uart_2_mcu_addr)
> - sdma->script_addrs->uart_2_mcu_addr = addr->uart_2_mcu_addr;
> - if (addr->uartsh_2_mcu_addr)
> - sdma->script_addrs->uartsh_2_mcu_addr = addr->uartsh_2_mcu_addr;
> -
> + if (sdma->script_number >= SDMA_SCRIPT_ADDRS_ARRAY_SIZE_V3) {
> + if (addr->uart_2_mcu_rom_addr)
> + sdma->script_addrs->uart_2_mcu_addr = addr->uart_2_mcu_rom_addr;
> + if (addr->uartsh_2_mcu_rom_addr)
> + sdma->script_addrs->uartsh_2_mcu_addr = addr->uartsh_2_mcu_rom_addr;
> + }
> }
>
> static void sdma_load_firmware(const struct firmware *fw, void *context)