[PATCHES] Fix PPP in libbsd and optimize ATSAM console

Mon Feb 7 17:11:07 UTC 2022

On Mon, Feb 7, 2022 at 9:05 AM Christian MAUDERER
<christian.mauderer at embedded-brains.de> wrote:
>
> Hello Gedare,
>
> Am 07.02.22 um 16:40 schrieb Gedare Bloom:
> > On Wed, Feb 2, 2022 at 8:20 AM Christian MAUDERER
> > <christian.mauderer at embedded-brains.de> wrote:
> >>
> >> Hello,
> >>
> >> I received no comments for these in the last two weeks. I assume there
> >> are no further objections and I can push the patches to master in about
> >> a week so that the bug is fixed at least for 6.
> >>
> > Yeah, that's fine with me, thanks for getting it tested on another
> > BSP, and for following up.
>
> Thanks for the response.
>
> >
> > If you can advise how the API change affects other BSPs on 5, I think
> > that can be accepted with a proper release note/documentation of what
> > those users may expect.
> There are two relevant changes in cpukit/include/rtems/termiostypes.h:
>
> 1. I added a field to the struct rtems_termios_tty. With that the binary
> interface changes.
>
This should be ok but needs to be noted. Without looking, the main
concern would be potential memory corruption.

> 2. I added a parameter to one of the line discipline functions.
>
> -  int (*l_start)(struct rtems_termios_tty *tp);
> +  int (*l_start)(struct rtems_termios_tty *tp,int len);
>
> If someone is using the line discipline functions that should trigger a
> warning that the function prototype doesn't match. I think that an extra
> parameter in a function pointer is just ignored otherwise. At least I
> don't know of a case where it would have an effect. Please correct me if
> I'm wrong.
>
Yes, it should just be a warning. There should be clear guidance about
it in the release note.

