| Message ID | 20220703170039.2058202-3-LinoSanfilippo@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes and cleanup for RS485 | expand |
On Sun, Jul 3, 2022 at 7:02 PM Lino Sanfilippo <LinoSanfilippo@gmx.de> wrote: > > From: Lino Sanfilippo <l.sanfilippo@kunbus.com> > > In serial8250_em485_config() the termination GPIO is set with the uart_port > spinlock held. This is an issue if setting the GPIO line can sleep (e.g. > since the concerning GPIO expander is connected via SPI or I2C). > > Fix this by setting the termination line outside of the uart_port spinlock > in the serial core and using gpiod_set_value_cansleep() which instead of > gpiod_set_value() allows to sleep. allows it to > Beside fixing the termination GPIO line setting for the 8250 driver this > change also makes setting the termination GPIO generic for all UART > drivers. ... > +static void uart_set_rs485_termination(struct uart_port *port, > + const struct serial_rs485 *rs485) > +{ > + if (!port->rs485_term_gpio This duplicates the check the GPIO library does. Drop it. > || !(rs485->flags & SER_RS485_ENABLED)) > + return; > + > + gpiod_set_value_cansleep(port->rs485_term_gpio, > + !!(rs485->flags & SER_RS485_TERMINATE_BUS)); > +}
On 03.07.22 20:31, Andy Shevchenko wrote: > On Sun, Jul 3, 2022 at 7:02 PM Lino Sanfilippo <LinoSanfilippo@gmx.de> wrote: >> >> From: Lino Sanfilippo <l.sanfilippo@kunbus.com> >> >> In serial8250_em485_config() the termination GPIO is set with the uart_port >> spinlock held. This is an issue if setting the GPIO line can sleep (e.g. >> since the concerning GPIO expander is connected via SPI or I2C). >> >> Fix this by setting the termination line outside of the uart_port spinlock >> in the serial core and using gpiod_set_value_cansleep() which instead of >> gpiod_set_value() allows to sleep. > > allows it to > Ok. >> Beside fixing the termination GPIO line setting for the 8250 driver this >> change also makes setting the termination GPIO generic for all UART >> drivers. > > ... > >> +static void uart_set_rs485_termination(struct uart_port *port, >> + const struct serial_rs485 *rs485) >> +{ > >> + if (!port->rs485_term_gpio > > This duplicates the check the GPIO library does. Drop it. > Ok. >> || !(rs485->flags & SER_RS485_ENABLED)) >> + return; >> + >> + gpiod_set_value_cansleep(port->rs485_term_gpio, >> + !!(rs485->flags & SER_RS485_TERMINATE_BUS)); >> +} > Thanks for the review! Regards, Lino
On Sun, 3 Jul 2022, Lino Sanfilippo wrote: > From: Lino Sanfilippo <l.sanfilippo@kunbus.com> > > In serial8250_em485_config() the termination GPIO is set with the uart_port > spinlock held. This is an issue if setting the GPIO line can sleep (e.g. > since the concerning GPIO expander is connected via SPI or I2C). > > Fix this by setting the termination line outside of the uart_port spinlock > in the serial core and using gpiod_set_value_cansleep() which instead of > gpiod_set_value() allows to sleep. > > Beside fixing the termination GPIO line setting for the 8250 driver this > change also makes setting the termination GPIO generic for all UART > drivers. > > Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> > --- > drivers/tty/serial/8250/8250_port.c | 3 --- > drivers/tty/serial/serial_core.c | 12 ++++++++++++ > 2 files changed, 12 insertions(+), 3 deletions(-) > > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c > index ed2a606f2da7..72252d956f17 100644 > --- a/drivers/tty/serial/8250/8250_port.c > +++ b/drivers/tty/serial/8250/8250_port.c > @@ -676,9 +676,6 @@ int serial8250_em485_config(struct uart_port *port, struct ktermios *termios, > rs485->flags &= ~SER_RS485_RTS_AFTER_SEND; > } > > - gpiod_set_value(port->rs485_term_gpio, > - rs485->flags & SER_RS485_TERMINATE_BUS); > - I sent a series to make .rs485_supported per uart_port and properly set SER_RS485_TERMINATE_BUS according to DT config. With that series added first, SER_RS485_TERMINATE_BUS should also be removed from serial8250_em485_supported so that serial core can properly manage it all.
On 04.07.22 11:51, Ilpo Järvinen wrote: > On Sun, 3 Jul 2022, Lino Sanfilippo wrote: > >> From: Lino Sanfilippo <l.sanfilippo@kunbus.com> >> >> In serial8250_em485_config() the termination GPIO is set with the uart_port >> spinlock held. This is an issue if setting the GPIO line can sleep (e.g. >> since the concerning GPIO expander is connected via SPI or I2C). >> >> Fix this by setting the termination line outside of the uart_port spinlock >> in the serial core and using gpiod_set_value_cansleep() which instead of >> gpiod_set_value() allows to sleep. >> >> Beside fixing the termination GPIO line setting for the 8250 driver this >> change also makes setting the termination GPIO generic for all UART >> drivers. >> >> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> >> --- >> drivers/tty/serial/8250/8250_port.c | 3 --- >> drivers/tty/serial/serial_core.c | 12 ++++++++++++ >> 2 files changed, 12 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c >> index ed2a606f2da7..72252d956f17 100644 >> --- a/drivers/tty/serial/8250/8250_port.c >> +++ b/drivers/tty/serial/8250/8250_port.c >> @@ -676,9 +676,6 @@ int serial8250_em485_config(struct uart_port *port, struct ktermios *termios, >> rs485->flags &= ~SER_RS485_RTS_AFTER_SEND; >> } >> >> - gpiod_set_value(port->rs485_term_gpio, >> - rs485->flags & SER_RS485_TERMINATE_BUS); >> - > > I sent a series to make .rs485_supported per uart_port and properly set > SER_RS485_TERMINATE_BUS according to DT config. With that series added > first, SER_RS485_TERMINATE_BUS should also be removed from > serial8250_em485_supported so that serial core can properly manage > it all. > Ok, I will rebase the next version of my patches on your series then. Regards, Lino
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c index ed2a606f2da7..72252d956f17 100644 --- a/drivers/tty/serial/8250/8250_port.c +++ b/drivers/tty/serial/8250/8250_port.c @@ -676,9 +676,6 @@ int serial8250_em485_config(struct uart_port *port, struct ktermios *termios, rs485->flags &= ~SER_RS485_RTS_AFTER_SEND; } - gpiod_set_value(port->rs485_term_gpio, - rs485->flags & SER_RS485_TERMINATE_BUS); - /* * Both serial8250_em485_init() and serial8250_em485_destroy() * are idempotent. diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c index 3768663dfa4d..9c29d031b404 100644 --- a/drivers/tty/serial/serial_core.c +++ b/drivers/tty/serial/serial_core.c @@ -1358,12 +1358,23 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4 memset(rs485->padding1, 0, sizeof(rs485->padding1)); } +static void uart_set_rs485_termination(struct uart_port *port, + const struct serial_rs485 *rs485) +{ + if (!port->rs485_term_gpio || !(rs485->flags & SER_RS485_ENABLED)) + return; + + gpiod_set_value_cansleep(port->rs485_term_gpio, + !!(rs485->flags & SER_RS485_TERMINATE_BUS)); +} + int uart_rs485_config(struct uart_port *port) { struct serial_rs485 *rs485 = &port->rs485; int ret; uart_sanitize_serial_rs485(port, rs485); + uart_set_rs485_termination(port, rs485); ret = port->rs485_config(port, NULL, rs485); if (ret) @@ -1406,6 +1417,7 @@ static int uart_set_rs485_config(struct tty_struct *tty, struct uart_port *port, if (ret) return ret; uart_sanitize_serial_rs485(port, &rs485); + uart_set_rs485_termination(port, &rs485); spin_lock_irqsave(&port->lock, flags); ret = port->rs485_config(port, &tty->termios, &rs485);