Re: [PATCH tty-next 2/2] serial: 8250: add driver for NI UARTs

[Brenda Streiff]

On 3/29/23 11:30, Greg Kroah-Hartman wrote:

Hi Greg, thanks for looking at this so quickly.

> On Wed, Mar 29, 2023 at 10:42:35AM -0500, Brenda Streiff wrote:
> > The National Instruments (NI) 16550 is a 16550-like UART with larger
> > FIFOs and embedded RS-232/RS-485 transceiver control circuitry. This
> > patch adds a driver that can operate this UART, which is used for
> > onboard serial ports in several NI embedded controller designs.
>
> People are still making new 8250-like variants with different ways of
> controlling them these days?  That's the design pattern that will not
> die, or at least, it keeps getting a "value-add" :(

This design has been on NI devices in some form since at least around
2009, so I hesitate to call it "new". But yes, it does seem like if you
let a hardware engineer be bored for too long you'll end up with a new
8250, a new SPI controller, or a new I2C controller. Sometimes all three!

> > +++ b/drivers/tty/serial/8250/8250_ni.c
> > @@ -0,0 +1,447 @@
> > +// SPDX-License-Identifier: GPL-2.0+
>
> Do you really mean "+"?  Sorry, I have to ask.

It sits atop 8250_core.c which is marked as GPL-2.0+, and has been marked
as 'any later version' since it had been added to our tree [1] in the
pre-SPDX times.

[1] https://github.com/ni/linux/commit/6bf16de92cc9

> > +/*
> > + * NI 16550 UART Driver
> > + *
> > + * The National Instruments (NI) 16550 is a UART that is compatible with the
> > + * TL16C550C and OX16C950B register interfaces, but has additional functions
> > + * for RS-485 transceiver control. This driver implements support for the
> > + * additional functionality on top of the standard serial8250 core.
> > + *
> > + * Copyright 2012-2023 National Instruments Corporation
>
> Um, 2012 and every year since then?  You all have had an out-of-tree
> driver for 11+ years that has been constantly updated every year?

It's been in _a_ tree-- NI's-- in some form for that long. But... yes,
this is correct.

I can't defend having it out of mainline for so long as having been a
good or wise choice, but that is the state of things.

> > +/* flags for ni16550_device_info */
> > +#define NI_HAS_PMR		BIT(0)
> > +
> > +struct ni16550_device_info {
> > +	unsigned int uartclk;
> > +	uint8_t prescaler;
>
> Please use proper kernel types, u8.

Okay, did a scrub to remove C99 types.

> > +	unsigned int flags;
>
> And that's a horrible packing, do you mean to have those offsets?

I wasn't thinking about struct packing here, but yes these can be
reordered.

> And why "unsigned int", don't you mean "u64" or "u32"?

For "uartclk" it was because struct uart_port::uartclk is an "unsigned
int" in include/linux/serial_8250.h.

For "flags" it was because there are loads of other places under
drivers/tty/serial/8250/* that use "flags" as an "unsigned int" or an
"unsigned long". Using a width-named types would be clearer (and I
don't mind making the change), but I tried to adhere to the convention
in nearby code.


> > +static int ni16550_rs485_config(struct uart_port *port,
> > +				struct ktermios *termios,
> > +				struct serial_rs485 *rs485)
> > +{
> > +	uint8_t pcr;
> > +	struct uart_8250_port *up = container_of(port, struct uart_8250_port,
> > +						 port);
> > +
> > +	/* "rs485" should be given to us non-NULL. */
> > +	if (WARN_ON(rs485 == NULL))
>
> Can this ever happen?  If not, don't check for it, otherwise you just
> rebooted a machine that has panic-on-warn enabled :(
>
> > +		return -EINVAL;
>
> Or better yet, handle the case and return the error, why the WARN_ON()?

I'm not sure if this was ever possible, but it doesn't look like any of
the other drivers supporting rs485_config do this check today. Into the
dustbin it goes.