[PATCHES] Fix PPP in libbsd and optimize ATSAM console

Mon Sep 27 11:31:59 UTC 2021

Hello Chris,

Am 27.09.21 um 10:51 schrieb 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?

Of course.

> 
>> 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?

The change / bug is not BSP specific. I added a performance improvement 
patch to the ATSAM BSP but that is more or less unrelated.

The bug in the pppd support should affect all BSPs at the moment and 
should be solved for all BSPs with a reasonable console driver after the 
patch. Basically the problem was that the pppd code didn't receive the 
information how many characters have been processed.

BSPs with a driver that can buffer a lot of characters in a FIFO or 
drivers that use a DMA worked with that (at least most of the time and 
depending on the packet size). But as far as I can tell, that has been 
more an accident than a defined behavior. BSPs with very small FIFOs 
(like the ATSAM) couldn't work with it.

> 
>> 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

Off-topic: Maybe we should have a look at some archive bot some-when.

> I will paste the conversation here:

OK. I just skimmed through it. I don't think that you have added any 
comments to it?

Best regards

Christian

> 
> 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
> 

-- 
--------------------------------------------
embedded brains GmbH
Herr Christian MAUDERER
Dornierstr. 4
82178 Puchheim
Germany
email: christian.mauderer at embedded-brains.de
phone: +49-89-18 94 741 - 18
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/