linux-kernel - Re: [PATCH] serial: Revert RS485 polarity change on UART open

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <383f5d66d6828a5f09f5705f6ff98e3727ecfdf2.camel@ew.tq-group.com>
Date:   Mon, 27 Jun 2022 10:40:36 +0200
From:   Matthias Schiffer <matthias.schiffer@...tq-group.com>
To:     Lukas Wunner <lukas@...ner.de>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
        Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
        Lino Sanfilippo <LinoSanfilippo@....de>
Subject: Re: [PATCH] serial: Revert RS485 polarity change on UART open

On Tue, 2022-03-29 at 12:39 +0200, Matthias Schiffer wrote:
> On Tue, 2022-03-29 at 12:03 +0200, Lukas Wunner wrote:
> > [cc += Ilpo, Lino]
> > 
> > On Tue, Mar 29, 2022 at 10:50:50AM +0200, Matthias Schiffer wrote:
> > > While the change of the RS485 polarity in
> > > commit d3b3404df318 ("serial: Fix incorrect rs485 polarity on uart
> > > open")
> > > might have made sense based on the original intention of the
> > > rs485-rts-active-low flag (*), this is not how it is implemented in
> > > various drivers:
> > [...]
> > > [(*) My understanding of the mentioned commit's description is that
> > > rs485-rts-active-low should have referred to the electical signal
> > > level
> > > of the RTS pin, rather than the logical RTS state as understood by
> > > the
> > > UART controller.]
> 
> Hi Lukas,
> 
> > Since RTS is often just a GPIO on a pin controller that's configured
> > to function as RTS, my expectation would be that the same rules apply
> > to RTS polarity as those that apply to *any* GPIO.
> > 
> > According to Documentation/devicetree/bindings/gpio/gpio.txt:
> > 
> > "A gpio-specifier should contain a flag indicating the GPIO polarity;
> > active-
> >  high or active-low. If it does, the following best practices should
> > be
> >  followed:
> >  The gpio-specifier's polarity flag should represent the physical
> > level at the
> >                                                          ^^^^^^^^^^^^
> > ^^
> >  GPIO controller that achieves (or represents, for inputs) a
> > logically asserted
> >  value at the device."
> 
> Yes, that would make sense to me as well, but as described, this is not
> how the majority of drivers that I looked at works at the moment :(
> 
> I'm not particularly attached to any of the interpretations, but it
> would be great to have some consistency here. And if the majority of
> drivers does it the "wrong" way, maybe we should accept that to keep
> the breakage as small as possible?
> 
> > 
> > > At least the 8250 and the i.MX UART drivers interpret rs485-rts-
> > > active-low
> > 
> > Which 8250 driver are you referring to specifically?  When developing
> > d3b3404df318, I tested with 8250_bcm2835aux.c and amba-pl011.c.  Both
> > worked exactly the way they should.
> 
> I tested with 8250_omap.c, which does not implement the RS485 handling
> itself, but refers to the generic code in 8250_port.c. In fact, my
> first attempt to get the RS485 to work on my device was the following
> (which matches the originally intended interpretation of the polarity
> flag, but breaks existing users):
> 
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1460,9 +1460,9 @@ void serial8250_em485_stop_tx(struct
> uart_8250_port *p)
>         unsigned char mcr = serial8250_in_MCR(p);
>  
>         if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
> -               mcr |= UART_MCR_RTS;
> -       else
>                 mcr &= ~UART_MCR_RTS;
> +       else
> +               mcr |= UART_MCR_RTS;
>         serial8250_out_MCR(p, mcr);
>  
>         /*
> @@ -1611,9 +1611,9 @@ void serial8250_em485_start_tx(struct
> uart_8250_port *up)
>                 serial8250_stop_rx(&up->port);
>  
>         if (up->port.rs485.flags & SER_RS485_RTS_ON_SEND)
> -               mcr |= UART_MCR_RTS;
> -       else
>                 mcr &= ~UART_MCR_RTS;
> +       else
> +               mcr |= UART_MCR_RTS;
>         serial8250_out_MCR(up, mcr);
>  }
> 
> > If imx.c and others have historically interpreted rs485-rts-active-
> > low
> > to mean that the physical level is "high" when active, then we could
> > just
> > amend imx_uart_probe() such that after calling uart_get_rs485_mode(),
> > the SER_RS485_RTS_ON_SEND and SER_RS485_RTS_AFTER_SEND bits are
> > flipped.  Would that work for you?
> > 
> 
> I guess that would work. The fact that even the different
> variants of the 8250 are implemented inconsistently makes this
> especially ugly... It certainly puts a damper on the efforts to make
> the handling of RS485 in serial drivers more generic.
> 
> 
> > I'll go through the drivers to check which ones are affected.  I'm
> > sorry
> > that you're seeing breakage, it's surprising to me that these
> > different
> > interpretations of rs485-rts-active-low exist.
> > Thanks,
> > 
> > Lukas
> 

Hi Lukas,

do you know if there has been any progress on this issue? I see that
there has been quite a bit of activity in the RS485 code in linux-next, 
but I didn't have time to check if that has any effect on the polarity
issue so far.

Thanks,
Matthias