[PATCHES] Fix PPP in libbsd and optimize ATSAM console

[Chris Johns]

On 27/9/21 6:24 pm, Christian MAUDERER wrote:
> same like for another patch set: There is a pending discussion for this one.

Yeap and so we need to resolve it.

I have reordered your original post so the chat follows. I hope that is OK?

> Is there still an open point left of can I push the patches?

What happens if another BSP needs the same change? Is it also OK for the same
reasons or is this the only one?

As the PPPD code needs specific drivers can the build in LibBSD be restricted to
the BSPs that support it and no others?

> Chris: Back when I posted the patch set you mentioned that you are not entirely
> happy about it in a discord chat (about 17.08.2021 6:50 in UTC time zone).

The link follows but you need a login is:

https://discord.com/channels/820452222382112799/820452222848335924/877082405545062410

Given discord does not seem to have a public archive we can access without an
account I will paste the conversation here:

kiwichris — 08/17/2021
@c-mauderer hi

c-mauderer — 08/17/2021
Hello @kiwichris

kiwichris — 08/17/2021
Just looking over the PPP patches

c-mauderer — 08/17/2021
Great, thanks.
[4:53 PM]
Any problems with them?

kiwichris — 08/17/2021
Can you please explain this fragment ...
-  if (ctx->transmitting && (csr & US_CSR_TXEMPTY) != 0) {
+  while (ctx->transmitting && (csr & US_CSR_TXRDY) != 0) {
     rtems_termios_dequeue_characters(tty, 1);
+    csr = regs->US_CSR;

c-mauderer — 08/17/2021
That's in the ATSAM console driver. Basically the same construct is used for
receiving.
[4:55 PM]
If the send buffer is empty, the first byte will more or less fall through and
the TXRDY flag is still set.
[4:55 PM]
A second byte can be queued.

kiwichris — 08/17/2021
I am concerned about the loop because it is dependent on the baurdate

c-mauderer — 08/17/2021
you mean that on very fast baud rates it could theoretically loop forever?

kiwichris — 08/17/2021
Slow?

c-mauderer — 08/17/2021
It will always only loop one or two times.
[4:58 PM]
If nothing has been sent before, the first will fill the send buffer and the
next will prepare one byte that is written to the register.

kiwichris — 08/17/2021
Is it timed on the bit shift time out?

c-mauderer — 08/17/2021
no

kiwichris — 08/17/2021
But the dequeue is removing it from the queue which means it is sent.

c-mauderer — 08/17/2021
it is given to the hardware and also it isn't yet on the line, it will be sent
in any case.
[4:59 PM]
It's basically a 1 byte FIFO

kiwichris — 08/17/2021
But you could loop more than once and if just one char why the loop on the dequeue?

c-mauderer — 08/17/2021
You are right: The loop would have also be possible and slightly more efficient
in the write. But it would have added quite a bit of logic to the write.

kiwichris — 08/17/2021


c-mauderer — 08/17/2021
The loop here was the version that was simpler to read and more similar to the
read path.
[5:03 PM]
But yes: It's a bit of a lazy solution. It's not the optimal one but it works
well enough in that driver.

kiwichris — 08/17/2021
I have a patch for the zynq that adds proper FIFO tx support.
[5:04 PM]
It will break. I have seen enough UART drivers to know the bit will stay set.
[5:04 PM]
And I have changed the transmitting flag logic because it look suspect to me

@kiwichris
It will break. I have seen enough UART drivers to know the bit will stay set.

c-mauderer — 08/17/2021
You mean the TXRDY flag?

kiwichris — 08/17/2021
I will chat to Joel and Gedare about the change for the extra args to the call.
I am not sure about this part because it fixes only one driver and leaves a
"functional bomb" in the code for another user to trip over.

@kiwichris
I will chat to Joel and Gedare about the change for the extra args to the call.
I am not sure about this part because it fixes only one driver and leaves a
"functional bomb" in the code for another user to trip over.

c-mauderer — 08/17/2021
OK. But what are the alternatives?
[5:06 PM]
It provides the feedback to the pppstart function how many characters have been
written.

kiwichris — 08/17/2021
Yes. A specific baudrate and data flow and you hit a threshold and a character
disappears from the stream and it would be hard to find.

c-mauderer — 08/17/2021
Our termios doesn't really have another feedback than that.

kiwichris — 08/17/2021
Yes. Do all other drivers still work with pppd?
[5:07 PM]
I know it does not. It is very bespoke to a console type termios

c-mauderer — 08/17/2021
I haven't tested all. At the moment I have tested the ATSAM and the SC16ISxxx

kiwichris — 08/17/2021
If the pppd depends on it and the driver ignores it how can it?
[5:08 PM]
I must be missing something here

c-mauderer — 08/17/2021
I don't think that the pppd ever really worked before that patch. That is a bug
that was there for a long time. It only wasn't really clear for all chips that
had enough FIFO or that used a DMA.
[5:10 PM]
If you take a look at the call in pppstart:
(*tp->handler.write)(ctx, (char *)sendBegin, (ioffset > 0) ? ioffset : 1);
: It just assumes that it can send any number of characters via the device write.
[5:10 PM]
That's not true for any driver.
[5:10 PM]
But some will put most of the packet (or all of it) in a FIFO.

kiwichris — 08/17/2021
Sure this is a valid point. The other side is leaving a further function bomb
where we do not know or document it. Could pppd use termios? (edited)
[5:11 PM]
Why is it directly access the drivers?

c-mauderer — 08/17/2021
That has a history. The pppd has been added to RTEMS at least 20 years back. The
one in libbsd is just a port from the original RTEMS one.
[5:11 PM]
Someone deeply integrated it into termios back than.
[5:12 PM]
It would be a really huge task to remove that integration.
[5:12 PM]
The most likely better alternative would be to just drop pppd some-when and use
the user-ppp that FreeBSD uses now.
[5:13 PM]
But that would break the API.

kiwichris — 08/17/2021
I have not looked at the code and so cannot comment. I am mixed minds about a
change to drivers. If the pppd has not worked then it there value in the API?
(edited)

c-mauderer — 08/17/2021
It worked most likely well enough on some targets (due to FIFO or DMA) that it
is used on some platforms.
[5:15 PM]
I was really annoyed that the pppd has a such a deep integration in our termios.
The fix is basically only fixing the code that is there.
[5:16 PM]
I don't really like the code

kiwichris — 08/17/2021
I understand there is no clear and simple solution here. We have a pppd send API
and users of it and we have a single termios driver interface but this changes that.

c-mauderer — 08/17/2021
I don't think that the termios line discipline functions are used often. I found
the pppd and a mouse driver in RTEMS. Beneath that: The change might trigger a
warning. But old code will still work.
[5:17 PM]
The additional parameter to the send function will just be ignored.
[5:18 PM]
Or do you know a platform where you can't pass a function that only uses one
parameter to a function pointer that is called with two?

kiwichris — 08/17/2021
There are other solutions equally not nice ...  for example we just flag the
pppd to be removed in the next version and the users need to step up? It has not
been the RTEMS way and not nice but I would like to avoid fragmenting things in
specific ways because the project ends up with the liability to clean it up.

c-mauderer — 08/17/2021
I only touched that code because there is a user. Otherwise I wouldn't have
touched it.

kiwichris — 08/17/2021
It is not about the technicality of being able to do this, it is a question of
do we want to do it?
[5:19 PM]
And what are the latent costs?

c-mauderer — 08/17/2021
That someone who wants to rewrite our termios has to do just what you suggested
first: Flag pppd out of date.

kiwichris — 08/17/2021
I appreciate that and users are important. We can live with transitional changes

c-mauderer — 08/17/2021
I don't think that PPP is used a lot nowadays. I know of two of our projects
where a user uses it.

kiwichris — 08/17/2021
I am mindful is the change going and everyone thinking all is OK
[5:24 PM]
I have used it in the past. I used it to connect metro-optical nodes where IP
was sent over PPP over GRE in ISO end points in SDH networks.

c-mauderer — 08/17/2021
Yes, I'm sure there are some users. Like I said: I know two myself. But I really
hope that it will get less often used.

Chris