Re: [PATCH V2 1/2] tty: serial: qcom_geni_serial: Allocate port->rx_fifo buffer in probe

[skakit@codeaurora.org]

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.
>>