Re: [PATCH 5/5] tty: serial: uartlite: Prevent changing fixed parameters

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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