linux-kernel - Re: [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file

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: <1453196024.2521.108.camel@linux.intel.com>
Date:	Tue, 19 Jan 2016 11:33:44 +0200
From:	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:	Peter Hung <hpeter@...il.com>,
	Paul Gortmaker <paul.gortmaker@...driver.com>
Cc:	gregkh@...uxfoundation.org, jslaby@...e.com,
	heikki.krogerus@...ux.intel.com, peter@...leysoftware.com,
	soeren.grunewald@...y.de, udknight@...il.com,
	adam.lee@...onical.com, arnd@...db.de,
	yamada.masahiro@...ionext.com, mans@...sr.com,
	scottwood@...escale.com, paul.burton@...tec.com,
	matthias.bgg@...il.com, manabian@...il.com, peter.ujfalusi@...com,
	linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
	peter_hong@...tek.com.tw,
	Peter Hung <hpeter+linux_kernel@...il.com>
Subject: Re: [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file

On Tue, 2016-01-19 at 16:45 +0800, Peter Hung wrote:
> Hi Paul,
> 
> Paul Gortmaker 於 2016/1/19 上午 11:56 寫道:
> > > The serial ports support from 50bps to 1.5Mbps with Linux
> > > baudrate
> > > define excluding 1.0Mbps due to not support 16MHz clock source.
> > 
> > How does this differ from what was achieved or possible with the
> > old way
> > of things?  What was the limitation in the existing 8250 code
> > sharing
> > that required Fintek code to fork and become independent?
> 
> The architecture of 8250_pci.c is good for PCIE device with 8250
> compatible serial ports. We want to implement all functions of
> F81504/508/512, but it'll make 8250_pci.c bloated and complex if we
> implement GPIOLIB in 8250_pci.c
> 
> Could I implement GPIOLIB within 8250_pci.c instead of a newer file?

Hm… So, can we stick with separate driver, or you're gonna shake for
each reviewer's comment?

> 
> > How much code was just copied 8250 boilerplate vs. being a new
> > implementation?  The diffstat shows approx 500 lines of new
> > code.  What
> > does that add vs. just copying?
> 
> Due to this IC contains 8250-compatible ports, the most functions is
> copy from fintek section of 8250_pci.c. The differences are highbaud
> rate & GPIOLIB implementations.

I agree with Paul, I think what you have done is to:

1) split out existing code to separate driver (no your changes, but
minimum necessary to this split) — one patch!
2) clean up it (at least I see the old PM code which should be
refactored)
3) enhance functionality accordingly to what you need.

> 
> > 
> > If someone had 8250 (PCI) builtin before, and Fintek stops working,
> > they will most guaranteed bisect to this commit above where you
> > remove
> > support.  That is less than ideal.  We try to avoid code deletions
> > or
> > Kconfig addtions that will be obvious bisect magnets.
> 
> It can be prevented if implements GPIOLIB in 8250_pci.c.

Yeah, see item 1) above.

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