> Best regards
>
> Christian
>
> >
> >> Best regards
> >>
> >> Christian
> >>
> >> Am 27.01.22 um 13:25 schrieb Christian MAUDERER:
> >>> Hello,
> >>>
> >>> again: What can I do to make the patches acceptable?
> >>>
> >>> Best regards
> >>>
> >>> Christian
> >>>
> >>> Am 21.01.22 um 11:57 schrieb Christian MAUDERER:
> >>>> Ping.
> >>>>
> >>>> Am 18.01.22 um 12:22 schrieb Christian MAUDERER:
> >>>>> Hello,
> >>>>>
> >>>>> I noted that I still have this patch set open. I first posted it in
> >>>>> August 2021 and later pinged it in September 2021. Both times no
> >>>>> conclusion has been found. I would like to finally finish this topic
> >>>>> and get the patches in an acceptable state. To simplify it a bit,
> >>>>> let's only discuss the following two:
> >>>>>
> >>>>> * [PATCH rtems 2/2] termios: Pass number of sent chars to l_start
> >>>>>     https://lists.rtems.org/pipermail/devel/2021-August/068892.html
> >>>>>
> >>>>> * [PATCH rtems-libbsd] ppp: Fix transmitting data
> >>>>>     https://lists.rtems.org/pipermail/devel/2021-August/068893.html
> >>>>>
> >>>>> The third one (or rather first one) is a BSP specific optimization
> >>>>> and not necessary for the other two. I'll not push that together with
> >>>>> these two. I'll start a separate discussion for it as soon as we are
> >>>>> done with these two.
> >>>>>
> >>>>>   From my point of view, the best case would be to apply the patches
> >>>>> to master and 5. Back when I started the discussion I created tickets
> >>>>> for both:
> >>>>>
> >>>>>     https://devel.rtems.org/ticket/4493
> >>>>>     https://devel.rtems.org/ticket/4494
> >>>>>
> >>>>>
> >>>>> @Gedare: In the following mail thread you asked about tests on
> >>>>> further BSPs:
> >>>>>
> >>>>> https://lists.rtems.org/pipermail/devel/2021-August/068908.html
> >>>>>
> >>>>> I now finally tested the patches on an i.MXRT BSP. PPP works with the
> >>>>> patches and doesn't work without them on that target. That's what I
> >>>>> expected. The patches are tested on three serial drivers now:
> >>>>>
> >>>>> - ATSAM and i.MXRT: Don't work without patches. Work with them.
> >>>>> - SC16IS752: Works for small packets only without patches. Works well
> >>>>> with them.
> >>>>>
> >>>>>
> >>>>> @Chris: We had two open discussions:
> >>>>>
> >>>>> 1. A style topic:
> >>>>> https://lists.rtems.org/pipermail/devel/2021-August/068912.html
> >>>>>
> >>>>> Like said there, I would prefer not to reformat the whole file as
> >>>>> long as we don't do that at least a bit automatic for most files.
> >>>>>
> >>>>> 2. A discussion about the direction and the necessary API change:
> >>>>> https://lists.rtems.org/pipermail/devel/2021-September/069468.html
> >>>>>
> >>>>> Like I explained: That PPP bug exists basically on all BSPs.
> >>>>> Depending on how many bytes a serial driver can process in it's write
> >>>>> function, PPP works better or worse. My assumption is that at the
> >>>>> moment, the PPP runs only on BSPs where it is "well enough" that no
> >>>>> one noted the bug.
> >>>>>
> >>>>> To fix that bug, the PPP needs the information how many characters
> >>>>> have been sent out. My patch set uses our RTEMS default path via
> >>>>> rtems_termios_dequeue_characters().
> >>>>>
> >>>>> I'm not entirely happy that I had to change the API. But I didn't
> >>>>> find a better solution and the API is still compatible. It will only
> >>>>> trigger a warning if someone doesn't use that extra parameter. If you
> >>>>> have a better idea how the bug could be fixed, I'm open to adapt the
> >>>>> patches. But I need a suggestion what would be a better solution
> >>>>> because - like I said - I didn't find one.
> >>>>>
> >>>>> Best regards
> >>>>>
> >>>>> Christian
> >>>>>
> >>>>> Am 12.08.21 um 13:41 schrieb Christian Mauderer:
> >>>>>> Hello,
> >>>>>>
> >>>>>> this set of patches fixes PPP. Basically the current implementation in
> >>>>>> libbsd can't work with console drivers that can't buffer a lot of
> >>>>>> characters. The pppstart() function just assumes that the low level
> >>>>>> console write can send an arbitrary number of characters without
> >>>>>> checking how many characters are actually send.
> >>>>>>
> >>>>>> In the extreme case of the ATSAM interfaces (only one character can be
> >>>>>> send at once) it's not even possible to establish a PPP connection with
> >>>>>> that. For UARTS that have some FIFO establishing might work but bigger
> >>>>>> packets won't go through. I opened tickets for 6 and 5 here:
> >>>>>>
> >>>>>> https://devel.rtems.org/ticket/4493
> >>>>>> https://devel.rtems.org/ticket/4494
> >>>>>>
> >>>>>> I would like to apply the patches to both branches (5 and 6).
> >>>>>>
> >>>>>> The solution I implemented in this patch set is the following: PPP
> >>>>>> output processing is done in the line discipline function l_start (or
> >>>>>> rather the function where l_start points to: pppstart). Our device
> >>>>>> writes don't return how many characters have been sent. Instead, that
> >>>>>> feedback is given via rtems_termios_dequeue(). Luckily that's the
> >>>>>> function that calls the l_start function.
> >>>>>>
> >>>>>> The RTEMS patch for termios extends the l_start function so that it
> >>>>>> gets
> >>>>>> the number of characters as a second argument. This extension shouldn't
> >>>>>> be a problem for existing code. In the worst case it will trigger a
> >>>>>> warning that the function doesn't match the pointer type. But in the
> >>>>>> linked code, that additional argument will just be ignored.
> >>>>>>
> >>>>>> The libbsd patch extends the pppstart function so that it then handles
> >>>>>> that new information.
> >>>>>>
> >>>>>> Patches belonging to this set:
> >>>>>> * [PATCH rtems 1/2] bsps/atsam: Improve UART / USART tx performance
> >>>>>> * [PATCH rtems 2/2] termios: Pass number of sent chars to l_start
> >>>>>> * [PATCH rtems-libbsd] ppp: Fix transmitting data
> >>>>>>
> >>>>>> Best regards
> >>>>>>
> >>>>>> Christian
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
> >> --
> >> --------------------------------------------
> >> 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/
>
> --
> --------------------------------------------
> 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/