[2/7] tty: serial: Add poll_get_irq() to the polling interface

Message ID 1592835984-28613-3-git-send-email-sumit.garg@linaro.org
State New
Headers show
Series
  • Enable support for kgdb NMI console feature
Related show

Commit Message

Sumit Garg June 22, 2020, 2:26 p.m.
From: Daniel Thompson <daniel.thompson@linaro.org>


Add new API: poll_get_irq() to the polling interface in order for user
of polling interface to retrieve allocated IRQ corresponding to
underlying serial device.

Although, serial interface still works in polling mode but interrupt
associated with serial device can be leveraged for special purposes like
debugger(kgdb) entry.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

---
 drivers/tty/serial/serial_core.c | 18 ++++++++++++++++++
 include/linux/serial_core.h      |  1 +
 include/linux/tty_driver.h       |  1 +
 3 files changed, 20 insertions(+)

-- 
2.7.4

Comments

Daniel Thompson June 22, 2020, 3:56 p.m. | #1
On Mon, Jun 22, 2020 at 07:56:19PM +0530, Sumit Garg wrote:
> From: Daniel Thompson <daniel.thompson@linaro.org>


Sumit, to some extent this mail is me yelling at myself two years ago
although, in my defence, at the time I choose not to post it because I
knew it wasn't right.

I'm a bit concerned to see the TODO: comment critiquing this interface
being removed (from patch 3) without this patch being changed to
address that comment.


> Add new API: poll_get_irq() to the polling interface in order for user

> of polling interface to retrieve allocated IRQ corresponding to

> underlying serial device.

> 

> Although, serial interface still works in polling mode but interrupt

> associated with serial device can be leveraged for special purposes like

> debugger(kgdb) entry.

> 

> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>

> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

> ---

>  drivers/tty/serial/serial_core.c | 18 ++++++++++++++++++

>  include/linux/serial_core.h      |  1 +

>  include/linux/tty_driver.h       |  1 +

>  3 files changed, 20 insertions(+)

> 

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c

> index 66a5e2f..1bb033c 100644

> --- a/drivers/tty/serial/serial_core.c

> +++ b/drivers/tty/serial/serial_core.c

> @@ -2470,6 +2470,23 @@ static void uart_poll_put_char(struct tty_driver *driver, int line, char ch)

>  	port->ops->poll_put_char(port, ch);

>  	uart_port_deref(port);

>  }

> +

> +static int uart_poll_get_irq(struct tty_driver *driver, int line)

> +{

> +	struct uart_driver *drv = driver->driver_state;

> +	struct uart_state *state = drv->state + line;

> +	struct uart_port *port;

> +	int ret = -ENODEV;

> +

> +	port = uart_port_ref(state);

> +	if (port && port->ops->poll_get_irq) {

> +		ret = port->ops->poll_get_irq(port);

> +		uart_port_deref(port);

> +	}

> +

> +	return ret;

> +}

> +

>  #endif

>  

