[PATCH 1/2] tty: serial: 8250: always call tx_chars under spinlock

[PATCH 1/2] tty: serial: 8250: always call tx_chars under spinlock

[Steffen Trumtrar]

In most cases serial8250_tx_chars is called with spinlock held.
Fix the remaining location, too.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 drivers/tty/serial/8250/8250_port.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index b0af13074cd3..3310c2b70138 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1559,6 +1559,7 @@ static void serial8250_stop_tx(struct uart_port *port)
 static inline void __start_tx(struct uart_port *port)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
+	unsigned long flags;
 
 	if (up->dma && !up->dma->tx_dma(up))
 		return;
@@ -1569,8 +1570,11 @@ static inline void __start_tx(struct uart_port *port)
 
 			lsr = serial_in(up, UART_LSR);
 			up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
-			if (lsr & UART_LSR_THRE)
+			if (lsr & UART_LSR_THRE) {
+				spin_lock_irqsave(&port->lock, flags);
 				serial8250_tx_chars(up);
+				spin_unlock_irqrestore(&port->lock, flags);
+			}
 		}
 	}
 
-- 
2.20.1

[PATCH 2/2] tty: serial: 8250: wait till transmitter is empty

[Steffen Trumtrar]

When only one single character is sent and RS485 signaling is used,
the driver runs into timing issues.

When serial8250_tx_chars is called the single character is transmitted.
The check on uart_circ_empty will be positive and __stop_tx is called.
The check on UART_LSR_TEMT in BOTH_EMPTY will then be negativ and the
function will return. On the next call to serial8250_tx_chars
uart_circ_empty will still be true but the check on BOTH_EMPTY in
__stop_tx might still fail. This leads to a deadlock.

Use readx_poll_timeout_atomic to allow the shift register to be emptied
before checking on BOTH_EMPTY.

The timeout value is copied from 8250_dw.c.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 drivers/tty/serial/8250/8250_port.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 3310c2b70138..87daf3758ff0 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -18,6 +18,7 @@
 #include <linux/console.h>
 #include <linux/gpio/consumer.h>
 #include <linux/sysrq.h>
+#include <linux/iopoll.h>
 #include <linux/delay.h>
 #include <linux/platform_device.h>
 #include <linux/tty.h>
@@ -1519,18 +1520,27 @@ static inline void __do_stop_tx(struct uart_8250_port *p)
 		serial8250_rpm_put_tx(p);
 }
 
