Message ID | 1582638862-9344-2-git-send-email-skakit@codeaurora.org (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | Fix RX cancel command failure | expand |
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);
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. >
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. >>
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);
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?
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);
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(-)