On Thursday 05 August 2021 10:50:31 Greg Kroah-Hartman wrote:
> On Thu, Aug 05, 2021 at 10:44:10AM +0200, Pali Rohár wrote:
> > On Thursday 05 August 2021 10:30:15 Greg Kroah-Hartman wrote:
> > > On Thu, Aug 05, 2021 at 10:22:43AM +0200, Pali Rohár wrote:
> > > > On Thursday 05 August 2021 07:52:03 Greg Kroah-Hartman wrote:
> > > > > On Thu, Aug 05, 2021 at 12:08:08AM +0200, Pali Rohár wrote:
> > > > > > + linux-serial
> > > > > > + Greg
> > > > > >
> > > > > > Greg, could I ask you for reviewing this documentation manpage patch?
> > > > >
> > > > > If it is submitted in a format I can review, sure (i.e. not top-post...)
> > > > >
> > > > > But I will dig down below to say one thing...
> > > > >
> > > > > >
> > > > > > On Sunday 01 August 2021 15:51:45 Pali Rohár wrote:
> > > > > > > Signed-off-by: Pali Rohár <pali@xxxxxxxxxx>
> > > > > > >
> > > > > > > ---
> > > > > > > Changes in v3:
> > > > > > > * Check support for custom baudrate only based on BOTHER macro
> > > > > > > * Use TCGETS/TCSETS/termios when TCGETS2/TCSETS2/termios2 is not available
> > > > > > >
> > > > > > > Changes in v2:
> > > > > > > * Use \e for backslash
> > > > > > > * Use exit(EXIT_*) instead of return num
> > > > > > > * Sort includes
> > > > > > > * Add comment about possible fallback
> > > > > > > ---
> > > > > > >
> > > > > > > Hello Alejandro!
> > > > > > >
> > > > > > > I found out that this stuff is more complicated as I originally thought.
> > > > > > > And seems that additional documentation on this topic is needed...
> > > > > > >
> > > > > > > For setting custom baudrate it is needed to set BOTHER flag in c_cflag
> > > > > > > field and baudrate value itself in c_ospeed and c_ispeed fields.
> > > > > > >
> > > > > > > So when BOTHER flag is not provided by <asm/termbits.h> then setting custom
> > > > > > > baudrate is not possible, fields c_ospeed and c_ispeed do not exist (and
> > > > > > > only some predefined Bnnn baudrate values are supported). This applies when
> > > > > > > compiling application with older version of header files (prior support for
> > > > > > > custom baudrate was introduced into header files).
> > > > > > >
> > > > > > > First caveat: BOTHER constant is different for different architectures.
> > > > > > > So it is not possible to provide fallback #ifndef..#define BOTHER.
> > > > > > >
> > > > > > > And now the biggest issue: Some architectures have these c_ospeed and
> > > > > > > c_ispeed fields in struct termios and some in struct termios2.
> > > > > > >
> > > > > > > TCGETS/TCSETS ioctls use struct termios and TCGETS/TCSETS2 use
> > > > > > > struct termios2.
> > > > > > >
> > > > > > > Some architectures (e.g. amd64) provide both struct termios and struct
> > > > > > > termios2, but c_ospeed and c_ispeed are only in struct termios2.
> > > > > > >
> > > > > > > Some other architectures (e.g. alpha) provide both struct termios and struct
> > > > > > > termios2 and both have c_ospeed and c_ispeed fields.
> > > > > > >
> > > > > > > And some other architectures (e.g. powerpc) provide only struct termios
> > > > > > > (no struct termios2) and it has c_ospeed and c_ispeed fields.
> > > > > > >
> > > > > > > So basically to support all architectures it is needed to use
> > > > > > > struct termios2 when TCGETS2/TCSETS2 is supported. Otherwise it is needed
> > > > > > > to use struct termios with TCGETS/TCSETS (case for e.g. powerpc).
> > > > > > >
> > > > > > > I updated v3 patch to handle this logic.
> > > > > > > ---
> > > > > > > man2/ioctl_tty.2 | 73 ++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > 1 file changed, 73 insertions(+)
> > > > > > >
> > > > > > > diff --git a/man2/ioctl_tty.2 b/man2/ioctl_tty.2
> > > > > > > index 3020f9984872..d83cbd17225b 100644
> > > > > > > --- a/man2/ioctl_tty.2
> > > > > > > +++ b/man2/ioctl_tty.2
> > > > > > > @@ -764,6 +764,79 @@ main(void)
> > > > > > > close(fd);
> > > > > > > }
> > > > > > > .EE
> > > > > > > +.PP
> > > > > > > +Get or set arbitrary baudrate on the serial port.
> > > > > > > +.PP
> > > > > > > +.EX
> > > > > > > +#include <asm/termbits.h>
> > > > > > > +#include <fcntl.h>
> > > > > > > +#include <stdio.h>
> > > > > > > +#include <stdlib.h>
> > > > > > > +#include <sys/ioctl.h>
> > > > > > > +#include <sys/types.h>
> > > > > > > +#include <unistd.h>
> > > > > > > +
> > > > > > > +int
> > > > > > > +main(int argc, char *argv[])
> > > > > > > +{
> > > > > > > +#ifndef BOTHER
> > > > > > > + fprintf(stderr, "BOTHER is unsupported\en");
> > > > > > > + /* Program may fallback to TCGETS/TCSETS with Bnnn constants */
> > > > > > > + exit(EXIT_FAILURE);
> > > > >
> > > > > So this is a BOTHER test only?
> > > >
> > > > Yes.
> > > >
> > > > > What is the goal of this program? Don't throw a bunch of #ifdef in here
> > > > > for no good reason. These options should all be present on all normal
> > > > > kernels, why wouldn't they be?
> > > >
> > > > I wanted to provide complete example which compiles fine on all Linux
> > > > systems, even with older include header files. I do not know right now
> > > > in which kernel version was introduced BOTHER support for all
> > > > architectures.
> > >
> > > We have all of the kernel source in a tool that would allow you to to
> > > determine this quite easily :)
> > >
> > > > If BOHTER is not supported then it is possible to still use Bnnn
> > > > constants to get / set baudrate. Just it is needed to write long code
> > > > for converting number to suitable Bnnn constant.
> > > >
> > > > Do you think that this BOTHER check is not useful in this case?
> > >
> > > I think you should provide an example of how to use BOTHER, yes, as it
> > > is hard to find good examples out there as they keep floating around.
> >
> > Exactly, and this is one of the reason why I sent this my patch for
> > ioctl_tty.2.
> >
> > > Here's one that I point people to a lot:
> > > https://github.com/GrantEdwards/Linux-arbitrary-baud
> >
> > I'm looking at this example at it has lot of problems:
> >
> > * Does not compile on powerpc (see explanation above).
> > * Does not include <sys/ioctl.h> and instead provide open-coded
> > declaration of ioctl: int ioctl(int d, int request, ...);
> > * Does not handle case when TCGETS/TCSETS contains t.c_ospeed
>
> Great, then fix all of that :)
It should have been already in this my patch which provides this
example. That is why I asked for review from other people :-)
> > In my opinion include header files should be used instead of writing own
> > declaration of functions.
>
> I agree.
>
> > > Make the example code easy to follow.
> > >
> > > Also, you forgot a license for this code, that is required if you want
> > > people to use it...
> >
> > Hm... I do not see any license in other manpage examples. Does not apply
> > for it global license defined in ioctl_tty.2 file?
>
> That does not mean you do not need it.
I will wait for Alejandro's reaction on this topic as I think he wants
to have all manpages consistent and with the same style, headers, etc...
