| Message ID | 20220712115306.26471-7-kabel@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | ftdi_sio driver improvements | expand |
On Tue, Jul 12, 2022 at 01:53:05PM +0200, Marek Behún wrote: > From: Pali Rohár <pali@kernel.org> > > When ASYNC_SPD_CUST is used, update custom_divisor and c_*speed fields > so that they correspond to the newly set baud rate value, so that > userspace GET ioctls will see the true baud rate that is being used. No, this is wrong. In fact, there's a long-standing bug in this driver which started reporting back the actual baud rate when using SPD_CUST. The rate should be left unchanged at 38400 in that case. > Signed-off-by: Pali Rohár <pali@kernel.org> > Tested-by: Marek Behún <kabel@kernel.org> > Signed-off-by: Marek Behún <kabel@kernel.org> > --- > drivers/usb/serial/ftdi_sio.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c > index 5db1293bb7a2..39e8c5d06157 100644 > --- a/drivers/usb/serial/ftdi_sio.c > +++ b/drivers/usb/serial/ftdi_sio.c > @@ -1303,6 +1303,7 @@ static u32 get_ftdi_divisor(struct tty_struct *tty, > { > struct ftdi_private *priv = usb_get_serial_port_data(port); > struct device *dev = &port->dev; > + int fix_custom_divisor = 0; > int div_value = 0; > int div_okay = 1; > int baud; > @@ -1317,6 +1318,7 @@ static u32 get_ftdi_divisor(struct tty_struct *tty, > if (baud == 38400 && > ((priv->flags & ASYNC_SPD_MASK) == ASYNC_SPD_CUST) && > (priv->custom_divisor)) { > + fix_custom_divisor = 1; > baud = DIV_ROUND_CLOSEST(priv->baud_base, priv->custom_divisor); > dev_dbg(dev, "%s - custom divisor %d sets baud rate to %d\n", > __func__, priv->custom_divisor, baud); > @@ -1401,7 +1403,19 @@ static u32 get_ftdi_divisor(struct tty_struct *tty, > ftdi_chip_name[priv->chip_type]); > } > > + /* Fix deprecated async-compatible custom_divisor hack and update tty baud rate */ > + if (fix_custom_divisor) { > + priv->custom_divisor = DIV_ROUND_CLOSEST(priv->baud_base, baud); > + old_baud = baud; > + baud = 38400; > + } > + > tty_encode_baud_rate(tty, baud, baud); > + > + /* For async-compatible custom_divisor store into TCGETS2 c_*speed fields real baud rate */ > + if (fix_custom_divisor) > + tty->termios.c_ispeed = tty->termios.c_ospeed = old_baud; > + > return div_value; > } > > @@ -2674,6 +2688,8 @@ static void ftdi_set_termios(struct tty_struct *tty, > dev_dbg(ddev, "%s: forcing baud rate for this device\n", __func__); > tty_encode_baud_rate(tty, priv->force_baud, > priv->force_baud); > + termios->c_ispeed = termios->c_ospeed = > + DIV_ROUND_CLOSEST(priv->baud_base, priv->custom_divisor); > } > > /* Force RTS-CTS if this device requires it. */ Johan
On Sunday 24 July 2022 14:28:43 Johan Hovold wrote: > On Tue, Jul 12, 2022 at 01:53:05PM +0200, Marek Behún wrote: > > From: Pali Rohár <pali@kernel.org> > > > > When ASYNC_SPD_CUST is used, update custom_divisor and c_*speed fields > > so that they correspond to the newly set baud rate value, so that > > userspace GET ioctls will see the true baud rate that is being used. > > No, this is wrong. > > In fact, there's a long-standing bug in this driver which started > reporting back the actual baud rate when using SPD_CUST. Hello! And this commit is fixing also this bug as a side change. > The rate should be left unchanged at 38400 in that case. With this change, rate in c_cflag is unchanged and stays at B38400. What is updated is the real baudrate in c_ispeed and c_ospeed extensions. It is really wrong? I thought that c_cflag should stay unchanged at B38400 when ASYNC_SPD_CUST is used. > > Signed-off-by: Pali Rohár <pali@kernel.org> > > Tested-by: Marek Behún <kabel@kernel.org> > > Signed-off-by: Marek Behún <kabel@kernel.org> > > --- > > drivers/usb/serial/ftdi_sio.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c > > index 5db1293bb7a2..39e8c5d06157 100644 > > --- a/drivers/usb/serial/ftdi_sio.c > > +++ b/drivers/usb/serial/ftdi_sio.c > > @@ -1303,6 +1303,7 @@ static u32 get_ftdi_divisor(struct tty_struct *tty, > > { > > struct ftdi_private *priv = usb_get_serial_port_data(port); > > struct device *dev = &port->dev; > > + int fix_custom_divisor = 0; > > int div_value = 0; > > int div_okay = 1; > > int baud; > > @@ -1317,6 +1318,7 @@ static u32 get_ftdi_divisor(struct tty_struct *tty, > > if (baud == 38400 && > > ((priv->flags & ASYNC_SPD_MASK) == ASYNC_SPD_CUST) && > > (priv->custom_divisor)) { > > + fix_custom_divisor = 1; > > baud = DIV_ROUND_CLOSEST(priv->baud_base, priv->custom_divisor); > > dev_dbg(dev, "%s - custom divisor %d sets baud rate to %d\n", > > __func__, priv->custom_divisor, baud); > > @@ -1401,7 +1403,19 @@ static u32 get_ftdi_divisor(struct tty_struct *tty, > > ftdi_chip_name[priv->chip_type]); > > } > > > > + /* Fix deprecated async-compatible custom_divisor hack and update tty baud rate */ > > + if (fix_custom_divisor) { > > + priv->custom_divisor = DIV_ROUND_CLOSEST(priv->baud_base, baud); > > + old_baud = baud; > > + baud = 38400; > > + } > > + > > tty_encode_baud_rate(tty, baud, baud); > > + > > + /* For async-compatible custom_divisor store into TCGETS2 c_*speed fields real baud rate */ > > + if (fix_custom_divisor) > > + tty->termios.c_ispeed = tty->termios.c_ospeed = old_baud; > > + > > return div_value; > > } > > > > @@ -2674,6 +2688,8 @@ static void ftdi_set_termios(struct tty_struct *tty, > > dev_dbg(ddev, "%s: forcing baud rate for this device\n", __func__); > > tty_encode_baud_rate(tty, priv->force_baud, > > priv->force_baud); > > + termios->c_ispeed = termios->c_ospeed = > > + DIV_ROUND_CLOSEST(priv->baud_base, priv->custom_divisor); > > } > > > > /* Force RTS-CTS if this device requires it. */ > > Johan
On Sun, Jul 24, 2022 at 02:33:51PM +0200, Pali Rohár wrote: > On Sunday 24 July 2022 14:28:43 Johan Hovold wrote: > > On Tue, Jul 12, 2022 at 01:53:05PM +0200, Marek Behún wrote: > > > From: Pali Rohár <pali@kernel.org> > > > > > > When ASYNC_SPD_CUST is used, update custom_divisor and c_*speed fields > > > so that they correspond to the newly set baud rate value, so that > > > userspace GET ioctls will see the true baud rate that is being used. > > > > No, this is wrong. > > > > In fact, there's a long-standing bug in this driver which started > > reporting back the actual baud rate when using SPD_CUST. > > Hello! And this commit is fixing also this bug as a side change. Ah, indeed it is, or at least to some extent. > > The rate should be left unchanged at 38400 in that case. > > With this change, rate in c_cflag is unchanged and stays at B38400. Right. > What is updated is the real baudrate in c_ispeed and c_ospeed > extensions. > > It is really wrong? I thought that c_cflag should stay unchanged at > B38400 when ASYNC_SPD_CUST is used. Yeah, cflags stay unchanged, but you shouldn't touch those fields when using the deprecated ASYNC_SPD_CUST hack. Note that this currently only works because the ftdi driver uses tty_get_baud_rate() instead of c_ospeed directly which is the recommended (new) way. > > > + /* Fix deprecated async-compatible custom_divisor hack and update tty baud rate */ > > > + if (fix_custom_divisor) { > > > + priv->custom_divisor = DIV_ROUND_CLOSEST(priv->baud_base, baud); > > > + old_baud = baud; > > > + baud = 38400; > > > + } > > > + > > > tty_encode_baud_rate(tty, baud, baud); > > > + > > > + /* For async-compatible custom_divisor store into TCGETS2 c_*speed fields real baud rate */ > > > + if (fix_custom_divisor) > > > + tty->termios.c_ispeed = tty->termios.c_ospeed = old_baud; > > > + > > > return div_value; > > > } > > > > > > @@ -2674,6 +2688,8 @@ static void ftdi_set_termios(struct tty_struct *tty, > > > dev_dbg(ddev, "%s: forcing baud rate for this device\n", __func__); > > > tty_encode_baud_rate(tty, priv->force_baud, > > > priv->force_baud); > > > + termios->c_ispeed = termios->c_ospeed = > > > + DIV_ROUND_CLOSEST(priv->baud_base, priv->custom_divisor); > > > } > > > > > > /* Force RTS-CTS if this device requires it. */ Johan
On Sunday 24 July 2022 14:54:58 Johan Hovold wrote: > On Sun, Jul 24, 2022 at 02:33:51PM +0200, Pali Rohár wrote: > > On Sunday 24 July 2022 14:28:43 Johan Hovold wrote: > > > On Tue, Jul 12, 2022 at 01:53:05PM +0200, Marek Behún wrote: > > > > From: Pali Rohár <pali@kernel.org> > > > > > > > > When ASYNC_SPD_CUST is used, update custom_divisor and c_*speed fields > > > > so that they correspond to the newly set baud rate value, so that > > > > userspace GET ioctls will see the true baud rate that is being used. > > > > > > No, this is wrong. > > > > > > In fact, there's a long-standing bug in this driver which started > > > reporting back the actual baud rate when using SPD_CUST. > > > > Hello! And this commit is fixing also this bug as a side change. > > Ah, indeed it is, or at least to some extent. > > > > The rate should be left unchanged at 38400 in that case. > > > > With this change, rate in c_cflag is unchanged and stays at B38400. > > Right. > > > What is updated is the real baudrate in c_ispeed and c_ospeed > > extensions. > > > > It is really wrong? I thought that c_cflag should stay unchanged at > > B38400 when ASYNC_SPD_CUST is used. > > Yeah, cflags stay unchanged, but you shouldn't touch those fields when > using the deprecated ASYNC_SPD_CUST hack. Hm... Why? I thought that new extended fields (c_ispeed and c_ospeed) should contain current real speed. What is the reason that c_*speed fields should have 38400 when ASYNC_SPD_CUST hack is set? > Note that this currently only works because the ftdi driver uses > tty_get_baud_rate() instead of c_ospeed directly which is the > recommended (new) way. Yes, tty_get_baud_rate() helper function is there for this purpose, right? > > > > + /* Fix deprecated async-compatible custom_divisor hack and update tty baud rate */ > > > > + if (fix_custom_divisor) { > > > > + priv->custom_divisor = DIV_ROUND_CLOSEST(priv->baud_base, baud); > > > > + old_baud = baud; > > > > + baud = 38400; > > > > + } > > > > + > > > > tty_encode_baud_rate(tty, baud, baud); > > > > + > > > > + /* For async-compatible custom_divisor store into TCGETS2 c_*speed fields real baud rate */ > > > > + if (fix_custom_divisor) > > > > + tty->termios.c_ispeed = tty->termios.c_ospeed = old_baud; > > > > + > > > > return div_value; > > > > } > > > > > > > > @@ -2674,6 +2688,8 @@ static void ftdi_set_termios(struct tty_struct *tty, > > > > dev_dbg(ddev, "%s: forcing baud rate for this device\n", __func__); > > > > tty_encode_baud_rate(tty, priv->force_baud, > > > > priv->force_baud); > > > > + termios->c_ispeed = termios->c_ospeed = > > > > + DIV_ROUND_CLOSEST(priv->baud_base, priv->custom_divisor); > > > > } > > > > > > > > /* Force RTS-CTS if this device requires it. */ > > Johan
On Sun, Jul 24, 2022 at 02:59:08PM +0200, Pali Rohár wrote: > On Sunday 24 July 2022 14:54:58 Johan Hovold wrote: > > On Sun, Jul 24, 2022 at 02:33:51PM +0200, Pali Rohár wrote: > > > What is updated is the real baudrate in c_ispeed and c_ospeed > > > extensions. > > > > > > It is really wrong? I thought that c_cflag should stay unchanged at > > > B38400 when ASYNC_SPD_CUST is used. > > > > Yeah, cflags stay unchanged, but you shouldn't touch those fields when > > using the deprecated ASYNC_SPD_CUST hack. > > Hm... Why? I thought that new extended fields (c_ispeed and c_ospeed) > should contain current real speed. What is the reason that c_*speed > fields should have 38400 when ASYNC_SPD_CUST hack is set? Because we shouldn't go adding new features built around the deprecated ASYNC_SPD_CUST hack. User picks 38400, sets that flag and magic happens with some drivers for a while still while we look the other way. This is not something that we should need to care about when using the new interfaces. > > Note that this currently only works because the ftdi driver uses > > tty_get_baud_rate() instead of c_ospeed directly which is the > > recommended (new) way. > > Yes, tty_get_baud_rate() helper function is there for this purpose, > right? No. Johan
On Sunday 24 July 2022 15:08:09 Johan Hovold wrote: > On Sun, Jul 24, 2022 at 02:59:08PM +0200, Pali Rohár wrote: > > On Sunday 24 July 2022 14:54:58 Johan Hovold wrote: > > > On Sun, Jul 24, 2022 at 02:33:51PM +0200, Pali Rohár wrote: > > > > > What is updated is the real baudrate in c_ispeed and c_ospeed > > > > extensions. > > > > > > > > It is really wrong? I thought that c_cflag should stay unchanged at > > > > B38400 when ASYNC_SPD_CUST is used. > > > > > > Yeah, cflags stay unchanged, but you shouldn't touch those fields when > > > using the deprecated ASYNC_SPD_CUST hack. > > > > Hm... Why? I thought that new extended fields (c_ispeed and c_ospeed) > > should contain current real speed. What is the reason that c_*speed > > fields should have 38400 when ASYNC_SPD_CUST hack is set? > > Because we shouldn't go adding new features built around the deprecated > ASYNC_SPD_CUST hack. But this is not a new feature in the old deprecated hack. It for the new interface. > User picks 38400, sets that flag and magic happens with some drivers for > a while still while we look the other way. > > This is not something that we should need to care about when using the > new interfaces. Exactly and with this patch it work like to described. User of new interface does not have to care about old deprecated stuff and new interface would always reports correct value. > > > Note that this currently only works because the ftdi driver uses > > > tty_get_baud_rate() instead of c_ospeed directly which is the > > > recommended (new) way. > > > > Yes, tty_get_baud_rate() helper function is there for this purpose, > > right? > > No. > > Johan So for what otherwise?
On Thu, Aug 18, 2022 at 04:09:52PM +0200, Pali Rohár wrote: > On Sunday 24 July 2022 15:08:09 Johan Hovold wrote: > > On Sun, Jul 24, 2022 at 02:59:08PM +0200, Pali Rohár wrote: > > > On Sunday 24 July 2022 14:54:58 Johan Hovold wrote: > > > > On Sun, Jul 24, 2022 at 02:33:51PM +0200, Pali Rohár wrote: > > > > > > > What is updated is the real baudrate in c_ispeed and c_ospeed > > > > > extensions. > > > > > > > > > > It is really wrong? I thought that c_cflag should stay unchanged at > > > > > B38400 when ASYNC_SPD_CUST is used. > > > > > > > > Yeah, cflags stay unchanged, but you shouldn't touch those fields when > > > > using the deprecated ASYNC_SPD_CUST hack. > > > > > > Hm... Why? I thought that new extended fields (c_ispeed and c_ospeed) > > > should contain current real speed. What is the reason that c_*speed > > > fields should have 38400 when ASYNC_SPD_CUST hack is set? > > > > Because we shouldn't go adding new features built around the deprecated > > ASYNC_SPD_CUST hack. > > But this is not a new feature in the old deprecated hack. It for the > new interface. I think I understand what you're getting at, but no. Let's not add more features built around ASYNC_SPD_CUST. Johan
On Tuesday 13 September 2022 16:59:22 Johan Hovold wrote: > On Thu, Aug 18, 2022 at 04:09:52PM +0200, Pali Rohár wrote: > > On Sunday 24 July 2022 15:08:09 Johan Hovold wrote: > > > On Sun, Jul 24, 2022 at 02:59:08PM +0200, Pali Rohár wrote: > > > > On Sunday 24 July 2022 14:54:58 Johan Hovold wrote: > > > > > On Sun, Jul 24, 2022 at 02:33:51PM +0200, Pali Rohár wrote: > > > > > > > > > What is updated is the real baudrate in c_ispeed and c_ospeed > > > > > > extensions. > > > > > > > > > > > > It is really wrong? I thought that c_cflag should stay unchanged at > > > > > > B38400 when ASYNC_SPD_CUST is used. > > > > > > > > > > Yeah, cflags stay unchanged, but you shouldn't touch those fields when > > > > > using the deprecated ASYNC_SPD_CUST hack. > > > > > > > > Hm... Why? I thought that new extended fields (c_ispeed and c_ospeed) > > > > should contain current real speed. What is the reason that c_*speed > > > > fields should have 38400 when ASYNC_SPD_CUST hack is set? > > > > > > Because we shouldn't go adding new features built around the deprecated > > > ASYNC_SPD_CUST hack. > > > > But this is not a new feature in the old deprecated hack. It for the > > new interface. > > I think I understand what you're getting at, but no. Let's not add more > features built around ASYNC_SPD_CUST. > > Johan Seems that you did not understand the point. So I will try to explain it again. This is not a new feature for _old_ ASYNC_SPD_CUST. This is the fix for the _new_ TCGETS2 API, to ensure that driver will always returns corrects values in c_*speed fields. If driver is not going to fix this _new_ TCGETS2 API then there is _NO_ point to use this new API in userspace and it is better to stick with the old ASYNC_SPD_CUST. And this is the current userspace state. So based on your input, it is the time to deprecate TCGETS2?
On Wed, Sep 14, 2022 at 10:48:31AM +0200, Pali Rohár wrote: > On Tuesday 13 September 2022 16:59:22 Johan Hovold wrote: > > On Thu, Aug 18, 2022 at 04:09:52PM +0200, Pali Rohár wrote: > > > On Sunday 24 July 2022 15:08:09 Johan Hovold wrote: > > > > On Sun, Jul 24, 2022 at 02:59:08PM +0200, Pali Rohár wrote: > > > > > On Sunday 24 July 2022 14:54:58 Johan Hovold wrote: > > > > > > On Sun, Jul 24, 2022 at 02:33:51PM +0200, Pali Rohár wrote: > > > > > > > > > > > What is updated is the real baudrate in c_ispeed and c_ospeed > > > > > > > extensions. > > > > > > > > > > > > > > It is really wrong? I thought that c_cflag should stay unchanged at > > > > > > > B38400 when ASYNC_SPD_CUST is used. > > > > > > > > > > > > Yeah, cflags stay unchanged, but you shouldn't touch those fields when > > > > > > using the deprecated ASYNC_SPD_CUST hack. > > > > > > > > > > Hm... Why? I thought that new extended fields (c_ispeed and c_ospeed) > > > > > should contain current real speed. What is the reason that c_*speed > > > > > fields should have 38400 when ASYNC_SPD_CUST hack is set? > > > > > > > > Because we shouldn't go adding new features built around the deprecated > > > > ASYNC_SPD_CUST hack. > > > > > > But this is not a new feature in the old deprecated hack. It for the > > > new interface. > > > > I think I understand what you're getting at, but no. Let's not add more > > features built around ASYNC_SPD_CUST. > Seems that you did not understand the point. So I will try to explain it > again. This is not a new feature for _old_ ASYNC_SPD_CUST. This is the > fix for the _new_ TCGETS2 API, to ensure that driver will always returns > corrects values in c_*speed fields. If driver is not going to fix this > _new_ TCGETS2 API then there is _NO_ point to use this new API in > userspace and it is better to stick with the old ASYNC_SPD_CUST. And > this is the current userspace state. So based on your input, it is the > time to deprecate TCGETS2? Stop being silly. As I've said repeatedly, we don't care about ASYNC_SPD_CUST. Just return 38400 regardless of whatever magic happens behind the scenes with the TIOCSSERIAL ioctl. Johan
On Wednesday 14 September 2022 10:58:48 Johan Hovold wrote: > On Wed, Sep 14, 2022 at 10:48:31AM +0200, Pali Rohár wrote: > > On Tuesday 13 September 2022 16:59:22 Johan Hovold wrote: > > > On Thu, Aug 18, 2022 at 04:09:52PM +0200, Pali Rohár wrote: > > > > On Sunday 24 July 2022 15:08:09 Johan Hovold wrote: > > > > > On Sun, Jul 24, 2022 at 02:59:08PM +0200, Pali Rohár wrote: > > > > > > On Sunday 24 July 2022 14:54:58 Johan Hovold wrote: > > > > > > > On Sun, Jul 24, 2022 at 02:33:51PM +0200, Pali Rohár wrote: > > > > > > > > > > > > > What is updated is the real baudrate in c_ispeed and c_ospeed > > > > > > > > extensions. > > > > > > > > > > > > > > > > It is really wrong? I thought that c_cflag should stay unchanged at > > > > > > > > B38400 when ASYNC_SPD_CUST is used. > > > > > > > > > > > > > > Yeah, cflags stay unchanged, but you shouldn't touch those fields when > > > > > > > using the deprecated ASYNC_SPD_CUST hack. > > > > > > > > > > > > Hm... Why? I thought that new extended fields (c_ispeed and c_ospeed) > > > > > > should contain current real speed. What is the reason that c_*speed > > > > > > fields should have 38400 when ASYNC_SPD_CUST hack is set? > > > > > > > > > > Because we shouldn't go adding new features built around the deprecated > > > > > ASYNC_SPD_CUST hack. > > > > > > > > But this is not a new feature in the old deprecated hack. It for the > > > > new interface. > > > > > > I think I understand what you're getting at, but no. Let's not add more > > > features built around ASYNC_SPD_CUST. > > > Seems that you did not understand the point. So I will try to explain it > > again. This is not a new feature for _old_ ASYNC_SPD_CUST. This is the > > fix for the _new_ TCGETS2 API, to ensure that driver will always returns > > corrects values in c_*speed fields. If driver is not going to fix this > > _new_ TCGETS2 API then there is _NO_ point to use this new API in > > userspace and it is better to stick with the old ASYNC_SPD_CUST. And > > this is the current userspace state. So based on your input, it is the > > time to deprecate TCGETS2? > > Stop being silly. As I've said repeatedly, we don't care about > ASYNC_SPD_CUST. Just return 38400 regardless of whatever magic happens > behind the scenes with the TIOCSSERIAL ioctl. > > Johan I'm not silly here. Look, those APIs are for userspace. And if userspace application cannot use this new TCGETS2 API (for more reasons) then they stick with the old one TIOCSSERIAL. And your inputs just say that it is not a good idea to switch TCGETS2 as this API stay broken in some drivers. Silly is the one who do not see (or do not want to see it; because of own API perfectionism) the reasons why new "proposed API" is still not (widely) used and applications stick with TCGETS + TIOCSSERIAL. That is why I'm asking, it is time to starting deprecating TCGETS2 and create for example TCGETS3? Only just few application use TCGETS2, so deprecation of TCGETS2 can be done _now_ without pain as this API is not widely used.
On Wed, Sep 14, 2022 at 11:10:06AM +0200, Pali Rohár wrote: > On Wednesday 14 September 2022 10:58:48 Johan Hovold wrote: > > On Wed, Sep 14, 2022 at 10:48:31AM +0200, Pali Rohár wrote: > > > Seems that you did not understand the point. So I will try to explain it > > > again. This is not a new feature for _old_ ASYNC_SPD_CUST. This is the > > > fix for the _new_ TCGETS2 API, to ensure that driver will always returns > > > corrects values in c_*speed fields. If driver is not going to fix this > > > _new_ TCGETS2 API then there is _NO_ point to use this new API in > > > userspace and it is better to stick with the old ASYNC_SPD_CUST. And > > > this is the current userspace state. So based on your input, it is the > > > time to deprecate TCGETS2? > > > > Stop being silly. As I've said repeatedly, we don't care about > > ASYNC_SPD_CUST. Just return 38400 regardless of whatever magic happens > > behind the scenes with the TIOCSSERIAL ioctl. > I'm not silly here. Look, those APIs are for userspace. And if userspace > application cannot use this new TCGETS2 API (for more reasons) then they > stick with the old one TIOCSSERIAL. And your inputs just say that it is > not a good idea to switch TCGETS2 as this API stay broken in some > drivers. Silly is the one who do not see (or do not want to see it; > because of own API perfectionism) the reasons why new "proposed API" is > still not (widely) used and applications stick with TCGETS + TIOCSSERIAL. > > That is why I'm asking, it is time to starting deprecating TCGETS2 and > create for example TCGETS3? Only just few application use TCGETS2, so > deprecation of TCGETS2 can be done _now_ without pain as this API is not > widely used. You're trying to keep the ASYNC_SPD hack alive by forcing drivers to take it into consideration for TCGETS2. Just stop using the former and switch to using BOTHER. And if something is missing in user space for that, then fix that. Johan
On Wednesday 14 September 2022 11:18:03 Johan Hovold wrote: > On Wed, Sep 14, 2022 at 11:10:06AM +0200, Pali Rohár wrote: > > On Wednesday 14 September 2022 10:58:48 Johan Hovold wrote: > > > On Wed, Sep 14, 2022 at 10:48:31AM +0200, Pali Rohár wrote: > > > > > Seems that you did not understand the point. So I will try to explain it > > > > again. This is not a new feature for _old_ ASYNC_SPD_CUST. This is the > > > > fix for the _new_ TCGETS2 API, to ensure that driver will always returns > > > > corrects values in c_*speed fields. If driver is not going to fix this > > > > _new_ TCGETS2 API then there is _NO_ point to use this new API in > > > > userspace and it is better to stick with the old ASYNC_SPD_CUST. And > > > > this is the current userspace state. So based on your input, it is the > > > > time to deprecate TCGETS2? > > > > > > Stop being silly. As I've said repeatedly, we don't care about > > > ASYNC_SPD_CUST. Just return 38400 regardless of whatever magic happens > > > behind the scenes with the TIOCSSERIAL ioctl. > > > I'm not silly here. Look, those APIs are for userspace. And if userspace > > application cannot use this new TCGETS2 API (for more reasons) then they > > stick with the old one TIOCSSERIAL. And your inputs just say that it is > > not a good idea to switch TCGETS2 as this API stay broken in some > > drivers. Silly is the one who do not see (or do not want to see it; > > because of own API perfectionism) the reasons why new "proposed API" is > > still not (widely) used and applications stick with TCGETS + TIOCSSERIAL. > > > > That is why I'm asking, it is time to starting deprecating TCGETS2 and > > create for example TCGETS3? Only just few application use TCGETS2, so > > deprecation of TCGETS2 can be done _now_ without pain as this API is not > > widely used. > > You're trying to keep the ASYNC_SPD hack alive by forcing drivers to > take it into consideration for TCGETS2. Just stop using the former and > switch to using BOTHER. And if something is missing in user space for > that, then fix that. > > Johan And what is the point of switching to BOTHER/TCGETS2 if some drivers do not return _correct_ values?
diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c index 5db1293bb7a2..39e8c5d06157 100644 --- a/drivers/usb/serial/ftdi_sio.c +++ b/drivers/usb/serial/ftdi_sio.c @@ -1303,6 +1303,7 @@ static u32 get_ftdi_divisor(struct tty_struct *tty, { struct ftdi_private *priv = usb_get_serial_port_data(port); struct device *dev = &port->dev; + int fix_custom_divisor = 0; int div_value = 0; int div_okay = 1; int baud; @@ -1317,6 +1318,7 @@ static u32 get_ftdi_divisor(struct tty_struct *tty, if (baud == 38400 && ((priv->flags & ASYNC_SPD_MASK) == ASYNC_SPD_CUST) && (priv->custom_divisor)) { + fix_custom_divisor = 1; baud = DIV_ROUND_CLOSEST(priv->baud_base, priv->custom_divisor); dev_dbg(dev, "%s - custom divisor %d sets baud rate to %d\n", __func__, priv->custom_divisor, baud); @@ -1401,7 +1403,19 @@ static u32 get_ftdi_divisor(struct tty_struct *tty, ftdi_chip_name[priv->chip_type]); } + /* Fix deprecated async-compatible custom_divisor hack and update tty baud rate */ + if (fix_custom_divisor) { + priv->custom_divisor = DIV_ROUND_CLOSEST(priv->baud_base, baud); + old_baud = baud; + baud = 38400; + } + tty_encode_baud_rate(tty, baud, baud); + + /* For async-compatible custom_divisor store into TCGETS2 c_*speed fields real baud rate */ + if (fix_custom_divisor) + tty->termios.c_ispeed = tty->termios.c_ospeed = old_baud; + return div_value; } @@ -2674,6 +2688,8 @@ static void ftdi_set_termios(struct tty_struct *tty, dev_dbg(ddev, "%s: forcing baud rate for this device\n", __func__); tty_encode_baud_rate(tty, priv->force_baud, priv->force_baud); + termios->c_ispeed = termios->c_ospeed = + DIV_ROUND_CLOSEST(priv->baud_base, priv->custom_divisor); } /* Force RTS-CTS if this device requires it. */