On Sat, 25 Jun 2022, Lukas Wunner wrote:
> On Wed, Jun 22, 2022 at 05:46:52PM +0200, Lino Sanfilippo wrote:
> > In uart_get_rs485_mode() only try to get a termination GPIO if RS485 bus
> > termination is supported by the driver.
> [...]
> > --- a/drivers/tty/serial/serial_core.c
> > +++ b/drivers/tty/serial/serial_core.c
> > @@ -3384,17 +3384,20 @@ int uart_get_rs485_mode(struct uart_port *port)
> > rs485conf->flags |= SER_RS485_RTS_AFTER_SEND;
> > }
> >
> > - /*
> > - * Disabling termination by default is the safe choice: Else if many
> > - * bus participants enable it, no communication is possible at all.
> > - * Works fine for short cables and users may enable for longer cables.
> > - */
> > - port->rs485_term_gpio = devm_gpiod_get_optional(dev, "rs485-term",
> > - GPIOD_OUT_LOW);
> > - if (IS_ERR(port->rs485_term_gpio)) {
> > - ret = PTR_ERR(port->rs485_term_gpio);
> > - port->rs485_term_gpio = NULL;
> > - return dev_err_probe(dev, ret, "Cannot get rs485-term-gpios\n");
> > + if (port->rs485_supported->flags & SER_RS485_TERMINATE_BUS) {
>
> So I think linux-next commit be2e2cb1d281 ("serial: Sanitize rs485_struct")
> contains a mistake in that it forces drivers to set SER_RS485_TERMINATE_BUS
> in their rs485_supported->flags to allow enabling bus termination.
>
> That's wrong because *every* rs485-capable driver can enable bus
> termination if a GPIO has been defined for that in the DT.
Do you mean every em485 using driver? Otherwise I don't see this "forces
drivers to set" happening anywhere in the code?
You're partially right because there are other bugs in this area such
as the one you propose a fix below. While I was making the sanitization
series, I entirely missed some parts related to termination because
SER_RS485_TERMINATE_BUS is seemingly not set/handled correctly by the
core.
Another thing that looks a bug is that on subsequent call to TIOCSRS485,
w/o SER_RS485_TERMINATE_BUS nothing happens (for non-em485 driver, that
is)? It seems to be taken care by 2/8 of this series though, I think. But
it should be properly marked as Fixes: ... in that case although nobody
has complained about it so likely not a huge issue to anyone.
> In fact, another commit which was applied as part of the same series,
> ebe2cf736a04 ("serial: pl011: Fill in rs485_supported") does not set
> SER_RS485_TERMINATE_BUS in amba-pl011.c's flags and thus forbids the
> driver from enabling bus termination, even though we know there are
> products out there which support bus termination on the pl011 through
> a GPIO (Revolution Pi RevPi Compact, Revpi Flat).
>
> I think what you want to do is amend uart_get_rs485_mode() to set
> SER_RS485_TERMINATE_BUS in port->rs485_supported_flags if a GPIO
> was found in the DT. Instead of the change proposed above.
That seems appropriate (and is a fix).
What makes it a bit complicated though is that it's a pointer currently
and what it points to is shared per driver (besides being const):
const struct serial_rs485 *rs485_supported;
While it could be embedded into uart_port, there's the .padding which we
might not want to bloat uart_port with. Perhaps create non-uapi struct
kserial_rs485 w/o .padding and add static_assert()s to ensure the
layout is identical to serial_rs485?
--
i.
