| Message ID | 20240418124349.26289-1-esa.laakso@fidelix.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] pty: Add parity enabling routine | expand |
On Thu, Apr 18, 2024 at 03:43:49PM +0300, Esa Laakso wrote: > There are some cases where parity selection is required for passing > it forward to a virtualized terminal. In this sepcific use-case, we > want to use pty to send and receive serial data to a serial > multiplexer. By using a pty, we avoid writing a custom tty driver. > > There is very little evidence on the reasoning on why this option is > hard-coded to be disabled. AFAIK it has been as such since 1996. With > the lack of information about why this is, and based on the fact there > are other similar fields that are not hard-coded, it is considered safe > to enable this option. > > Still, in order not to be too intrusive about the change, add it only on > the condition that the termios flag `EXTPROC` is turned on. This way > there is very little chance it will cause any unintended problems in any > other implementation. You need to document that EXTPROC thing somewhere, otherwise someone is going to ask about this in 20 years and be confused :) > Signed-off-by: Esa Laakso <esa.laakso@fidelix.com> > Signed-off-by: Esa Laakso <fidelix.laakso@gmail.com> Do not sign off on something twice, use your real email address only once, that's all that is needed. > --- > > We are looking for some assistance on this patch, or just a green light > to submit it, if it is good to go. > > We would need to know if the change is valid in context of pty, as > there seems to be fairly little information about the reasoning behind > the hard-coded values. > > Our guess is that they have been as such forever and nobody has had a > reason to change them. We have a reason to change them, and we would > like to know if it is safe to do so and possibly contribute in the > process. Either by just using the patch for ourselves or submitting > it here. > > For the record, this is my first patch submitted upstream. Please let > me know if something is incorrect or missing. > --- > > drivers/tty/pty.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c > index 07394fdaf522..e2d9718dcea0 100644 > --- a/drivers/tty/pty.c > +++ b/drivers/tty/pty.c > @@ -267,7 +267,9 @@ static void pty_set_termios(struct tty_struct *tty, > } > } > > - tty->termios.c_cflag &= ~(CSIZE | PARENB); > + tty->termios.c_cflag &= ~(CSIZE); > + if (!L_EXTPROC(tty)) > + tty->termios.c_cflag &= ~(PARENB); Some description of how you tested this in the changelog would also be good, and a comment here too as to what you are doing. thanks, greg k-h
to 18. huhtik. 2024 klo 15.53 Greg KH (gregkh@linuxfoundation.org) kirjoitti: > > On Thu, Apr 18, 2024 at 03:43:49PM +0300, Esa Laakso wrote: > > There are some cases where parity selection is required for passing > > it forward to a virtualized terminal. In this sepcific use-case, we > > want to use pty to send and receive serial data to a serial > > multiplexer. By using a pty, we avoid writing a custom tty driver. > > > > There is very little evidence on the reasoning on why this option is > > hard-coded to be disabled. AFAIK it has been as such since 1996. With > > the lack of information about why this is, and based on the fact there > > are other similar fields that are not hard-coded, it is considered safe > > to enable this option. > > > > Still, in order not to be too intrusive about the change, add it only on > > the condition that the termios flag `EXTPROC` is turned on. This way > > there is very little chance it will cause any unintended problems in any > > other implementation. > > You need to document that EXTPROC thing somewhere, otherwise someone is > going to ask about this in 20 years and be confused :) True! Is it enough if this is documented as a comment? I would like to add it to the actual documentation, but AFAICS there does not seem to be a proper location for it... > > > Signed-off-by: Esa Laakso <esa.laakso@fidelix.com> > > Signed-off-by: Esa Laakso <fidelix.laakso@gmail.com> > > Do not sign off on something twice, use your real email address only > once, that's all that is needed. Alright. I'm unable to use our company e-mail as it adds a signature by force, so I wasn't sure what the proper protocol for this was. Thanks for the info. > > > --- > > > > We are looking for some assistance on this patch, or just a green light > > to submit it, if it is good to go. > > > > We would need to know if the change is valid in context of pty, as > > there seems to be fairly little information about the reasoning behind > > the hard-coded values. > > > > Our guess is that they have been as such forever and nobody has had a > > reason to change them. We have a reason to change them, and we would > > like to know if it is safe to do so and possibly contribute in the > > process. Either by just using the patch for ourselves or submitting > > it here. > > > > For the record, this is my first patch submitted upstream. Please let > > me know if something is incorrect or missing. > > --- > > > > drivers/tty/pty.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c > > index 07394fdaf522..e2d9718dcea0 100644 > > --- a/drivers/tty/pty.c > > +++ b/drivers/tty/pty.c > > @@ -267,7 +267,9 @@ static void pty_set_termios(struct tty_struct *tty, > > } > > } > > > > - tty->termios.c_cflag &= ~(CSIZE | PARENB); > > + tty->termios.c_cflag &= ~(CSIZE); > > + if (!L_EXTPROC(tty)) > > + tty->termios.c_cflag &= ~(PARENB); > > Some description of how you tested this in the changelog would also be With "changelog", do you mean the commit message? I'll do that. > good, and a comment here too as to what you are doing. We are using a (legacy) hardware serial port multiplexer in our device. In order to avoid writing a kernel driver for this, we decided the next best approach is to have a rust driver in userspace and use pty:s instead of tty:s. This way we get to have access to the needed parts of the termios structure, plus any other niceties that come with pty code without having to implement those ourselves. Our rust code hosts a pty master, takes the termios settings defined by the slave endpoint and passes the relevant parts to the multiplexer. This way we can take almost any software that uses a serial and it will have a high chance working out of the box. We use EXTPROC flag to get the termios settings asyncronously from the slaves to the master. When termios is used by the process attached to the slave pty, it will send a byte to indicate change of options. We then pass those settings on the multiplexer, resulting in a minimal delay. When doing this, we noted that we were unable to control the parity flag, as it would always be disabled. This meant that we were unable to pass it onto the multiplexer. This did not apply to other similar, non-effecting settings like baudrate, which does nothing on pty:s either (AFAIK). Hence the careful change here, where we used the EXTPROC to determine the behavior of the PARENB bit. We can still change this to NOT be EXTPROC dependent, but we felt that such we could cause a regression due to lack of knowledge. Very little to no info on the PARENB flag, or any of the other relevant flags could be found on the pty. Let us know if you deem otherwise. > > thanks, > > greg k-h
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c index 07394fdaf522..e2d9718dcea0 100644 --- a/drivers/tty/pty.c +++ b/drivers/tty/pty.c @@ -267,7 +267,9 @@ static void pty_set_termios(struct tty_struct *tty, } } - tty->termios.c_cflag &= ~(CSIZE | PARENB); + tty->termios.c_cflag &= ~(CSIZE); + if (!L_EXTPROC(tty)) + tty->termios.c_cflag &= ~(PARENB); tty->termios.c_cflag |= (CS8 | CREAD); }