[1/5] serial: Introduce UPSTAT_SYNC_FIFO for synchronised FIFOs

This change adds a flag to indicate that a UART is has an external means
of synchronising its FIFO, without needing CTSRTS or XON/XOFF.

This allows us to use the throttle/unthrottle callbacks, without having
to claim other methods of flow control.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
---
 drivers/tty/serial/serial_core.c | 4 ++--
 include/linux/serial_core.h      | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

On Wed, Mar 21, 2018 at 10:52:37AM +0800, Jeremy Kerr wrote:
> This change adds a flag to indicate that a UART is has an external means
> of synchronising its FIFO, without needing CTSRTS or XON/XOFF.
> 
> This allows us to use the throttle/unthrottle callbacks, without having
> to claim other methods of flow control.
> 
> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
> ---
>  drivers/tty/serial/serial_core.c | 4 ++--
>  include/linux/serial_core.h      | 1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index f534a40aebde..8f3dfc8b5307 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -678,7 +678,7 @@ static void uart_throttle(struct tty_struct *tty)
>  	if (C_CRTSCTS(tty))
>  		mask |= UPSTAT_AUTORTS;
>  
> -	if (port->status & mask) {
> +	if (port->status & (mask | UPSTAT_SYNC_FIFO)) {
>  		port->ops->throttle(port);
>  		mask &= ~port->status;
>  	}

Why not just set mask to UPSTAT_SYNC_FIFO at the top of this function?

> @@ -707,7 +707,7 @@ static void uart_unthrottle(struct tty_struct *tty)
>  	if (C_CRTSCTS(tty))
>  		mask |= UPSTAT_AUTORTS;
>  
> -	if (port->status & mask) {
> +	if (port->status & (mask | UPSTAT_SYNC_FIFO)) {
>  		port->ops->unthrottle(port);
>  		mask &= ~port->status;
>  	}

Same here.  Would make it a bit more obvious (at least to me...)

thanks,

greg k-h

On 03/20/2018 09:52 PM, Jeremy Kerr wrote:
> This change adds a flag to indicate that a UART is has an external means
> of synchronising its FIFO, without needing CTSRTS or XON/XOFF.
>
> This allows us to use the throttle/unthrottle callbacks, without having
> to claim other methods of flow control.

This series fixed extremely heavy cpu usage we were getting with a lot 
of traffic coming in on the console. System is usable again with console 
getting hammered.

Tested-by: Eddie James <eajames@linux.vnet.ibm.com>

>
> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
> ---
>   drivers/tty/serial/serial_core.c | 4 ++--
>   include/linux/serial_core.h      | 1 +
>   2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index f534a40aebde..8f3dfc8b5307 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -678,7 +678,7 @@ static void uart_throttle(struct tty_struct *tty)
>   	if (C_CRTSCTS(tty))
>   		mask |= UPSTAT_AUTORTS;
>
> -	if (port->status & mask) {
> +	if (port->status & (mask | UPSTAT_SYNC_FIFO)) {
>   		port->ops->throttle(port);
>   		mask &= ~port->status;
>   	}
> @@ -707,7 +707,7 @@ static void uart_unthrottle(struct tty_struct *tty)
>   	if (C_CRTSCTS(tty))
>   		mask |= UPSTAT_AUTORTS;
>
> -	if (port->status & mask) {
> +	if (port->status & (mask | UPSTAT_SYNC_FIFO)) {
>   		port->ops->unthrottle(port);
>   		mask &= ~port->status;
>   	}
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 1775500294bb..bf600ae0290d 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -232,6 +232,7 @@ struct uart_port {
>   #define UPSTAT_AUTORTS		((__force upstat_t) (1 << 2))
>   #define UPSTAT_AUTOCTS		((__force upstat_t) (1 << 3))
>   #define UPSTAT_AUTOXOFF		((__force upstat_t) (1 << 4))
> +#define UPSTAT_SYNC_FIFO	((__force upstat_t) (1 << 5))
>
>   	int			hw_stopped;		/* sw-assisted CTS flow state */
>   	unsigned int		mctrl;			/* current modem ctrl settings */

Hi Greg,

>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -678,7 +678,7 @@ static void uart_throttle(struct tty_struct *tty)
>>  	if (C_CRTSCTS(tty))
>>  		mask |= UPSTAT_AUTORTS;
>>  
>> -	if (port->status & mask) {
>> +	if (port->status & (mask | UPSTAT_SYNC_FIFO)) {
>>  		port->ops->throttle(port);
>>  		mask &= ~port->status;
>>  	}
> 
> Why not just set mask to UPSTAT_SYNC_FIFO at the top of this function?

That was a bit of a line call - my thinking was that SYNC_FIFO is a
little different from the calculated bit values in mask, hence keeping
it separate and adding it explicitly at the check

However, adding it to the declaration of mask is a little simpler, so
I'll redo that in v2.

Cheers,


Jeremy