| Message ID | 20231011181544.7893-4-l.sanfilippo@kunbus.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fixes and improvements for RS485 | expand |
On Wed, 11 Oct 2023, Lino Sanfilippo wrote: > Among other things uart_sanitize_serial_rs485() tests the sanity of the RTS > settings in a RS485 configuration that has been passed by userspace. > If RTS-on-send and RTS-after-send are both set or unset the configuration > is adjusted and RTS-after-send is disabled and RTS-on-send enabled. > > This however makes only sense if both RTS modes are actually supported by > the driver. > > With commit be2e2cb1d281 ("serial: Sanitize rs485_struct") the code does > take the driver support into account but only checks if one of both RTS > modes are supported. This may lead to the errorneous result of RTS-on-send > being set even if only RTS-after-send is supported. > > Fix this by changing the implemented logic: First clear all unsupported > flags in the RS485 configuration, then adjust an invalid RTS setting by > taking into account which RTS mode is supported. > > Cc: stable@vger.kernel.org > Fixes: be2e2cb1d281 ("serial: Sanitize rs485_struct") > Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> > --- > drivers/tty/serial/serial_core.c | 28 ++++++++++++++++++---------- > 1 file changed, 18 insertions(+), 10 deletions(-) > > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c > index 697c36dc7ec8..f4feebf8200f 100644 > --- a/drivers/tty/serial/serial_core.c > +++ b/drivers/tty/serial/serial_core.c > @@ -1370,19 +1370,27 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4 > return; > } > > + rs485->flags &= supported_flags; > + > /* Pick sane settings if the user hasn't */ > - if ((supported_flags & (SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND)) && > - !(rs485->flags & SER_RS485_RTS_ON_SEND) == > + if (!(rs485->flags & SER_RS485_RTS_ON_SEND) == > !(rs485->flags & SER_RS485_RTS_AFTER_SEND)) { > - dev_warn_ratelimited(port->dev, > - "%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n", > - port->name, port->line); > - rs485->flags |= SER_RS485_RTS_ON_SEND; > - rs485->flags &= ~SER_RS485_RTS_AFTER_SEND; > - supported_flags |= SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND; > - } > + if (supported_flags & SER_RS485_RTS_ON_SEND) { > + rs485->flags |= SER_RS485_RTS_ON_SEND; > + rs485->flags &= ~SER_RS485_RTS_AFTER_SEND; > > - rs485->flags &= supported_flags; > + dev_warn_ratelimited(port->dev, > + "%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n", > + port->name, port->line); > + } else { > + rs485->flags |= SER_RS485_RTS_AFTER_SEND; > + rs485->flags &= ~SER_RS485_RTS_ON_SEND; So if neither of the flags is supported, what will happen? You might want add if after that else?
Hi, On 12.10.23 15:10, Ilpo Järvinen wrote: > > > On Wed, 11 Oct 2023, Lino Sanfilippo wrote: > >> Among other things uart_sanitize_serial_rs485() tests the sanity of the RTS >> settings in a RS485 configuration that has been passed by userspace. >> If RTS-on-send and RTS-after-send are both set or unset the configuration >> is adjusted and RTS-after-send is disabled and RTS-on-send enabled. >> >> This however makes only sense if both RTS modes are actually supported by >> the driver. >> >> With commit be2e2cb1d281 ("serial: Sanitize rs485_struct") the code does >> take the driver support into account but only checks if one of both RTS >> modes are supported. This may lead to the errorneous result of RTS-on-send >> being set even if only RTS-after-send is supported. >> >> Fix this by changing the implemented logic: First clear all unsupported >> flags in the RS485 configuration, then adjust an invalid RTS setting by >> taking into account which RTS mode is supported. >> >> Cc: stable@vger.kernel.org >> Fixes: be2e2cb1d281 ("serial: Sanitize rs485_struct") >> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> >> --- >> drivers/tty/serial/serial_core.c | 28 ++++++++++++++++++---------- >> 1 file changed, 18 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c >> index 697c36dc7ec8..f4feebf8200f 100644 >> --- a/drivers/tty/serial/serial_core.c >> +++ b/drivers/tty/serial/serial_core.c >> @@ -1370,19 +1370,27 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4 >> return; >> } >> >> + rs485->flags &= supported_flags; >> + >> /* Pick sane settings if the user hasn't */ >> - if ((supported_flags & (SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND)) && >> - !(rs485->flags & SER_RS485_RTS_ON_SEND) == >> + if (!(rs485->flags & SER_RS485_RTS_ON_SEND) == >> !(rs485->flags & SER_RS485_RTS_AFTER_SEND)) { >> - dev_warn_ratelimited(port->dev, >> - "%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n", >> - port->name, port->line); >> - rs485->flags |= SER_RS485_RTS_ON_SEND; >> - rs485->flags &= ~SER_RS485_RTS_AFTER_SEND; >> - supported_flags |= SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND; >> - } >> + if (supported_flags & SER_RS485_RTS_ON_SEND) { >> + rs485->flags |= SER_RS485_RTS_ON_SEND; >> + rs485->flags &= ~SER_RS485_RTS_AFTER_SEND; >> >> - rs485->flags &= supported_flags; >> + dev_warn_ratelimited(port->dev, >> + "%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n", >> + port->name, port->line); >> + } else { >> + rs485->flags |= SER_RS485_RTS_AFTER_SEND; >> + rs485->flags &= ~SER_RS485_RTS_ON_SEND; > > So if neither of the flags is supported, what will happen? You might want > add if after that else? > I would consider this a bug in the driver, as at least one of both modes has to be supported. If the driver does not have at least one of both flags set in rs485_supported.flags we could print a warning though. Would you prefer that? Regards, Lino
On Thu, 12 Oct 2023, Lino Sanfilippo wrote: > On 12.10.23 15:10, Ilpo Järvinen wrote: > > On Wed, 11 Oct 2023, Lino Sanfilippo wrote: > > > >> Among other things uart_sanitize_serial_rs485() tests the sanity of the RTS > >> settings in a RS485 configuration that has been passed by userspace. > >> If RTS-on-send and RTS-after-send are both set or unset the configuration > >> is adjusted and RTS-after-send is disabled and RTS-on-send enabled. > >> > >> This however makes only sense if both RTS modes are actually supported by > >> the driver. > >> > >> With commit be2e2cb1d281 ("serial: Sanitize rs485_struct") the code does > >> take the driver support into account but only checks if one of both RTS > >> modes are supported. This may lead to the errorneous result of RTS-on-send > >> being set even if only RTS-after-send is supported. > >> > >> Fix this by changing the implemented logic: First clear all unsupported > >> flags in the RS485 configuration, then adjust an invalid RTS setting by > >> taking into account which RTS mode is supported. > >> > >> Cc: stable@vger.kernel.org > >> Fixes: be2e2cb1d281 ("serial: Sanitize rs485_struct") > >> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> > >> --- > >> drivers/tty/serial/serial_core.c | 28 ++++++++++++++++++---------- > >> 1 file changed, 18 insertions(+), 10 deletions(-) > >> > >> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c > >> index 697c36dc7ec8..f4feebf8200f 100644 > >> --- a/drivers/tty/serial/serial_core.c > >> +++ b/drivers/tty/serial/serial_core.c > >> @@ -1370,19 +1370,27 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4 > >> return; > >> } > >> > >> + rs485->flags &= supported_flags; > >> + > >> /* Pick sane settings if the user hasn't */ > >> - if ((supported_flags & (SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND)) && > >> - !(rs485->flags & SER_RS485_RTS_ON_SEND) == > >> + if (!(rs485->flags & SER_RS485_RTS_ON_SEND) == > >> !(rs485->flags & SER_RS485_RTS_AFTER_SEND)) { > >> - dev_warn_ratelimited(port->dev, > >> - "%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n", > >> - port->name, port->line); > >> - rs485->flags |= SER_RS485_RTS_ON_SEND; > >> - rs485->flags &= ~SER_RS485_RTS_AFTER_SEND; > >> - supported_flags |= SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND; > >> - } > >> + if (supported_flags & SER_RS485_RTS_ON_SEND) { > >> + rs485->flags |= SER_RS485_RTS_ON_SEND; > >> + rs485->flags &= ~SER_RS485_RTS_AFTER_SEND; > >> > >> - rs485->flags &= supported_flags; > >> + dev_warn_ratelimited(port->dev, > >> + "%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n", > >> + port->name, port->line); > >> + } else { > >> + rs485->flags |= SER_RS485_RTS_AFTER_SEND; > >> + rs485->flags &= ~SER_RS485_RTS_ON_SEND; > > > > So if neither of the flags is supported, what will happen? You might want > > add if after that else? > > > > I would consider this a bug in the driver, as at least one of both modes > has to be supported. If the driver does not have at least one of both flags > set in rs485_supported.flags we could print a warning though. Would you prefer that? 8250_exar.c needs to fixed then? I was taking these as things one can "configure" even if when there's support only for a one of them there's not that much to configure. As there was neither in 8250_exar's code, I didn't add either flag. But I suppose your interpretation of those flag makes more sense.
Hi Ilpo, On 13.10.23 12:24, Ilpo Järvinen wrote: > On Thu, 12 Oct 2023, Lino Sanfilippo wrote: >> On 12.10.23 15:10, Ilpo Järvinen wrote: >>> On Wed, 11 Oct 2023, Lino Sanfilippo wrote: >>> >>>> Among other things uart_sanitize_serial_rs485() tests the sanity of the RTS >>>> settings in a RS485 configuration that has been passed by userspace. >>>> If RTS-on-send and RTS-after-send are both set or unset the configuration >>>> is adjusted and RTS-after-send is disabled and RTS-on-send enabled. >>>> >>>> This however makes only sense if both RTS modes are actually supported by >>>> the driver. >>>> >>>> With commit be2e2cb1d281 ("serial: Sanitize rs485_struct") the code does >>>> take the driver support into account but only checks if one of both RTS >>>> modes are supported. This may lead to the errorneous result of RTS-on-send >>>> being set even if only RTS-after-send is supported. >>>> >>>> Fix this by changing the implemented logic: First clear all unsupported >>>> flags in the RS485 configuration, then adjust an invalid RTS setting by >>>> taking into account which RTS mode is supported. >>>> >>>> Cc: stable@vger.kernel.org >>>> Fixes: be2e2cb1d281 ("serial: Sanitize rs485_struct") >>>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> >>>> --- >>>> drivers/tty/serial/serial_core.c | 28 ++++++++++++++++++---------- >>>> 1 file changed, 18 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c >>>> index 697c36dc7ec8..f4feebf8200f 100644 >>>> --- a/drivers/tty/serial/serial_core.c >>>> +++ b/drivers/tty/serial/serial_core.c >>>> @@ -1370,19 +1370,27 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4 >>>> return; >>>> } >>>> >>>> + rs485->flags &= supported_flags; >>>> + >>>> /* Pick sane settings if the user hasn't */ >>>> - if ((supported_flags & (SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND)) && >>>> - !(rs485->flags & SER_RS485_RTS_ON_SEND) == >>>> + if (!(rs485->flags & SER_RS485_RTS_ON_SEND) == >>>> !(rs485->flags & SER_RS485_RTS_AFTER_SEND)) { >>>> - dev_warn_ratelimited(port->dev, >>>> - "%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n", >>>> - port->name, port->line); >>>> - rs485->flags |= SER_RS485_RTS_ON_SEND; >>>> - rs485->flags &= ~SER_RS485_RTS_AFTER_SEND; >>>> - supported_flags |= SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND; >>>> - } >>>> + if (supported_flags & SER_RS485_RTS_ON_SEND) { >>>> + rs485->flags |= SER_RS485_RTS_ON_SEND; >>>> + rs485->flags &= ~SER_RS485_RTS_AFTER_SEND; >>>> >>>> - rs485->flags &= supported_flags; >>>> + dev_warn_ratelimited(port->dev, >>>> + "%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n", >>>> + port->name, port->line); >>>> + } else { >>>> + rs485->flags |= SER_RS485_RTS_AFTER_SEND; >>>> + rs485->flags &= ~SER_RS485_RTS_ON_SEND; >>> >>> So if neither of the flags is supported, what will happen? You might want >>> add if after that else? >>> >> >> I would consider this a bug in the driver, as at least one of both modes >> has to be supported. If the driver does not have at least one of both flags >> set in rs485_supported.flags we could print a warning though. Would you prefer that? > > 8250_exar.c needs to fixed then? I was taking these as things one can > "configure" even if when there's support only for a one of them there's > not that much to configure. As there was neither in 8250_exar's code, I > didn't add either flag. > But I suppose your interpretation of those flag makes more sense. > IMHO this is consistent with what we have in uart_get_rs485_mode(). This function ensures that we have at least one RTS mode set (with default to RTS_ON_SEND). So concerning 8250_exar.c, I think it should be fixed (havent noticed the missing RTS mode though until you mentioned it). Would you like to provide a fix for this or shall I include one into the next version of this series? BR, Lino
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c index 697c36dc7ec8..f4feebf8200f 100644 --- a/drivers/tty/serial/serial_core.c +++ b/drivers/tty/serial/serial_core.c @@ -1370,19 +1370,27 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4 return; } + rs485->flags &= supported_flags; + /* Pick sane settings if the user hasn't */ - if ((supported_flags & (SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND)) && - !(rs485->flags & SER_RS485_RTS_ON_SEND) == + if (!(rs485->flags & SER_RS485_RTS_ON_SEND) == !(rs485->flags & SER_RS485_RTS_AFTER_SEND)) { - dev_warn_ratelimited(port->dev, - "%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n", - port->name, port->line); - rs485->flags |= SER_RS485_RTS_ON_SEND; - rs485->flags &= ~SER_RS485_RTS_AFTER_SEND; - supported_flags |= SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND; - } + if (supported_flags & SER_RS485_RTS_ON_SEND) { + rs485->flags |= SER_RS485_RTS_ON_SEND; + rs485->flags &= ~SER_RS485_RTS_AFTER_SEND; - rs485->flags &= supported_flags; + dev_warn_ratelimited(port->dev, + "%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n", + port->name, port->line); + } else { + rs485->flags |= SER_RS485_RTS_AFTER_SEND; + rs485->flags &= ~SER_RS485_RTS_ON_SEND; + + dev_warn_ratelimited(port->dev, + "%s (%d): invalid RTS setting, using RTS_AFTER_SEND instead\n", + port->name, port->line); + } + } uart_sanitize_serial_rs485_delays(port, rs485);
Among other things uart_sanitize_serial_rs485() tests the sanity of the RTS settings in a RS485 configuration that has been passed by userspace. If RTS-on-send and RTS-after-send are both set or unset the configuration is adjusted and RTS-after-send is disabled and RTS-on-send enabled. This however makes only sense if both RTS modes are actually supported by the driver. With commit be2e2cb1d281 ("serial: Sanitize rs485_struct") the code does take the driver support into account but only checks if one of both RTS modes are supported. This may lead to the errorneous result of RTS-on-send being set even if only RTS-after-send is supported. Fix this by changing the implemented logic: First clear all unsupported flags in the RS485 configuration, then adjust an invalid RTS setting by taking into account which RTS mode is supported. Cc: stable@vger.kernel.org Fixes: be2e2cb1d281 ("serial: Sanitize rs485_struct") Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> --- drivers/tty/serial/serial_core.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)