linux-kernel - Re: [PATCH 14/15] tty: serial: Add Nuvoton ma35d1 serial driver support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZBF1z5Bx9jnrpxox@kroah.com>
Date:   Wed, 15 Mar 2023 08:37:51 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Jacky Huang <ychuang570808@...il.com>
Cc:     robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        lee@...nel.org, mturquette@...libre.com, sboyd@...nel.org,
        p.zabel@...gutronix.de, jirislaby@...nel.org,
        devicetree@...r.kernel.org, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
        schung@...oton.com, Jacky Huang <ychuang3@...oton.com>
Subject: Re: [PATCH 14/15] tty: serial: Add Nuvoton ma35d1 serial driver
 support

On Wed, Mar 15, 2023 at 07:29:01AM +0000, Jacky Huang wrote:
> From: Jacky Huang <ychuang3@...oton.com>
> 
> This adds UART and console driver for Nuvoton ma35d1 Soc.
> 
> MA35D1 SoC provides up to 17 UART controllers, each with one uart port.
> The ma35d1 uart controller is not compatible with 8250.

A new UART being designed that is not an 8250 compatible?  Why????

Anyway, some minor comments:

>  drivers/tty/serial/ma35d1_serial.c | 842 +++++++++++++++++++++++++++++
>  drivers/tty/serial/ma35d1_serial.h |  93 ++++

Why do you have a .h file for only a single .c file?  Please just put
them both together into one .c file.

>  include/uapi/linux/serial_core.h   |   3 +

Why do you need this #define?  Are you using it in userspace now?  If
not, why have it?

> +static void
> +receive_chars(struct uart_ma35d1_port *up)

Please just put all one one line.


> +{
> +	u8 ch;
> +	u32 fsr;
> +	u32 isr;
> +	u32 dcnt;
> +	char flag;
> +
> +	isr = serial_in(up, UART_REG_ISR);
> +	fsr = serial_in(up, UART_REG_FSR);
> +
> +	while (!(fsr & RX_EMPTY)) {

You have no way out if the hardware is stuck?  That feels wrong.

> +static int ma35d1serial_ioctl(struct uart_port *port, u32 cmd, unsigned long arg)
> +{
> +	switch (cmd) {
> +	default:
> +		return -ENOIOCTLCMD;
> +	}
> +	return 0;
> +}

You do not need to handle any ioctls?

> +static void ma35d1serial_console_putchar(struct uart_port *port,
> +					 unsigned char ch)
> +{
> +	struct uart_ma35d1_port *up = (struct uart_ma35d1_port *)port;
> +
> +	do {
> +	} while ((serial_in(up, UART_REG_FSR) & TX_FULL));

Again, no way out if this gets stuck in a loop?

> +/**
> + *  Suspend one serial port.
> + */
> +void ma35d1serial_suspend_port(int line)
> +{
> +	uart_suspend_port(&ma35d1serial_reg, &ma35d1serial_ports[line].port);
> +}
> +EXPORT_SYMBOL(ma35d1serial_suspend_port);

Why is this exported?  Who uses it?

And why not EXPORT_SYMBOL_GPL()?

> +
> +/**
> + *  Resume one serial port.
> + */
> +void ma35d1serial_resume_port(int line)
> +{
> +	struct uart_ma35d1_port *up = &ma35d1serial_ports[line];
> +
> +	uart_resume_port(&ma35d1serial_reg, &up->port);
> +}
> +EXPORT_SYMBOL(ma35d1serial_resume_port);

Same here, who calls this and why?

> --- a/include/uapi/linux/serial_core.h
> +++ b/include/uapi/linux/serial_core.h
> @@ -279,4 +279,7 @@
>  /* Sunplus UART */
>  #define PORT_SUNPLUS	123
>  
> +/* Nuvoton MA35D1 UART */
> +#define PORT_MA35D1	124

Again, why is this define needed?

thanks,

greg k-h