[PATCH] USB: serial: cp210x: map B0 to B9600

[PATCH] USB: serial: cp210x: map B0 to B9600

[Alex Henrie]

When a baud rate of 0 is requested, both the 8250 driver and the FTDI
driver reset the baud rate to the default of 9600 (see the comment above
the uart_get_baud_rate function). Some old versions of the NXP blhost
utility depend on this behavior. However, the CP210x driver resets the
baud rate to the minimum supported rate of 300. Special-case B0 so that
it returns the baud rate to the more sensible default of 9600.

Signed-off-by: Alex Henrie <alexh@vpitech.com>
---
 drivers/usb/serial/cp210x.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 3bcec419f463..2c910550dca8 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -1051,9 +1051,14 @@ static void cp210x_change_speed(struct tty_struct *tty,
 	 * This maps the requested rate to the actual rate, a valid rate on
 	 * cp2102 or cp2103, or to an arbitrary rate in [1M, max_speed].
 	 *
-	 * NOTE: B0 is not implemented.
+	 * NOTE: B0 is not implemented, apart from returning the baud rate to
+	 * the default of B9600.
 	 */
-	baud = clamp(tty->termios.c_ospeed, priv->min_speed, priv->max_speed);
+	if (tty->termios.c_ospeed) {
+		baud = clamp(tty->termios.c_ospeed, priv->min_speed, priv->max_speed);
+	} else {
+		baud = 9600;
+	}
 
 	if (priv->use_actual_rate)
 		baud = cp210x_get_actual_rate(baud);
@@ -1069,7 +1074,8 @@ static void cp210x_change_speed(struct tty_struct *tty,
 			baud = 9600;
 	}
 
-	tty_encode_baud_rate(tty, baud, baud);
+	if (tty->termios.c_ospeed)
+		tty_encode_baud_rate(tty, baud, baud);
 }
 
 static void cp210x_enable_event_mode(struct usb_serial_port *port)
-- 
2.38.1

Re: [PATCH] USB: serial: cp210x: map B0 to B9600

[Greg KH]

