linux-kernel - Re: [PATCH] RFC: WIP: sc16is7xx [v0.4]

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]

Date:	Mon, 10 Mar 2014 14:17:43 +0000
From:	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>
To:	jon@...gle.org
Cc:	gregkh@...uxfoundation.org, jslaby@...e.cz,
	linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
	Jon Ringle <jringle@...dpoint.com>
Subject: Re: [PATCH] RFC: WIP: sc16is7xx [v0.4]

> +#define DRV_NAME	"sc16is7xx"
> +#define DRV_VERSION	"0.4"
> +#define SC16IS7XX_MAJOR	204
> +#define SC16IS7XX_MINOR	8

Use dynamic minors for any new device

> +static inline u8 sc16is7xx_read(struct uart_port *port, u8 reg)

I wouldn't inline this - the compiler will figure it out for you and
you've got a fair bit code there.


> +static void sc16is7xx_handle_rx(struct uart_port *port)

This gets called from handle_events which gets called from the ist
handler with IRQs disabled. I'm surprised it works at all but you
definitely don't want to be doing all this work with interrupts blocked.
That may be part of your problem.

> +static void sc16is7xx_handle_tx(struct uart_port *port)

Ditto


> +static void sc16is7xx_set_termios(struct uart_port *port,
> +				  struct ktermios *termios, struct ktermios *old)
> +{
> +	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
> +	unsigned long flags;
> +	u8 cval;
> +	u8 fcr;
> +	int baud;
> +
> +	spin_lock_irqsave(&s->lock, flags);
> +
> +	/* Mask termios capabilities we don't support */
> +	termios->c_cflag &= ~CMSPAR;
> +
> +	/* Disable RX & TX, reset break condition, status and FIFOs */
> +	fcr = sc16is7xx_read(port, UART_FCR);
> +	fcr |= UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT;
> +	fcr &= ~UART_FCR_ENABLE_FIFO;
> +	sc16is7xx_write(port, UART_FCR, fcr);
> +
> +	/* Word size */
> +	switch (termios->c_cflag & CSIZE) {
> +	case CS5:
> +		cval = UART_LCR_WLEN5;
> +		break;
> +	case CS6:
> +		cval = UART_LCR_WLEN6;
> +		break;
> +	case CS7:
> +		cval = UART_LCR_WLEN7;
> +		break;
> +	case CS8:
> +	default:
> +		cval = UART_LCR_WLEN8;

(for the unknown case you also need to do
		termios->c_cflag &= ~CSIZE;
		termios->c_cflag |= CS8;

so the caller gets informed they didn't get their requested feature

Likewise btw for unsupported features. In this case CMSPAR appears not to
be supported ?)

Reading drivers/tty/serial/max310x.c may be helpful with regards to the
locking. It is SPI based so also has to defer some of the processing from
the IRQ handler. The threaded IRQ support we now have may also help you.

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/