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

[Lucas Stach]

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)