linux-serial.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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).