linux-kernel - Re: [PATCH v3 0/4] tty: TX helpers

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] [day] [month] [year] [list]

Date:   Fri, 9 Sep 2022 14:41:41 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Jiri Slaby <jirislaby@...nel.org>
Cc:     Arnd Bergmann <arnd@...db.de>,
        Russell King <linux@...linux.org.uk>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
        linux-serial <linux-serial@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Tobias Klauser <tklauser@...tanz.ch>,
        Richard Genoud <richard.genoud@...il.com>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Claudiu Beznea <claudiu.beznea@...rochip.com>,
        Vladimir Zapolskiy <vz@...ia.com>,
        Liviu Dudau <liviu.dudau@....com>,
        Sudeep Holla <sudeep.holla@....com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        NXP Linux Team <linux-imx@....com>,
        Andreas Färber <afaerber@...e.de>,
        Manivannan Sadhasivam <mani@...nel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        bcm-kernel-feedback-list@...adcom.com,
        Pali Rohár <pali@...nel.org>,
        Kevin Cernekee <cernekee@...il.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Orson Zhai <orsonzhai@...il.com>,
        Baolin Wang <baolin.wang7@...il.com>,
        Chunyan Zhang <zhang.lyra@...il.com>,
        Patrice Chotard <patrice.chotard@...s.st.com>,
        linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v3 0/4] tty: TX helpers

On Fri, Sep 09, 2022 at 12:53:04PM +0200, Jiri Slaby wrote:
> On 07. 09. 22, 16:56, Arnd Bergmann wrote:
> > On Wed, Sep 7, 2022, at 3:52 PM, Russell King (Oracle) wrote:
> >> On Wed, Sep 07, 2022 at 02:36:37PM +0200, Greg Kroah-Hartman wrote:
> >>
> >> Of course, it would have been nicer to see the definition of this
> >> macro, because then we can understand what the "ch" argument is to
> >> this macro, and how that relates to the macro argument that is
> >> shown in the example as a writel().
> > 
> > I pulled out the 'ch' variable from the macro to avoid having
> > the macro define local variables that are then passed to the
> > inner expressions.
> 
> Note that I had "port" and "ch" as a part of the macro parameters in 
> [v2], but it didn't help the situation much.
> >> Maybe a more complete example would help clear up the confusion?
> >> Arnd?
> > 
> > Here is a patch on top of the series that would implement the
> > uart_port_tx_helper_limited() and uart_port_tx_helper()
> > macros that can be used directly from drivers in place of defining
> > local functions, with the (alphabetically) first two drivers
> > converted to that.
> 
> If there are no objections, I will push the patches this directorin. I 
> like this more than [v2] or [v3] (the helper macros). Actually, I 
> mentioned this wait_event() style in [v1], but I perhaps simplified the 
> concept too much to completely eliminate the need of a wrapper function. 
> And that made it too complicated/too hard to understand.

This sounds much better. You also had some users that needed some
preamble which could now go in the same function (e.g.
altera_jtaguart_tx_chars()).

> Except I'd drop the "_helper" part from the name. Originally (in [v1]), 
> I had uart_port_tx() and uart_port_tx_limited() functions. In [v2+v3], I 
> added _helper to avoid confusion as we were generating a helpers using 
> the macros. Yes, technically, uart_port_tx() is still a helper, but I 
> think it's superfluous to have it in the name now.

That would also be an in improvement. For the altera example you could
end up with something like:

static void altera_jtaguart_tx_chars(struct altera_jtaguart *pp)
{
	struct uart_port *port = &pp->port;
	unsigned int space;

	space = (readl(port->membase + ALTERA_JTAGUART_CONTROL_REG) &
			ALTERA_JTAGUART_CONTROL_WSPACE_MSK) >>
			ALTERA_JTAGUART_CONTROL_WSPACE_OFF;

	uart_port_tx_chars(port, space,
			   writel(ch, port->membase + ALTERA_JTAGUART_DATA_REG));
}

which would be understandable.

If there are too many arguments to be passed in, then perhaps you can
explore Arnd's (and you own) suggestion to use inline helpers so that
the arguments are named.

Johan