On Fri, Nov 25, 2022 at 08:58:25PM -0700, Alex Henrie wrote:
> When a baud rate of 0 is requested, both the 8250 driver and the FTDI
> driver reset the baud rate to the default of 9600 (see the comment above
> the uart_get_baud_rate function). Some old versions of the NXP blhost
> utility depend on this behavior. However, the CP210x driver resets the
> baud rate to the minimum supported rate of 300. Special-case B0 so that
> it returns the baud rate to the more sensible default of 9600.
> 
> Signed-off-by: Alex Henrie <alexh@vpitech.com>
> ---
>  drivers/usb/serial/cp210x.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index 3bcec419f463..2c910550dca8 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -1051,9 +1051,14 @@ static void cp210x_change_speed(struct tty_struct *tty,
>  	 * This maps the requested rate to the actual rate, a valid rate on
>  	 * cp2102 or cp2103, or to an arbitrary rate in [1M, max_speed].
>  	 *
> -	 * NOTE: B0 is not implemented.
> +	 * NOTE: B0 is not implemented, apart from returning the baud rate to
> +	 * the default of B9600.
>  	 */
> -	baud = clamp(tty->termios.c_ospeed, priv->min_speed, priv->max_speed);
> +	if (tty->termios.c_ospeed) {
> +		baud = clamp(tty->termios.c_ospeed, priv->min_speed, priv->max_speed);
> +	} else {
> +		baud = 9600;
> +	}

No need for { } here (checkpatch.pl should have warned about this.)

thanks,

greg k-h

Re: [PATCH] USB: serial: cp210x: map B0 to B9600

[Sergey Shtylyov]

Hello!

On 11/26/22 6:58 AM, Alex Henrie wrote:

> When a baud rate of 0 is requested, both the 8250 driver and the FTDI
> driver reset the baud rate to the default of 9600 (see the comment above
> the uart_get_baud_rate function). Some old versions of the NXP blhost
> utility depend on this behavior. However, the CP210x driver resets the
> baud rate to the minimum supported rate of 300. Special-case B0 so that
> it returns the baud rate to the more sensible default of 9600.
> 
> Signed-off-by: Alex Henrie <alexh@vpitech.com>
> ---
>  drivers/usb/serial/cp210x.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index 3bcec419f463..2c910550dca8 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -1051,9 +1051,14 @@ static void cp210x_change_speed(struct tty_struct *tty,
>  	 * This maps the requested rate to the actual rate, a valid rate on
>  	 * cp2102 or cp2103, or to an arbitrary rate in [1M, max_speed].
>  	 *
> -	 * NOTE: B0 is not implemented.
> +	 * NOTE: B0 is not implemented, apart from returning the baud rate to
> +	 * the default of B9600.
>  	 */
> -	baud = clamp(tty->termios.c_ospeed, priv->min_speed, priv->max_speed);
> +	if (tty->termios.c_ospeed) {
> +		baud = clamp(tty->termios.c_ospeed, priv->min_speed, priv->max_speed);
> +	} else {
> +		baud = 9600;
> +	}

   {} not needed at all...

[...]

MBR, Sergey

Re: [PATCH] USB: serial: cp210x: map B0 to B9600

[Johan Hovold]

On Fri, Nov 25, 2022 at 08:58:25PM -0700, Alex Henrie wrote:
> When a baud rate of 0 is requested, both the 8250 driver and the FTDI
> driver reset the baud rate to the default of 9600 (see the comment above
> the uart_get_baud_rate function). Some old versions of the NXP blhost
> utility depend on this behavior. 

What exactly do you mean by "depend on" here? Setting B0 is supposed to
hang up a modem connection by deasserting the modem control lines, but
there's nothing mandating any particular line speed to be set in
hardware. Why would that even matter?

If the user space tool is thrown off by the fact that B0 isn't
implemented, perhaps that's what should be addressed.

Johan

Re: [PATCH] USB: serial: cp210x: map B0 to B9600

[Alex Henrie]

On Saturday, November 26, 2022 12:10:07 AM MST Greg KH wrote:
> No need for { } here (checkpatch.pl should have warned about this.)

On Saturday, November 26, 2022 3:34:57 AM MST Sergey Shtylyov wrote:
>    {} not needed at all...

Thanks for the feedback. I will send a new version after running 
checkpatch.pl, and in the future I'll try to remember to run checkpatch.pl 
over my patches before sending them.

-Alex

Re: [PATCH] USB: serial: cp210x: map B0 to B9600

[Alex Henrie]

On Monday, November 28, 2022 7:41:59 AM MST Johan Hovold wrote:
> On Fri, Nov 25, 2022 at 08:58:25PM -0700, Alex Henrie wrote:
> > When a baud rate of 0 is requested, both the 8250 driver and the FTDI
> > driver reset the baud rate to the default of 9600 (see the comment above
> > the uart_get_baud_rate function). Some old versions of the NXP blhost
> > utility depend on this behavior.
> 
> What exactly do you mean by "depend on" here? Setting B0 is supposed to
> hang up a modem connection by deasserting the modem control lines, but
> there's nothing mandating any particular line speed to be set in
> hardware. Why would that even matter?
> 
> If the user space tool is thrown off by the fact that B0 isn't
> implemented, perhaps that's what should be addressed.

Oh, it's definitely a bug in blhost. The program sets the baud rate to 0, then 
tries to communicate over the UART assuming that the baud rate is 9600. It's 
been fixed in the latest version of blhost, but I'm stuck on an old version and 
there's nothing I can do about that.

I don't know why the 8250 and FTDI drivers map B0 to B9600, however it's very 
old behavior that must have had a purpose. Maybe Russell knows? Alternatively, 
leaving the baud rate unchanged seems like reasonable behavior and would also 
work with the old blhost. But mapping B0 to B300 makes even less sense than 
mapping it to B9600.

-Alex

[PATCH v2] USB: serial: cp210x: map B0 to B9600

[Alex Henrie]

When a baud rate of 0 is requested, both the 8250 driver and the FTDI
driver reset the baud rate to the default of 9600 (see the comment above
the uart_get_baud_rate function). Some old versions of the NXP blhost
utility depend on this behavior. However, the CP210x driver resets the
baud rate to the minimum supported rate of 300. Special-case B0 so that
it returns the baud rate to the more sensible default of 9600.

Signed-off-by: Alex Henrie <alexh@vpitech.com>
---
 drivers/usb/serial/cp210x.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 3bcec419f463..b2409167b27f 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -1051,9 +1051,13 @@ static void cp210x_change_speed(struct tty_struct *tty,
 	 * This maps the requested rate to the actual rate, a valid rate on
 	 * cp2102 or cp2103, or to an arbitrary rate in [1M, max_speed].
 	 *
-	 * NOTE: B0 is not implemented.
+	 * NOTE: B0 is not implemented, apart from returning the baud rate to
+	 * the default of B9600.
 	 */
-	baud = clamp(tty->termios.c_ospeed, priv->min_speed, priv->max_speed);
+	if (tty->termios.c_ospeed)
+		baud = clamp(tty->termios.c_ospeed, priv->min_speed, priv->max_speed);
+	else
+		baud = 9600;
 
 	if (priv->use_actual_rate)
 		baud = cp210x_get_actual_rate(baud);
@@ -1069,7 +1073,8 @@ static void cp210x_change_speed(struct tty_struct *tty,
 			baud = 9600;
 	}
 
-	tty_encode_baud_rate(tty, baud, baud);
+	if (tty->termios.c_ospeed)
+		tty_encode_baud_rate(tty, baud, baud);
 }
 
 static void cp210x_enable_event_mode(struct usb_serial_port *port)
-- 
2.38.1

Re: [PATCH] USB: serial: cp210x: map B0 to B9600

[Russell King (Oracle)]

On Mon, Nov 28, 2022 at 11:08:25AM -0700, Alex Henrie wrote:
> On Monday, November 28, 2022 7:41:59 AM MST Johan Hovold wrote:
> > On Fri, Nov 25, 2022 at 08:58:25PM -0700, Alex Henrie wrote:
> > > When a baud rate of 0 is requested, both the 8250 driver and the FTDI
> > > driver reset the baud rate to the default of 9600 (see the comment above
> > > the uart_get_baud_rate function). Some old versions of the NXP blhost
> > > utility depend on this behavior.
> > 
> > What exactly do you mean by "depend on" here? Setting B0 is supposed to
> > hang up a modem connection by deasserting the modem control lines, but
> > there's nothing mandating any particular line speed to be set in
> > hardware. Why would that even matter?
> > 
> > If the user space tool is thrown off by the fact that B0 isn't
> > implemented, perhaps that's what should be addressed.
> 
> Oh, it's definitely a bug in blhost. The program sets the baud rate to 0, then 
> tries to communicate over the UART assuming that the baud rate is 9600. It's 
> been fixed in the latest version of blhost, but I'm stuck on an old version and 
> there's nothing I can do about that.
> 
> I don't know why the 8250 and FTDI drivers map B0 to B9600, however it's very 
> old behavior that must have had a purpose. Maybe Russell knows?

What exactly do you think should be done when a baud rate of zero is
requested?

The fact of the matter is that at hardware level, the UART takes a
clock, and divides that down. To get to a baud rate of zero, one
would need an infinitely large divisor, which (a) is impossible to
program into the hardware and (b) would trigger a divide-by-zero
error in the kernel. So, we have to choose something.

That decision was made before my time, when Ted Ts'o was maintaining
what was the original serial.c 8250-based driver, and the behaviour
he adopted was to set a baud rate of 9600 when B0 was requested,
which is reasonable - 9600 baud is the default setting.

POSIX (which is what we use to define the behaviour of the TTY layer,
or at least what we _used_ to) doesn't specify the behaviour of B0
as the output rate, other than it shall cause the model control lines
to be deasserted. What baud rate you get on the line is unspecified,
and thus left up to the implementation.

So basically, 9600 baud for B0 is the behaviour of the 8250 driver
going back to the very early Linux versions and that has become the
standard Linux implementation shared by a great many serial drivers.
In effect, it's almost a de-facto standard.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] USB: serial: cp210x: map B0 to B9600

[Alex Henrie]

On Monday, November 28, 2022 11:20:41 AM MST Russell King (Oracle) wrote:
> What exactly do you think should be done when a baud rate of zero is
> requested?

I see two reasonable options: Leave the baud rate alone, or reset it to the 
default (i.e. 9600). In my opinion, either of those options is just fine.

> The fact of the matter is that at hardware level, the UART takes a
> clock, and divides that down. To get to a baud rate of zero, one
> would need an infinitely large divisor, which (a) is impossible to
> program into the hardware and (b) would trigger a divide-by-zero
> error in the kernel. So, we have to choose something.
> 
> That decision was made before my time, when Ted Ts'o was maintaining
> what was the original serial.c 8250-based driver, and the behaviour
> he adopted was to set a baud rate of 9600 when B0 was requested,
> which is reasonable - 9600 baud is the default setting.
> 
> POSIX (which is what we use to define the behaviour of the TTY layer,
> or at least what we _used_ to) doesn't specify the behaviour of B0
> as the output rate, other than it shall cause the model control lines
> to be deasserted. What baud rate you get on the line is unspecified,
> and thus left up to the implementation.
> 
> So basically, 9600 baud for B0 is the behaviour of the 8250 driver
> going back to the very early Linux versions and that has become the
> standard Linux implementation shared by a great many serial drivers.
> In effect, it's almost a de-facto standard.

That is really interesting, thanks for the explanation! I like the idea of 
having consistent behavior across the Linux serial drivers, so it seems to me 
that mapping B0 to B9600 in all drivers is the way to go.

-Alex

Re: [PATCH] USB: serial: cp210x: map B0 to B9600

[Johan Hovold]

On Mon, Nov 28, 2022 at 11:08:25AM -0700, Alex Henrie wrote:
> On Monday, November 28, 2022 7:41:59 AM MST Johan Hovold wrote:
> > On Fri, Nov 25, 2022 at 08:58:25PM -0700, Alex Henrie wrote:
> > > When a baud rate of 0 is requested, both the 8250 driver and the FTDI
> > > driver reset the baud rate to the default of 9600 (see the comment above
> > > the uart_get_baud_rate function). Some old versions of the NXP blhost
> > > utility depend on this behavior.
> > 
> > What exactly do you mean by "depend on" here? Setting B0 is supposed to
> > hang up a modem connection by deasserting the modem control lines, but
> > there's nothing mandating any particular line speed to be set in
> > hardware. Why would that even matter?
> > 
> > If the user space tool is thrown off by the fact that B0 isn't
> > implemented, perhaps that's what should be addressed.
> 
> Oh, it's definitely a bug in blhost. The program sets the baud rate to 0, then 
> tries to communicate over the UART assuming that the baud rate is 9600. It's 
> been fixed in the latest version of blhost, but I'm stuck on an old version and 
> there's nothing I can do about that.
> 
> I don't know why the 8250 and FTDI drivers map B0 to B9600, however it's very 
> old behavior that must have had a purpose.

Heh, not everything has a purpose even if it's old.

For implementation and protocol reasons a driver may need to pick some
arbitrary speed to use for B0, but this is generally not needed for USB
serial devices and only about half of the drivers do so (and then tend
to pick 9600).

Also note that the FTDI driver does in fact leave the line speed
unchanged when B0 is requested (that zero-baud check in
get_ftdi_divisor() is only used for ASYNC_SPD_CUST).

> Maybe Russell knows? Alternatively, 
> leaving the baud rate unchanged seems like reasonable behavior and would also 
> work with the old blhost. But mapping B0 to B300 makes even less sense than 
> mapping it to B9600.

Your application really should not depend on any particular hardware
setting after requesting B0.

That said, I've implemented support for B0 in cp210x which leaves the
current settings unchanged (and which incidentally allows you to
use the buggy tool).

Johan

Re: [PATCH] USB: serial: cp210x: map B0 to B9600

[Alex Henrie]

On Tuesday, November 29, 2022 7:15:08 AM MST Johan Hovold wrote:
> Also note that the FTDI driver does in fact leave the line speed
> unchanged when B0 is requested (that zero-baud check in
> get_ftdi_divisor() is only used for ASYNC_SPD_CUST).

I tested it empirically today, and you're right, the FTDI driver simply does 
not change the baud rate. Thank you for correcting my misunderstanding.

> Your application really should not depend on any particular hardware
> setting after requesting B0.

Understood.

> That said, I've implemented support for B0 in cp210x which leaves the
> current settings unchanged (and which incidentally allows you to
> use the buggy tool).

Excellent! Many thanks!

-Alex