* [PATCH V2 0/2] Fix RX cancel command failure
@ 2020-02-25 13:54 satya priya
2020-02-25 13:54 ` [PATCH V2 1/2] tty: serial: qcom_geni_serial: Allocate port->rx_fifo buffer in probe satya priya
2020-02-25 13:54 ` [PATCH V2 2/2] tty: serial: qcom_geni_serial: Fix RX cancel command failure satya priya
0 siblings, 2 replies; 8+ messages in thread
From: satya priya @ 2020-02-25 13:54 UTC (permalink / raw)
To: gregkh
Cc: swboyd, mgautam, linux-arm-msm, linux-serial, akashast, rojay,
msavaliy, satya priya
Changes in V2:
- Add patch to allocate port->rx_fifo buffer in probe to resolve
NULL pointer dereference crash reported by stephen.
The crash is caused due to set_termios call starting RX engine and RX engine
is sampling some garbage data from line, and by the time startup is called,
we have few data to read but port->rx_fifo buffer is not allocated causing
NULL pointer dereference.
satya priya (2):
tty: serial: qcom_geni_serial: Allocate port->rx_fifo buffer in probe
tty: serial: qcom_geni_serial: Fix RX cancel command failure
drivers/tty/serial/qcom_geni_serial.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH V2 1/2] tty: serial: qcom_geni_serial: Allocate port->rx_fifo buffer in probe
2020-02-25 13:54 [PATCH V2 0/2] Fix RX cancel command failure satya priya
@ 2020-02-25 13:54 ` satya priya
2020-02-28 22:54 ` Stephen Boyd
2020-02-28 23:01 ` Stephen Boyd
2020-02-25 13:54 ` [PATCH V2 2/2] tty: serial: qcom_geni_serial: Fix RX cancel command failure satya priya
1 sibling, 2 replies; 8+ messages in thread
From: satya priya @ 2020-02-25 13:54 UTC (permalink / raw)
To: gregkh
Cc: swboyd, mgautam, linux-arm-msm, linux-serial, akashast, rojay,
msavaliy, satya priya
To fix the RX cancel command failure, rx_fifo buffer needs to be
flushed in stop_rx() by calling handle_rx().
If set_termios is called before startup, by this time memory is not
allocated to port->rx_fifo buffer, which leads to a NULL pointer
dereference.
To avoid this NULL pointer dereference allocate memory to port->rx_fifo
in probe itself.
Signed-off-by: satya priya <skakit@codeaurora.org>
---
drivers/tty/serial/qcom_geni_serial.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 191abb1..d2a909c 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -858,12 +858,6 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
false, false, true);
geni_se_init(&port->se, UART_RX_WM, port->rx_fifo_depth - 2);
geni_se_select_mode(&port->se, GENI_SE_FIFO);
- if (!uart_console(uport)) {
- port->rx_fifo = devm_kcalloc(uport->dev,
- port->rx_fifo_depth, sizeof(u32), GFP_KERNEL);
- if (!port->rx_fifo)
- return -ENOMEM;
- }
port->setup = true;
return 0;
@@ -1274,6 +1268,13 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
+ if (!console) {
+ port->rx_fifo = devm_kcalloc(uport->dev,
+ port->rx_fifo_depth, sizeof(u32), GFP_KERNEL);
+ if (!port->rx_fifo)
+ return -ENOMEM;
+ }
+
port->name = devm_kasprintf(uport->dev, GFP_KERNEL,
"qcom_geni_serial_%s%d",
uart_console(uport) ? "console" : "uart", uport->line);
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH V2 2/2] tty: serial: qcom_geni_serial: Fix RX cancel command failure
2020-02-25 13:54 [PATCH V2 0/2] Fix RX cancel command failure satya priya
2020-02-25 13:54 ` [PATCH V2 1/2] tty: serial: qcom_geni_serial: Allocate port->rx_fifo buffer in probe satya priya
@ 2020-02-25 13:54 ` satya priya
1 sibling, 0 replies; 8+ messages in thread
From: satya priya @ 2020-02-25 13:54 UTC (permalink / raw)
To: gregkh
Cc: swboyd, mgautam, linux-arm-msm, linux-serial, akashast, rojay,
msavaliy, satya priya
RX cancel command fails when BT is switched on and off multiple times.
To handle this, poll for the cancel bit in SE_GENI_S_IRQ_STATUS register
instead of SE_GENI_S_CMD_CTRL_REG.
As per the HPG update, handle the RX last bit after cancel command
and flush out the RX FIFO buffer.
Signed-off-by: satya priya <skakit@codeaurora.org>
---
drivers/tty/serial/qcom_geni_serial.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index d2a909c..22d99b0 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -129,6 +129,7 @@ static int handle_rx_console(struct uart_port *uport, u32 bytes, bool drop);
static int handle_rx_uart(struct uart_port *uport, u32 bytes, bool drop);
static unsigned int qcom_geni_serial_tx_empty(struct uart_port *port);
static void qcom_geni_serial_stop_rx(struct uart_port *uport);
+static void qcom_geni_serial_handle_rx(struct uart_port *uport, bool drop);
static const unsigned long root_freq[] = {7372800, 14745600, 19200000, 29491200,
32000000, 48000000, 64000000, 80000000,
@@ -599,7 +600,7 @@ static void qcom_geni_serial_stop_rx(struct uart_port *uport)
u32 irq_en;
u32 status;
struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
- u32 irq_clear = S_CMD_DONE_EN;
+ u32 s_irq_status;
irq_en = readl(uport->membase + SE_GENI_S_IRQ_EN);
irq_en &= ~(S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN);
@@ -615,10 +616,19 @@ static void qcom_geni_serial_stop_rx(struct uart_port *uport)
return;
geni_se_cancel_s_cmd(&port->se);
- qcom_geni_serial_poll_bit(uport, SE_GENI_S_CMD_CTRL_REG,
- S_GENI_CMD_CANCEL, false);
+ qcom_geni_serial_poll_bit(uport, SE_GENI_S_IRQ_STATUS,
+ S_CMD_CANCEL_EN, true);
+ /*
+ * If timeout occurs secondary engine remains active
+ * and Abort sequence is executed.
+ */
+ s_irq_status = readl(uport->membase + SE_GENI_S_IRQ_STATUS);
+ /* Flush the Rx buffer */
+ if (s_irq_status & S_RX_FIFO_LAST_EN)
+ qcom_geni_serial_handle_rx(uport, true);
+ writel(s_irq_status, uport->membase + SE_GENI_S_IRQ_CLEAR);
+
status = readl(uport->membase + SE_GENI_STATUS);
- writel(irq_clear, uport->membase + SE_GENI_S_IRQ_CLEAR);
if (status & S_GENI_CMD_ACTIVE)
qcom_geni_serial_abort_rx(uport);
}
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH V2 1/2] tty: serial: qcom_geni_serial: Allocate port->rx_fifo buffer in probe
2020-02-25 13:54 ` [PATCH V2 1/2] tty: serial: qcom_geni_serial: Allocate port->rx_fifo buffer in probe satya priya
@ 2020-02-28 22:54 ` Stephen Boyd
2020-03-04 13:38 ` skakit
2020-02-28 23:01 ` Stephen Boyd
1 sibling, 1 reply; 8+ messages in thread
From: Stephen Boyd @ 2020-02-28 22:54 UTC (permalink / raw)
To: gregkh, satya priya
Cc: mgautam, linux-arm-msm, linux-serial, akashast, rojay, msavaliy,
satya priya
Quoting satya priya (2020-02-25 05:54:21)
> To fix the RX cancel command failure, rx_fifo buffer needs to be
> flushed in stop_rx() by calling handle_rx().
>
> If set_termios is called before startup, by this time memory is not
> allocated to port->rx_fifo buffer, which leads to a NULL pointer
> dereference.
>
> To avoid this NULL pointer dereference allocate memory to port->rx_fifo
> in probe itself.
>
> Signed-off-by: satya priya <skakit@codeaurora.org>
Please give me reported-by credit.
Reported-by: Stephen Boyd <swboyd@chromium.org>
> ---
> drivers/tty/serial/qcom_geni_serial.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 191abb1..d2a909c 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -858,12 +858,6 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
> false, false, true);
> geni_se_init(&port->se, UART_RX_WM, port->rx_fifo_depth - 2);
> geni_se_select_mode(&port->se, GENI_SE_FIFO);
> - if (!uart_console(uport)) {
> - port->rx_fifo = devm_kcalloc(uport->dev,
> - port->rx_fifo_depth, sizeof(u32), GFP_KERNEL);
> - if (!port->rx_fifo)
> - return -ENOMEM;
> - }
> port->setup = true;
>
> return 0;
> @@ -1274,6 +1268,13 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
> port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
> port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
>
> + if (!console) {
> + port->rx_fifo = devm_kcalloc(uport->dev,
> + port->rx_fifo_depth, sizeof(u32), GFP_KERNEL);
> + if (!port->rx_fifo)
> + return -ENOMEM;
> + }
Is there any reason the rx_fifo pointer is a u32 instead of a u8 or void
pointer? ioread32_rep() doesn't care what the pointer is and then we
have to cast it, so it seems like we should do something like below too.
-----8<-----
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 191abb18fc2a..b4875dfef6aa 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -113,7 +113,7 @@ struct qcom_geni_serial_port {
unsigned int baud;
unsigned int tx_bytes_pw;
unsigned int rx_bytes_pw;
- u32 *rx_fifo;
+ u8 *rx_fifo;
u32 loopback;
bool brk;
@@ -504,7 +504,6 @@ static int handle_rx_console(struct uart_port *uport, u32 bytes, bool drop)
static int handle_rx_uart(struct uart_port *uport, u32 bytes, bool drop)
{
- unsigned char *buf;
struct tty_port *tport;
struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
u32 num_bytes_pw = port->tx_fifo_width / BITS_PER_BYTE;
@@ -516,8 +515,7 @@ static int handle_rx_uart(struct uart_port *uport, u32 bytes, bool drop)
if (drop)
return 0;
- buf = (unsigned char *)port->rx_fifo;
- ret = tty_insert_flip_string(tport, buf, bytes);
+ ret = tty_insert_flip_string(tport, port->rx_fifo, bytes);
if (ret != bytes) {
dev_err(uport->dev, "%s:Unable to push data ret %d_bytes %d\n",
__func__, ret, bytes);
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH V2 1/2] tty: serial: qcom_geni_serial: Allocate port->rx_fifo buffer in probe
2020-02-25 13:54 ` [PATCH V2 1/2] tty: serial: qcom_geni_serial: Allocate port->rx_fifo buffer in probe satya priya
2020-02-28 22:54 ` Stephen Boyd
@ 2020-02-28 23:01 ` Stephen Boyd
2020-03-04 13:34 ` skakit
1 sibling, 1 reply; 8+ messages in thread
From: Stephen Boyd @ 2020-02-28 23:01 UTC (permalink / raw)
To: gregkh, satya priya
Cc: mgautam, linux-arm-msm, linux-serial, akashast, rojay, msavaliy,
satya priya
Quoting satya priya (2020-02-25 05:54:21)
> To fix the RX cancel command failure, rx_fifo buffer needs to be
> flushed in stop_rx() by calling handle_rx().
>
> If set_termios is called before startup, by this time memory is not
> allocated to port->rx_fifo buffer, which leads to a NULL pointer
> dereference.
Also, clearly set_termios() isn't being called in the warning stack that
I sent last round:
pc : handle_rx_uart+0x64/0x278
lr : qcom_geni_serial_handle_rx+0x84/0x90
sp : ffffff814348f960
x29: ffffff814348f960 x28: ffffffd01ac24288
x27: 0000000000000018 x26: 0000000000000002
x25: 0000000000000001 x24: ffffff8146341348
x23: ffffff8146341000 x22: ffffffd01accc978
x21: ffffff8146341000 x20: 0000000000000001
x19: 0000000000000001 x18: ffffffd01b22d000
x17: 0000000000008000 x16: 00000000000000b0
x15: ffffffd01afdbdd0 x14: ffffffd01b3edde0
x13: ffffffd01b7fb000 x12: 0000000000000001
x11: 0000000000000000 x10: 0000000000000000
x9 : ffffffd010344780 x8 : 0000000000000000
x7 : ffffffd019d8e768 x6 : 0000000000000000
x5 : ffffffd01adbb000 x4 : 0000000000008004
x3 : 0000000000000001 x2 : 0000000000000001
x1 : 0000000000000001 x0 : ffffffd01accc978
Call trace:
handle_rx_uart+0x64/0x278
qcom_geni_serial_handle_rx+0x84/0x90
qcom_geni_serial_stop_rx+0x110/0x180
qcom_geni_serial_port_setup+0x68/0x1b0
qcom_geni_serial_startup+0x24/0x70
uart_startup+0x164/0x28c
uart_port_activate+0x6c/0xbc
tty_port_open+0xa8/0x114
uart_open+0x28/0x38
ttyport_open+0x7c/0x164
serdev_device_open+0x38/0xe4
hci_uart_register_device+0x54/0x2e8 [hci_uart]
qca_serdev_probe+0x1c4/0x374 [hci_uart]
serdev_drv_probe+0x3c/0x64
really_probe+0x144/0x3f8
driver_probe_device+0x70/0x140
__driver_attach_async_helper+0x7c/0xa8
async_run_entry_fn+0x60/0x178
process_one_work+0x33c/0x640
worker_thread+0x2a0/0x470
kthread+0x128/0x138
ret_from_fork+0x10/0x18
Code: 1aca096a 911e0129 b940012b 7100054a (b800450b)
This shows that uart_startup() is the one that is calling
qcom_geni_serial_startup() and that's running the newly added cancel
path. So even if we allocate the buffer in probe vs. in startup we're
going to flip a buffer full of junk that we're trying to cancel out of
the fifo into the tty layer. That seems wrong. We should have a
different qcom_geni_serial_stop_rx() function that knows we're starting
up vs. handling a normal rx event and call something besides handle_rx()
because that pushes bytes up into the tty layer.
>
> To avoid this NULL pointer dereference allocate memory to port->rx_fifo
> in probe itself.
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2 1/2] tty: serial: qcom_geni_serial: Allocate port->rx_fifo buffer in probe
2020-02-28 23:01 ` Stephen Boyd
@ 2020-03-04 13:34 ` skakit
2020-03-04 17:48 ` Stephen Boyd
0 siblings, 1 reply; 8+ messages in thread
From: skakit @ 2020-03-04 13:34 UTC (permalink / raw)
To: Stephen Boyd
Cc: gregkh, mgautam, linux-arm-msm, linux-serial, akashast, rojay, msavaliy
On 2020-02-29 04:31, Stephen Boyd wrote:
> Quoting satya priya (2020-02-25 05:54:21)
>> To fix the RX cancel command failure, rx_fifo buffer needs to be
>> flushed in stop_rx() by calling handle_rx().
>>
>> If set_termios is called before startup, by this time memory is not
>> allocated to port->rx_fifo buffer, which leads to a NULL pointer
>> dereference.
>
> Also, clearly set_termios() isn't being called in the warning stack
> that
> I sent last round:
>
> pc : handle_rx_uart+0x64/0x278
>
>
> lr : qcom_geni_serial_handle_rx+0x84/0x90
>
>
> sp : ffffff814348f960
>
>
> x29: ffffff814348f960 x28: ffffffd01ac24288
>
>
> x27: 0000000000000018 x26: 0000000000000002
>
>
> x25: 0000000000000001 x24: ffffff8146341348
>
>
> x23: ffffff8146341000 x22: ffffffd01accc978
>
>
> x21: ffffff8146341000 x20: 0000000000000001
>
>
> x19: 0000000000000001 x18: ffffffd01b22d000
>
>
> x17: 0000000000008000 x16: 00000000000000b0
>
>
> x15: ffffffd01afdbdd0 x14: ffffffd01b3edde0
>
>
> x13: ffffffd01b7fb000 x12: 0000000000000001
>
>
> x11: 0000000000000000 x10: 0000000000000000
>
>
> x9 : ffffffd010344780 x8 : 0000000000000000
>
>
> x7 : ffffffd019d8e768 x6 : 0000000000000000
>
>
> x5 : ffffffd01adbb000 x4 : 0000000000008004
>
>
> x3 : 0000000000000001 x2 : 0000000000000001
>
>
> x1 : 0000000000000001 x0 : ffffffd01accc978
>
>
> Call trace:
>
>
> handle_rx_uart+0x64/0x278
>
>
> qcom_geni_serial_handle_rx+0x84/0x90
>
>
> qcom_geni_serial_stop_rx+0x110/0x180
>
>
> qcom_geni_serial_port_setup+0x68/0x1b0
>
>
> qcom_geni_serial_startup+0x24/0x70
>
>
> uart_startup+0x164/0x28c
>
>
> uart_port_activate+0x6c/0xbc
>
>
> tty_port_open+0xa8/0x114
>
>
> uart_open+0x28/0x38
>
>
> ttyport_open+0x7c/0x164
>
>
> serdev_device_open+0x38/0xe4
>
>
> hci_uart_register_device+0x54/0x2e8 [hci_uart]
>
>
> qca_serdev_probe+0x1c4/0x374 [hci_uart]
>
>
> serdev_drv_probe+0x3c/0x64
>
>
> really_probe+0x144/0x3f8
>
>
> driver_probe_device+0x70/0x140
>
>
> __driver_attach_async_helper+0x7c/0xa8
>
>
> async_run_entry_fn+0x60/0x178
>
>
> process_one_work+0x33c/0x640
>
>
> worker_thread+0x2a0/0x470
>
>
> kthread+0x128/0x138
>
>
> ret_from_fork+0x10/0x18
>
>
> Code: 1aca096a 911e0129 b940012b 7100054a (b800450b)
>
>
> This shows that uart_startup() is the one that is calling
> qcom_geni_serial_startup() and that's running the newly added cancel
> path. So even if we allocate the buffer in probe vs. in startup we're
> going to flip a buffer full of junk that we're trying to cancel out of
> the fifo into the tty layer. That seems wrong. We should have a
> different qcom_geni_serial_stop_rx() function that knows we're starting
> up vs. handling a normal rx event and call something besides
> handle_rx()
> because that pushes bytes up into the tty layer.
>
As we mentioned in the V1 patch, we are passing drop="true" to handle_rx
function so it will read and discard whatever data present in RX FIFO,
it won't send to upper layers.
static int handle_rx_uart(struct uart_port *uport, u32 bytes, bool drop)
{
....
ioread32_rep(uport->membase + SE_GENI_RX_FIFOn, port->rx_fifo,
words);
if (drop)
return 0;
....
}
In general uart_startup() is called before set_termios() ,but as per the
crash logs shared, it seems RX engine is active(which can only happen
from set_termios) before startup() is called.So, if we allocate
port->rx_fifo in probe we can overcome this crash.
>>
>> To avoid this NULL pointer dereference allocate memory to
>> port->rx_fifo
>> in probe itself.
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2 1/2] tty: serial: qcom_geni_serial: Allocate port->rx_fifo buffer in probe
2020-02-28 22:54 ` Stephen Boyd
@ 2020-03-04 13:38 ` skakit
0 siblings, 0 replies; 8+ messages in thread
From: skakit @ 2020-03-04 13:38 UTC (permalink / raw)
To: Stephen Boyd
Cc: gregkh, mgautam, linux-arm-msm, linux-serial, akashast, rojay, msavaliy
On 2020-02-29 04:24, Stephen Boyd wrote:
> Quoting satya priya (2020-02-25 05:54:21)
>> To fix the RX cancel command failure, rx_fifo buffer needs to be
>> flushed in stop_rx() by calling handle_rx().
>>
>> If set_termios is called before startup, by this time memory is not
>> allocated to port->rx_fifo buffer, which leads to a NULL pointer
>> dereference.
>>
>> To avoid this NULL pointer dereference allocate memory to
>> port->rx_fifo
>> in probe itself.
>>
>> Signed-off-by: satya priya <skakit@codeaurora.org>
>
> Please give me reported-by credit.
>
> Reported-by: Stephen Boyd <swboyd@chromium.org>
Ok.
>
>> ---
>> drivers/tty/serial/qcom_geni_serial.c | 13 +++++++------
>> 1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/tty/serial/qcom_geni_serial.c
>> b/drivers/tty/serial/qcom_geni_serial.c
>> index 191abb1..d2a909c 100644
>> --- a/drivers/tty/serial/qcom_geni_serial.c
>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>> @@ -858,12 +858,6 @@ static int qcom_geni_serial_port_setup(struct
>> uart_port *uport)
>> false, false, true);
>> geni_se_init(&port->se, UART_RX_WM, port->rx_fifo_depth - 2);
>> geni_se_select_mode(&port->se, GENI_SE_FIFO);
>> - if (!uart_console(uport)) {
>> - port->rx_fifo = devm_kcalloc(uport->dev,
>> - port->rx_fifo_depth, sizeof(u32), GFP_KERNEL);
>> - if (!port->rx_fifo)
>> - return -ENOMEM;
>> - }
>> port->setup = true;
>>
>> return 0;
>> @@ -1274,6 +1268,13 @@ static int qcom_geni_serial_probe(struct
>> platform_device *pdev)
>> port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
>> port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
>>
>> + if (!console) {
>> + port->rx_fifo = devm_kcalloc(uport->dev,
>> + port->rx_fifo_depth, sizeof(u32), GFP_KERNEL);
>> + if (!port->rx_fifo)
>> + return -ENOMEM;
>> + }
>
> Is there any reason the rx_fifo pointer is a u32 instead of a u8 or
> void
> pointer? ioread32_rep() doesn't care what the pointer is and then we
> have to cast it, so it seems like we should do something like below
> too.
>
Yes, we can use void instead of u32, will add this change in next patch.
> -----8<-----
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c
> b/drivers/tty/serial/qcom_geni_serial.c
> index 191abb18fc2a..b4875dfef6aa 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -113,7 +113,7 @@ struct qcom_geni_serial_port {
> unsigned int baud;
> unsigned int tx_bytes_pw;
> unsigned int rx_bytes_pw;
> - u32 *rx_fifo;
> + u8 *rx_fifo;
> u32 loopback;
> bool brk;
>
> @@ -504,7 +504,6 @@ static int handle_rx_console(struct uart_port
> *uport, u32 bytes, bool drop)
>
> static int handle_rx_uart(struct uart_port *uport, u32 bytes, bool
> drop)
> {
> - unsigned char *buf;
> struct tty_port *tport;
> struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> u32 num_bytes_pw = port->tx_fifo_width / BITS_PER_BYTE;
> @@ -516,8 +515,7 @@ static int handle_rx_uart(struct uart_port *uport,
> u32 bytes, bool drop)
> if (drop)
> return 0;
>
> - buf = (unsigned char *)port->rx_fifo;
> - ret = tty_insert_flip_string(tport, buf, bytes);
> + ret = tty_insert_flip_string(tport, port->rx_fifo, bytes);
> if (ret != bytes) {
> dev_err(uport->dev, "%s:Unable to push data ret %d_bytes %d\n",
> __func__, ret, bytes);
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2 1/2] tty: serial: qcom_geni_serial: Allocate port->rx_fifo buffer in probe
2020-03-04 13:34 ` skakit
@ 2020-03-04 17:48 ` Stephen Boyd
0 siblings, 0 replies; 8+ messages in thread
From: Stephen Boyd @ 2020-03-04 17:48 UTC (permalink / raw)
To: skakit
Cc: gregkh, mgautam, linux-arm-msm, linux-serial, akashast, rojay, msavaliy
Quoting skakit@codeaurora.org (2020-03-04 05:34:20)
> As we mentioned in the V1 patch, we are passing drop="true" to handle_rx
> function so it will read and discard whatever data present in RX FIFO,
> it won't send to upper layers.
> static int handle_rx_uart(struct uart_port *uport, u32 bytes, bool drop)
> {
> ....
> ioread32_rep(uport->membase + SE_GENI_RX_FIFOn, port->rx_fifo,
> words);
> if (drop)
> return 0;
> ....
> }
> In general uart_startup() is called before set_termios() ,but as per the
> crash logs shared, it seems RX engine is active(which can only happen
> from set_termios) before startup() is called.So, if we allocate
> port->rx_fifo in probe we can overcome this crash.
Ok. Thanks for clarifying. Can you please mention in the commit text
that the fifo contents are read into rx_fifo but then dropped?
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-03-04 17:48 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-25 13:54 [PATCH V2 0/2] Fix RX cancel command failure satya priya
2020-02-25 13:54 ` [PATCH V2 1/2] tty: serial: qcom_geni_serial: Allocate port->rx_fifo buffer in probe satya priya
2020-02-28 22:54 ` Stephen Boyd
2020-03-04 13:38 ` skakit
2020-02-28 23:01 ` Stephen Boyd
2020-03-04 13:34 ` skakit
2020-03-04 17:48 ` Stephen Boyd
2020-02-25 13:54 ` [PATCH V2 2/2] tty: serial: qcom_geni_serial: Fix RX cancel command failure satya priya
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).