diff mbox series

[v4,1/5] serial: core: tidy invalid baudrate handling in uart_get_baud_rate

Message ID 20230928151631.149333-2-jcmvbkbc@gmail.com
State New
Headers
Series serial: add drivers for the ESP32xx serial devices |

Commit Message

Max Filippov Sept. 28, 2023, 3:16 p.m. UTC 
  uart_get_baud_rate has input parameters 'min' and 'max' limiting the
range of acceptable baud rates from the caller's perspective. If neither
current or old termios structures have acceptable baud rate setting and
9600 is not in the min/max range either the function returns 0 and
issues a warning.
However for a UART that does not support speed of 9600 baud this is
expected behavior.
Clarify that 0 can be (and always could be) returned from the
uart_get_baud_rate. Don't issue a warning in that case.
Move the warinng to the uart_get_divisor instead, which is often called
with the uart_get_baud_rate return value.

Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
Changes v3->v4:
- drop WARN_ON from uart_get_divisor()

 drivers/tty/serial/serial_core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Ilpo Järvinen Sept. 29, 2023, 6:34 a.m. UTC | #1 
On Thu, 28 Sep 2023, Max Filippov wrote:

> uart_get_baud_rate has input parameters 'min' and 'max' limiting the
> range of acceptable baud rates from the caller's perspective. If neither
> current or old termios structures have acceptable baud rate setting and
> 9600 is not in the min/max range either the function returns 0 and
> issues a warning.
> However for a UART that does not support speed of 9600 baud this is
> expected behavior.
> Clarify that 0 can be (and always could be) returned from the
> uart_get_baud_rate. Don't issue a warning in that case.
> Move the warinng to the uart_get_divisor instead, which is often called
> with the uart_get_baud_rate return value.
> 
> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> ---
> Changes v3->v4:
> - drop WARN_ON from uart_get_divisor()
> 
>  drivers/tty/serial/serial_core.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 7bdc21d5e13b..3f130fe9f1a0 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -431,7 +431,7 @@ EXPORT_SYMBOL(uart_update_timeout);
>   * baud.
>   *
>   * If the new baud rate is invalid, try the @old termios setting. If it's still
> - * invalid, we try 9600 baud.
> + * invalid, we try 9600 baud. If that is also invalid 0 is returned.
>   *
>   * The @termios structure is updated to reflect the baud rate we're actually
>   * going to be using. Don't do this for the case where B0 is requested ("hang
> @@ -515,8 +515,6 @@ uart_get_baud_rate(struct uart_port *port, struct ktermios *termios,
>  							max - 1, max - 1);
>  		}
>  	}
> -	/* Should never happen */
> -	WARN_ON(1);
>  	return 0;
>  }
>  EXPORT_SYMBOL(uart_get_baud_rate);

While looking into this, I found this old commit:

commit 16ae2a877bf4179737921235e85ceffd7b79354f
Author: Alan Cox <alan@linux.intel.com>
Date:   Mon Jan 4 16:26:21 2010 +0000

    serial: Fix crash if the minimum rate of the device is > 9600 baud
    
    In that situation if the old rate is invalid and the new rate is invalid
    and the chip cannot do 9600 baud we report zero, which makes all the
    drivers explode.
    
    Instead force the rate based on min/max

But for some reason it does not work as advertized here? What is the exact 
cause for that?

Is something wrong with how min/max have that +1/-1 there or what?
Max Filippov Sept. 29, 2023, 7:26 p.m. UTC | #2 
On Thu, Sep 28, 2023 at 11:34 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
> While looking into this, I found this old commit:
>
> commit 16ae2a877bf4179737921235e85ceffd7b79354f
> Author: Alan Cox <alan@linux.intel.com>
> Date:   Mon Jan 4 16:26:21 2010 +0000
>
>     serial: Fix crash if the minimum rate of the device is > 9600 baud
>
>     In that situation if the old rate is invalid and the new rate is invalid
>     and the chip cannot do 9600 baud we report zero, which makes all the
>     drivers explode.
>
>     Instead force the rate based on min/max
>
> But for some reason it does not work as advertized here? What is the exact
> cause for that?

In my case I see that tty_termios_encode_baud_rate() is called with
ibaud == obaud == 9769, but it finds the closest rate 9600, which is
within 2% of the actual minimum, but is outside the min/max range
supported by the hardware.

> Is something wrong with how min/max have that +1/-1 there or what?
Greg KH Oct. 3, 2023, 12:39 p.m. UTC | #3 
On Thu, Sep 28, 2023 at 08:16:27AM -0700, Max Filippov wrote:
> uart_get_baud_rate has input parameters 'min' and 'max' limiting the
> range of acceptable baud rates from the caller's perspective. If neither
> current or old termios structures have acceptable baud rate setting and
> 9600 is not in the min/max range either the function returns 0 and
> issues a warning.
> However for a UART that does not support speed of 9600 baud this is
> expected behavior.
> Clarify that 0 can be (and always could be) returned from the
> uart_get_baud_rate. Don't issue a warning in that case.
> Move the warinng to the uart_get_divisor instead, which is often called
> with the uart_get_baud_rate return value.

This doesn't match up with the patch contents anymore :(
diff mbox series

Patch

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 7bdc21d5e13b..3f130fe9f1a0 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -431,7 +431,7 @@  EXPORT_SYMBOL(uart_update_timeout);
  * baud.
  *
  * If the new baud rate is invalid, try the @old termios setting. If it's still
- * invalid, we try 9600 baud.
+ * invalid, we try 9600 baud. If that is also invalid 0 is returned.
  *
  * The @termios structure is updated to reflect the baud rate we're actually
  * going to be using. Don't do this for the case where B0 is requested ("hang
@@ -515,8 +515,6 @@  uart_get_baud_rate(struct uart_port *port, struct ktermios *termios,
 							max - 1, max - 1);
 		}
 	}
-	/* Should never happen */
-	WARN_ON(1);
 	return 0;
 }
 EXPORT_SYMBOL(uart_get_baud_rate);