[RFC,1/4] spi: reenable sync SPI transfers

Commit Message

Jeff Epler Sept. 14, 2014, 2:45 p.m. UTC

one source of excess latency in hm2_spi / hal_spidev is latency due
to async transfers.  Make the __spi_sync primitive actually synchronous,
rather than building on an asynchronous primitive.
---
 drivers/spi/spi.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

Comments

Mark Brown Sept. 28, 2014, 12:24 p.m. UTC | #1

On Sun, Sep 14, 2014 at 09:45:05AM -0500, Jeff Epler wrote:
> one source of excess latency in hm2_spi / hal_spidev is latency due
> to async transfers.  Make the __spi_sync primitive actually synchronous,
> rather than building on an asynchronous primitive.
> ---

Please submit patches using the process that is documented in 
Documentation/SubmittingPatches, in particular it is *essential* that
you sign off your patches and you need CC maintainers otherwise it's
likely your patch will be missed.

> +        if(!master->cur_msg)
> +                return;
> +

Please also follow the kernel coding style.

>  
> -	status = spi_async_locked(spi, message);
> +        if(master->prepare_transfer_hardware)
> +            status = master->prepare_transfer_hardware(master);
> +        if(status >= 0)
> +            status = master->transfer_one_message(master, message);
> +        if(status >= 0 && master->unprepare_transfer_hardware)
> +            status = master->unprepare_transfer_hardware(master);
>  
>  	if (!bus_locked)
>  		mutex_unlock(&master->bus_lock_mutex);
>  
> -	if (status == 0) {
> -		wait_for_completion(&done);
> -		status = message->status;
> -	}

There's many problems with this - the most critical are that it is broken
for multithreaded use, it's not even trying to do locking so both other
callers and the thread will break, and it's not waiting for the transfer
to complete.

It would be good to do as much as we can inline but doing so needs much
more work than this and needs to consider other aspects of performance -
your patch will also make performance worse in many situations.  My talk
at ELC this year covers a lot of this, the slides should be googleable.

Patch

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 19ee901..55404b5 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -664,10 +664,18 @@  void spi_finalize_current_message(struct spi_master *master)
 	struct spi_message *mesg;
 	unsigned long flags;
 
+        if(!master->cur_msg)
+                return;
+
 	spin_lock_irqsave(&master->queue_lock, flags);
 	mesg = master->cur_msg;
 	master->cur_msg = NULL;
 
+        if(!mesg) {
+                spin_unlock_irqrestore(&master->queue_lock, flags);
+                return;
+        }
+
 	queue_kthread_work(&master->kworker, &master->pump_messages);
 	spin_unlock_irqrestore(&master->queue_lock, flags);
 
@@ -1490,24 +1498,26 @@  static int __spi_sync(struct spi_device *spi, struct spi_message *message,
 		      int bus_locked)
 {
 	DECLARE_COMPLETION_ONSTACK(done);
-	int status;
+	int status = 0;
 	struct spi_master *master = spi->master;
 
 	message->complete = spi_complete;
 	message->context = &done;
+	message->spi = spi;
 
 	if (!bus_locked)
 		mutex_lock(&master->bus_lock_mutex);
 
-	status = spi_async_locked(spi, message);
+        if(master->prepare_transfer_hardware)
+            status = master->prepare_transfer_hardware(master);
+        if(status >= 0)
+            status = master->transfer_one_message(master, message);
+        if(status >= 0 && master->unprepare_transfer_hardware)
+            status = master->unprepare_transfer_hardware(master);
 
 	if (!bus_locked)
 		mutex_unlock(&master->bus_lock_mutex);
 
-	if (status == 0) {
-		wait_for_completion(&done);
-		status = message->status;
-	}
 	message->context = NULL;
 	return status;
 }