+static unsigned char serial8250_read_lsr(struct uart_8250_port *p)
+{
+	return serial_in(p, UART_LSR);
+}
+
 static inline void __stop_tx(struct uart_8250_port *p)
 {
 	struct uart_8250_em485 *em485 = p->em485;
 
 	if (em485) {
-		unsigned char lsr = serial_in(p, UART_LSR);
+		unsigned char lsr;
+
 		/*
 		 * To provide required timeing and allow FIFO transfer,
 		 * __stop_tx_rs485() must be called only when both FIFO and
 		 * shift register are empty. It is for device driver to enable
 		 * interrupt on TEMT.
 		 */
+		readx_poll_timeout_atomic(serial8250_read_lsr, p, lsr,
+					  lsr & UART_LSR_TEMT, 1, 20000);
+
 		if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
 			return;
 
-- 
2.20.1

Re: [PATCH 1/2] tty: serial: 8250: always call tx_chars under spinlock

[Johan Hovold]

On Wed, Dec 09, 2020 at 10:17:27AM +0100, Steffen Trumtrar wrote:
> In most cases serial8250_tx_chars is called with spinlock held.
> Fix the remaining location, too.

Please explain where __start_tx() is called without holding the port
lock and consider fixing that up.

> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
>  drivers/tty/serial/8250/8250_port.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index b0af13074cd3..3310c2b70138 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1559,6 +1559,7 @@ static void serial8250_stop_tx(struct uart_port *port)
>  static inline void __start_tx(struct uart_port *port)
>  {
>  	struct uart_8250_port *up = up_to_u8250p(port);
> +	unsigned long flags;
>  
>  	if (up->dma && !up->dma->tx_dma(up))
>  		return;
> @@ -1569,8 +1570,11 @@ static inline void __start_tx(struct uart_port *port)
>  
>  			lsr = serial_in(up, UART_LSR);
>  			up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
> -			if (lsr & UART_LSR_THRE)
> +			if (lsr & UART_LSR_THRE) {
> +				spin_lock_irqsave(&port->lock, flags);
>  				serial8250_tx_chars(up);
> +				spin_unlock_irqrestore(&port->lock, flags);
> +			}

Since several callers of __start_tx() do hold the lock, this change will
introduce a deadlock.

>  		}
>  	}

Johan

Re: [PATCH 2/2] tty: serial: 8250: wait till transmitter is empty

[Johan Hovold]

On Wed, Dec 09, 2020 at 10:17:28AM +0100, Steffen Trumtrar wrote:
> When only one single character is sent and RS485 signaling is used,
> the driver runs into timing issues.
> 
> When serial8250_tx_chars is called the single character is transmitted.
> The check on uart_circ_empty will be positive and __stop_tx is called.
> The check on UART_LSR_TEMT in BOTH_EMPTY will then be negativ and the
> function will return. On the next call to serial8250_tx_chars
> uart_circ_empty will still be true but the check on BOTH_EMPTY in
> __stop_tx might still fail. This leads to a deadlock.
> 
> Use readx_poll_timeout_atomic to allow the shift register to be emptied
> before checking on BOTH_EMPTY.
> 
> The timeout value is copied from 8250_dw.c.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
>  drivers/tty/serial/8250/8250_port.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 3310c2b70138..87daf3758ff0 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -18,6 +18,7 @@
>  #include <linux/console.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/sysrq.h>
> +#include <linux/iopoll.h>
>  #include <linux/delay.h>
>  #include <linux/platform_device.h>
>  #include <linux/tty.h>
> @@ -1519,18 +1520,27 @@ static inline void __do_stop_tx(struct uart_8250_port *p)
>  		serial8250_rpm_put_tx(p);
>  }
>  
> +static unsigned char serial8250_read_lsr(struct uart_8250_port *p)
> +{
> +	return serial_in(p, UART_LSR);
> +}
> +
>  static inline void __stop_tx(struct uart_8250_port *p)
>  {
>  	struct uart_8250_em485 *em485 = p->em485;
>  
>  	if (em485) {
> -		unsigned char lsr = serial_in(p, UART_LSR);
> +		unsigned char lsr;
> +
>  		/*
>  		 * To provide required timeing and allow FIFO transfer,
>  		 * __stop_tx_rs485() must be called only when both FIFO and
>  		 * shift register are empty. It is for device driver to enable
>  		 * interrupt on TEMT.
>  		 */
> +		readx_poll_timeout_atomic(serial8250_read_lsr, p, lsr,
> +					  lsr & UART_LSR_TEMT, 1, 20000);

Tight polling (1 us) for 20 ms with interrupts disabled?!

Without having looked at the details, there's got to be a better way to
handle this.

> +
>  		if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
>  			return;

Johan

Re: [PATCH 1/2] tty: serial: 8250: always call tx_chars under spinlock

[Steffen Trumtrar]

Hi!

Johan Hovold <johan@kernel.org> writes:

> On Wed, Dec 09, 2020 at 10:17:27AM +0100, Steffen Trumtrar wrote:
>> In most cases serial8250_tx_chars is called with spinlock held.
>> Fix the remaining location, too.
>
> Please explain where __start_tx() is called without holding the port
> lock and consider fixing that up.
>
>> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
>> ---
>>  drivers/tty/serial/8250/8250_port.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
>> index b0af13074cd3..3310c2b70138 100644
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -1559,6 +1559,7 @@ static void serial8250_stop_tx(struct uart_port *port)
>>  static inline void __start_tx(struct uart_port *port)
>>  {
>>  	struct uart_8250_port *up = up_to_u8250p(port);
>> +	unsigned long flags;
>>
>>  	if (up->dma && !up->dma->tx_dma(up))
>>  		return;
>> @@ -1569,8 +1570,11 @@ static inline void __start_tx(struct uart_port *port)
>>
>>  			lsr = serial_in(up, UART_LSR);
>>  			up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
>> -			if (lsr & UART_LSR_THRE)
>> +			if (lsr & UART_LSR_THRE) {
>> +				spin_lock_irqsave(&port->lock, flags);
>>  				serial8250_tx_chars(up);
>> +				spin_unlock_irqrestore(&port->lock, flags);
>> +			}
>
> Since several callers of __start_tx() do hold the lock, this change will
> introduce a deadlock.
>

Meh, yeah :(

Seem like the correct solution would be to fix start_tx_rs485 and
serial8250_start_tx instead.


Best regards,
Steffen Trumtrar

--
Pengutronix e.K.                | Dipl.-Inform. Steffen Trumtrar |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |

Re: [PATCH 2/2] tty: serial: 8250: wait till transmitter is empty

[Steffen Trumtrar]

Hi!

Johan Hovold <johan@kernel.org> writes:

> On Wed, Dec 09, 2020 at 10:17:28AM +0100, Steffen Trumtrar wrote:
>> When only one single character is sent and RS485 signaling is used,
>> the driver runs into timing issues.
>>
>> When serial8250_tx_chars is called the single character is transmitted.
>> The check on uart_circ_empty will be positive and __stop_tx is called.
>> The check on UART_LSR_TEMT in BOTH_EMPTY will then be negativ and the
>> function will return. On the next call to serial8250_tx_chars
>> uart_circ_empty will still be true but the check on BOTH_EMPTY in
>> __stop_tx might still fail. This leads to a deadlock.
>>
>> Use readx_poll_timeout_atomic to allow the shift register to be emptied
>> before checking on BOTH_EMPTY.
>>
>> The timeout value is copied from 8250_dw.c.
>>
>> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
>> ---
>>  drivers/tty/serial/8250/8250_port.c | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
>> index 3310c2b70138..87daf3758ff0 100644
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/console.h>
>>  #include <linux/gpio/consumer.h>
>>  #include <linux/sysrq.h>
>> +#include <linux/iopoll.h>
>>  #include <linux/delay.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/tty.h>
>> @@ -1519,18 +1520,27 @@ static inline void __do_stop_tx(struct uart_8250_port *p)
>>  		serial8250_rpm_put_tx(p);
>>  }
>>
>> +static unsigned char serial8250_read_lsr(struct uart_8250_port *p)
>> +{
>> +	return serial_in(p, UART_LSR);
>> +}
>> +
>>  static inline void __stop_tx(struct uart_8250_port *p)
>>  {
>>  	struct uart_8250_em485 *em485 = p->em485;
>>
>>  	if (em485) {
>> -		unsigned char lsr = serial_in(p, UART_LSR);
>> +		unsigned char lsr;
>> +
>>  		/*
>>  		 * To provide required timeing and allow FIFO transfer,
>>  		 * __stop_tx_rs485() must be called only when both FIFO and
>>  		 * shift register are empty. It is for device driver to enable
>>  		 * interrupt on TEMT.
>>  		 */
>> +		readx_poll_timeout_atomic(serial8250_read_lsr, p, lsr,
>> +					  lsr & UART_LSR_TEMT, 1, 20000);
>
> Tight polling (1 us) for 20 ms with interrupts disabled?!
>
> Without having looked at the details, there's got to be a better way to
> handle this.
>

I'm sure there is. I just "copied" from 8250_dw.c O:-)


Best regards,
Steffen Trumtrar

--
Pengutronix e.K.                | Dipl.-Inform. Steffen Trumtrar |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |

Re: [PATCH 1/2] tty: serial: 8250: always call tx_chars under spinlock

[Steffen Trumtrar]

Steffen Trumtrar <s.trumtrar@pengutronix.de> writes:

> Hi!
>
> Johan Hovold <johan@kernel.org> writes:
>
>> On Wed, Dec 09, 2020 at 10:17:27AM +0100, Steffen Trumtrar wrote:
>>> In most cases serial8250_tx_chars is called with spinlock held.
>>> Fix the remaining location, too.
>>
>> Please explain where __start_tx() is called without holding the port
>> lock and consider fixing that up.
>>
>>> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
>>> ---
>>>  drivers/tty/serial/8250/8250_port.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
>>> index b0af13074cd3..3310c2b70138 100644
>>> --- a/drivers/tty/serial/8250/8250_port.c
>>> +++ b/drivers/tty/serial/8250/8250_port.c
>>> @@ -1559,6 +1559,7 @@ static void serial8250_stop_tx(struct uart_port *port)
>>>  static inline void __start_tx(struct uart_port *port)
>>>  {
>>>  	struct uart_8250_port *up = up_to_u8250p(port);
>>> +	unsigned long flags;
>>>
>>>  	if (up->dma && !up->dma->tx_dma(up))
>>>  		return;
>>> @@ -1569,8 +1570,11 @@ static inline void __start_tx(struct uart_port *port)
>>>
>>>  			lsr = serial_in(up, UART_LSR);
>>>  			up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
>>> -			if (lsr & UART_LSR_THRE)
>>> +			if (lsr & UART_LSR_THRE) {
>>> +				spin_lock_irqsave(&port->lock, flags);
>>>  				serial8250_tx_chars(up);
>>> +				spin_unlock_irqrestore(&port->lock, flags);
>>> +			}
>>
>> Since several callers of __start_tx() do hold the lock, this change will
>> introduce a deadlock.
>>
>
> Meh, yeah :(
>
> Seem like the correct solution would be to fix start_tx_rs485 and
> serial8250_start_tx instead.
>

Scratch that. This patch is bogus, I'm stupid, port-lock is already held
by uart_write apart from the hrtimer-callback.

Sorry for the noise.


Best regards,
Steffen Trumtrar

--
Pengutronix e.K.                | Dipl.-Inform. Steffen Trumtrar |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |