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

From: Johan Hovold <johan@kernel.org>
To: Jiri Slaby <jirislaby@kernel.org>
Cc: gregkh@linuxfoundation.org,
	"Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>,
	linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
	"Tobias Klauser" <tklauser@distanz.ch>,
	"Richard Genoud" <richard.genoud@gmail.com>,
	"Nicolas Ferre" <nicolas.ferre@microchip.com>,
	"Alexandre Belloni" <alexandre.belloni@bootlin.com>,
	"Claudiu Beznea" <claudiu.beznea@microchip.com>,
	"Vladimir Zapolskiy" <vz@mleia.com>,
	"Liviu Dudau" <liviu.dudau@arm.com>,
	"Sudeep Holla" <sudeep.holla@arm.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Shawn Guo" <shawnguo@kernel.org>,
	"Sascha Hauer" <s.hauer@pengutronix.de>,
	"Pengutronix Kernel Team" <kernel@pengutronix.de>,
	"Fabio Estevam" <festevam@gmail.com>,
	"NXP Linux Team" <linux-imx@nxp.com>,
	"Andreas Färber" <afaerber@suse.de>,
	"Manivannan Sadhasivam" <mani@kernel.org>,
	"Russell King" <linux@armlinux.org.uk>,
	"Florian Fainelli" <f.fainelli@gmail.com>,
	bcm-kernel-feedback-list@broadcom.com,
	"Pali Rohár" <pali@kernel.org>,
	"Kevin Cernekee" <cernekee@gmail.com>,
	"Palmer Dabbelt" <palmer@dabbelt.com>,
	"Paul Walmsley" <paul.walmsley@sifive.com>,
	"Orson Zhai" <orsonzhai@gmail.com>,
	"Baolin Wang" <baolin.wang7@gmail.com>,
	"Chunyan Zhang" <zhang.lyra@gmail.com>,
	"Patrice Chotard" <patrice.chotard@foss.st.com>,
	linux-riscv@lists.infradead.org
Subject: Re: [PATCH v3 0/4] tty: TX helpers
Date: Fri, 9 Sep 2022 14:13:12 +0200	[thread overview]
Message-ID: <Yxst2PGeiQN+dL9e@hovoldconsulting.com> (raw)
In-Reply-To: <dec6d5c4-45b7-f087-95f4-bf1dae9e9d27@kernel.org>

On Wed, Sep 07, 2022 at 09:19:25AM +0200, Jiri Slaby wrote:
> On 06. 09. 22, 13:30, Johan Hovold wrote:
> > On Tue, Sep 06, 2022 at 12:48:01PM +0200, Jiri Slaby wrote:
> >> This series introduces DEFINE_UART_PORT_TX_HELPER +
> >> DEFINE_UART_PORT_TX_HELPER_LIMITED TX helpers. See PATCH 2/4 for the
> >> details. Comments welcome.
> >>
> >> Then it switches drivers to use them. First, to
> >> DEFINE_UART_PORT_TX_HELPER() in 3/4 and then
> >> DEFINE_UART_PORT_TX_HELPER_LIMITED() in 4/4.
> >>
> >> The diffstat of patches 3+4 is as follows:
> >>   26 files changed, 191 insertions(+), 823 deletions(-)
> >> which appears to be nice.
> > 
> > Not really. This is horrid. Quality can't be measured in LoC (only).
> > 
> > The resulting code is unreadable. And for no good reason.
> 
> IMO, it's much more readable than the original ~ 30 various (and buggy 
> -- see Ilpo's fixes) copies of this code. Apart from that, it makes 
> further rework much easier (I have switch to kfifo in my mind for example).

Sure, but you can't have that at the cost of something that is
unreadable. Arnd's suggestions seems to improve things somewhat in this
respect, though.

> > [ And note that you're "saving" something like 20 lines per driver:
> 
> It's not about saving, it's about deduplicating and unifying.

It was you who brought the diff stat that up and I put this in
parentheses for a reason.

> > 	 12 files changed, 84 insertions(+), 349 deletions(-)
> > ]
> > 
> > NAK
> 
> I'd love to come up with something nicer. That would be a function in 
> serial-core calling hooks like I had [1] for example. But provided all 
> those CPU workarounds/thunks, it'd be quite expensive to call two 
> functions per character.

Yeah, that was even worse so no need to reiterate that.

> Or creating a static inline (having ± the macro content) and the hooks 
> as parameters and hope for optimizations to eliminate thunks (also 
> suggested in the past [1]).
> 
> [1] https://lore.kernel.org/all/20220411105405.9519-1-jslaby@suse.cz/

If that works then that should improve readability as well.

Johan