linux-kernel - Re: [PATCH] tty: serial: amba-pl011: added RS485 support

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4e082c29-9a47-accc-425b-8d1854fb6ac6@3ksolutions.sk>
Date:   Fri, 17 Jan 2020 01:58:57 +0100
From:   Ivan Šištík - 3K Solutions, s. r. o. 
        <sistik@...olutions.sk>
To:     Lukas Wunner <lukas@...ner.de>
Cc:     Russell King <linux@...linux.org.uk>,
        Florian Fainelli <f.fainelli@...il.com>,
        Ray Jui <rjui@...adcom.com>,
        Scott Branden <sbranden@...adcom.com>,
        bcm-kernel-feedback-list@...adcom.com,
        Eric Anholt <eric@...olt.net>,
        Stefan Wahren <stefan.wahren@...e.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>,
        linux-arm-kernel@...ts.infradead.org,
        linux-rpi-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-serial@...r.kernel.org,
        Nicolas Saenz Julienne <nsaenzjulienne@...e.de>
Subject: Re: [PATCH] tty: serial: amba-pl011: added RS485 support

On 16. 1. 2020 at 14:29 Lukas Wunner wrote:

> So I've implemented rs485 support for amba-pl011.c two years ago
> but the patches need a little more polishing before they can be
> upstreamed and I haven't gotten around to that yet.  I apologize
> that it meant you had to reinvent the wheel.
> You can find my implementation on this branch:
> https://github.com/RevolutionPi/linux/commits/revpi-4.19
> 
> Specifically this commit:
> https://github.com/RevolutionPi/linux/commit/0099313962a5

The wheel with octagonal shape is still not perfect. I made it more
smoother. Your implementation in recommended commit use an active
waiting (pl011_rs485_tx_start, pl011_rs485_tx_stop) and that could
cause lots of problems in upper layers of tty driver or application.
I think you forgot to implement possibility to start TX during
"delay after send", too.

> You've used hrtimers in case delays are necessary after assertion
> or before deassertion of RTS.  Note that 8250_port.c already contains
> code for that.  If one wants to go that route, it would probably be
> best to move that code into serial_core.c to make it available to
> non-8250 ports.

The 8250_port.c use DMA. Do you thin that it shoud be moved to
serial_core.c? If there will be default implementation of handling
RTS by serial_core.c using timers, than I will refactor this driver
to use it.

> I took a completely different approach:  I converted amba-pl011.c
> to threaded interrupt handling using two kthreads, one for sending,
> one for receiving.  This allows simultaneous writing to and reading
> from the FIFO.  The driver keeps track of the FIFO fill level,
> which allows writing to the FIFO blindly.  The hardirq handler
> updates the fill level counter and wakes either of the IRQ threads.

I do not see any used thread in link:
https://github.com/RevolutionPi/linux/commit/0099313962a5
I am not kernel thread expert but I think that thread is not as
lightweight as hrtimer. According to my knowledge the hrtimer use some
kind of interrupt. Compare to this the kthread is created as thread
with all its scheduling structures. Did you implemented proper thread
shutdown? Has the thread right priority? There are many questions
like this...

> Once the driver was converted to threaded interrupts, it became
> possible to sleep in the IRQ handler, so I just used msleep()
> for the RTS delays.

I think that thread with main purpose to wait is waist of resources.
This kind of task should be handled by timers. I saw this passion for
threads in Windows CE 6 drivers. Did you read some of them?

> The tty layer lets you know when there's nothing more to transmit by
> calling the ->stop_tx() hook.  Then you just busy-wait for the FIFO
> to empty before you deassert RTS.

This would be wasting of CPU time and as I mentioned above it can cause
problems in above layers. Busy-wait in any method require deep
knowledge of "caller".

> Another idea would be to set TXIFLSEL (TX interrupt FIFO level select)
> in the UARTIFLS register to the lowest possible setting.  Then you'll
> get an interrupt when the TX FIFO only contains 2 bytes (on a PL011
> with 16 byte FIFOs), thus minimizing the busy-wait duration.

TX interrupt is used by other parts of driver. I would not recommend to
change this behavior without complete analysis of buffer refill timing.
There can be some devices which can be "IDLE" sensitive. This devices
would not work properly on higher baud rates.
I do not use busy-wait in timer tick. If there is data in FIFO I do not
stop timer and let it tick one more time.

Thanks,

Ivan