linux-kernel - Re: [PATCH v1 09/12] serial: 8250_lpss: split LPSS driver to separate module

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, 11 Apr 2016 16:09:02 +0300
From:	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:	Peter Hurley <peter@...leysoftware.com>,
	Andy Shevchenko <andy.shevchenko@...il.com>
Cc:	Vinod Koul <vinod.koul@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	dmaengine <dmaengine@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Puustinen, Ismo" <ismo.puustinen@...el.com>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	"linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>
Subject: Re: [PATCH v1 09/12] serial: 8250_lpss: split LPSS driver to
 separate module

On Fri, 2016-04-08 at 15:44 -0700, Peter Hurley wrote:
> On 04/08/2016 01:17 AM, Andy Shevchenko wrote:
> > 
> > On Fri, Apr 8, 2016 at 4:42 AM, Peter Hurley <peter@...leysoftware.c
> > om> wrote:
> > > 
> > > On 04/07/2016 01:37 PM, Andy Shevchenko wrote:
> > > > 
> > > > Intes SoCs, such as Braswell, have DesignWare UART. Split out to
> > > > separate
> > > > module which also will be used for Intel Quark later.
> > > What's the rationale?
> > 1. Not poison 8250_pci with too many quirks.
> > 2. They all use same DMA engine, otherwise we might end up in all
> > possible DMA engines included in one file.
> > 3. All of them are actually DesignWare, so, in the future we might
> > share code between 8250_dw and 8250_lpss.
> Just my opinion, but I like to see the rationale in the changelog.

Agreed. Already did locally.

> 
> > > And this really isn't a split; this patch introduces a number of
> > > significant
> > > changes from the pci version.
> > Some style changes, yes, but "significant"?
> > For example?
> I'm just pointing out the changelog doesn't really match the
> commit. I'm not suggesting necessarily to redo the series, but just
> more
> adequately reflect the change. See below.


> +struct lpss8250_board {
> > > > +     unsigned long freq;
> > > > +     unsigned int base_baud;
> > > > +     int (*setup)(struct lpss8250 *, struct uart_port *p);
> > > > +};
> > > New concept.

> > > +struct lpss8250 {
> > > > +     int line;
> > > > +     struct lpss8250_board *board;
> > > > +
> > > > +     /* DMA parameters */
> > > > +     struct uart_8250_dma dma;
> > > > +     struct dw_dma_slave dma_param;
> > > > +     u8 dma_maxburst;
> > > > +};

> > > > +static int byt_serial_setup(struct lpss8250 *lpss, struct
> > > > uart_port *port)
> > > This would have been much easier to review if you had simply moved
> > > it here
> > > and done the rework in a follow-on patch.
> > I didn't quite get this one.
> Well, just comparing byt_serial_setup() from the two versions:
> 1) dma setup is in a completely separate function
> 2) the tx & rx dma parameters are folded together
> 3) the port setup is split out
> 4) introduction of struct lpss8250
> ...


> 
> > 
> > How series should look like?
> I would have just moved byt_serial_setup() without any of the other
> changes
> except perhaps replacing pciserial_board with lpss8250_board, and
> then made the other changes on top before the Quark patches.
> 
> There is no changelog describing the purpose of struct lpss8250_board,
> or
> struct lpss8250. Or of the dma changes. Or why dma_maxburst was
> parameterized.
> ...
> 
> Naturally, I can figure all of that out on my own, but it would have
> been
> better to read your reasoning.
> 
> It looks alot of work to split out now, so I guess what's done is
> done, and I'll
> just review this *really* carefully. But imagine if you hadn't wrote
> it, and
> were reviewing this: it's very difficult to mentally separate out the
> changes
> and keep track of them while reviewing. Side-by-side diff is nearly
> useless...
> 

I sent new version, please, review.

> > > > 
> > > > +             ret = lpss->board->setup(lpss, &uart.port);
> > > > +             if (ret)
> > > > +                     return ret;
> > > > +     }
> > > > +
> > > > +     ret = lpss8250_dma_setup(lpss, &uart);
> > > > +     if (ret)
> > > > +             return ret;
> > > I would fold this call into board setup which avoids the
> > > ugliness when this error pathway is reworked in the
> > > follow-on patches.
> > Each of them?
> I'm assuming there's just the two: byt_serial_setup()
> and qrk_serial_setup()?

For now yes.

> 
> Did I overlook something? Perhaps I missed some design goal?

Nope. 
But I still prefer to have separate _dma_setup() helper. I didn't see
too much ugliness in the next patches.

> > > > +     .probe          = lpss8250_probe,
> > > > +     .remove         = lpss8250_remove,
> > > No power management?
> > PCI does the trick, no *special* power management treatment
> > required, yes.
> I realized that later this am; sorry about that.
> [Maybe just put a small note in the changelog about that though?]

OK.

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