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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux