linux-kernel - Re: [PATCH v6] serial: support for 16550A serial ports on LP-8x4x

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, 29 Feb 2016 12:29:46 +0200
From:	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:	Sergei Ianovich <ynvich@...il.com>, linux-kernel@...r.kernel.org
Cc:	Alan Cox <gnomes@...rguk.ukuu.org.uk>,
	Arnd Bergmann <arnd@...db.de>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jslaby@...e.com>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Masahiro Yamada <yamada.masahiro@...ionext.com>,
	Paul Burton <paul.burton@...tec.com>,
	Paul Gortmaker <paul.gortmaker@...driver.com>,
	Mans Rullgard <mans@...sr.com>,
	Scott Wood <scottwood@...escale.com>,
	Joachim Eastwood <manabian@...il.com>,
	Peter Ujfalusi <peter.ujfalusi@...com>,
	Peter Hurley <peter@...leysoftware.com>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@...r.kernel.org>,
	"open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>
Subject: Re: [PATCH v6] serial: support for 16550A serial ports on LP-8x4x

On Sat, 2016-02-27 at 19:14 +0300, Sergei Ianovich wrote:
> The patch adds support for 3 additional LP-8x4x built-in serial
> ports.
> 
> The device can also host up to 8 extension cards with 4 serial ports
> on each card for a total of 35 ports. However, I don't have
> the hardware to test extension cards, so they are not supported, yet.

My comments below.
After addressing them:
Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>

> +++ b/drivers/tty/serial/8250/8250_lp8841.c
> @@ -0,0 +1,166 @@
> +/*  linux/drivers/tty/serial/8250/8250_lp8841.c
> + *
> + *  Support for 16550A serial ports on ICP DAS LP-8841
> + *
> + *  Copyright (C) 2013 Sergei Ianovich <ynvich@...il.com>
> + *
> + *  This program is free software; you can redistribute it and/or
> modify
> + *  it under the terms of the GNU General Public License version 2
> as
> + *  published by the Free Software Foundation.
> + */
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/module.h>
> +#include <linux/serial_8250.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +struct lp8841_serial_data {
> +	int			line;
> +	void			*ios_mem;

__iomem

> +};

> +	if (termios->c_cflag & CSTOPB)
> +		len++;
> +	if (termios->c_cflag & PARENB)
> +		len++;
> +	if (!(termios->c_cflag & PARODD))
> +		len++;
> +#ifdef CMSPAR
> +	if (termios->c_cflag & CMSPAR)
> +		len++;
> +#endif
> +
> +	len -= 9;

If you have 5 bit mode, no parity, 1/1.5 stop bits, you may end up with
negative value here. Am I right? If so, is it expected?

> +	len &= 3;
> +	len <<= 3;

> +	writeb(len, data->ios_mem);
> +
> +}


> +static int lp8841_serial_probe(struct platform_device *pdev)
> +{
> +	struct uart_8250_port uart = {};
> +	struct lp8841_serial_data *data;
> +	struct resource *mmres, *mires;
> +	int ret;
> +
> +	mmres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	mires = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (!mmres || !mires)
> +		return -ENODEV;

No need to check mires here, devm_ioremap_resource() will take care
about.

> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->ios_mem = devm_ioremap_resource(&pdev->dev, mires);
> +	if (!data->ios_mem)
> +		return -EFAULT;

You have to propagate the actual error code from
devm_ioremap_resource().

> +
> +	uart.port.iotype = UPIO_MEM;
> 

> +	uart.port.mapbase = mmres->start;
> +	uart.port.iobase = mmres->start;

I'm not sure about this. If you ask for UPIO_MEM why do you need to
fill iobase?
And I suppose iobase can't hold (at the end inb/outb calls) big port
numbers anyway (16 bit on x86, for example).

> +	uart.port.regshift = 1;
> +	uart.port.irq = platform_get_irq(pdev, 0);
> +	uart.port.flags = UPF_IOREMAP;
> +	uart.port.dev = &pdev->dev;
> +	uart.port.uartclk = 14745600;
> +	uart.port.set_termios = lp8841_serial_set_termios;
> +	uart.port.private_data = data;
> +
> +	ret = serial8250_register_8250_port(&uart);
> +	if (ret < 0)
> +		return ret;
> +
> +	data->line = ret;
> +
> +	platform_set_drvdata(pdev, data);
> +
> +	return 0;
> +}
> +
> +static int lp8841_serial_remove(struct platform_device *pdev)
> +{
> +	struct lp8841_serial_data *data =
> platform_get_drvdata(pdev);
> +
> +	serial8250_unregister_port(data->line);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver lp8841_serial_driver = {
> +	.probe          = lp8841_serial_probe,
> +	.remove         = lp8841_serial_remove,
> +
> +	.driver		= {
> +		.name	= "uart-lp8841",
> +		.of_match_table = lp8841_serial_dt_ids,
> +	},
> +};

-- 
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Intel Finland Oy