>  static const struct tty_operations uart_ops = {

> @@ -2505,6 +2522,7 @@ static const struct tty_operations uart_ops = {

>  	.poll_init	= uart_poll_init,

>  	.poll_get_char	= uart_poll_get_char,

>  	.poll_put_char	= uart_poll_put_char,

> +	.poll_get_irq	= uart_poll_get_irq,


The TODO comments claimed this API was bogus because it doesn't permit
a free and that can cause interoperation problems with the real serial
driver. I'll cover some of that in a reply to patch 3 but for now.

Anyhow, for this patch, what are the expected semantics for
uart_poll_get_irq().

In particular how do they ensure correct interlocking with the real
serial driver underlying it (if kgdb_nmi is active on a serial port
then the underlying driver better not be active at the same time).


Daniel.


>  #endif

>  };

>  

> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h

> index 92f5eba..8b132e6 100644

> --- a/include/linux/serial_core.h

> +++ b/include/linux/serial_core.h

> @@ -78,6 +78,7 @@ struct uart_ops {

>  	int		(*poll_init)(struct uart_port *);

>  	void		(*poll_put_char)(struct uart_port *, unsigned char);

>  	int		(*poll_get_char)(struct uart_port *);

> +	int		(*poll_get_irq)(struct uart_port *);

>  #endif

>  };

>  

> diff --git a/include/linux/tty_driver.h b/include/linux/tty_driver.h

> index 3584462..d6da5c5 100644

> --- a/include/linux/tty_driver.h

> +++ b/include/linux/tty_driver.h

> @@ -295,6 +295,7 @@ struct tty_operations {

>  	int (*poll_init)(struct tty_driver *driver, int line, char *options);

>  	int (*poll_get_char)(struct tty_driver *driver, int line);

>  	void (*poll_put_char)(struct tty_driver *driver, int line, char ch);

> +	int (*poll_get_irq)(struct tty_driver *driver, int line);

>  #endif

>  	int (*proc_show)(struct seq_file *, void *);

>  } __randomize_layout;

> -- 

> 2.7.4

>
Sumit Garg June 23, 2020, 7:48 a.m. | #2
On Mon, 22 Jun 2020 at 21:26, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>

> On Mon, Jun 22, 2020 at 07:56:19PM +0530, Sumit Garg wrote:

> > From: Daniel Thompson <daniel.thompson@linaro.org>

>

> Sumit, to some extent this mail is me yelling at myself two years ago

> although, in my defence, at the time I choose not to post it because I

> knew it wasn't right.

>

> I'm a bit concerned to see the TODO: comment critiquing this interface

> being removed (from patch 3) without this patch being changed to

> address that comment.

>


I did consider that comment but I couldn't think of a normal scenario
where request_irq() should fail. And in case it fails as well, I did
put in "WARN_ON(res);" so that the user is notified of that particular
error scenario.

>

> > Add new API: poll_get_irq() to the polling interface in order for user

> > of polling interface to retrieve allocated IRQ corresponding to

> > underlying serial device.

> >

> > Although, serial interface still works in polling mode but interrupt

> > associated with serial device can be leveraged for special purposes like

> > debugger(kgdb) entry.

> >

> > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>

> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

> > ---

> >  drivers/tty/serial/serial_core.c | 18 ++++++++++++++++++

> >  include/linux/serial_core.h      |  1 +

> >  include/linux/tty_driver.h       |  1 +

> >  3 files changed, 20 insertions(+)

> >

> > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c

> > index 66a5e2f..1bb033c 100644

> > --- a/drivers/tty/serial/serial_core.c

> > +++ b/drivers/tty/serial/serial_core.c

> > @@ -2470,6 +2470,23 @@ static void uart_poll_put_char(struct tty_driver *driver, int line, char ch)

> >       port->ops->poll_put_char(port, ch);

> >       uart_port_deref(port);

> >  }

> > +

> > +static int uart_poll_get_irq(struct tty_driver *driver, int line)

> > +{

> > +     struct uart_driver *drv = driver->driver_state;

> > +     struct uart_state *state = drv->state + line;

> > +     struct uart_port *port;

> > +     int ret = -ENODEV;

> > +

> > +     port = uart_port_ref(state);

> > +     if (port && port->ops->poll_get_irq) {

> > +             ret = port->ops->poll_get_irq(port);

> > +             uart_port_deref(port);

> > +     }

> > +

> > +     return ret;

> > +}

> > +

> >  #endif

> >

