Re: 8250: set_ier(), clear_ier() and the concept of atomic console writes

[John Ogness <john.ogness@linutronix.de>]

Hi Dick,

On 2019-12-20, Dick Hollenbeck <dick@softplc.com> wrote:
> I've included a partial patch against Greg KH's tree below just to
> convey the design concepts that I introduce below in 4), 5) and 6).
>
> Hopefully this can draw a bridge between mainline, RT and where Greg
> is going with his tree, and make everyone happy.

After spending time working with and completing your patch, I realized
that the 8250 atomic console implementation does not need to be so
complex. Only serial8250_console_write_atomic() needs to track if it
needs to re-enable IER. Everything else can be as in mainline.

I will post a patch that reverts the unneeded complexity, which will
solve your problem.

Creating a macro for clearing IER is probably a good idea anyway to
eliminate all the capabilities-checking redundancy. But I'll leave that
as an excercise for mainline.

Your patch helped me to realize the insanity in what I was doing. Thank
you.

John Ogness

> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index 33ad9d6de..e035e91ac 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -130,12 +130,31 @@ static inline void serial_dl_write(struct uart_8250_port *up, int value)
>  	up->dl_write(up, value);
>  }
>
> +/* All write access to IER comes through here, for instrumentation sake. */
> +static inline unsigned char serial8250_set_IER(struct uart_8250_port *up, unsigned char ier)
> +{
> +#ifdef CONFIG_NMI_CONSOLE_CORNER_CASE
> +	return __serial9250_set_IER(p, ier)
> +#else
> +	unsigned char prior = up->ier;
> +
> +	up->ier = ier;	/* struct's ier field is always current with hardware */
> +	serial_out(up, UART_IER, ier);
> +	return prior;
> +#endif
> +}
> +
> +static inline unsigned char serial8250_clear_IER(struct uart_8250_port *up)
> +{
> +	return serial8250_set_IER(up, up->capabilities & UART_CAP_UUE ? UART_IER_UUE : 0);
> +}
> +
>  static inline bool serial8250_set_THRI(struct uart_8250_port *up)
>  {
>  	if (up->ier & UART_IER_THRI)
>  		return false;
> -	up->ier |= UART_IER_THRI;
> -	serial_out(up, UART_IER, up->ier);
> +
> +	serial8250_set_IER(up, up->ier | UART_IER_THRI);
>  	return true;
>  }
>
> @@ -143,8 +162,7 @@ static inline bool serial8250_clear_THRI(struct uart_8250_port *up)
>  {
>  	if (!(up->ier & UART_IER_THRI))
>  		return false;
> -	up->ier &= ~UART_IER_THRI;
> -	serial_out(up, UART_IER, up->ier);
> +	serial8250_set_IER(up, up->ier & ~UART_IER_THRI);
>  	return true;
>  }
>
> @@ -343,3 +361,7 @@ static inline int serial_index(struct uart_port *port)
>  {
>  	return port->minor - 64;
>  }
> +
> +#ifdef CONFIG_NMI_CONSOLE_CORNER_CASE
> +unsigned char __serial8250_set_IER(struct uart_8250_port *up, unsigned char ier);
> +#endif