| Message ID | 20231209125836.16294-1-l.sanfilippo@kunbus.com |
|---|---|
| Headers | show |
| Series | Fixes and improvements for RS485 | expand |
On Sat, 9 Dec 2023, Lino Sanfilippo wrote: > If the imx driver cannot support RS485 it sets the ports rs485_supported > structure to NULL. No, an embedded struct inside struct uart_port cannot be set to NULL, it's always there. Looking into the code, that setting of rs485_supported from imx_no_rs485 is actually superfluous as it should be already cleared to zeros on alloc. > But it still calls uart_get_rs485_mode() which may set > the RS485_ENABLED flag nevertheless. > > This may lead to an attempt to configure RS485 even if it is not supported > when the flag is evaluated in uart_configure_port() at port startup. > > Avoid this by bailing out of uart_get_rs485_mode() if the RS485_ENABLED > flag is not supported by the caller. > > With this fix a check for RTS availability is now obsolete in the imx > driver, since it can not evaluate to true any more. Remove this check, too. > > Fixes: 00d7a00e2a6f ("serial: imx: Fill in rs485_supported") > Cc: Shawn Guo <shawnguo@kernel.org> > Cc: Sascha Hauer <s.hauer@pengutronix.de> > Cc: stable@vger.kernel.org > Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
On Sat, 9 Dec 2023 13:58:30 +0100 Lino Sanfilippo <l.sanfilippo@kunbus.com> wrote: > Both the imx and stm32 driver set the rx-during-tx GPIO in rs485_config(). > Since this function is called with the port lock held, this can be an > problem in case that setting the GPIO line can sleep (e.g. if a GPIO > expander is used which is connected via SPI or I2C). > > Avoid this issue by moving the GPIO setting outside of the port lock into > the serial core and thus making it a generic feature. > > Fixes: c54d48543689 ("serial: stm32: Add support for rs485 RX_DURING_TX output GPIO") > Fixes: ca530cfa968c ("serial: imx: Add support for RS485 RX_DURING_TX output GPIO") > Cc: Shawn Guo <shawnguo@kernel.org> > Cc: Sascha Hauer <s.hauer@pengutronix.de> > Cc: stable@vger.kernel.org > Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> > --- > drivers/tty/serial/imx.c | 4 ---- > drivers/tty/serial/serial_core.c | 12 ++++++++++++ > drivers/tty/serial/stm32-usart.c | 5 +---- > 3 files changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > index 708b9852a575..9cffeb23112b 100644 > --- a/drivers/tty/serial/imx.c > +++ b/drivers/tty/serial/imx.c > @@ -1943,10 +1943,6 @@ static int imx_uart_rs485_config(struct uart_port *port, struct ktermios *termio > rs485conf->flags & SER_RS485_RX_DURING_TX) > imx_uart_start_rx(port); > > - if (port->rs485_rx_during_tx_gpio) > - gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio, > - !!(rs485conf->flags & SER_RS485_RX_DURING_TX)); > - > return 0; > } > > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c > index f1348a509552..a0290a5fe8b3 100644 > --- a/drivers/tty/serial/serial_core.c > +++ b/drivers/tty/serial/serial_core.c > @@ -1402,6 +1402,16 @@ static void uart_set_rs485_termination(struct uart_port *port, > !!(rs485->flags & SER_RS485_TERMINATE_BUS)); > } > > +static void uart_set_rs485_rx_during_tx(struct uart_port *port, > + const struct serial_rs485 *rs485) > +{ > + if (!(rs485->flags & SER_RS485_ENABLED)) > + return; > + > + gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio, > + !!(rs485->flags & SER_RS485_RX_DURING_TX)); > +} > + > static int uart_rs485_config(struct uart_port *port) > { > struct serial_rs485 *rs485 = &port->rs485; > @@ -1413,6 +1423,7 @@ static int uart_rs485_config(struct uart_port *port) > > uart_sanitize_serial_rs485(port, rs485); > uart_set_rs485_termination(port, rs485); > + uart_set_rs485_rx_during_tx(port, rs485); > > uart_port_lock_irqsave(port, &flags); > ret = port->rs485_config(port, NULL, rs485); > @@ -1457,6 +1468,7 @@ static int uart_set_rs485_config(struct tty_struct *tty, struct uart_port *port, > return ret; > uart_sanitize_serial_rs485(port, &rs485); > uart_set_rs485_termination(port, &rs485); > + uart_set_rs485_rx_during_tx(port, &rs485); > > uart_port_lock_irqsave(port, &flags); > ret = port->rs485_config(port, &tty->termios, &rs485); > diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c > index 3048620315d6..ec9a72a5bea9 100644 > --- a/drivers/tty/serial/stm32-usart.c > +++ b/drivers/tty/serial/stm32-usart.c > @@ -226,10 +226,7 @@ static int stm32_usart_config_rs485(struct uart_port *port, struct ktermios *ter > > stm32_usart_clr_bits(port, ofs->cr1, BIT(cfg->uart_enable_bit)); > > - if (port->rs485_rx_during_tx_gpio) > - gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio, > - !!(rs485conf->flags & SER_RS485_RX_DURING_TX)); > - else > + if (!port->rs485_rx_during_tx_gpio) > rs485conf->flags |= SER_RS485_RX_DURING_TX; > > if (rs485conf->flags & SER_RS485_ENABLED) { > -- > 2.42.0 > Reviewed-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> Hugo
Hi, On 11.12.23 11:35, Ilpo Järvinen wrote: > On Sat, 9 Dec 2023, Lino Sanfilippo wrote: > >> Both the imx and stm32 driver set the rx-during-tx GPIO in rs485_config(). >> Since this function is called with the port lock held, this can be an >> problem in case that setting the GPIO line can sleep (e.g. if a GPIO >> expander is used which is connected via SPI or I2C). >> >> Avoid this issue by moving the GPIO setting outside of the port lock into >> the serial core and thus making it a generic feature. >> >> Fixes: c54d48543689 ("serial: stm32: Add support for rs485 RX_DURING_TX output GPIO") >> Fixes: ca530cfa968c ("serial: imx: Add support for RS485 RX_DURING_TX output GPIO") >> Cc: Shawn Guo <shawnguo@kernel.org> >> Cc: Sascha Hauer <s.hauer@pengutronix.de> >> Cc: stable@vger.kernel.org >> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> >> --- >> drivers/tty/serial/imx.c | 4 ---- >> drivers/tty/serial/serial_core.c | 12 ++++++++++++ >> drivers/tty/serial/stm32-usart.c | 5 +---- >> 3 files changed, 13 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c >> index 708b9852a575..9cffeb23112b 100644 >> --- a/drivers/tty/serial/imx.c >> +++ b/drivers/tty/serial/imx.c >> @@ -1943,10 +1943,6 @@ static int imx_uart_rs485_config(struct uart_port *port, struct ktermios *termio >> rs485conf->flags & SER_RS485_RX_DURING_TX) >> imx_uart_start_rx(port); >> >> - if (port->rs485_rx_during_tx_gpio) >> - gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio, >> - !!(rs485conf->flags & SER_RS485_RX_DURING_TX)); >> - >> return 0; >> } >> >> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c >> index f1348a509552..a0290a5fe8b3 100644 >> --- a/drivers/tty/serial/serial_core.c >> +++ b/drivers/tty/serial/serial_core.c >> @@ -1402,6 +1402,16 @@ static void uart_set_rs485_termination(struct uart_port *port, >> !!(rs485->flags & SER_RS485_TERMINATE_BUS)); >> } >> >> +static void uart_set_rs485_rx_during_tx(struct uart_port *port, >> + const struct serial_rs485 *rs485) >> +{ >> + if (!(rs485->flags & SER_RS485_ENABLED)) >> + return; >> + >> + gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio, >> + !!(rs485->flags & SER_RS485_RX_DURING_TX)); >> +} >> + >> static int uart_rs485_config(struct uart_port *port) >> { >> struct serial_rs485 *rs485 = &port->rs485; >> @@ -1413,6 +1423,7 @@ static int uart_rs485_config(struct uart_port *port) >> >> uart_sanitize_serial_rs485(port, rs485); >> uart_set_rs485_termination(port, rs485); >> + uart_set_rs485_rx_during_tx(port, rs485); >> >> uart_port_lock_irqsave(port, &flags); >> ret = port->rs485_config(port, NULL, rs485); >> @@ -1457,6 +1468,7 @@ static int uart_set_rs485_config(struct tty_struct *tty, struct uart_port *port, >> return ret; >> uart_sanitize_serial_rs485(port, &rs485); >> uart_set_rs485_termination(port, &rs485); >> + uart_set_rs485_rx_during_tx(port, &rs485); >> >> uart_port_lock_irqsave(port, &flags); >> ret = port->rs485_config(port, &tty->termios, &rs485); > > Also a nice simplification of driver-side code. > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > Just noting since this is now in core that if ->rs485_config() fails, > I suppose it's just normal to not rollback gpiod_set_value_cansleep() > (skimming through existing users in tree, it looks it's practically > never touched on the error rollback paths so I guess it's the normal > practice)? > > Anyway, since neither of the users currently don't fail in their > ->rs485_config() so it doesn't seem a critical issue. > Thats a good point actually. Rolling back is not hard to implement and although it may not matter right now since currently no driver returns an error code, this can change very soon. So I will rework this patch for the next version, thanks! Regards, Lino
On 11.12.23 12:00, Ilpo Järvinen wrote: > On Sat, 9 Dec 2023, Lino Sanfilippo wrote: > >> If the imx driver cannot support RS485 it sets the ports rs485_supported >> structure to NULL. > > No, an embedded struct inside struct uart_port cannot be set to NULL, > it's always there. > Hmm, ok. What I meant was that the structure is nullified. "set to NULL" is maybe a bit misleading. I will correct this. > Looking into the code, that setting of rs485_supported from imx_no_rs485 > is actually superfluous as it should be already cleared to zeros on alloc. > Yes. BTW: Another "no_rs485" configuration setting can be found in the ar933x driver. If we do not want to keep those assignments I can remove the one for the imx driver with the next version of this patch... >> But it still calls uart_get_rs485_mode() which may set >> the RS485_ENABLED flag nevertheless. >> >> This may lead to an attempt to configure RS485 even if it is not supported >> when the flag is evaluated in uart_configure_port() at port startup. >> >> Avoid this by bailing out of uart_get_rs485_mode() if the RS485_ENABLED >> flag is not supported by the caller. >> >> With this fix a check for RTS availability is now obsolete in the imx >> driver, since it can not evaluate to true any more. Remove this check, too. >> >> Fixes: 00d7a00e2a6f ("serial: imx: Fill in rs485_supported") >> Cc: Shawn Guo <shawnguo@kernel.org> >> Cc: Sascha Hauer <s.hauer@pengutronix.de> >> Cc: stable@vger.kernel.org >> Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> >
On Wed, 13 Dec 2023, Lino Sanfilippo wrote: > On 11.12.23 12:00, Ilpo Järvinen wrote: > > On Sat, 9 Dec 2023, Lino Sanfilippo wrote: > > > Looking into the code, that setting of rs485_supported from imx_no_rs485 > > is actually superfluous as it should be already cleared to zeros on alloc. > > > > Yes. BTW: Another "no_rs485" configuration setting can be found in the ar933x driver. > If we do not want to keep those assignments I can remove the one for the imx > driver with the next version of this patch... I think they can just be dropped as it's normal in Linux code to assume that things are zeroed by default. Those "no"-variants originate from the time when supported_rs485 was not yet embedded but just a pointer to a const struct and I didn't realize I could have removed them when I ended up embedding the struct so it can be altered per port.
The following series includes some fixes and improvements around RS485 in the serial core and UART drivers: Patch 1: Do not hold the port lock when setting rx-during-tx GPIO Patch 2: set missing supported flag for RX during TX GPIO Patch 3: fix sanitizing check for RTS settings Patch 4: make sure RS485 is cannot be enabled when it is not supported Patch 5: imx: do not set RS485 enabled if it is not supported Patch 6: omap: do not override settings for rs485 support Patch 7: exar: set missing RS485 supported flag Changes in v5: - do not combine the functions that the RS484 GPIOs (as Hugo originally suggested) Changes in v4: - add comment for function uart_set_rs485_gpios after hint from Hugo - correct commit message as pointed out by Hugo - rephrase commit messages - add patch 7 after discussion with Ilpo Changes in v3 - Drop patch "Get rid of useless wrapper pl011_get_rs485_mode()" as requested by Greg Changes in v2: - add missing 'Fixes' tags as requested by Greg - corrected a typo as pointed out by Hugo - fix issue in imx driver in the serial core as suggested by Uwe - partly rephrase some commit messages - add patch 7 Lino Sanfilippo (7): serial: Do not hold the port lock when setting rx-during-tx GPIO serial: core: set missing supported flag for RX during TX GPIO serial: core: fix sanitizing check for RTS settings serial: core: make sure RS485 cannot be enabled when it is not supported serial: core, imx: do not set RS485 enabled if it is not supported serial: omap: do not override settings for RS485 support serial: 8250_exar: Set missing rs485_supported flag drivers/tty/serial/8250/8250_exar.c | 5 +-- drivers/tty/serial/imx.c | 8 ----- drivers/tty/serial/omap-serial.c | 8 ++--- drivers/tty/serial/serial_core.c | 50 ++++++++++++++++++++++------- drivers/tty/serial/stm32-usart.c | 5 +-- 5 files changed, 47 insertions(+), 29 deletions(-) base-commit: 33cc938e65a98f1d29d0a18403dbbee050dcad9a