spidev and the use of spi_async instead of spi_sync

spidev and the use of spi_async instead of spi_sync

[Martin Sperl]

With the 4.0 kernel there has been a nice improvement for
spi_sync where, when there is nothing in the queue the message
pump will get executed in the context of the caller.

This means reduction in the number of context switches and more,
which is nice.

Unfortunately this does not impact spidev, as it (re)implements
spi_sync on its own.

Here the code as of now:
/*
 * We can't use the standard synchronous wrappers for file I/O; we
 * need to protect against async removal of the underlying spi_device.
 */
static void spidev_complete(void *arg)
{
        complete(arg);
}

static ssize_t
spidev_sync(struct spidev_data *spidev, struct spi_message *message)
{
        DECLARE_COMPLETION_ONSTACK(done);
        int status;

        message->complete = spidev_complete;
        message->context = &done;

        spin_lock_irq(&spidev->spi_lock);
        if (spidev->spi == NULL)
                status = -ESHUTDOWN;
        else
                status = spi_async(spidev->spi, message);
        spin_unlock_irq(&spidev->spi_lock);

        if (status == 0) {
                wait_for_completion(&done);
                status = message->status;
                if (status == 0)
                        status = message->actual_length;
        }
        return status;
}

As this code is from 2008 the situation may be different
now and we can use spi_sync instead?

Maybe a patch along the following lines:
static ssize_t
spidev_sync(struct spidev_data *spidev, struct spi_message *message)
{
        DECLARE_COMPLETION_ONSTACK(done);
        int status;
	struct spi_device *spi;

        spin_lock_irq(&spidev->spi_lock);
	spi = spidev->spi;
        spin_unlock_irq(&spidev->spi_lock);

	if (!spi)
		return -ESHUTDOWN;
	
	status = spi_sync(spi, message);

	if (!status)
		status = message->actual_length;

        return status;
}

would be a working solution, but then I am unsure about the reason why
this locking is in place.

So I just want to point out that this may be a candidate for
improvement.

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: spidev and the use of spi_async instead of spi_sync

[Mark Brown]

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

On Wed, Apr 22, 2015 at 05:55:44PM +0200, Martin Sperl wrote:

> As this code is from 2008 the situation may be different
> now and we can use spi_sync instead?

Yes, that seems sensible.  I can't think of any reason why it shouldn't
have done that in the first place.  For things like this please just
provide a patch directly, it's much quicker and easier.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH] spi: spidev: use spi_sync instead of spi_async

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

This has the benefit that the "optimization" of the framework in regards
to spi_sync will also benefit spidev users directly and allow running
spi transfers without a necessary context-switch to message-pump.

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spidev.c |   30 +++++++++---------------------
 1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index 92c909e..b00ae96 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -95,37 +95,25 @@ MODULE_PARM_DESC(bufsiz, "data bytes in biggest supported SPI message");
 
 /*-------------------------------------------------------------------------*/
 
-/*
- * We can't use the standard synchronous wrappers for file I/O; we
- * need to protect against async removal of the underlying spi_device.
- */
-static void spidev_complete(void *arg)
-{
-	complete(arg);
-}
-
 static ssize_t
 spidev_sync(struct spidev_data *spidev, struct spi_message *message)
 {
 	DECLARE_COMPLETION_ONSTACK(done);
 	int status;
-
-	message->complete = spidev_complete;
-	message->context = &done;
+	struct spi_device *spi;
 
 	spin_lock_irq(&spidev->spi_lock);
-	if (spidev->spi == NULL)
+	spi = spidev->spi;
+	spin_unlock_irq(&spidev->spi_lock);
+
+	if (spi == NULL)
 		status = -ESHUTDOWN;
 	else
-		status = spi_async(spidev->spi, message);
-	spin_unlock_irq(&spidev->spi_lock);
+		status = spi_sync(spi, message);
+
+	if (status == 0)
+		status = message->actual_length;
 
-	if (status == 0) {
-		wait_for_completion(&done);
-		status = message->status;
-		if (status == 0)
-			status = message->actual_length;
-	}
 	return status;
 }
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: spidev: use spi_sync instead of spi_async

[Mark Brown]

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

On Thu, Apr 23, 2015 at 07:56:01AM +0000, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> 
> This has the benefit that the "optimization" of the framework in regards
> to spi_sync will also benefit spidev users directly and allow running
> spi transfers without a necessary context-switch to message-pump.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]