> >  static const struct tty_operations uart_ops = {

> > @@ -2505,6 +2522,7 @@ static const struct tty_operations uart_ops = {

> >       .poll_init      = uart_poll_init,

> >       .poll_get_char  = uart_poll_get_char,

> >       .poll_put_char  = uart_poll_put_char,

> > +     .poll_get_irq   = uart_poll_get_irq,

>

> The TODO comments claimed this API was bogus because it doesn't permit

> a free and that can cause interoperation problems with the real serial

> driver. I'll cover some of that in a reply to patch 3 but for now.

>

> Anyhow, for this patch, what are the expected semantics for

> uart_poll_get_irq().


Currently, the expected use for this API is to enable uart RX
interrupts and return corresponding IRQ id.

Although, we can make this interface modular as follows:

.poll_get_irq
.poll_enable_rx_int
.poll_disable_rx_int

Your views?

>

> In particular how do they ensure correct interlocking with the real

> serial driver underlying it (if kgdb_nmi is active on a serial port

> then the underlying driver better not be active at the same time).

>


AFAIU kgdb_nmi feature, it registers a new tty driver (ttyNMI0) which
is expected to work alongside underlying tty driver (eg. ttyAMA0 with
amba-pl011). So ttyAMA0 will only become active if user-space tries to
interact with /dev/ttyAMA0 like:

# echo "Hello World!" > /dev/ttyAMA0

So I would like to understand the downsides of having both of them
active at the same time using shared IRQ (although that won't be
possible with NMI as that doesn't support shared mode)?

-Sumit

>

> Daniel.

>

>

> >  #endif

> >  };

> >

> > diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h

> > index 92f5eba..8b132e6 100644

> > --- a/include/linux/serial_core.h

> > +++ b/include/linux/serial_core.h

> > @@ -78,6 +78,7 @@ struct uart_ops {

> >       int             (*poll_init)(struct uart_port *);

> >       void            (*poll_put_char)(struct uart_port *, unsigned char);

> >       int             (*poll_get_char)(struct uart_port *);

> > +     int             (*poll_get_irq)(struct uart_port *);

> >  #endif

> >  };

> >

> > diff --git a/include/linux/tty_driver.h b/include/linux/tty_driver.h

> > index 3584462..d6da5c5 100644

> > --- a/include/linux/tty_driver.h

> > +++ b/include/linux/tty_driver.h

> > @@ -295,6 +295,7 @@ struct tty_operations {

> >       int (*poll_init)(struct tty_driver *driver, int line, char *options);

> >       int (*poll_get_char)(struct tty_driver *driver, int line);

> >       void (*poll_put_char)(struct tty_driver *driver, int line, char ch);

> > +     int (*poll_get_irq)(struct tty_driver *driver, int line);

> >  #endif

> >       int (*proc_show)(struct seq_file *, void *);

> >  } __randomize_layout;

> > --

> > 2.7.4

> >
Daniel Thompson June 23, 2020, 10:52 a.m. | #3
On Tue, Jun 23, 2020 at 01:18:25PM +0530, Sumit Garg wrote:
> On Mon, 22 Jun 2020 at 21:26, Daniel Thompson

> <daniel.thompson@linaro.org> wrote:

> >

> > On Mon, Jun 22, 2020 at 07:56:19PM +0530, Sumit Garg wrote:

> > > From: Daniel Thompson <daniel.thompson@linaro.org>

> >

> > Sumit, to some extent this mail is me yelling at myself two years ago

> > although, in my defence, at the time I choose not to post it because I

> > knew it wasn't right.

> >

> > I'm a bit concerned to see the TODO: comment critiquing this interface

> > being removed (from patch 3) without this patch being changed to

> > address that comment.

> >

> 

> I did consider that comment but I couldn't think of a normal scenario

> where request_irq() should fail. And in case it fails as well, I did

> put in "WARN_ON(res);" so that the user is notified of that particular

> error scenario.

> 

> >

> > > Add new API: poll_get_irq() to the polling interface in order for user

> > > of polling interface to retrieve allocated IRQ corresponding to

> > > underlying serial device.

> > >

> > > Although, serial interface still works in polling mode but interrupt

> > > associated with serial device can be leveraged for special purposes like

> > > debugger(kgdb) entry.

> > >

> > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>

> > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

> > > ---

> > >  drivers/tty/serial/serial_core.c | 18 ++++++++++++++++++

> > >  include/linux/serial_core.h      |  1 +

> > >  include/linux/tty_driver.h       |  1 +

> > >  3 files changed, 20 insertions(+)

> > >

> > > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c

> > > index 66a5e2f..1bb033c 100644

> > > --- a/drivers/tty/serial/serial_core.c

> > > +++ b/drivers/tty/serial/serial_core.c

> > > @@ -2470,6 +2470,23 @@ static void uart_poll_put_char(struct tty_driver *driver, int line, char ch)

> > >       port->ops->poll_put_char(port, ch);

> > >       uart_port_deref(port);

> > >  }

> > > +

> > > +static int uart_poll_get_irq(struct tty_driver *driver, int line)

> > > +{

> > > +     struct uart_driver *drv = driver->driver_state;

> > > +     struct uart_state *state = drv->state + line;

> > > +     struct uart_port *port;

> > > +     int ret = -ENODEV;

> > > +

> > > +     port = uart_port_ref(state);

> > > +     if (port && port->ops->poll_get_irq) {

> > > +             ret = port->ops->poll_get_irq(port);

> > > +             uart_port_deref(port);

> > > +     }

> > > +

> > > +     return ret;

> > > +}

> > > +

> > >  #endif

> > >

> > >  static const struct tty_operations uart_ops = {

> > > @@ -2505,6 +2522,7 @@ static const struct tty_operations uart_ops = {

> > >       .poll_init      = uart_poll_init,

> > >       .poll_get_char  = uart_poll_get_char,

> > >       .poll_put_char  = uart_poll_put_char,

> > > +     .poll_get_irq   = uart_poll_get_irq,

> >

> > The TODO comments claimed this API was bogus because it doesn't permit

> > a free and that can cause interoperation problems with the real serial

> > driver. I'll cover some of that in a reply to patch 3 but for now.

> >

> > Anyhow, for this patch, what are the expected semantics for

> > uart_poll_get_irq().

> 

> Currently, the expected use for this API is to enable uart RX

> interrupts and return corresponding IRQ id.

> 

> Although, we can make this interface modular as follows:

> 

> .poll_get_irq

> .poll_enable_rx_int

> .poll_disable_rx_int

> 

> Your views?


A direct reply is to ask what the purpose of poll_enable_rx_int()
is? It appears to be making something modular without any reason to do
so.

More generally lets ask some more questions:

1. What serial hardware and/or driver state change happens when we call
   poll_get_irq()

2. After a call to poll_get_irq() are there any new restrictions on
   the underlying serial driver?

3. Does implementing poll_get_irq() create an implied change of
   contract to any other polling function (I think yes, because
   poll_read_char() will now have to acknowledge an interrupt if
   the interrupt flags are edge triggered)?

4. To what extent, if any, do we have to support disconnection of
   kgdb_nmi?

5. Is the API matched with how it will be used by the layers above?
   (top level uses a request_irq approach, low level is using a
   let me have the irq number approach)

An interesting idea to explore is using a different verb for
poll_get_irq(). Perhaps poll_borrow_irq()? It is now much clearer that
we are stealing the IRQ and making it unusable by the underlying serial
driver.


> > In particular how do they ensure correct interlocking with the real

> > serial driver underlying it (if kgdb_nmi is active on a serial port

> > then the underlying driver better not be active at the same time).

> >

> 

> AFAIU kgdb_nmi feature, it registers a new tty driver (ttyNMI0) which

> is expected to work alongside underlying tty driver (eg. ttyAMA0 with

> amba-pl011). So ttyAMA0 will only become active if user-space tries to

> interact with /dev/ttyAMA0 like:

> 

> # echo "Hello World!" > /dev/ttyAMA0

> 

> So I would like to understand the downsides of having both of them

> active at the same time using shared IRQ (although that won't be

> possible with NMI as that doesn't support shared mode)?


It is likely that one of the interrupt handlers will become unreachable
since the first handler to execute will clear the interrupt flags. This
will result in anything waiting for the broken driver getting stuck and
not reporting an error code.

That isn't acceptable.


Daniel.

Patch

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 66a5e2f..1bb033c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2470,6 +2470,23 @@  static void uart_poll_put_char(struct tty_driver *driver, int line, char ch)
 	port->ops->poll_put_char(port, ch);
 	uart_port_deref(port);
 }
+
+static int uart_poll_get_irq(struct tty_driver *driver, int line)
+{
+	struct uart_driver *drv = driver->driver_state;
+	struct uart_state *state = drv->state + line;
+	struct uart_port *port;
+	int ret = -ENODEV;
+
+	port = uart_port_ref(state);
+	if (port && port->ops->poll_get_irq) {
+		ret = port->ops->poll_get_irq(port);
+		uart_port_deref(port);
+	}
+
+	return ret;
+}
+
 #endif
 
 static const struct tty_operations uart_ops = {
@@ -2505,6 +2522,7 @@  static const struct tty_operations uart_ops = {
 	.poll_init	= uart_poll_init,
 	.poll_get_char	= uart_poll_get_char,
 	.poll_put_char	= uart_poll_put_char,
+	.poll_get_irq	= uart_poll_get_irq,
 #endif
 };
 
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 92f5eba..8b132e6 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -78,6 +78,7 @@  struct uart_ops {
 	int		(*poll_init)(struct uart_port *);
 	void		(*poll_put_char)(struct uart_port *, unsigned char);
 	int		(*poll_get_char)(struct uart_port *);
+	int		(*poll_get_irq)(struct uart_port *);
 #endif
 };
 
diff --git a/include/linux/tty_driver.h b/include/linux/tty_driver.h
index 3584462..d6da5c5 100644
--- a/include/linux/tty_driver.h
+++ b/include/linux/tty_driver.h
@@ -295,6 +295,7 @@  struct tty_operations {
 	int (*poll_init)(struct tty_driver *driver, int line, char *options);
 	int (*poll_get_char)(struct tty_driver *driver, int line);
 	void (*poll_put_char)(struct tty_driver *driver, int line, char ch);
+	int (*poll_get_irq)(struct tty_driver *driver, int line);
 #endif
 	int (*proc_show)(struct seq_file *, void *);
 } __randomize_layout;