linux-kernel - Re: [PATCH] max3100 driver

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: <20080920151149.21dbad1d@lxorguk.ukuu.org.uk>
Date:	Sat, 20 Sep 2008 15:11:49 +0100
From:	Alan Cox <alan@...rguk.ukuu.org.uk>
To:	Christian Pellegrin <chripell@...il.com>
Cc:	linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
	Christian Pellegrin <chripell@...e.org>
Subject: Re: [PATCH] max3100 driver

> +#define MAX3100_MAJOR 204
> +#define MAX3100_MINOR 128
> +/* 4 MAX3100s should be enough for everyone */
> +#define MAX_MAX3100 4

These need to be officially allocated if you need constant numbers

> +static int max3100_sr(struct max3100_port_s *s, u16 tx, u16 *rx)
> +{
> +	struct spi_message message;
> +	struct spi_transfer tran;
> +	u16 etx, erx;
> +	int status;
> +
> +	etx = htons(tx);

Use cpu_to_le/be or le/be_to_cpu functions, these make the intended
endianness clear.

> +	*rx = ntohs(erx);

Ditto

> +		if (rxchars > 0)
> +			tty_flip_buffer_push(s->port.info->port.tty);
> +		if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)

If there has been a hangup the port.tty will be NULL...

> +static void
> +max3100_set_termios(struct uart_port *port, struct ktermios *termios,
> +		   struct ktermios *old)
> +{
> +	struct max3100_port_s *s = container_of(port,
> +						struct max3100_port_s,

> +	if (!old || (termios->c_cflag != old->c_cflag)) {

This optimisation is wrong and not worth doing anyway

> +		i = cflag & CBAUD;
> +		switch (i) {

Use tty_get_baud_rate() to get the actual baud rate requested which is an
arbitary value.

> +		default:
> +			param_new = 1;
> +			dev_warn(&s->spi->dev, "invalid baudrate\n");

Shouldn't warn on these, just be sure to use tty_encode_baud_rate to pass
back the actual rate the user ends up with.

> +		if (termios->c_iflag & IGNPAR)
> +			s->port.ignore_status_mask |=
> +				MAX3100_STATUS_PE | MAX3100_STATUS_FE |
> +				MAX3100_STATUS_OE;

Bits you don't support should also be cleared in the tty->termios struct
(eg markspace you don't seem to do)


> +	max3100s[i] = kzalloc(sizeof(struct max3100_port_s), GFP_KERNEL);
> +	if (!max3100s[i]) {
> +		dev_warn(&spi->dev,
> +			 "kmalloc for max3100 structure %d failed!\n", i);

Does this not then need to unregister the driver ?



Looks basically sound to me - just some minor cleanups needed.

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/