2023-03-07 15:57:04

by Srinivas Kandagatla

[permalink] [raw]
Subject: [PATCH 1/2] tty: serial: qcom-geni-serial: check correct dma address before unprep

looks like there was a typo while checking validatity of tx_dma_addr, the
code was checking rx instead of tx.
This can potentially lead to memory leak, this patch fixes the typo.

Fixes: 2aaa43c70778 ("tty: serial: qcom-geni-serial: add support for serial engine DMA")
Signed-off-by: Srinivas Kandagatla <[email protected]>
Reviewed-by: Bartosz Golaszewski <[email protected]>
---
drivers/tty/serial/qcom_geni_serial.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index d69592e5e2ec..5972b5c317d3 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -596,7 +596,7 @@ static void qcom_geni_serial_stop_tx_dma(struct uart_port *uport)
if (!qcom_geni_serial_main_active(uport))
return;

- if (port->rx_dma_addr) {
+ if (port->tx_dma_addr) {
geni_se_tx_dma_unprep(&port->se, port->tx_dma_addr,
port->tx_remaining);
port->tx_dma_addr = 0;
--
2.21.0



2023-03-07 15:57:08

by Srinivas Kandagatla

[permalink] [raw]
Subject: [PATCH 2/2] tty: serial: qcom-geni-serial: check for valid size before starting dma

Check if there are valid length of bytes to transfer before starting dma.

without this check we can see below kernel warning when we try to map a zero size buffers.

------------[ cut here ]------------
WARNING: CPU: 0 PID: 0 at drivers/iommu/dma-iommu.c:1046 iommu_dma_unmap_page+0xe0/0xfc
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 6.3.0-rc1-dirty #347
Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
pstate: 804000c5 (Nzcv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : iommu_dma_unmap_page+0xe0/0xfc
lr : iommu_dma_unmap_page+0x38/0xfc
...
Call trace:
iommu_dma_unmap_page+0xe0/0xfc
dma_unmap_page_attrs+0x30/0x1ec
geni_se_tx_dma_unprep+0x58/0x80
qcom_geni_serial_isr+0x350/0x750
__handle_irq_event_percpu+0x58/0x148
handle_irq_event_percpu+0x18/0x4c
handle_irq_event+0x48/0x88
handle_fasteoi_irq+0xb0/0x130
generic_handle_domain_irq+0x2c/0x44
gic_handle_irq+0xd4/0x140
call_on_irq_stack+0x24/0x4c
do_interrupt_handler+0x80/0x84
el1_interrupt+0x38/0x6c
el1h_64_irq_handler+0x18/0x24
el1h_64_irq+0x64/0x68
cpuidle_enter_state+0x1e4/0x310
cpuidle_enter+0x3c/0x54
call_cpuidle+0x1c/0x40
do_idle+0x204/0x260
cpu_startup_entry+0x28/0x2c
kernel_init+0x0/0x12c
arch_post_acpi_subsys_init+0x0/0x8
start_kernel+0x3cc/0x74c
__primary_switched+0xbc/0xc4

Fixes: 2aaa43c70778 ("tty: serial: qcom-geni-serial: add support for serial engine DMA")
Signed-off-by: Srinivas Kandagatla <[email protected]>
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/tty/serial/qcom_geni_serial.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 5972b5c317d3..bb63a00f4c07 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -637,6 +637,9 @@ static void qcom_geni_serial_start_tx_dma(struct uart_port *uport)

xmit_size = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE);

+ if (!xmit_size)
+ return;
+
qcom_geni_serial_setup_tx(uport, xmit_size);

ret = geni_se_tx_dma_prep(&port->se, &xmit->buf[xmit->tail],
--
2.21.0


2023-03-07 16:20:02

by Konrad Dybcio

[permalink] [raw]
Subject: Re: [PATCH 1/2] tty: serial: qcom-geni-serial: check correct dma address before unprep



On 7.03.2023 16:55, Srinivas Kandagatla wrote:
> looks like there was a typo while checking validatity of tx_dma_addr, the
> code was checking rx instead of tx.
> This can potentially lead to memory leak, this patch fixes the typo.
>
> Fixes: 2aaa43c70778 ("tty: serial: qcom-geni-serial: add support for serial engine DMA")
> Signed-off-by: Srinivas Kandagatla <[email protected]>
> Reviewed-by: Bartosz Golaszewski <[email protected]>
> ---
Reviewed-by: Konrad Dybcio <[email protected]>

Konrad
> drivers/tty/serial/qcom_geni_serial.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index d69592e5e2ec..5972b5c317d3 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -596,7 +596,7 @@ static void qcom_geni_serial_stop_tx_dma(struct uart_port *uport)
> if (!qcom_geni_serial_main_active(uport))
> return;
>
> - if (port->rx_dma_addr) {
> + if (port->tx_dma_addr) {
> geni_se_tx_dma_unprep(&port->se, port->tx_dma_addr,
> port->tx_remaining);
> port->tx_dma_addr = 0;

2023-03-07 16:21:13

by Konrad Dybcio

[permalink] [raw]
Subject: Re: [PATCH 2/2] tty: serial: qcom-geni-serial: check for valid size before starting dma



On 7.03.2023 16:55, Srinivas Kandagatla wrote:
> Check if there are valid length of bytes to transfer before starting dma.
>
> without this check we can see below kernel warning when we try to map a zero size buffers.
>
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 0 at drivers/iommu/dma-iommu.c:1046 iommu_dma_unmap_page+0xe0/0xfc
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 6.3.0-rc1-dirty #347
> Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
> pstate: 804000c5 (Nzcv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : iommu_dma_unmap_page+0xe0/0xfc
> lr : iommu_dma_unmap_page+0x38/0xfc
> ...
> Call trace:
> iommu_dma_unmap_page+0xe0/0xfc
> dma_unmap_page_attrs+0x30/0x1ec
> geni_se_tx_dma_unprep+0x58/0x80
> qcom_geni_serial_isr+0x350/0x750
> __handle_irq_event_percpu+0x58/0x148
> handle_irq_event_percpu+0x18/0x4c
> handle_irq_event+0x48/0x88
> handle_fasteoi_irq+0xb0/0x130
> generic_handle_domain_irq+0x2c/0x44
> gic_handle_irq+0xd4/0x140
> call_on_irq_stack+0x24/0x4c
> do_interrupt_handler+0x80/0x84
> el1_interrupt+0x38/0x6c
> el1h_64_irq_handler+0x18/0x24
> el1h_64_irq+0x64/0x68
> cpuidle_enter_state+0x1e4/0x310
> cpuidle_enter+0x3c/0x54
> call_cpuidle+0x1c/0x40
> do_idle+0x204/0x260
> cpu_startup_entry+0x28/0x2c
> kernel_init+0x0/0x12c
> arch_post_acpi_subsys_init+0x0/0x8
> start_kernel+0x3cc/0x74c
> __primary_switched+0xbc/0xc4
>
> Fixes: 2aaa43c70778 ("tty: serial: qcom-geni-serial: add support for serial engine DMA")
> Signed-off-by: Srinivas Kandagatla <[email protected]>
S-o-b but no C-d-b? Weird..

> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
Anyway, the change is good!

Reviewed-by: Konrad Dybcio <[email protected]>

Konrad
> drivers/tty/serial/qcom_geni_serial.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 5972b5c317d3..bb63a00f4c07 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -637,6 +637,9 @@ static void qcom_geni_serial_start_tx_dma(struct uart_port *uport)
>
> xmit_size = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE);
>
> + if (!xmit_size)
> + return;
> +
> qcom_geni_serial_setup_tx(uport, xmit_size);
>
> ret = geni_se_tx_dma_prep(&port->se, &xmit->buf[xmit->tail],

2023-03-07 16:47:56

by Bartosz Golaszewski

[permalink] [raw]
Subject: Re: [PATCH 2/2] tty: serial: qcom-geni-serial: check for valid size before starting dma

On Tue, 7 Mar 2023 at 17:20, Konrad Dybcio <[email protected]> wrote:
>
>
>
> On 7.03.2023 16:55, Srinivas Kandagatla wrote:
> > Check if there are valid length of bytes to transfer before starting dma.
> >
> > without this check we can see below kernel warning when we try to map a zero size buffers.
> >
> > ------------[ cut here ]------------
> > WARNING: CPU: 0 PID: 0 at drivers/iommu/dma-iommu.c:1046 iommu_dma_unmap_page+0xe0/0xfc
> > Modules linked in:
> > CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 6.3.0-rc1-dirty #347
> > Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
> > pstate: 804000c5 (Nzcv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > pc : iommu_dma_unmap_page+0xe0/0xfc
> > lr : iommu_dma_unmap_page+0x38/0xfc
> > ...
> > Call trace:
> > iommu_dma_unmap_page+0xe0/0xfc
> > dma_unmap_page_attrs+0x30/0x1ec
> > geni_se_tx_dma_unprep+0x58/0x80
> > qcom_geni_serial_isr+0x350/0x750
> > __handle_irq_event_percpu+0x58/0x148
> > handle_irq_event_percpu+0x18/0x4c
> > handle_irq_event+0x48/0x88
> > handle_fasteoi_irq+0xb0/0x130
> > generic_handle_domain_irq+0x2c/0x44
> > gic_handle_irq+0xd4/0x140
> > call_on_irq_stack+0x24/0x4c
> > do_interrupt_handler+0x80/0x84
> > el1_interrupt+0x38/0x6c
> > el1h_64_irq_handler+0x18/0x24
> > el1h_64_irq+0x64/0x68
> > cpuidle_enter_state+0x1e4/0x310
> > cpuidle_enter+0x3c/0x54
> > call_cpuidle+0x1c/0x40
> > do_idle+0x204/0x260
> > cpu_startup_entry+0x28/0x2c
> > kernel_init+0x0/0x12c
> > arch_post_acpi_subsys_init+0x0/0x8
> > start_kernel+0x3cc/0x74c
> > __primary_switched+0xbc/0xc4
> >
> > Fixes: 2aaa43c70778 ("tty: serial: qcom-geni-serial: add support for serial engine DMA")
> > Signed-off-by: Srinivas Kandagatla <[email protected]>
> S-o-b but no C-d-b? Weird..
>

It was supposed to be Reviewed-by actually.

Bart

> > Signed-off-by: Bartosz Golaszewski <[email protected]>
> > ---
> Anyway, the change is good!
>
> Reviewed-by: Konrad Dybcio <[email protected]>
>
> Konrad
> > drivers/tty/serial/qcom_geni_serial.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> > index 5972b5c317d3..bb63a00f4c07 100644
> > --- a/drivers/tty/serial/qcom_geni_serial.c
> > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > @@ -637,6 +637,9 @@ static void qcom_geni_serial_start_tx_dma(struct uart_port *uport)
> >
> > xmit_size = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE);
> >
> > + if (!xmit_size)
> > + return;
> > +
> > qcom_geni_serial_setup_tx(uport, xmit_size);
> >
> > ret = geni_se_tx_dma_prep(&port->se, &xmit->buf[xmit->tail],

2023-03-07 16:53:07

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH 2/2] tty: serial: qcom-geni-serial: check for valid size before starting dma



On 07/03/2023 16:20, Konrad Dybcio wrote:
>
>
> On 7.03.2023 16:55, Srinivas Kandagatla wrote:
>> Check if there are valid length of bytes to transfer before starting dma.
>>
>> without this check we can see below kernel warning when we try to map a zero size buffers.
>>
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 0 at drivers/iommu/dma-iommu.c:1046 iommu_dma_unmap_page+0xe0/0xfc
>> Modules linked in:
>> CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 6.3.0-rc1-dirty #347
>> Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
>> pstate: 804000c5 (Nzcv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>> pc : iommu_dma_unmap_page+0xe0/0xfc
>> lr : iommu_dma_unmap_page+0x38/0xfc
>> ...
>> Call trace:
>> iommu_dma_unmap_page+0xe0/0xfc
>> dma_unmap_page_attrs+0x30/0x1ec
>> geni_se_tx_dma_unprep+0x58/0x80
>> qcom_geni_serial_isr+0x350/0x750
>> __handle_irq_event_percpu+0x58/0x148
>> handle_irq_event_percpu+0x18/0x4c
>> handle_irq_event+0x48/0x88
>> handle_fasteoi_irq+0xb0/0x130
>> generic_handle_domain_irq+0x2c/0x44
>> gic_handle_irq+0xd4/0x140
>> call_on_irq_stack+0x24/0x4c
>> do_interrupt_handler+0x80/0x84
>> el1_interrupt+0x38/0x6c
>> el1h_64_irq_handler+0x18/0x24
>> el1h_64_irq+0x64/0x68
>> cpuidle_enter_state+0x1e4/0x310
>> cpuidle_enter+0x3c/0x54
>> call_cpuidle+0x1c/0x40
>> do_idle+0x204/0x260
>> cpu_startup_entry+0x28/0x2c
>> kernel_init+0x0/0x12c
>> arch_post_acpi_subsys_init+0x0/0x8
>> start_kernel+0x3cc/0x74c
>> __primary_switched+0xbc/0xc4
>>
>> Fixes: 2aaa43c70778 ("tty: serial: qcom-geni-serial: add support for serial engine DMA")
>> Signed-off-by: Srinivas Kandagatla <[email protected]>
> S-o-b but no C-d-b? Weird..
>
Yes, I did mess up this part.
thanks for spotting this, Will fix this in next spin!

--srini
>> Signed-off-by: Bartosz Golaszewski <[email protected]>
>> ---
> Anyway, the change is good!
>
> Reviewed-by: Konrad Dybcio <[email protected]>
>
> Konrad
>> drivers/tty/serial/qcom_geni_serial.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
>> index 5972b5c317d3..bb63a00f4c07 100644
>> --- a/drivers/tty/serial/qcom_geni_serial.c
>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>> @@ -637,6 +637,9 @@ static void qcom_geni_serial_start_tx_dma(struct uart_port *uport)
>>
>> xmit_size = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE);
>>
>> + if (!xmit_size)
>> + return;
>> +
>> qcom_geni_serial_setup_tx(uport, xmit_size);
>>
>> ret = geni_se_tx_dma_prep(&port->se, &xmit->buf[xmit->tail],