| Message ID | 20210723223152.648326-6-sean.anderson@seco.com |
|---|---|
| State | New |
| Headers | show |
| Series | tty: serial: uartlite: Disable changing fixed parameters | expand |
On Fri, Jul 23, 2021 at 06:31:51PM -0400, Sean Anderson wrote: > This device does not support changing baud, parity, data bits, stop > bits, or detecting breaks. Disable "changing" these settings to prevent > their termios from diverging from the actual state of the uart. To inform > users of these limitations, warn if the new termios change these > parameters. We only do this once to avoid spamming the log. These > warnings are inspired by those in the sifive driver. > > Signed-off-by: Sean Anderson <sean.anderson@seco.com> > --- > > drivers/tty/serial/uartlite.c | 52 +++++++++++++++++++++++++++++++++-- > 1 file changed, 49 insertions(+), 3 deletions(-) > > diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c > index 39c17ab206ca..0aed70039f46 100644 > --- a/drivers/tty/serial/uartlite.c > +++ b/drivers/tty/serial/uartlite.c > @@ -314,7 +314,54 @@ static void ulite_set_termios(struct uart_port *port, struct ktermios *termios, > struct ktermios *old) > { > unsigned long flags; > - unsigned int baud; > + struct uartlite_data *pdata = port->private_data; > + tcflag_t old_cflag; > + > + if (termios->c_iflag & BRKINT) > + dev_err_once(port->dev, "BREAK detection not supported\n"); > + termios->c_iflag &= ~BRKINT; > + > + if (termios->c_cflag & CSTOPB) > + dev_err_once(port->dev, "only one stop bit supported\n"); > + termios->c_cflag &= ~CSTOPB; > + > + old_cflag = termios->c_cflag; > + termios->c_cflag &= ~(PARENB | PARODD); > + if (pdata->parity == 'e') > + termios->c_cflag |= PARENB; > + else if (pdata->parity == 'o') > + termios->c_cflag |= PARENB | PARODD; > + > + if (termios->c_cflag != old_cflag) > + dev_err_once(port->dev, "only '%c' parity supported\n", > + pdata->parity); Through all of this, you are warning that nothing is supported, yet you are continuing on as if all of this worked just fine. That feels odd, why is this needed at all? If you really do not support any changes here, why even fake it? thanks, greg k-h
On 7/29/21 11:01 AM, Greg Kroah-Hartman wrote: > On Fri, Jul 23, 2021 at 06:31:51PM -0400, Sean Anderson wrote: >> This device does not support changing baud, parity, data bits, stop >> bits, or detecting breaks. Disable "changing" these settings to prevent >> their termios from diverging from the actual state of the uart. To inform >> users of these limitations, warn if the new termios change these >> parameters. We only do this once to avoid spamming the log. These >> warnings are inspired by those in the sifive driver. >> >> Signed-off-by: Sean Anderson <sean.anderson@seco.com> >> --- >> >> drivers/tty/serial/uartlite.c | 52 +++++++++++++++++++++++++++++++++-- >> 1 file changed, 49 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c >> index 39c17ab206ca..0aed70039f46 100644 >> --- a/drivers/tty/serial/uartlite.c >> +++ b/drivers/tty/serial/uartlite.c >> @@ -314,7 +314,54 @@ static void ulite_set_termios(struct uart_port *port, struct ktermios *termios, >> struct ktermios *old) >> { >> unsigned long flags; >> - unsigned int baud; >> + struct uartlite_data *pdata = port->private_data; >> + tcflag_t old_cflag; >> + >> + if (termios->c_iflag & BRKINT) >> + dev_err_once(port->dev, "BREAK detection not supported\n"); >> + termios->c_iflag &= ~BRKINT; >> + >> + if (termios->c_cflag & CSTOPB) >> + dev_err_once(port->dev, "only one stop bit supported\n"); >> + termios->c_cflag &= ~CSTOPB; >> + >> + old_cflag = termios->c_cflag; >> + termios->c_cflag &= ~(PARENB | PARODD); >> + if (pdata->parity == 'e') >> + termios->c_cflag |= PARENB; >> + else if (pdata->parity == 'o') >> + termios->c_cflag |= PARENB | PARODD; >> + >> + if (termios->c_cflag != old_cflag) >> + dev_err_once(port->dev, "only '%c' parity supported\n", >> + pdata->parity); > > Through all of this, you are warning that nothing is supported, yet you > are continuing on as if all of this worked just fine. We don't. The idea is that we see if (e.g.) CSIZE is something the hardware can't produce, warn about it (once), and then set it to what we can support. That way the user can (programmatically) detect if this device can support their use-case. So e.g. if you you have a serial bus or something, the driver can find out that (e.g.) the UART has the wrong CSIZE, and it can fail to probe. Before this series, it would continue along as if nothing was wrong, and the user then has to debug why their device does not work as expected. --Sean > > That feels odd, why is this needed at all? If you really do not support > any changes here, why even fake it? > > thanks, > > greg k-h >
On Thu, Jul 29, 2021 at 11:26:59AM -0400, Sean Anderson wrote: > > > On 7/29/21 11:01 AM, Greg Kroah-Hartman wrote: > > On Fri, Jul 23, 2021 at 06:31:51PM -0400, Sean Anderson wrote: > > > This device does not support changing baud, parity, data bits, stop > > > bits, or detecting breaks. Disable "changing" these settings to prevent > > > their termios from diverging from the actual state of the uart. To inform > > > users of these limitations, warn if the new termios change these > > > parameters. We only do this once to avoid spamming the log. These > > > warnings are inspired by those in the sifive driver. > > > > > > Signed-off-by: Sean Anderson <sean.anderson@seco.com> > > > --- > > > > > > drivers/tty/serial/uartlite.c | 52 +++++++++++++++++++++++++++++++++-- > > > 1 file changed, 49 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c > > > index 39c17ab206ca..0aed70039f46 100644 > > > --- a/drivers/tty/serial/uartlite.c > > > +++ b/drivers/tty/serial/uartlite.c > > > @@ -314,7 +314,54 @@ static void ulite_set_termios(struct uart_port *port, struct ktermios *termios, > > > struct ktermios *old) > > > { > > > unsigned long flags; > > > - unsigned int baud; > > > + struct uartlite_data *pdata = port->private_data; > > > + tcflag_t old_cflag; > > > + > > > + if (termios->c_iflag & BRKINT) > > > + dev_err_once(port->dev, "BREAK detection not supported\n"); > > > + termios->c_iflag &= ~BRKINT; > > > + > > > + if (termios->c_cflag & CSTOPB) > > > + dev_err_once(port->dev, "only one stop bit supported\n"); > > > + termios->c_cflag &= ~CSTOPB; > > > + > > > + old_cflag = termios->c_cflag; > > > + termios->c_cflag &= ~(PARENB | PARODD); > > > + if (pdata->parity == 'e') > > > + termios->c_cflag |= PARENB; > > > + else if (pdata->parity == 'o') > > > + termios->c_cflag |= PARENB | PARODD; > > > + > > > + if (termios->c_cflag != old_cflag) > > > + dev_err_once(port->dev, "only '%c' parity supported\n", > > > + pdata->parity); > > > > Through all of this, you are warning that nothing is supported, yet you > > are continuing on as if all of this worked just fine. > > We don't. The idea is that we see if (e.g.) CSIZE is something the > hardware can't produce, warn about it (once), and then set it to what we > can support. So you are ignoring what the user wanted, and doing whatever you wanted. As you can only support one setting, why even care? Just set it to what you want and ignore userspace's requests. Of course that is a pain but no one is going to notice kernel log messages either, right? > That way the user can (programmatically) detect if this > device can support their use-case. How will a user program read the kernel error log for this? > So e.g. if you you have a serial bus > or something, the driver can find out that (e.g.) the UART has the wrong > CSIZE, and it can fail to probe. What will fail to probe? Where? > Before this series, it would continue along as if nothing was wrong, > and the user then has to debug why their device does not work as > expected. Why not fix your broken uart? :) thanks, greg k-h
On 7/29/21 11:32 AM, Greg Kroah-Hartman wrote: > On Thu, Jul 29, 2021 at 11:26:59AM -0400, Sean Anderson wrote: >> >> >> On 7/29/21 11:01 AM, Greg Kroah-Hartman wrote: >> > On Fri, Jul 23, 2021 at 06:31:51PM -0400, Sean Anderson wrote: >> > > This device does not support changing baud, parity, data bits, stop >> > > bits, or detecting breaks. Disable "changing" these settings to prevent >> > > their termios from diverging from the actual state of the uart. To inform >> > > users of these limitations, warn if the new termios change these >> > > parameters. We only do this once to avoid spamming the log. These >> > > warnings are inspired by those in the sifive driver. >> > > >> > > Signed-off-by: Sean Anderson <sean.anderson@seco.com> >> > > --- >> > > >> > > drivers/tty/serial/uartlite.c | 52 +++++++++++++++++++++++++++++++++-- >> > > 1 file changed, 49 insertions(+), 3 deletions(-) >> > > >> > > diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c >> > > index 39c17ab206ca..0aed70039f46 100644 >> > > --- a/drivers/tty/serial/uartlite.c >> > > +++ b/drivers/tty/serial/uartlite.c >> > > @@ -314,7 +314,54 @@ static void ulite_set_termios(struct uart_port *port, struct ktermios *termios, >> > > struct ktermios *old) >> > > { >> > > unsigned long flags; >> > > - unsigned int baud; >> > > + struct uartlite_data *pdata = port->private_data; >> > > + tcflag_t old_cflag; >> > > + >> > > + if (termios->c_iflag & BRKINT) >> > > + dev_err_once(port->dev, "BREAK detection not supported\n"); >> > > + termios->c_iflag &= ~BRKINT; >> > > + >> > > + if (termios->c_cflag & CSTOPB) >> > > + dev_err_once(port->dev, "only one stop bit supported\n"); >> > > + termios->c_cflag &= ~CSTOPB; >> > > + >> > > + old_cflag = termios->c_cflag; >> > > + termios->c_cflag &= ~(PARENB | PARODD); >> > > + if (pdata->parity == 'e') >> > > + termios->c_cflag |= PARENB; >> > > + else if (pdata->parity == 'o') >> > > + termios->c_cflag |= PARENB | PARODD; >> > > + >> > > + if (termios->c_cflag != old_cflag) >> > > + dev_err_once(port->dev, "only '%c' parity supported\n", >> > > + pdata->parity); >> > >> > Through all of this, you are warning that nothing is supported, yet you >> > are continuing on as if all of this worked just fine. >> >> We don't. The idea is that we see if (e.g.) CSIZE is something the >> hardware can't produce, warn about it (once), and then set it to what we >> can support. > > So you are ignoring what the user wanted, and doing whatever you wanted. > > As you can only support one setting, why even care? Just set it to what > you want and ignore userspace's requests. That is exactly what we are doing. We set it to what we can support and ignore what userspace requested. > Of course that is a pain but > no one is going to notice kernel log messages either, right? *shrug* Why does sifive_serial_set_termios do it? > >> That way the user can (programmatically) detect if this >> device can support their use-case. > > How will a user program read the kernel error log for this? The idea is that after calling tcsetattr, you call tcgetattr to see which of your termios got applied. Then you can handle that however you want. This is what (e.g.) stty does: # stty parity [ 7.221696] uartlite 84000000.serial: only 'n' parity supported [ 7.222139] uartlite 84000000.serial: only 8 data bits supported stty: standard input: cannot perform all requested operations The dmesg lines are just to help out whoever is trying to figure out why their stty command failed. > >> So e.g. if you you have a serial bus >> or something, the driver can find out that (e.g.) the UART has the wrong >> CSIZE, and it can fail to probe. > > What will fail to probe? Where? The serial bus device. Though, it looks like things such as ttyport_set_baudrate/serdev_device_set_baudrate do not check the termios after setting them, so in-kernel drivers don't handle this as well as userspace can. >> Before this series, it would continue along as if nothing was wrong, >> and the user then has to debug why their device does not work as >> expected. > > Why not fix your broken uart? :) I believe this is a feature, designed to reduce the amount of FPGA resources :) --Sean
On Thu, Jul 29, 2021 at 11:43:08AM -0400, Sean Anderson wrote: > > > On 7/29/21 11:32 AM, Greg Kroah-Hartman wrote: > > On Thu, Jul 29, 2021 at 11:26:59AM -0400, Sean Anderson wrote: > >> > >> > >> On 7/29/21 11:01 AM, Greg Kroah-Hartman wrote: > >> > On Fri, Jul 23, 2021 at 06:31:51PM -0400, Sean Anderson wrote: > >> > > This device does not support changing baud, parity, data bits, stop > >> > > bits, or detecting breaks. Disable "changing" these settings to prevent > >> > > their termios from diverging from the actual state of the uart. To inform > >> > > users of these limitations, warn if the new termios change these > >> > > parameters. We only do this once to avoid spamming the log. These > >> > > warnings are inspired by those in the sifive driver. > >> > > > >> > > Signed-off-by: Sean Anderson <sean.anderson@seco.com> > >> > > --- > >> > > > >> > > drivers/tty/serial/uartlite.c | 52 +++++++++++++++++++++++++++++++++-- > >> > > 1 file changed, 49 insertions(+), 3 deletions(-) > >> > > > >> > > diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c > >> > > index 39c17ab206ca..0aed70039f46 100644 > >> > > --- a/drivers/tty/serial/uartlite.c > >> > > +++ b/drivers/tty/serial/uartlite.c > >> > > @@ -314,7 +314,54 @@ static void ulite_set_termios(struct uart_port *port, struct ktermios *termios, > >> > > struct ktermios *old) > >> > > { > >> > > unsigned long flags; > >> > > - unsigned int baud; > >> > > + struct uartlite_data *pdata = port->private_data; > >> > > + tcflag_t old_cflag; > >> > > + > >> > > + if (termios->c_iflag & BRKINT) > >> > > + dev_err_once(port->dev, "BREAK detection not supported\n"); > >> > > + termios->c_iflag &= ~BRKINT; > >> > > + > >> > > + if (termios->c_cflag & CSTOPB) > >> > > + dev_err_once(port->dev, "only one stop bit supported\n"); > >> > > + termios->c_cflag &= ~CSTOPB; > >> > > + > >> > > + old_cflag = termios->c_cflag; > >> > > + termios->c_cflag &= ~(PARENB | PARODD); > >> > > + if (pdata->parity == 'e') > >> > > + termios->c_cflag |= PARENB; > >> > > + else if (pdata->parity == 'o') > >> > > + termios->c_cflag |= PARENB | PARODD; > >> > > + > >> > > + if (termios->c_cflag != old_cflag) > >> > > + dev_err_once(port->dev, "only '%c' parity supported\n", > >> > > + pdata->parity); > >> > > >> > Through all of this, you are warning that nothing is supported, yet you > >> > are continuing on as if all of this worked just fine. > >> > >> We don't. The idea is that we see if (e.g.) CSIZE is something the > >> hardware can't produce, warn about it (once), and then set it to what we > >> can support. > > > > So you are ignoring what the user wanted, and doing whatever you wanted. > > > > As you can only support one setting, why even care? Just set it to what > > you want and ignore userspace's requests. > > That is exactly what we are doing. We set it to what we can support and > ignore what userspace requested. If you can only support one set of options, just set it and always fail the tcsetattr call which will allow userspace to know it shouldn't have tried to do that. > > Of course that is a pain but > > no one is going to notice kernel log messages either, right? > > *shrug* Why does sifive_serial_set_termios do it? I didn't notice it during the review process. Doesn't mean you should copy a bad example :) thanks, greg k-h
On 7/30/21 1:25 AM, Greg Kroah-Hartman wrote: > On Thu, Jul 29, 2021 at 11:43:08AM -0400, Sean Anderson wrote: >> >> >> On 7/29/21 11:32 AM, Greg Kroah-Hartman wrote: >> > On Thu, Jul 29, 2021 at 11:26:59AM -0400, Sean Anderson wrote: >> >> >> >> >> >> On 7/29/21 11:01 AM, Greg Kroah-Hartman wrote: >> >> > On Fri, Jul 23, 2021 at 06:31:51PM -0400, Sean Anderson wrote: >> >> > > This device does not support changing baud, parity, data bits, stop >> >> > > bits, or detecting breaks. Disable "changing" these settings to prevent >> >> > > their termios from diverging from the actual state of the uart. To inform >> >> > > users of these limitations, warn if the new termios change these >> >> > > parameters. We only do this once to avoid spamming the log. These >> >> > > warnings are inspired by those in the sifive driver. >> >> > > >> >> > > Signed-off-by: Sean Anderson <sean.anderson@seco.com> >> >> > > --- >> >> > > >> >> > > drivers/tty/serial/uartlite.c | 52 +++++++++++++++++++++++++++++++++-- >> >> > > 1 file changed, 49 insertions(+), 3 deletions(-) >> >> > > >> >> > > diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c >> >> > > index 39c17ab206ca..0aed70039f46 100644 >> >> > > --- a/drivers/tty/serial/uartlite.c >> >> > > +++ b/drivers/tty/serial/uartlite.c >> >> > > @@ -314,7 +314,54 @@ static void ulite_set_termios(struct uart_port *port, struct ktermios *termios, >> >> > > struct ktermios *old) >> >> > > { >> >> > > unsigned long flags; >> >> > > - unsigned int baud; >> >> > > + struct uartlite_data *pdata = port->private_data; >> >> > > + tcflag_t old_cflag; >> >> > > + >> >> > > + if (termios->c_iflag & BRKINT) >> >> > > + dev_err_once(port->dev, "BREAK detection not supported\n"); >> >> > > + termios->c_iflag &= ~BRKINT; >> >> > > + >> >> > > + if (termios->c_cflag & CSTOPB) >> >> > > + dev_err_once(port->dev, "only one stop bit supported\n"); >> >> > > + termios->c_cflag &= ~CSTOPB; >> >> > > + >> >> > > + old_cflag = termios->c_cflag; >> >> > > + termios->c_cflag &= ~(PARENB | PARODD); >> >> > > + if (pdata->parity == 'e') >> >> > > + termios->c_cflag |= PARENB; >> >> > > + else if (pdata->parity == 'o') >> >> > > + termios->c_cflag |= PARENB | PARODD; >> >> > > + >> >> > > + if (termios->c_cflag != old_cflag) >> >> > > + dev_err_once(port->dev, "only '%c' parity supported\n", >> >> > > + pdata->parity); >> >> > >> >> > Through all of this, you are warning that nothing is supported, yet you >> >> > are continuing on as if all of this worked just fine. >> >> >> >> We don't. The idea is that we see if (e.g.) CSIZE is something the >> >> hardware can't produce, warn about it (once), and then set it to what we >> >> can support. >> > >> > So you are ignoring what the user wanted, and doing whatever you wanted. >> > >> > As you can only support one setting, why even care? Just set it to what >> > you want and ignore userspace's requests. >> >> That is exactly what we are doing. We set it to what we can support and >> ignore what userspace requested. > > If you can only support one set of options, just set it and always fail > the tcsetattr call which will allow userspace to know it shouldn't have > tried to do that. We can't. set_termios returns void. All we can do is set the termios to what they should be. > >> > Of course that is a pain but >> > no one is going to notice kernel log messages either, right? >> >> *shrug* Why does sifive_serial_set_termios do it? > > I didn't notice it during the review process. Doesn't mean you should > copy a bad example :) I didn't know it was a bad example :) Generally, I presume that any recently-added drivers are using the best practices for their subsystems. If they weren't, I'd have expected them to have been rejected during review. Of course, that doesn't always happen (such as in this case), but in the absence of documentation, code is the next best source of how things should be. In any case, I am not particularly attached to these particular warnings, as long as the termios get set to what we support. --Sean
On 2021-07-30 07:25, Greg Kroah-Hartman wrote: > On Thu, Jul 29, 2021 at 11:43:08AM -0400, Sean Anderson wrote: >> >> >> On 7/29/21 11:32 AM, Greg Kroah-Hartman wrote: >> > On Thu, Jul 29, 2021 at 11:26:59AM -0400, Sean Anderson wrote: >> >> >> >> >> >> On 7/29/21 11:01 AM, Greg Kroah-Hartman wrote: >> >> > On Fri, Jul 23, 2021 at 06:31:51PM -0400, Sean Anderson wrote: >> >> > Through all of this, you are warning that nothing is supported, yet you >> >> > are continuing on as if all of this worked just fine. >> >> >> >> We don't. The idea is that we see if (e.g.) CSIZE is something the >> >> hardware can't produce, warn about it (once), and then set it to what we >> >> can support. >> > >> > So you are ignoring what the user wanted, and doing whatever you wanted. >> > >> > As you can only support one setting, why even care? Just set it to what >> > you want and ignore userspace's requests. >> >> That is exactly what we are doing. We set it to what we can support >> and >> ignore what userspace requested. > > If you can only support one set of options, just set it and always fail > the tcsetattr call which will allow userspace to know it shouldn't have > tried to do that. I have to disagree strongly here against *always*. If the user calls tcsetattr to set it to exactly what is supported it *should not fail*. Every decent (terminal) program will start by setting or getting the initial settings. Maarten
diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c index 39c17ab206ca..0aed70039f46 100644 --- a/drivers/tty/serial/uartlite.c +++ b/drivers/tty/serial/uartlite.c @@ -314,7 +314,54 @@ static void ulite_set_termios(struct uart_port *port, struct ktermios *termios, struct ktermios *old) { unsigned long flags; - unsigned int baud; + struct uartlite_data *pdata = port->private_data; + tcflag_t old_cflag; + + if (termios->c_iflag & BRKINT) + dev_err_once(port->dev, "BREAK detection not supported\n"); + termios->c_iflag &= ~BRKINT; + + if (termios->c_cflag & CSTOPB) + dev_err_once(port->dev, "only one stop bit supported\n"); + termios->c_cflag &= ~CSTOPB; + + old_cflag = termios->c_cflag; + termios->c_cflag &= ~(PARENB | PARODD); + if (pdata->parity == 'e') + termios->c_cflag |= PARENB; + else if (pdata->parity == 'o') + termios->c_cflag |= PARENB | PARODD; + + if (termios->c_cflag != old_cflag) + dev_err_once(port->dev, "only '%c' parity supported\n", + pdata->parity); + + old_cflag = termios->c_cflag; + termios->c_cflag &= ~CSIZE; + switch (termios->c_cflag & CSIZE) { + case 5: + termios->c_cflag |= CS5; + break; + case 6: + termios->c_cflag |= CS6; + break; + case 7: + termios->c_cflag |= CS7; + break; + default: + case 8: + termios->c_cflag |= CS8; + break; + } + if (termios->c_cflag != old_cflag) + dev_err_once(port->dev, "only %d data bits supported\n", + pdata->bits); + + old_cflag = termios->c_cflag; + tty_termios_encode_baud_rate(termios, pdata->baud, pdata->baud); + if (termios->c_cflag != old_cflag) + dev_err_once(port->dev, "only %d baud supported\n", + pdata->baud); spin_lock_irqsave(&port->lock, flags); @@ -337,8 +384,7 @@ static void ulite_set_termios(struct uart_port *port, struct ktermios *termios, | ULITE_STATUS_FRAME | ULITE_STATUS_OVERRUN; /* update timeout */ - baud = uart_get_baud_rate(port, termios, old, 0, 460800); - uart_update_timeout(port, termios->c_cflag, baud); + uart_update_timeout(port, termios->c_cflag, pdata->baud); spin_unlock_irqrestore(&port->lock, flags); }
This device does not support changing baud, parity, data bits, stop bits, or detecting breaks. Disable "changing" these settings to prevent their termios from diverging from the actual state of the uart. To inform users of these limitations, warn if the new termios change these parameters. We only do this once to avoid spamming the log. These warnings are inspired by those in the sifive driver. Signed-off-by: Sean Anderson <sean.anderson@seco.com> --- drivers/tty/serial/uartlite.c | 52 +++++++++++++++++++++++++++++++++-- 1 file changed, 49 insertions(+), 3 deletions(-)