linux-kernel - Re: [PATCH 5/7] PPS: serial clients 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: <20080410162812.39414ec5@core>
Date:	Thu, 10 Apr 2008 16:28:12 +0100
From:	Alan Cox <alan@...rguk.ukuu.org.uk>
To:	Rodolfo Giometti <giometti@...ux.it>
Cc:	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	David Woodhouse <dwmw2@...radead.org>,
	Dave Jones <davej@...hat.com>, Sam Ravnborg <sam@...nborg.org>,
	Greg KH <greg@...ah.com>,
	Randy Dunlap <randy.dunlap@...cle.com>,
	Kay Sievers <kay.sievers@...y.org>,
	Rodolfo Giometti <giometti@...ux.it>
Subject: Re: [PATCH 5/7] PPS: serial clients support.

On Thu, 10 Apr 2008 17:15:58 +0200
Rodolfo Giometti <giometti@...ux.it> wrote:

> Adds support for the PPS sources connected with the CD (Carrier
> Detect) pin of a serial port.

Ok why does this need to hack the driver layer ?

> -static void uart_change_speed(struct uart_state *state,
> -					struct ktermios *old_termios);
> +static void uart_change_speed(struct uart_state *state, struct ktermios *old_termios);

NAK. Please remove random formatting changes

> -#define uart_set_mctrl(port, set)	uart_update_mctrl(port, set, 0)
> -#define uart_clear_mctrl(port, clear)	uart_update_mctrl(port, 0, clear)
> +#define uart_set_mctrl(port,set)	uart_update_mctrl(port,set,0)
> +#define uart_clear_mctrl(port,clear)	uart_update_mctrl(port,0,clear)

Ditto

>  
>  /*
>   * Startup the port.  This will be called once per open.  All calls
> @@ -291,7 +291,7 @@ uart_update_timeout(struct uart_port *port, unsigned int cflag,
>  		break;
>  	default:
>  		bits = 10;
> -		break; /* CS8 */
> +		break; // CS8

Ditto

> -			baud = tty_termios_baud_rate(old);
> -			tty_termios_encode_baud_rate(termios, baud, baud);
> +			termios->c_cflag |= old->c_cflag & CBAUD;

NAK. Breaks the tty layer code

>  			old = NULL;
>  			continue;
>  		}
> @@ -382,7 +381,7 @@ uart_get_baud_rate(struct uart_port *port, struct ktermios *termios,
>  		 * As a last resort, if the quotient is zero,
>  		 * default to 9600 bps
>  		 */
> -		tty_termios_encode_baud_rate(termios, 9600, 9600);
> +		termios->c_cflag |= B9600;

NAK breaks the tty layer code

> +#ifdef CONFIG_PPS_CLIENT_UART

wrong layer - this means only some ports will work.


> @@ -789,8 +845,7 @@ static int uart_set_info(struct uart_state *state,
>  			 * We failed anyway.
>  			 */
>  			retval = -EBUSY;
> -			/* Added to return the correct error -Ram Gupta */
> -			goto exit;
> +			goto exit;  // Added to return the correct error -Ram Gupta

More format mangling

> +	/* PPS support enabled/disabled? */
> +	if ((old_flags & UPF_HARDPPS_CD) != (new_flags & UPF_HARDPPS_CD)) {

How is this flag set ?


> -static void uart_set_termios(struct tty_struct *tty,
> -						struct ktermios *old_termios)
> +static void uart_set_termios(struct tty_struct *tty, struct ktermios *old_termios)

NAK - more format mangling

>  {
>  	struct uart_state *state = tty->driver_data;
>  	unsigned long flags;
> @@ -1464,8 +1526,9 @@ uart_block_til_ready(struct file *filp, struct uart_state *state)
>  		 */
>  		if ((filp->f_flags & O_NONBLOCK) ||
>  		    (info->tty->termios->c_cflag & CLOCAL) ||
> -		    (info->tty->flags & (1 << TTY_IO_ERROR)))
> +		    (info->tty->flags & (1 << TTY_IO_ERROR))) {

And more - and so it goes on. Please clean up the mess and submit a
proposal which is just the relevant changes so it can be read. Then we
can talk about putting it in the right place.

My guess is that PPS like FIR should be a line discipline and they both
and a couple of other cases want the tty layer to grow

	tty->ops->report_event();

and
	tty->ops->set_event_mask();

for nicer ways to monitor modem bits in the ldisc.

Alan
--
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/