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]

Message-ID: <1456750995.23036.87.camel@gmail.com>
Date:	Mon, 29 Feb 2016 16:03:15 +0300
From:	Sergei Ianovich <ynvich@...il.com>
To:	Andy Shevchenko <andriy.shevchenko@...ux.intel.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 Mon, 2016-02-29 at 12:29 +0200, Andy Shevchenko wrote:
> On Sat, 2016-02-27 at 19:14 +0300, Sergei Ianovich wrote:
> > +struct lp8841_serial_data {
> > +	int			line;
> > +	void			*ios_mem;
> 
> __iomem

OK

> > +};
> 
> > +	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;

I haven't tested this mode. I am pretty sure it will fail. There is
also no support for speeds higher than 115200.

CS7 and CS8 at speeds up to 115200 work well.

However, there is no way to report errors from set_termios(). Should
anything be done about those limitations?

> +	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.

OK

> > +	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().

OK

> > +
> > +	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).

There is no need for iobase. I'll remove that line.