[PATCH] spi: avoid double validation in __spi_sync()

[PATCH] spi: avoid double validation in __spi_sync()

[David Lechner]

The __spi_sync() function calls __spi_validate() early in the function.
Later, it can call spi_async_locked() which calls __spi_validate()
again. __spi_validate() is an expensive function, so we can improve
performance measurably by avoiding calling it twice.

Instead of calling spi_async_locked(), we can call __spi_async() with
the spin lock held.

spi_async_locked() is removed since there are no more callers.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---

I tested this using an ADC driver that does a lot of spi_sync() calls to read
samples. I disabled the no-queue fast path by setting ctlr->must_async so that
the modified code always runs.

With this change I was able to increase the sample rate of the ADC about 6%
from 14.5 kHz to 15.4 kHz.

 drivers/spi/spi.c | 58 +++++------------------------------------------
 1 file changed, 6 insertions(+), 52 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 7a70ef47cdf6..6610aeced765 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -4278,57 +4278,6 @@ int spi_async(struct spi_device *spi, struct spi_message *message)
 }
 EXPORT_SYMBOL_GPL(spi_async);
 
-/**
- * spi_async_locked - version of spi_async with exclusive bus usage
- * @spi: device with which data will be exchanged
- * @message: describes the data transfers, including completion callback
- * Context: any (IRQs may be blocked, etc)
- *
- * This call may be used in_irq and other contexts which can't sleep,
- * as well as from task contexts which can sleep.
- *
- * The completion callback is invoked in a context which can't sleep.
- * Before that invocation, the value of message->status is undefined.
- * When the callback is issued, message->status holds either zero (to
- * indicate complete success) or a negative error code.  After that
- * callback returns, the driver which issued the transfer request may
- * deallocate the associated memory; it's no longer in use by any SPI
- * core or controller driver code.
- *
- * Note that although all messages to a spi_device are handled in
- * FIFO order, messages may go to different devices in other orders.
- * Some device might be higher priority, or have various "hard" access
- * time requirements, for example.
- *
- * On detection of any fault during the transfer, processing of
- * the entire message is aborted, and the device is deselected.
- * Until returning from the associated message completion callback,
- * no other spi_message queued to that device will be processed.
- * (This rule applies equally to all the synchronous transfer calls,
- * which are wrappers around this core asynchronous primitive.)
- *
- * Return: zero on success, else a negative error code.
- */
-static int spi_async_locked(struct spi_device *spi, struct spi_message *message)
-{
-	struct spi_controller *ctlr = spi->controller;
-	int ret;
-	unsigned long flags;
-
-	ret = __spi_validate(spi, message);
-	if (ret != 0)
-		return ret;
-
-	spin_lock_irqsave(&ctlr->bus_lock_spinlock, flags);
-
-	ret = __spi_async(spi, message);
-
-	spin_unlock_irqrestore(&ctlr->bus_lock_spinlock, flags);
-
-	return ret;
-
-}
-
 static void __spi_transfer_message_noqueue(struct spi_controller *ctlr, struct spi_message *msg)
 {
 	bool was_busy;
@@ -4376,6 +4325,7 @@ static void spi_complete(void *arg)
 static int __spi_sync(struct spi_device *spi, struct spi_message *message)
 {
 	DECLARE_COMPLETION_ONSTACK(done);
+	unsigned long flags;
 	int status;
 	struct spi_controller *ctlr = spi->controller;
 
@@ -4419,7 +4369,11 @@ static int __spi_sync(struct spi_device *spi, struct spi_message *message)
 	 */
 	message->complete = spi_complete;
 	message->context = &done;
-	status = spi_async_locked(spi, message);
+
+	spin_lock_irqsave(&ctlr->bus_lock_spinlock, flags);
+	status = __spi_async(spi, message);
+	spin_unlock_irqrestore(&ctlr->bus_lock_spinlock, flags);
+
 	if (status == 0) {
 		wait_for_completion(&done);
 		status = message->status;
-- 
2.43.0

Re: [PATCH] spi: avoid double validation in __spi_sync()

[Mark Brown]

On Thu, 25 Jan 2024 17:47:31 -0600, David Lechner wrote:
> The __spi_sync() function calls __spi_validate() early in the function.
> Later, it can call spi_async_locked() which calls __spi_validate()
> again. __spi_validate() is an expensive function, so we can improve
> performance measurably by avoiding calling it twice.
> 
> Instead of calling spi_async_locked(), we can call __spi_async() with
> the spin lock held.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: avoid double validation in __spi_sync()
      commit: 0da9a5794cfda615668eaefde811e8ef378134fe

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark