Re: [RFC] [PATCH 3/3] drivers: spi: spi.c: Don't use the message queue if possible in spi_sync

From: Mark Brown <broonie@kernel.org>
To: David Jander <david@protonic.nl>
Cc: linux-spi@vger.kernel.org, Andrew Lunn <andrew@lunn.ch>,
	Marc Kleine-Budde <mkl@pengutronix.de>,
	Oleksij Rempel <o.rempel@pengutronix.de>
Subject: Re: [RFC] [PATCH 3/3] drivers: spi: spi.c: Don't use the message queue if possible in spi_sync
Date: Fri, 10 Jun 2022 19:17:22 +0100	[thread overview]
Message-ID: <YqOKsumo2aXgCvFB@sirena.org.uk> (raw)
In-Reply-To: <YqNKCegdejr522lH@sirena.org.uk>

[-- Attachment #1: Type: text/plain, Size: 2420 bytes --]

On Fri, Jun 10, 2022 at 02:41:34PM +0100, Mark Brown wrote:
> On Fri, Jun 10, 2022 at 09:27:53AM +0200, David Jander wrote:
> > Mark Brown <broonie@kernel.org> wrote:

> > Ok, I first thought that this wouldn't be possible without taking the
> > necessary spinlock, but looking a little closer, I think I understand now.
> > One question to confirm I understand the code correctly:
> > An SPI driver that implements its own transfer_one_message() is required to
> > _always_ call spi_finalize_current_message() _before_ returning, right?

> Yes, it should.

Sorry, my mistake - I misremembered how that worked.  They might return
without having completed the message since the message pump is a
workqueue so it'll just go idle, spi_sync() will work because the caller
will block on the completion in the message.  It's cur_msg that's
stopping any new work being scheduled.  I was confusing this with the
handling of individual transfers, it's been a while since I got deep
into this.

The bit about allowing us to finalise in any context still applies - the
goal with that interface is to avoid the need to context switch back to
the message pump to report that the message completed, and ideally
immediately start work on the next message if the controller can cope
with that.

> > If this is a guarantee and we take the io_mutex at the beginning of
> > __spi_pump_messages(), then ctlr->cur_msg is only manipulated with the
> > io_mutex held, and that would make it safe to be used in the sync path, which
> > is also behind the io_mutex.
> > Would appreciate if you could confirm this, just to be sure I understand the
> > code correctly.

> I think that should work.  If there's something missed it should be
> possible to make things work that way.

Can we move the cleanup of cur_msg out of spi_finalize_current_message()
and into the context holding the io_mutex with that blocking on a
completion?  Don't know what the overhead of that is like, I think it
should be fine for the case where we don't actually block on the
completion and it shouldn't be any worse in the case where we're
completing in the interrupt.  Also the kthread_queue_work() that's in
there could be moved out to only the queued case since with your new
approach for spi_sync() we'll idle the hardware in the calling context
and don't need to schedule the thread at all, that should save some
overhead.

Sorry about misremembering this bit.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]