[RFC,INTERNAL,2/2] serial: amba-pl011: Allow uart interrupt to be NMI

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
Related show

Commit Message

Sumit Garg July 7, 2020, 3:05 p.m.
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

Comments

Daniel Thompson July 8, 2020, 4 p.m. | #1
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

>
Sumit Garg July 9, 2020, 7:15 a.m. | #2
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

> >
Daniel Thompson July 9, 2020, 8:31 a.m. | #3
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.
Sumit Garg July 9, 2020, 10:09 a.m. | #4
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.

Patch

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