diff mbox series

[v2,2/7] serial: qcom-geni: Fix the timeout in qcom_geni_serial_poll_bit()

Message ID 20240530154553.v2.2.I3e1968bbeee67e28fd4e15509950805b6665484a@changeid (mailing list archive)
State Superseded
Headers show
Series serial: qcom-geni: Overhaul TX handling to fix crashes/hangs | expand

Commit Message

Doug Anderson May 30, 2024, 10:45 p.m. UTC
The qcom_geni_serial_poll_bit() is supposed to be able to be used to
poll a bit that's will become set when a TX transfer finishes. Because
of this it tries to set its timeout based on how long the UART will
take to shift out all of the queued bytes. There are two problems
here:
1. There appears to be a hidden extra word on the firmware side which
   is the word that the firmware has already taken out of the FIFO and
   is currently shifting out. We need to account for this.
2. The timeout calculation was assuming that it would only need 8 bits
   on the wire to shift out 1 byte. This isn't true. Typically 10 bits
   are used (8 data bits, 1 start and 1 stop bit), but as much as 13
   bits could be used (14 if we allowed 9 bits per byte, which we
   don't).

The too-short timeout was seen causing problems in a future patch
which more properly waited for bytes to transfer out of the UART
before cancelling.

Fixes: c4f528795d1a ("tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- New

 drivers/tty/serial/qcom_geni_serial.c | 32 ++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

Comments

Andy Shevchenko May 31, 2024, 8:34 a.m. UTC | #1
On Thu, May 30, 2024 at 03:45:54PM -0700, Douglas Anderson wrote:
> The qcom_geni_serial_poll_bit() is supposed to be able to be used to
> poll a bit that's will become set when a TX transfer finishes. Because
> of this it tries to set its timeout based on how long the UART will
> take to shift out all of the queued bytes. There are two problems
> here:
> 1. There appears to be a hidden extra word on the firmware side which
>    is the word that the firmware has already taken out of the FIFO and
>    is currently shifting out. We need to account for this.
> 2. The timeout calculation was assuming that it would only need 8 bits
>    on the wire to shift out 1 byte. This isn't true. Typically 10 bits
>    are used (8 data bits, 1 start and 1 stop bit), but as much as 13
>    bits could be used (14 if we allowed 9 bits per byte, which we
>    don't).
> 
> The too-short timeout was seen causing problems in a future patch
> which more properly waited for bytes to transfer out of the UART
> before cancelling.

...

> +		/*
> +		 * Add 1 to tx_fifo_depth to account for the hidden register
> +		 * on the firmware side that can hold a word.
> +		 */
> +		max_queued_bytes =
> +			DIV_ROUND_UP((port->tx_fifo_depth + 1) * port->tx_fifo_width,
> +				     BITS_PER_BYTE);

BITS_TO_BYTES()

...

> -		timeout_us = ((fifo_bits * USEC_PER_SEC) / baud) + 500;
> +		timeout_us = ((max_queued_bits * USEC_PER_SEC) / baud) + 500;

Too many parentheses. (The outer ones can be dropped.
Ilpo Järvinen May 31, 2024, 2:57 p.m. UTC | #2
On Thu, 30 May 2024, Douglas Anderson wrote:

> The qcom_geni_serial_poll_bit() is supposed to be able to be used to
> poll a bit that's will become set when a TX transfer finishes. Because
> of this it tries to set its timeout based on how long the UART will
> take to shift out all of the queued bytes. There are two problems
> here:
> 1. There appears to be a hidden extra word on the firmware side which
>    is the word that the firmware has already taken out of the FIFO and
>    is currently shifting out. We need to account for this.
> 2. The timeout calculation was assuming that it would only need 8 bits
>    on the wire to shift out 1 byte. This isn't true. Typically 10 bits
>    are used (8 data bits, 1 start and 1 stop bit), but as much as 13
>    bits could be used (14 if we allowed 9 bits per byte, which we
>    don't).
> 
> The too-short timeout was seen causing problems in a future patch
> which more properly waited for bytes to transfer out of the UART
> before cancelling.
> 
> Fixes: c4f528795d1a ("tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> Changes in v2:
> - New
> 
>  drivers/tty/serial/qcom_geni_serial.c | 32 ++++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 2bd25afe0d92..32e025705f99 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -271,7 +271,8 @@ static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
>  	u32 reg;
>  	struct qcom_geni_serial_port *port;
>  	unsigned int baud;
> -	unsigned int fifo_bits;
> +	unsigned int max_queued_bytes;
> +	unsigned int max_queued_bits;
>  	unsigned long timeout_us = 20000;
>  	struct qcom_geni_private_data *private_data = uport->private_data;
>  
> @@ -280,12 +281,37 @@ static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
>  		baud = port->baud;
>  		if (!baud)
>  			baud = 115200;
> -		fifo_bits = port->tx_fifo_depth * port->tx_fifo_width;
> +
> +		/*
> +		 * Add 1 to tx_fifo_depth to account for the hidden register
> +		 * on the firmware side that can hold a word.
> +		 */
> +		max_queued_bytes =
> +			DIV_ROUND_UP((port->tx_fifo_depth + 1) * port->tx_fifo_width,
> +				     BITS_PER_BYTE);
> +
> +		/*
> +		 * The maximum number of bits per byte on the wire is 13 from:
> +		 * - 1 start bit
> +		 * - 8 data bits
> +		 * - 1 parity bit
> +		 * - 3 stop bits
> +		 *
> +		 * While we could try count the actual bits per byte based on
> +		 * the port configuration, this is a rough timeout anyway so
> +		 * using the max is fine.
> +		 */
> +		max_queued_bits = max_queued_bytes * 13;
> +
>  		/*
>  		 * Total polling iterations based on FIFO worth of bytes to be
>  		 * sent at current baud. Add a little fluff to the wait.
> +		 *
> +		 * NOTE: this assumes that flow control isn't used, but with
> +		 * flow control we could wait indefinitely and that wouldn't
> +		 * be OK.
>  		 */
> -		timeout_us = ((fifo_bits * USEC_PER_SEC) / baud) + 500;
> +		timeout_us = ((max_queued_bits * USEC_PER_SEC) / baud) + 500;

You should try to generalize the existing uart_fifo_timeout() to suit what 
you're trying to do here instead of writing more variants of code with 
this same intent.
diff mbox series

Patch

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 2bd25afe0d92..32e025705f99 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -271,7 +271,8 @@  static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
 	u32 reg;
 	struct qcom_geni_serial_port *port;
 	unsigned int baud;
-	unsigned int fifo_bits;
+	unsigned int max_queued_bytes;
+	unsigned int max_queued_bits;
 	unsigned long timeout_us = 20000;
 	struct qcom_geni_private_data *private_data = uport->private_data;
 
@@ -280,12 +281,37 @@  static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
 		baud = port->baud;
 		if (!baud)
 			baud = 115200;
-		fifo_bits = port->tx_fifo_depth * port->tx_fifo_width;
+
+		/*
+		 * Add 1 to tx_fifo_depth to account for the hidden register
+		 * on the firmware side that can hold a word.
+		 */
+		max_queued_bytes =
+			DIV_ROUND_UP((port->tx_fifo_depth + 1) * port->tx_fifo_width,
+				     BITS_PER_BYTE);
+
+		/*
+		 * The maximum number of bits per byte on the wire is 13 from:
+		 * - 1 start bit
+		 * - 8 data bits
+		 * - 1 parity bit
+		 * - 3 stop bits
+		 *
+		 * While we could try count the actual bits per byte based on
+		 * the port configuration, this is a rough timeout anyway so
+		 * using the max is fine.
+		 */
+		max_queued_bits = max_queued_bytes * 13;
+
 		/*
 		 * Total polling iterations based on FIFO worth of bytes to be
 		 * sent at current baud. Add a little fluff to the wait.
+		 *
+		 * NOTE: this assumes that flow control isn't used, but with
+		 * flow control we could wait indefinitely and that wouldn't
+		 * be OK.
 		 */
-		timeout_us = ((fifo_bits * USEC_PER_SEC) / baud) + 500;
+		timeout_us = ((max_queued_bits * USEC_PER_SEC) / baud) + 500;
 	}
 
 	/*