linux-kernel - Re: patch "U6715 8250 serial like driver" added to gregkh-2.6 tree

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: <20100723090119.5d63cfb7@lxorguk.ukuu.org.uk>
Date:	Fri, 23 Jul 2010 09:01:19 +0100
From:	Alan Cox <alan@...rguk.ukuu.org.uk>
To:	<gregkh@...e.de>
Cc:	philippe.langlais@...ricsson.com, linux-kernel@...r.kernel.org,
	ludovic.barre@...ricsson.com
Subject: Re: patch "U6715 8250 serial like driver" added to gregkh-2.6 tree

On Thu, 22 Jul 2010 16:57:57 -0700
<gregkh@...e.de> wrote:

> 
> This is a note to let you know that I've just added the patch titled

NAK this

I've nakked several similar patches already from people trying to
convolute the 8250 driver even further, doubly so when the patch
redefines properties of existing chips according to a compile time option


>  	[PORT_16550A] = {
>  		.name		= "16550A",
> +#if defined(CONFIG_SERIAL_8250_U6XXX)
> +		.fifo_size	= 64,
> +		.tx_loadsz	= 64,
> +		.flags		= UART_CAP_FIFO | UART_CAP_AFE,
> +#else
>  		.fifo_size	= 16,
>  		.tx_loadsz	= 16,
> -		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
>  		.flags		= UART_CAP_FIFO,
> +#endif
> +		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
>  	},

Define yourself a new port type


> @@ -2268,6 +2274,13 @@ serial8250_set_termios(struct uart_port
>  	/*
>  	 * Ask the core to calculate the divisor for us.
>  	 */
> +#ifdef CONFIG_SERIAL_8250_CUSTOM_CLOCK
> +	baud = uart_get_baud_rate(port, termios, old, 0,
> +			CONFIG_SERIAL_8250_CUSTOM_MAX_BAUDRATE);
> +	/* Calculate the new uart clock frequency if it is tunable */
> +	port->uartclk = serial8250_get_custom_clock(port, baud);
> +#endif
> +
>  	baud = uart_get_baud_rate(port, termios, old,
>  				  port->uartclk / 16 / 0xffff,
>  				  port->uartclk / 16);


I think what you need to keep this clean is to create a
port->set_termios() for the 8250 port (akin to
port->serial_in/serial_out) which does the specials you need each end and
calls the standard serial8250_set_termios)

> @@ -2298,6 +2311,13 @@ serial8250_set_termios(struct uart_port
>  		up->mcr &= ~UART_MCR_AFE;
>  		if (termios->c_cflag & CRTSCTS)
>  			up->mcr |= UART_MCR_AFE;
> +#if defined(CONFIG_SERIAL_8250_U6XXX)
> +		/**
> +		 * When AFE is active, let the HW handle the stop/restart TX
> +		 * upon CTS change. It reacts much quicker than the SW driver.
> +		 */
> +		port->flags &= ~ASYNC_CTS_FLOW;
> +#endif

Why do you need to clear this flag when doing so ?




> +unsigned int serial8250_enable_clock(struct uart_port *port)
> +{
> +	struct u6_uart *uart_u6 = port->private_data;
> +
> +	if (!uart_u6)
> +		return uart_enable_clock(port);
> +
> +	if (IS_ERR(uart_u6->uartClk)) {
> +		printk(KERN_WARNING "%s - uart clock failed error:%ld\n",
> +		       __func__, PTR_ERR(uart_u6->uartClk));

pr_warn()

Also as the functions are specific to the u6175 can you use u6715_ names
so any printk, traceback and the like is obvious in where to look.


> +unsigned int serial8250_get_custom_clock(struct uart_port *port,
> +					 unsigned int baud)
> +{
> +	switch (baud) {
> +	case 3250000:
> +		return 52000000;
> +	case 2000000:
> +		return 32000000;
> +	case 1843200:
> +		return 29491200;
> +	case 921600:
> +		return 14745600;
> +	default:
> +		return 7372800;
> +	}
> +}

Baud rates are arbitary values, so surely this should doing range checks ?


> +config SERIAL_8250_CUSTOM_CLOCK
> +	bool "Support serial ports with tunable input clock frequency"
> +	depends on SERIAL_8250_EXTENDED && SERIAL_8250_U6XXX
> +	default y
> +	help
> +	  Say Y here if your platform has specific registers to change UART clock frequency.

Except the patch you've posted is actually a patch for a U6715 not
generic tunable input clock support, so this Kconfig option/help will
confuse people. Better to describe it as U6715 support.


> +#ifdef CONFIG_SERIAL_8250_CUSTOM_CLOCK
> +unsigned int serial8250_enable_clock(struct uart_port *port);
> +unsigned int serial8250_disable_clock(struct uart_port *port);
> +unsigned int serial8250_get_custom_clock(struct uart_port *port,
> +		unsigned int baud);
> +void serial8250_set_custom_clock(struct uart_port *port);
> +#endif

This shouldn't need ifdefs in the header

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/