* [PATCH] dmaengine: imx-sdma: fix regression with uart scripts
@ 2022-04-06 22:48 Kevin Groeneveld
2022-04-07 10:18 ` Lucas Stach
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Kevin Groeneveld @ 2022-04-06 22:48 UTC (permalink / raw)
To: Vinod Koul, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, NXP Linux Team, Lucas Stach, Robin Gong,
dmaengine, linux-arm-kernel, linux-kernel
Cc: 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@lenbrook.com>
---
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)
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] dmaengine: imx-sdma: fix regression with uart scripts
2022-04-06 22:48 [PATCH] dmaengine: imx-sdma: fix regression with uart scripts Kevin Groeneveld
@ 2022-04-07 10:18 ` Lucas Stach
2022-04-08 1:23 ` Fabio Estevam
2022-04-08 17:39 ` Vinod Koul
2 siblings, 0 replies; 7+ messages in thread
From: Lucas Stach @ 2022-04-07 10:18 UTC (permalink / raw)
To: Kevin Groeneveld, Vinod Koul, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
Robin Gong, dmaengine, linux-arm-kernel, linux-kernel
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@lenbrook.com>
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@pengutronix.de>
> ---
> 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)
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] dmaengine: imx-sdma: fix regression with uart scripts
2022-04-06 22:48 [PATCH] dmaengine: imx-sdma: fix regression with uart scripts Kevin Groeneveld
2022-04-07 10:18 ` Lucas Stach
@ 2022-04-08 1:23 ` Fabio Estevam
2022-04-08 17:39 ` Vinod Koul
2 siblings, 0 replies; 7+ messages in thread
From: Fabio Estevam @ 2022-04-08 1:23 UTC (permalink / raw)
To: Kevin Groeneveld
Cc: Vinod Koul, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
NXP Linux Team, Lucas Stach, Robin Gong, dmaengine,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel
Hi Kevin,
On Wed, Apr 6, 2022 at 7:48 PM Kevin Groeneveld
<kgroeneveld@lenbrook.com> wrote:
>
> 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@lenbrook.com>
Thanks for the fix:
Reviewed-by: Fabio Estevam <festevam@gmail.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] dmaengine: imx-sdma: fix regression with uart scripts
2022-04-06 22:48 [PATCH] dmaengine: imx-sdma: fix regression with uart scripts Kevin Groeneveld
2022-04-07 10:18 ` Lucas Stach
2022-04-08 1:23 ` Fabio Estevam
@ 2022-04-08 17:39 ` Vinod Koul
2022-04-10 22:28 ` Kevin Groeneveld
2 siblings, 1 reply; 7+ messages in thread
From: Vinod Koul @ 2022-04-08 17:39 UTC (permalink / raw)
To: Kevin Groeneveld
Cc: Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
NXP Linux Team, Lucas Stach, Robin Gong, dmaengine,
linux-arm-kernel, linux-kernel
On 06-04-22, 18:48, Kevin Groeneveld wrote:
> 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.
1. Patch title should reflect the change introduced, so the title is not
apt, pls revise
2. Is this in response to rmk's report, if so, please add reported-by
3. Lastly, I would like to see some tested by for this patch..
>
> Fixes: b98ce2f4e32b ("dmaengine: imx-sdma: add uart rom script")
cc: stable ?
> Signed-off-by: Kevin Groeneveld <kgroeneveld@lenbrook.com>
> ---
> 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)
> --
> 2.17.1
--
~Vinod
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] dmaengine: imx-sdma: fix regression with uart scripts
2022-04-08 17:39 ` Vinod Koul
@ 2022-04-10 22:28 ` Kevin Groeneveld
2022-04-11 5:59 ` Vinod Koul
0 siblings, 1 reply; 7+ messages in thread
From: Kevin Groeneveld @ 2022-04-10 22:28 UTC (permalink / raw)
To: Vinod Koul
Cc: Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
NXP Linux Team, Lucas Stach, Robin Gong, dmaengine,
linux-arm-kernel, linux-kernel
On 2022-04-08 13:39, Vinod Koul wrote:
> 1. Patch title should reflect the change introduced, so the title is not
> apt, pls revise
In hindsight the title was not very descriptive. I will update and send
a v2. Maybe something like:
dmaengine: imx-sdma: fix init of uart scripts
> 2. Is this in response to rmk's report, if so, please add reported-by
No. I am not even aware of any report on this issue. I discovered the
issue on my own and found the problem commit by doing a bisect.
> 3. Lastly, I would like to see some tested by for this patch..
I have tested on imx5, imx6 and imx8 systems. I will add some brief
details of this to the commit message in the v2 patch. I am not sure if
I as the author should include a Tested-by tag.
>> Fixes: b98ce2f4e32b ("dmaengine: imx-sdma: add uart rom script")
>
> cc: stable ?
That sounds reasonable. I am relatively new to submitting kernel patches
and that thought never crossed by mind.
Kevin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] dmaengine: imx-sdma: fix regression with uart scripts
2022-04-10 22:28 ` Kevin Groeneveld
@ 2022-04-11 5:59 ` Vinod Koul
2022-04-11 7:16 ` Russell King (Oracle)
0 siblings, 1 reply; 7+ messages in thread
From: Vinod Koul @ 2022-04-11 5:59 UTC (permalink / raw)
To: Kevin Groeneveld, Russell King (Oracle)
Cc: Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
NXP Linux Team, Lucas Stach, Robin Gong, dmaengine,
linux-arm-kernel, linux-kernel
Hi Kevin,
On 10-04-22, 18:28, Kevin Groeneveld wrote:
> On 2022-04-08 13:39, Vinod Koul wrote:
> > 1. Patch title should reflect the change introduced, so the title is not
> > apt, pls revise
>
> In hindsight the title was not very descriptive. I will update and send a
> v2. Maybe something like:
>
> dmaengine: imx-sdma: fix init of uart scripts
>
> > 2. Is this in response to rmk's report, if so, please add reported-by
>
> No. I am not even aware of any report on this issue. I discovered the issue
> on my own and found the problem commit by doing a bisect.
Okay I am adding Russell here to see if this fixes his issue as well..
> > 3. Lastly, I would like to see some tested by for this patch..
>
> I have tested on imx5, imx6 and imx8 systems. I will add some brief details
> of this to the commit message in the v2 patch. I am not sure if I as the
> author should include a Tested-by tag.
>
> > > Fixes: b98ce2f4e32b ("dmaengine: imx-sdma: add uart rom script")
> >
> > cc: stable ?
>
> That sounds reasonable. I am relatively new to submitting kernel patches and
> that thought never crossed by mind.
No worries, fixes should be backport to stable kernels, refer Documentation/process/stable-kernel-rules.rst
Thanks
--
~Vinod
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] dmaengine: imx-sdma: fix regression with uart scripts
2022-04-11 5:59 ` Vinod Koul
@ 2022-04-11 7:16 ` Russell King (Oracle)
0 siblings, 0 replies; 7+ messages in thread
From: Russell King (Oracle) @ 2022-04-11 7:16 UTC (permalink / raw)
To: Vinod Koul
Cc: Kevin Groeneveld, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
Lucas Stach, Robin Gong, dmaengine, linux-arm-kernel,
linux-kernel
On Mon, Apr 11, 2022 at 11:29:23AM +0530, Vinod Koul wrote:
> Hi Kevin,
>
> On 10-04-22, 18:28, Kevin Groeneveld wrote:
> > On 2022-04-08 13:39, Vinod Koul wrote:
> > > 1. Patch title should reflect the change introduced, so the title is not
> > > apt, pls revise
> >
> > In hindsight the title was not very descriptive. I will update and send a
> > v2. Maybe something like:
> >
> > dmaengine: imx-sdma: fix init of uart scripts
> >
> > > 2. Is this in response to rmk's report, if so, please add reported-by
> >
> > No. I am not even aware of any report on this issue. I discovered the issue
> > on my own and found the problem commit by doing a bisect.
>
> Okay I am adding Russell here to see if this fixes his issue as well..
I "fixed" my issue by updating my SDMA firmware to the later version,
which I think is fine provided I don't down-grade the kernel.
That said, I do support this attempt to fix NXP's cockup, so, having
looked at the patch:
Acked-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-04-11 7:17 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-06 22:48 [PATCH] dmaengine: imx-sdma: fix regression with uart scripts Kevin Groeneveld
2022-04-07 10:18 ` Lucas Stach
2022-04-08 1:23 ` Fabio Estevam
2022-04-08 17:39 ` Vinod Koul
2022-04-10 22:28 ` Kevin Groeneveld
2022-04-11 5:59 ` Vinod Koul
2022-04-11 7:16 ` Russell King (Oracle)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).