| Message ID | 1594134325-7912-2-git-send-email-sumit.garg@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,INTERNAL,1/2] tty/sysrq: Make sysrq handler NMI aware | expand |
On Tue, Jul 07, 2020 at 08:35:25PM +0530, Sumit Garg wrote: > With the advent on pseudo NMIs on arm64 its now possible to request > serial interrupt as an NMI. So allow uart interrupt to be requested as > an NMI in polling mode. This feaure is especially useful for NMI > debugging where the serial device is operating in polling mode. > > Currently only RX interrupt has been enabled as NMI and TX operates in > polling mode which has a performance overhead when compared with TX > operating in interrupt mode. > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > --- > drivers/tty/serial/amba-pl011.c | 177 +++++++++++++++++++++++++++++++++++++++- > 1 file changed, 175 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c > index 8efd7c2..05bfe9f 100644 > --- a/drivers/tty/serial/amba-pl011.c > +++ b/drivers/tty/serial/amba-pl011.c > @@ -41,6 +41,10 @@ > #include <linux/sizes.h> > #include <linux/io.h> > #include <linux/acpi.h> > +#include <linux/irq.h> > +#include <linux/irqdesc.h> > +#include <linux/irq_work.h> > +#include <linux/kfifo.h> > > #include "amba-pl011.h" > > @@ -273,6 +277,12 @@ struct uart_amba_port { > struct pl011_dmatx_data dmatx; > bool dma_probed; > #endif > +#ifdef CONFIG_CONSOLE_POLL > + bool poll_mode_active; > + struct irq_work nmi_work; > + DECLARE_KFIFO(nmi_fifo, unsigned int, 256); > + DECLARE_KFIFO(nmi_fifo_flag, unsigned int, 256); > +#endif Will these four values be common to multiple serial drivers? Can that be wrapped in a type and come from the serial core? Likewise there should be no need to have two FIFOs if you define a type to hold the ch/flag pair. > }; > > static unsigned int pl011_reg_to_offset(const struct uart_amba_port *uap, > @@ -347,6 +357,13 @@ static int pl011_fifo_to_tty(struct uart_amba_port *uap) > if (uart_handle_sysrq_char(&uap->port, ch & 255)) > continue; Currently the implementation of uart_handle_sysrq_char() is not safe to call from NMI. > > +#ifdef CONFIG_CONSOLE_POLL > + if (in_nmi()) { > + kfifo_in(&uap->nmi_fifo, &ch, 1); > + kfifo_in(&uap->nmi_fifo_flag, &flag, 1); > + continue; > + } > +#endif Can this be library code from serial core? In otherwords: if (uart_handle_nmi_and_sysrq_char(&uap->port, ch & 255)) continue; Also the return value from kfifo_in needs to be checked and handled. > uart_insert_char(&uap->port, ch, UART011_DR_OE, ch, flag); > } > > @@ -1286,6 +1303,10 @@ static void pl011_stop_tx(struct uart_port *port) > struct uart_amba_port *uap = > container_of(port, struct uart_amba_port, port); > > +#ifdef CONFIG_CONSOLE_POLL > + if (uap->poll_mode_active) > + return; > +#endif This pattern appears all over the place. It should be an inline function (to avoid the #ifdef'ry) to tell us whether or not to bail out of the function. Can the inline come from the serial core? > uap->im &= ~UART011_TXIM; > pl011_write(uap->im, uap, REG_IMSC); > pl011_dma_tx_stop(uap); > @@ -1302,11 +1323,21 @@ static void pl011_start_tx_pio(struct uart_amba_port *uap) > } > } > > +#ifdef CONFIG_CONSOLE_POLL > +static void pl011_poll_tx_chars(struct uart_amba_port *uap); > +#endif > + Can this be removed by changing the declaration order? > static void pl011_start_tx(struct uart_port *port) > { > struct uart_amba_port *uap = > container_of(port, struct uart_amba_port, port); > > +#ifdef CONFIG_CONSOLE_POLL > + if (uap->poll_mode_active) { > + pl011_poll_tx_chars(uap); > + return; > + } > +#endif Doesn't seem like the right place to do this (using_tx_dma should never be set if we have enabled NMI and it is inserting code into a function that we wouldn't be touching if we were implementing TX using NMI). > if (!pl011_dma_tx_start(uap)) > pl011_start_tx_pio(uap); > } > @@ -1316,6 +1347,10 @@ static void pl011_stop_rx(struct uart_port *port) > struct uart_amba_port *uap = > container_of(port, struct uart_amba_port, port); > > +#ifdef CONFIG_CONSOLE_POLL > + if (uap->poll_mode_active) > + return; > +#endif > uap->im &= ~(UART011_RXIM|UART011_RTIM|UART011_FEIM| > UART011_PEIM|UART011_BEIM|UART011_OEIM); > pl011_write(uap->im, uap, REG_IMSC); > @@ -1637,6 +1672,128 @@ static void pl011_put_poll_char(struct uart_port *port, > pl011_write(ch, uap, REG_DR); > } > > +static void pl011_poll_tx_chars(struct uart_amba_port *uap) > +{ > + struct circ_buf *xmit = &uap->port.state->xmit; > + > + if (uap->port.x_char) { > + pl011_put_poll_char(&uap->port, uap->port.x_char); > + uap->port.x_char = 0; > + } > + > + while (!uart_circ_empty(xmit)) { > + pl011_put_poll_char(&uap->port, xmit->buf[xmit->tail]); > + uap->port.icount.tx++; > + > + xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1); > + } > +} Again this looks generic code and could be in the serial core. However I wonder if we have to try and get TX interrupt driven. We'll never really be able to figure out what code should be in the core and what in the driver unless we do. > + > +static void pl011_nmi_tty_work(struct irq_work *nmi_work) > +{ > + struct uart_amba_port *uap = > + container_of(nmi_work, struct uart_amba_port, nmi_work); > + unsigned int ch, flag; > + > + if (!uap->port.state || !tty_port_active(&uap->port.state->port)) { > + kfifo_reset(&uap->nmi_fifo); > + kfifo_reset(&uap->nmi_fifo_flag); > + return; > + } How can this happen? Why is silent failure the right thing to do if it does? > + if (unlikely(!kfifo_len(&uap->nmi_fifo) || > + !kfifo_len(&uap->nmi_fifo_flag))) > + return; As above. > + > + spin_lock(&uap->port.lock); > + while (kfifo_out(&uap->nmi_fifo, &ch, 1) && > + kfifo_out(&uap->nmi_fifo_flag, &flag, 1)) > + uart_insert_char(&uap->port, ch, UART011_DR_OE, ch, flag); > + spin_unlock(&uap->port.lock); > + > + tty_flip_buffer_push(&uap->port.state->port); > +} This whole function looks like it could be library code in serial-core (. > + > +static void pl011_nmi_rx_chars(struct uart_amba_port *uap) > +{ > + pl011_fifo_to_tty(uap); This could call uart_handle_break(). I don't think this function is safe to call from NMI. I think this needs to be passed down in the flags. > + irq_work_queue(&uap->nmi_work); > +} > + > +static irqreturn_t pl011_nmi_int(int irq, void *dev_id) > +{ > + struct uart_amba_port *uap = dev_id; > + unsigned int status, pass_counter = AMBA_ISR_PASS_LIMIT; > + int handled = 0; > + > + status = pl011_read(uap, REG_MIS); > + if (status) { > + do { > + check_apply_cts_event_workaround(uap); > + > + pl011_write(status, uap, REG_ICR); > + > + if (status) > + pl011_nmi_rx_chars(uap); > + > + if (pass_counter-- == 0) > + break; > + > + status = pl011_read(uap, REG_MIS); > + } while (status != 0); > + handled = 1; > + } > + > + return IRQ_RETVAL(handled); > +} > + > +static int pl011_allocate_nmi(struct uart_amba_port *uap) > +{ > + int ret; > + > + irq_set_status_flags(uap->port.irq, IRQ_NOAUTOEN); > + ret = request_nmi(uap->port.irq, pl011_nmi_int, IRQF_PERCPU, > + "uart-pl011", uap); > + if (ret) { > + irq_clear_status_flags(uap->port.irq, IRQ_NOAUTOEN); > + return ret; > + } > + > + enable_irq(uap->port.irq); > + > + return ret; > +} Not much special here except for serial specific boilerplate. Can this be moved into the core? > + > +static int pl011_hwinit(struct uart_port *port); Again, can we avoid the prototype by ordering better. > + > +static int pl011_poll_init(struct uart_port *port) > +{ > + struct uart_amba_port *uap = > + container_of(port, struct uart_amba_port, port); > + int retval; > + > + retval = pl011_hwinit(port); This enables RX interrupts... > + if (retval) > + goto clk_dis; > + > + /* In case NMI isn't supported, fallback to normal interrupt mode */ > + retval = pl011_allocate_nmi(uap); ... and this unmasks the non-maskable interrupt ;-) ... > + if (retval) > + return 0; > + > + INIT_KFIFO(uap->nmi_fifo); > + INIT_KFIFO(uap->nmi_fifo_flag); > + init_irq_work(&uap->nmi_work, pl011_nmi_tty_work); > + > + uap->poll_mode_active = true; ... which means that these initializations happens too late. However... other than the use inline functions rather than #ifdef'ry then I think that is about it! Daniel. > + > + return 0; > + > + clk_dis: > + clk_disable_unprepare(uap->clk); > + return retval; > +} > + > #endif /* CONFIG_CONSOLE_POLL */ > > static int pl011_hwinit(struct uart_port *port) > @@ -1748,6 +1905,10 @@ static int pl011_startup(struct uart_port *port) > unsigned int cr; > int retval; > > +#ifdef CONFIG_CONSOLE_POLL > + if (uap->poll_mode_active) > + return 0; > +#endif > retval = pl011_hwinit(port); > if (retval) > goto clk_dis; > @@ -1790,6 +1951,10 @@ static int sbsa_uart_startup(struct uart_port *port) > container_of(port, struct uart_amba_port, port); > int retval; > > +#ifdef CONFIG_CONSOLE_POLL > + if (uap->poll_mode_active) > + return 0; > +#endif > retval = pl011_hwinit(port); > if (retval) > return retval; > @@ -1859,6 +2024,10 @@ static void pl011_shutdown(struct uart_port *port) > struct uart_amba_port *uap = > container_of(port, struct uart_amba_port, port); > > +#ifdef CONFIG_CONSOLE_POLL > + if (uap->poll_mode_active) > + return; > +#endif > pl011_disable_interrupts(uap); > > pl011_dma_shutdown(uap); > @@ -1891,6 +2060,10 @@ static void sbsa_uart_shutdown(struct uart_port *port) > struct uart_amba_port *uap = > container_of(port, struct uart_amba_port, port); > > +#ifdef CONFIG_CONSOLE_POLL > + if (uap->poll_mode_active) > + return; > +#endif > pl011_disable_interrupts(uap); > > free_irq(uap->port.irq, uap); > @@ -2142,7 +2315,7 @@ static const struct uart_ops amba_pl011_pops = { > .config_port = pl011_config_port, > .verify_port = pl011_verify_port, > #ifdef CONFIG_CONSOLE_POLL > - .poll_init = pl011_hwinit, > + .poll_init = pl011_poll_init, > .poll_get_char = pl011_get_poll_char, > .poll_put_char = pl011_put_poll_char, > #endif > @@ -2173,7 +2346,7 @@ static const struct uart_ops sbsa_uart_pops = { > .config_port = pl011_config_port, > .verify_port = pl011_verify_port, > #ifdef CONFIG_CONSOLE_POLL > - .poll_init = pl011_hwinit, > + .poll_init = pl011_poll_init, > .poll_get_char = pl011_get_poll_char, > .poll_put_char = pl011_put_poll_char, > #endif > -- > 2.7.4 >
On Wed, 8 Jul 2020 at 21:30, Daniel Thompson <daniel.thompson@linaro.org> wrote: > > On Tue, Jul 07, 2020 at 08:35:25PM +0530, Sumit Garg wrote: > > With the advent on pseudo NMIs on arm64 its now possible to request > > serial interrupt as an NMI. So allow uart interrupt to be requested as > > an NMI in polling mode. This feaure is especially useful for NMI > > debugging where the serial device is operating in polling mode. > > > > Currently only RX interrupt has been enabled as NMI and TX operates in > > polling mode which has a performance overhead when compared with TX > > operating in interrupt mode. > > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > > --- > > drivers/tty/serial/amba-pl011.c | 177 +++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 175 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c > > index 8efd7c2..05bfe9f 100644 > > --- a/drivers/tty/serial/amba-pl011.c > > +++ b/drivers/tty/serial/amba-pl011.c > > @@ -41,6 +41,10 @@ > > #include <linux/sizes.h> > > #include <linux/io.h> > > #include <linux/acpi.h> > > +#include <linux/irq.h> > > +#include <linux/irqdesc.h> > > +#include <linux/irq_work.h> > > +#include <linux/kfifo.h> > > > > #include "amba-pl011.h" > > > > @@ -273,6 +277,12 @@ struct uart_amba_port { > > struct pl011_dmatx_data dmatx; > > bool dma_probed; > > #endif > > +#ifdef CONFIG_CONSOLE_POLL > > + bool poll_mode_active; > > + struct irq_work nmi_work; > > + DECLARE_KFIFO(nmi_fifo, unsigned int, 256); > > + DECLARE_KFIFO(nmi_fifo_flag, unsigned int, 256); > > +#endif > > Will these four values be common to multiple serial drivers? Can that > be wrapped in a type and come from the serial core? Yes, I think these can be common to multiple serial drivers. So, will create a common type which comes from the serial core. > > Likewise there should be no need to have two FIFOs if you define a type to > hold the ch/flag pair. Okay. > > > }; > > > > static unsigned int pl011_reg_to_offset(const struct uart_amba_port *uap, > > @@ -347,6 +357,13 @@ static int pl011_fifo_to_tty(struct uart_amba_port *uap) > > if (uart_handle_sysrq_char(&uap->port, ch & 255)) > > continue; > > Currently the implementation of uart_handle_sysrq_char() is not safe to > call from NMI. > Ah, I see. I think I missed the call to uart_try_toggle_sysrq() which isn't enabled by default (CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE = ""). > > > > > +#ifdef CONFIG_CONSOLE_POLL > > + if (in_nmi()) { > > + kfifo_in(&uap->nmi_fifo, &ch, 1); > > + kfifo_in(&uap->nmi_fifo_flag, &flag, 1); > > + continue; > > + } > > +#endif > > Can this be library code from serial core? In otherwords: > > if (uart_handle_nmi_and_sysrq_char(&uap->port, ch & 255)) > continue; Will give it a try. > > Also the return value from kfifo_in needs to be checked and handled. > Okay. > > > uart_insert_char(&uap->port, ch, UART011_DR_OE, ch, flag); > > } > > > > @@ -1286,6 +1303,10 @@ static void pl011_stop_tx(struct uart_port *port) > > struct uart_amba_port *uap = > > container_of(port, struct uart_amba_port, port); > > > > +#ifdef CONFIG_CONSOLE_POLL > > + if (uap->poll_mode_active) > > + return; > > +#endif > > This pattern appears all over the place. It should be an inline function > (to avoid the #ifdef'ry) to tell us whether or not to bail out of the > function. Can the inline come from the serial core? > Okay, will add an inline function for this that comes from serial core. > > > uap->im &= ~UART011_TXIM; > > pl011_write(uap->im, uap, REG_IMSC); > > pl011_dma_tx_stop(uap); > > @@ -1302,11 +1323,21 @@ static void pl011_start_tx_pio(struct uart_amba_port *uap) > > } > > } > > > > +#ifdef CONFIG_CONSOLE_POLL > > +static void pl011_poll_tx_chars(struct uart_amba_port *uap); > > +#endif > > + > > Can this be removed by changing the declaration order? > Do you mean to rather move pl011_poll_tx_chars() definition here? I just wanted to keep all poll related APIs definition together. > > > static void pl011_start_tx(struct uart_port *port) > > { > > struct uart_amba_port *uap = > > container_of(port, struct uart_amba_port, port); > > > > +#ifdef CONFIG_CONSOLE_POLL > > + if (uap->poll_mode_active) { > > + pl011_poll_tx_chars(uap); > > + return; > > + } > > +#endif > > Doesn't seem like the right place to do this (using_tx_dma should never > be set if we have enabled NMI and it is inserting code into a function > that we wouldn't be touching if we were implementing TX using NMI). > So, I guess the right place would rather be pl011_start_tx_pio(). Will change it accordingly. Regarding TX using NMI, I think it should be possible to achieve with pl011_tx_chars() queued on IRQ workqueue. Will give it a try. > > > if (!pl011_dma_tx_start(uap)) > > pl011_start_tx_pio(uap); > > > } > > @@ -1316,6 +1347,10 @@ static void pl011_stop_rx(struct uart_port *port) > > struct uart_amba_port *uap = > > container_of(port, struct uart_amba_port, port); > > > > +#ifdef CONFIG_CONSOLE_POLL > > + if (uap->poll_mode_active) > > + return; > > +#endif > > uap->im &= ~(UART011_RXIM|UART011_RTIM|UART011_FEIM| > > UART011_PEIM|UART011_BEIM|UART011_OEIM); > > pl011_write(uap->im, uap, REG_IMSC); > > @@ -1637,6 +1672,128 @@ static void pl011_put_poll_char(struct uart_port *port, > > pl011_write(ch, uap, REG_DR); > > } > > > > +static void pl011_poll_tx_chars(struct uart_amba_port *uap) > > +{ > > + struct circ_buf *xmit = &uap->port.state->xmit; > > + > > + if (uap->port.x_char) { > > + pl011_put_poll_char(&uap->port, uap->port.x_char); > > + uap->port.x_char = 0; > > + } > > + > > + while (!uart_circ_empty(xmit)) { > > + pl011_put_poll_char(&uap->port, xmit->buf[xmit->tail]); > > + uap->port.icount.tx++; > > + > > + xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1); > > + } > > +} > > Again this looks generic code and could be in the serial core. However I > wonder if we have to try and get TX interrupt driven. We'll never really > be able to figure out what code should be in the core and what in the > driver unless we do. > Okay, will try to make TX NMI driven as well. > > > + > > +static void pl011_nmi_tty_work(struct irq_work *nmi_work) > > +{ > > + struct uart_amba_port *uap = > > + container_of(nmi_work, struct uart_amba_port, nmi_work); > > + unsigned int ch, flag; > > + > > + if (!uap->port.state || !tty_port_active(&uap->port.state->port)) { > > + kfifo_reset(&uap->nmi_fifo); > > + kfifo_reset(&uap->nmi_fifo_flag); > > + return; > > + } > > How can this happen? Why is silent failure the right thing to do if it > does? > Sorry, this deserved a proper comment. Anyhow, we would like the polling mode to be active irrespective of whether tty port is active or not. Consider a boot hang case or if a user hasn't opened the serial port. So in that case we will just discard any other RX data apart from sysrq or debugger entry commands. This is something based on Doug's suggestion on the upstream thread. > > > + if (unlikely(!kfifo_len(&uap->nmi_fifo) || > > + !kfifo_len(&uap->nmi_fifo_flag))) > > + return; > > As above. > > > > + > > + spin_lock(&uap->port.lock); > > + while (kfifo_out(&uap->nmi_fifo, &ch, 1) && > > + kfifo_out(&uap->nmi_fifo_flag, &flag, 1)) > > + uart_insert_char(&uap->port, ch, UART011_DR_OE, ch, flag); > > + spin_unlock(&uap->port.lock); > > + > > + tty_flip_buffer_push(&uap->port.state->port); > > +} > > This whole function looks like it could be library code in serial-core > (. Okay, will move it to serial core instead. > > > + > > +static void pl011_nmi_rx_chars(struct uart_amba_port *uap) > > +{ > > + pl011_fifo_to_tty(uap); > > This could call uart_handle_break(). I don't think this function is safe > to call from NMI. Are you referring to RX accounting stuff not being NMI safe? > I think this needs to be passed down in the flags. > > > > + irq_work_queue(&uap->nmi_work); > > +} > > + > > +static irqreturn_t pl011_nmi_int(int irq, void *dev_id) > > +{ > > + struct uart_amba_port *uap = dev_id; > > + unsigned int status, pass_counter = AMBA_ISR_PASS_LIMIT; > > + int handled = 0; > > + > > + status = pl011_read(uap, REG_MIS); > > + if (status) { > > + do { > > + check_apply_cts_event_workaround(uap); > > + > > + pl011_write(status, uap, REG_ICR); > > + > > + if (status) > > + pl011_nmi_rx_chars(uap); > > + > > + if (pass_counter-- == 0) > > + break; > > + > > + status = pl011_read(uap, REG_MIS); > > + } while (status != 0); > > + handled = 1; > > + } > > + > > + return IRQ_RETVAL(handled); > > +} > > + > > +static int pl011_allocate_nmi(struct uart_amba_port *uap) > > +{ > > + int ret; > > + > > + irq_set_status_flags(uap->port.irq, IRQ_NOAUTOEN); > > + ret = request_nmi(uap->port.irq, pl011_nmi_int, IRQF_PERCPU, > > + "uart-pl011", uap); > > + if (ret) { > > + irq_clear_status_flags(uap->port.irq, IRQ_NOAUTOEN); > > + return ret; > > + } > > + > > + enable_irq(uap->port.irq); > > + > > + return ret; > > +} > > Not much special here except for serial specific boilerplate. Can this > be moved into the core? > Yeah, I think it could be moved to core to allow its reuse. > > > + > > +static int pl011_hwinit(struct uart_port *port); > > Again, can we avoid the prototype by ordering better. > Okay, I will move pl011_hwinit() definition above. > > > + > > +static int pl011_poll_init(struct uart_port *port) > > +{ > > + struct uart_amba_port *uap = > > + container_of(port, struct uart_amba_port, port); > > + int retval; > > + > > + retval = pl011_hwinit(port); > > This enables RX interrupts... > > > + if (retval) > > + goto clk_dis; > > + > > + /* In case NMI isn't supported, fallback to normal interrupt mode */ > > + retval = pl011_allocate_nmi(uap); > > ... and this unmasks the non-maskable interrupt ;-) ... > > > + if (retval) > > + return 0; > > + > > + INIT_KFIFO(uap->nmi_fifo); > > + INIT_KFIFO(uap->nmi_fifo_flag); > > + init_irq_work(&uap->nmi_work, pl011_nmi_tty_work); > > + > > + uap->poll_mode_active = true; > > ... which means that these initializations happens too late. Will fix it via moving RX interrupts enable snippet from pl011_hwinit() to here. > > However... other than the use inline functions rather than #ifdef'ry > then I think that is about it! > Thanks for your detailed review. -Sumit > > Daniel. > > > > + > > + return 0; > > + > > + clk_dis: > > + clk_disable_unprepare(uap->clk); > > + return retval; > > +} > > + > > #endif /* CONFIG_CONSOLE_POLL */ > > > > static int pl011_hwinit(struct uart_port *port) > > @@ -1748,6 +1905,10 @@ static int pl011_startup(struct uart_port *port) > > unsigned int cr; > > int retval; > > > > +#ifdef CONFIG_CONSOLE_POLL > > + if (uap->poll_mode_active) > > + return 0; > > +#endif > > retval = pl011_hwinit(port); > > if (retval) > > goto clk_dis; > > @@ -1790,6 +1951,10 @@ static int sbsa_uart_startup(struct uart_port *port) > > container_of(port, struct uart_amba_port, port); > > int retval; > > > > +#ifdef CONFIG_CONSOLE_POLL > > + if (uap->poll_mode_active) > > + return 0; > > +#endif > > retval = pl011_hwinit(port); > > if (retval) > > return retval; > > @@ -1859,6 +2024,10 @@ static void pl011_shutdown(struct uart_port *port) > > struct uart_amba_port *uap = > > container_of(port, struct uart_amba_port, port); > > > > +#ifdef CONFIG_CONSOLE_POLL > > + if (uap->poll_mode_active) > > + return; > > +#endif > > pl011_disable_interrupts(uap); > > > > pl011_dma_shutdown(uap); > > @@ -1891,6 +2060,10 @@ static void sbsa_uart_shutdown(struct uart_port *port) > > struct uart_amba_port *uap = > > container_of(port, struct uart_amba_port, port); > > > > +#ifdef CONFIG_CONSOLE_POLL > > + if (uap->poll_mode_active) > > + return; > > +#endif > > pl011_disable_interrupts(uap); > > > > free_irq(uap->port.irq, uap); > > @@ -2142,7 +2315,7 @@ static const struct uart_ops amba_pl011_pops = { > > .config_port = pl011_config_port, > > .verify_port = pl011_verify_port, > > #ifdef CONFIG_CONSOLE_POLL > > - .poll_init = pl011_hwinit, > > + .poll_init = pl011_poll_init, > > .poll_get_char = pl011_get_poll_char, > > .poll_put_char = pl011_put_poll_char, > > #endif > > @@ -2173,7 +2346,7 @@ static const struct uart_ops sbsa_uart_pops = { > > .config_port = pl011_config_port, > > .verify_port = pl011_verify_port, > > #ifdef CONFIG_CONSOLE_POLL > > - .poll_init = pl011_hwinit, > > + .poll_init = pl011_poll_init, > > .poll_get_char = pl011_get_poll_char, > > .poll_put_char = pl011_put_poll_char, > > #endif > > -- > > 2.7.4 > >
On Thu, Jul 09, 2020 at 12:45:18PM +0530, Sumit Garg wrote: > > > + > > > +static void pl011_nmi_rx_chars(struct uart_amba_port *uap) > > > +{ > > > + pl011_fifo_to_tty(uap); > > > > This could call uart_handle_break(). I don't think this function is safe > > to call from NMI. > > Are you referring to RX accounting stuff not being NMI safe? No. I mean that uart_handle_break() calls schedule_work() and cannot be called from NMI context. > > > +static int pl011_poll_init(struct uart_port *port) > > > +{ > > > + struct uart_amba_port *uap = > > > + container_of(port, struct uart_amba_port, port); > > > + int retval; > > > + > > > + retval = pl011_hwinit(port); > > > > This enables RX interrupts... > > > > > + if (retval) > > > + goto clk_dis; > > > + > > > + /* In case NMI isn't supported, fallback to normal interrupt mode */ > > > + retval = pl011_allocate_nmi(uap); > > > > ... and this unmasks the non-maskable interrupt ;-) ... > > > > > + if (retval) > > > + return 0; > > > + > > > + INIT_KFIFO(uap->nmi_fifo); > > > + INIT_KFIFO(uap->nmi_fifo_flag); > > > + init_irq_work(&uap->nmi_work, pl011_nmi_tty_work); > > > + > > > + uap->poll_mode_active = true; > > > > ... which means that these initializations happens too late. > > Will fix it via moving RX interrupts enable snippet from pl011_hwinit() to here. That would work... but why not just move the software preparation earlier in the call? Daniel.
On Thu, 9 Jul 2020 at 14:01, Daniel Thompson <daniel.thompson@linaro.org> wrote: > > On Thu, Jul 09, 2020 at 12:45:18PM +0530, Sumit Garg wrote: > > > > + > > > > +static void pl011_nmi_rx_chars(struct uart_amba_port *uap) > > > > +{ > > > > + pl011_fifo_to_tty(uap); > > > > > > This could call uart_handle_break(). I don't think this function is safe > > > to call from NMI. > > > > Are you referring to RX accounting stuff not being NMI safe? > > No. I mean that uart_handle_break() calls schedule_work() and cannot > be called from NMI context. > Ah, I missed it. Will try to make it NMI safe. > > > > > +static int pl011_poll_init(struct uart_port *port) > > > > +{ > > > > + struct uart_amba_port *uap = > > > > + container_of(port, struct uart_amba_port, port); > > > > + int retval; > > > > + > > > > + retval = pl011_hwinit(port); > > > > > > This enables RX interrupts... > > > > > > > + if (retval) > > > > + goto clk_dis; > > > > + > > > > + /* In case NMI isn't supported, fallback to normal interrupt mode */ > > > > + retval = pl011_allocate_nmi(uap); > > > > > > ... and this unmasks the non-maskable interrupt ;-) ... > > > > > > > + if (retval) > > > > + return 0; > > > > + > > > > + INIT_KFIFO(uap->nmi_fifo); > > > > + INIT_KFIFO(uap->nmi_fifo_flag); > > > > + init_irq_work(&uap->nmi_work, pl011_nmi_tty_work); > > > > + > > > > + uap->poll_mode_active = true; > > > > > > ... which means that these initializations happens too late. > > > > Will fix it via moving RX interrupts enable snippet from pl011_hwinit() to here. > > That would work... but why not just move the software preparation > earlier in the call? > Yes it could be done but still code to enable RX interrupts in pl011_hwinit() doesn't sound explicit and should be executed after "poll_mode_active" is set and "pl011_hwinit()" should be invoked before "poll_mode_active" is set. -Sumit > > Daniel.
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index 8efd7c2..05bfe9f 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -41,6 +41,10 @@ #include <linux/sizes.h> #include <linux/io.h> #include <linux/acpi.h> +#include <linux/irq.h> +#include <linux/irqdesc.h> +#include <linux/irq_work.h> +#include <linux/kfifo.h> #include "amba-pl011.h" @@ -273,6 +277,12 @@ struct uart_amba_port { struct pl011_dmatx_data dmatx; bool dma_probed; #endif +#ifdef CONFIG_CONSOLE_POLL + bool poll_mode_active; + struct irq_work nmi_work; + DECLARE_KFIFO(nmi_fifo, unsigned int, 256); + DECLARE_KFIFO(nmi_fifo_flag, unsigned int, 256); +#endif }; static unsigned int pl011_reg_to_offset(const struct uart_amba_port *uap, @@ -347,6 +357,13 @@ static int pl011_fifo_to_tty(struct uart_amba_port *uap) if (uart_handle_sysrq_char(&uap->port, ch & 255)) continue; +#ifdef CONFIG_CONSOLE_POLL + if (in_nmi()) { + kfifo_in(&uap->nmi_fifo, &ch, 1); + kfifo_in(&uap->nmi_fifo_flag, &flag, 1); + continue; + } +#endif uart_insert_char(&uap->port, ch, UART011_DR_OE, ch, flag); } @@ -1286,6 +1303,10 @@ static void pl011_stop_tx(struct uart_port *port) struct uart_amba_port *uap = container_of(port, struct uart_amba_port, port); +#ifdef CONFIG_CONSOLE_POLL + if (uap->poll_mode_active) + return; +#endif uap->im &= ~UART011_TXIM; pl011_write(uap->im, uap, REG_IMSC); pl011_dma_tx_stop(uap); @@ -1302,11 +1323,21 @@ static void pl011_start_tx_pio(struct uart_amba_port *uap) } } +#ifdef CONFIG_CONSOLE_POLL +static void pl011_poll_tx_chars(struct uart_amba_port *uap); +#endif + static void pl011_start_tx(struct uart_port *port) { struct uart_amba_port *uap = container_of(port, struct uart_amba_port, port); +#ifdef CONFIG_CONSOLE_POLL + if (uap->poll_mode_active) { + pl011_poll_tx_chars(uap); + return; + } +#endif if (!pl011_dma_tx_start(uap)) pl011_start_tx_pio(uap); } @@ -1316,6 +1347,10 @@ static void pl011_stop_rx(struct uart_port *port) struct uart_amba_port *uap = container_of(port, struct uart_amba_port, port); +#ifdef CONFIG_CONSOLE_POLL + if (uap->poll_mode_active) + return; +#endif uap->im &= ~(UART011_RXIM|UART011_RTIM|UART011_FEIM| UART011_PEIM|UART011_BEIM|UART011_OEIM); pl011_write(uap->im, uap, REG_IMSC); @@ -1637,6 +1672,128 @@ static void pl011_put_poll_char(struct uart_port *port, pl011_write(ch, uap, REG_DR); } +static void pl011_poll_tx_chars(struct uart_amba_port *uap) +{ + struct circ_buf *xmit = &uap->port.state->xmit; + + if (uap->port.x_char) { + pl011_put_poll_char(&uap->port, uap->port.x_char); + uap->port.x_char = 0; + } + + while (!uart_circ_empty(xmit)) { + pl011_put_poll_char(&uap->port, xmit->buf[xmit->tail]); + uap->port.icount.tx++; + + xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1); + } +} + +static void pl011_nmi_tty_work(struct irq_work *nmi_work) +{ + struct uart_amba_port *uap = + container_of(nmi_work, struct uart_amba_port, nmi_work); + unsigned int ch, flag; + + if (!uap->port.state || !tty_port_active(&uap->port.state->port)) { + kfifo_reset(&uap->nmi_fifo); + kfifo_reset(&uap->nmi_fifo_flag); + return; + } + + if (unlikely(!kfifo_len(&uap->nmi_fifo) || + !kfifo_len(&uap->nmi_fifo_flag))) + return; + + spin_lock(&uap->port.lock); + while (kfifo_out(&uap->nmi_fifo, &ch, 1) && + kfifo_out(&uap->nmi_fifo_flag, &flag, 1)) + uart_insert_char(&uap->port, ch, UART011_DR_OE, ch, flag); + spin_unlock(&uap->port.lock); + + tty_flip_buffer_push(&uap->port.state->port); +} + +static void pl011_nmi_rx_chars(struct uart_amba_port *uap) +{ + pl011_fifo_to_tty(uap); + irq_work_queue(&uap->nmi_work); +} + +static irqreturn_t pl011_nmi_int(int irq, void *dev_id) +{ + struct uart_amba_port *uap = dev_id; + unsigned int status, pass_counter = AMBA_ISR_PASS_LIMIT; + int handled = 0; + + status = pl011_read(uap, REG_MIS); + if (status) { + do { + check_apply_cts_event_workaround(uap); + + pl011_write(status, uap, REG_ICR); + + if (status) + pl011_nmi_rx_chars(uap); + + if (pass_counter-- == 0) + break; + + status = pl011_read(uap, REG_MIS); + } while (status != 0); + handled = 1; + } + + return IRQ_RETVAL(handled); +} + +static int pl011_allocate_nmi(struct uart_amba_port *uap) +{ + int ret; + + irq_set_status_flags(uap->port.irq, IRQ_NOAUTOEN); + ret = request_nmi(uap->port.irq, pl011_nmi_int, IRQF_PERCPU, + "uart-pl011", uap); + if (ret) { + irq_clear_status_flags(uap->port.irq, IRQ_NOAUTOEN); + return ret; + } + + enable_irq(uap->port.irq); + + return ret; +} + +static int pl011_hwinit(struct uart_port *port); + +static int pl011_poll_init(struct uart_port *port) +{ + struct uart_amba_port *uap = + container_of(port, struct uart_amba_port, port); + int retval; + + retval = pl011_hwinit(port); + if (retval) + goto clk_dis; + + /* In case NMI isn't supported, fallback to normal interrupt mode */ + retval = pl011_allocate_nmi(uap); + if (retval) + return 0; + + INIT_KFIFO(uap->nmi_fifo); + INIT_KFIFO(uap->nmi_fifo_flag); + init_irq_work(&uap->nmi_work, pl011_nmi_tty_work); + + uap->poll_mode_active = true; + + return 0; + + clk_dis: + clk_disable_unprepare(uap->clk); + return retval; +} + #endif /* CONFIG_CONSOLE_POLL */ static int pl011_hwinit(struct uart_port *port) @@ -1748,6 +1905,10 @@ static int pl011_startup(struct uart_port *port) unsigned int cr; int retval; +#ifdef CONFIG_CONSOLE_POLL + if (uap->poll_mode_active) + return 0; +#endif retval = pl011_hwinit(port); if (retval) goto clk_dis; @@ -1790,6 +1951,10 @@ static int sbsa_uart_startup(struct uart_port *port) container_of(port, struct uart_amba_port, port); int retval; +#ifdef CONFIG_CONSOLE_POLL + if (uap->poll_mode_active) + return 0; +#endif retval = pl011_hwinit(port); if (retval) return retval; @@ -1859,6 +2024,10 @@ static void pl011_shutdown(struct uart_port *port) struct uart_amba_port *uap = container_of(port, struct uart_amba_port, port); +#ifdef CONFIG_CONSOLE_POLL + if (uap->poll_mode_active) + return; +#endif pl011_disable_interrupts(uap); pl011_dma_shutdown(uap); @@ -1891,6 +2060,10 @@ static void sbsa_uart_shutdown(struct uart_port *port) struct uart_amba_port *uap = container_of(port, struct uart_amba_port, port); +#ifdef CONFIG_CONSOLE_POLL + if (uap->poll_mode_active) + return; +#endif pl011_disable_interrupts(uap); free_irq(uap->port.irq, uap); @@ -2142,7 +2315,7 @@ static const struct uart_ops amba_pl011_pops = { .config_port = pl011_config_port, .verify_port = pl011_verify_port, #ifdef CONFIG_CONSOLE_POLL - .poll_init = pl011_hwinit, + .poll_init = pl011_poll_init, .poll_get_char = pl011_get_poll_char, .poll_put_char = pl011_put_poll_char, #endif @@ -2173,7 +2346,7 @@ static const struct uart_ops sbsa_uart_pops = { .config_port = pl011_config_port, .verify_port = pl011_verify_port, #ifdef CONFIG_CONSOLE_POLL - .poll_init = pl011_hwinit, + .poll_init = pl011_poll_init, .poll_get_char = pl011_get_poll_char, .poll_put_char = pl011_put_poll_char, #endif
With the advent on pseudo NMIs on arm64 its now possible to request serial interrupt as an NMI. So allow uart interrupt to be requested as an NMI in polling mode. This feaure is especially useful for NMI debugging where the serial device is operating in polling mode. Currently only RX interrupt has been enabled as NMI and TX operates in polling mode which has a performance overhead when compared with TX operating in interrupt mode. Signed-off-by: Sumit Garg <sumit.garg@linaro.org> --- drivers/tty/serial/amba-pl011.c | 177 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 175 insertions(+), 2 deletions(-) -- 2.7.4