linux-kernel - Re: [PATCH v4 3/4] i2c: imx: use readb_relaxed and writeb

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: <Zv1aVatIlToneign@eichest-laptop>
Date: Wed, 2 Oct 2024 16:36:05 +0200
From: Stefan Eichenberger <eichest@...il.com>
To: Arnd Bergmann <arnd@...db.de>
Cc: o.rempel@...gutronix.de,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Andi Shyti <andi.shyti@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>, Frank Li <Frank.Li@....com>,
	linux-i2c@...r.kernel.org, imx@...ts.linux.dev,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Francesco Dolcini <francesco.dolcini@...adex.com>,
	Stefan Eichenberger <stefan.eichenberger@...adex.com>
Subject: Re: [PATCH v4 3/4] i2c: imx: use readb_relaxed and writeb_relaxed

On Wed, Oct 02, 2024 at 01:36:04PM +0000, Arnd Bergmann wrote:
> On Wed, Oct 2, 2024, at 13:08, Stefan Eichenberger wrote:
> > On Wed, Oct 02, 2024 at 11:51:22AM +0000, Arnd Bergmann wrote:
> >> On Wed, Oct 2, 2024, at 11:19, Stefan Eichenberger wrote:
> >> > From: Stefan Eichenberger <stefan.eichenberger@...adex.com>
> >> >
> >> > Use the relaxed version of readb and writeb to reduce overhead. It is
> >> > safe to use the relaxed version because we either do not rely on dma
> >> > completion, or we use a dma callback to ensure that the dma transfer is
> >> > complete before we continue.
> >> 
> >> I would still consider this a bug in general, you should
> >> never default to the unsafe variants.
> >> 
> >> If there is a codepath that needs the barrierless version,
> >> please add imx_i2c_write_reg_relaxed()/imx_i2c_read_reg_relaxed()
> >> helpers that use those only in the places where it makes
> >> a measurable difference, with a comment that explains
> >> the usage.
> >
> > I added the patch because of the following dicussion:
> > https://lore.kernel.org/linux-i2c/ZpVWXlR6j2i0ZtVQ@lizhi-Precision-Tower-5810/
> >
> > I can't determine if the relaxed version improves performance. The
> > 'normal' version worked well for our use case too. Therefore, dropping
> > the change would be acceptable for us. Another potential solution could
> > be to use the relaxed version only inside the ISR. Would that be an
> > acceptable solution? What is your impression, Frank Li
> > <Frank.Li@....com>?
> 
> I'm pretty sure that Frank meant to use readb_relaxed()/writeb_relaxed()
> inside of the FIFO access loop, not for everything else. This
> makes a lot of sense, since the FIFO read in particular is
> clearly performance sensitive and already serialized by the
> implied control dependency.
> 
> If you can read multiple bytes, the best interface to use
> would in fact be readsb() or possibly readsl() to read
> four bytes with each access.
> 
> It appears that you did not implement the suggestion to
> read the entire FIFO though, so you can probably just skip
> the _relaxed() change entirely.

This makes sense, it appears this was a misunderstanding. If no one
objects, I will drop the patch in the next version. Thank you for the
clarification.

Regards,
Stefan