[PATCH 0/2] spi: Add support for non-blocking synchronous transfers

[PATCH 0/2] spi: Add support for non-blocking synchronous transfers

[Balaji Rao]

Hi,

During the course of development of an accelerometer driver, we saw the
necessity to execute spi transfers synchronously within an interrupt handler.
When using a workqueue instead, we observed a huge number of overruns with very
high cpu utlization, which is unacceptable.

This series adds a new interface for this and modifies no existing ones.

[PATCH 1/2] - Adds synchronous non-blocking transfer support to spi core
[PATCH 2/2] - Implements this support in spi_bitbang.c


Balaji Rao (2):
      spi_bitbang: Add support for non-blocking synchronous transfers
      spi: Add support for non-blocking synchronous transfers


 drivers/spi/spi_bitbang.c       |  227 +++++++++++++++++++++------------------
 include/linux/spi/spi.h         |   31 +++++
 include/linux/spi/spi_bitbang.h |    5 +
 3 files changed, 156 insertions(+), 107 deletions(-)


--
Balaji Rao

[PATCH 1/2] spi: Add support for non-blocking synchronous transfers

[Balaji Rao]

A new function namely 'transfer_sync' is added to struct spi_master.
A function 'spi_non_blocking_transfer' is introduced , along the lines
of spi_sync and spi_async.

Signed-off-by: Balaji Rao <balajirrao@openmoko.org>
Cc: Andy Green <andy@openmoko.com>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: spi-devel-general@lists.sourceforge.net
---
 include/linux/spi/spi.h |   31 +++++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 68bb1c5..4466021 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -264,6 +264,13 @@ struct spi_master {
 	int			(*transfer)(struct spi_device *spi,
 						struct spi_message *mesg);
 
+	/*
+	 * Synchronous non blocking transfer function. Should guarantee
+	 * data availability when it returns.
+	 */
+	int			(*transfer_sync)(struct spi_device *spi,
+						struct spi_message *mesg);
+
 	/* called on release() to free memory provided by spi_master */
 	void			(*cleanup)(struct spi_device *spi);
 };
@@ -573,6 +580,30 @@ spi_async(struct spi_device *spi, struct spi_message *message)
 	return spi->master->transfer(spi, message);
 }
 
+/**
+ * spi_non_blocking_transfer - Synchronous, non blocking transfer
+ * @spi: device with which data will be exchanged
+ * @message: describes the data transfers with optional completion handlers
+ * Context: any (irqs may be blocked, etc)
+ *
+ * Data is guaranteed to be written or read when this function returns.
+ * The completion callback in spi_message is optional.
+ *
+ * Note : This may not be supported by all spi masters.
+ */
+
+static inline int
+spi_non_blocking_transfer(struct spi_device *spi, struct spi_message *message)
+{
+	if (unlikely(!spi->master->transfer_sync)) {
+		dev_err(&spi->master->dev,
+				"non-blocking transfers not supported\n");
+		return -EIO;
+	}
+
+	return spi->master->transfer_sync(spi, message);
+}
+
 /*---------------------------------------------------------------------------*/
 
 /* All these synchronous SPI transfer routines are utilities layered

[PATCH 2/2] spi_bitbang: Add support for non-blocking synchronous transfers

[Balaji Rao]

The synchronous transfer code in bitbang_work workqueue function is
refactored into a spearate function which optionally becomes the master's
transfer_sync method.

Some initial part of the work was done by Simon Kagstrom.

Signed-off-by: Balaji Rao <balajirrao@openmoko.org>
Signed-off-by: Simon Kagstrom <simon.kagstrom@gmail.com>
Cc: Andy Green <andy@openmoko.com>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: spi-devel-general@lists.sourceforge.net
---
 drivers/spi/spi_bitbang.c       |  227 +++++++++++++++++++++------------------
 include/linux/spi/spi_bitbang.h |    5 +
 2 files changed, 125 insertions(+), 107 deletions(-)

diff --git a/drivers/spi/spi_bitbang.c b/drivers/spi/spi_bitbang.c
index 85e61f4..39dd736 100644
--- a/drivers/spi/spi_bitbang.c
+++ b/drivers/spi/spi_bitbang.c
@@ -264,130 +264,140 @@ static int spi_bitbang_bufs(struct spi_device *spi, struct spi_transfer *t)
  * Drivers can provide word-at-a-time i/o primitives, or provide
  * transfer-at-a-time ones to leverage dma or fifo hardware.
  */
-static void bitbang_work(struct work_struct *work)
-{
-	struct spi_bitbang	*bitbang =
-		container_of(work, struct spi_bitbang, work);
-	unsigned long		flags;
-
-	spin_lock_irqsave(&bitbang->lock, flags);
-	bitbang->busy = 1;
-	while (!list_empty(&bitbang->queue)) {
-		struct spi_message	*m;
-		struct spi_device	*spi;
-		unsigned		nsecs;
-		struct spi_transfer	*t = NULL;
-		unsigned		tmp;
-		unsigned		cs_change;
-		int			status;
-		int			(*setup_transfer)(struct spi_device *,
-						struct spi_transfer *);
 
-		m = container_of(bitbang->queue.next, struct spi_message,
-				queue);
-		list_del_init(&m->queue);
-		spin_unlock_irqrestore(&bitbang->lock, flags);
+/* Synchronous non blocking transfer */
+int
+spi_bitbang_transfer_sync(struct spi_device *spi, struct spi_message *m)
+{
+	struct spi_bitbang *bitbang = spi_master_get_devdata(spi->master);
+	struct spi_transfer *t;
+	unsigned long flags;
+	int cs_change = 1;
+	int status;
+	int nsecs;
+	int (*setup_transfer)(struct spi_device *, struct spi_transfer *);
+
+	/* FIXME this is made-up ... the correct value is known to
+	 * word-at-a-time bitbang code, and presumably chipselect()
+	 * should enforce these requirements too?
+	 */
+	nsecs = 100;
+	cs_change = 1;
+	status = 0;
+	setup_transfer = NULL;
+
+	local_irq_save(flags);
+	list_for_each_entry(t, &m->transfers, transfer_list) {
+		/* override or restore speed and wordsize */
+		if (t->speed_hz || t->bits_per_word) {
+			setup_transfer = bitbang->setup_transfer;
+			if (!setup_transfer) {
+				status = -ENOPROTOOPT;
+				break;
+			}
+		}
+		if (setup_transfer) {
+			status = setup_transfer(spi, t);
+			if (status < 0)
+				break;
+		}
 
-		/* FIXME this is made-up ... the correct value is known to
-		 * word-at-a-time bitbang code, and presumably chipselect()
-		 * should enforce these requirements too?
+		/* set up default clock polarity, and activate chip;
+		 * this implicitly updates clock and spi modes as
+		 * previously recorded for this device via setup().
+		 * (and also deselects any other chip that might be
+		 * selected ...)
 		 */
-		nsecs = 100;
-
-		spi = m->spi;
-		tmp = 0;
-		cs_change = 1;
-		status = 0;
-		setup_transfer = NULL;
 
-		list_for_each_entry (t, &m->transfers, transfer_list) {
-
-			/* override or restore speed and wordsize */
-			if (t->speed_hz || t->bits_per_word) {
-				setup_transfer = bitbang->setup_transfer;
-				if (!setup_transfer) {
-					status = -ENOPROTOOPT;
-					break;
-				}
-			}
-			if (setup_transfer) {
-				status = setup_transfer(spi, t);
-				if (status < 0)
-					break;
-			}
+		if (cs_change) {
+			bitbang->chipselect(spi, BITBANG_CS_ACTIVE);
+			ndelay(nsecs);
+		}
 
-			/* set up default clock polarity, and activate chip;
-			 * this implicitly updates clock and spi modes as
-			 * previously recorded for this device via setup().
-			 * (and also deselects any other chip that might be
-			 * selected ...)
-			 */
-			if (cs_change) {
-				bitbang->chipselect(spi, BITBANG_CS_ACTIVE);
-				ndelay(nsecs);
-			}
-			cs_change = t->cs_change;
-			if (!t->tx_buf && !t->rx_buf && t->len) {
-				status = -EINVAL;
-				break;
-			}
+		cs_change = t->cs_change;
+		if (!t->tx_buf && !t->rx_buf && t->len) {
+			status = -EINVAL;
+			break;
+		}
 
-			/* transfer data.  the lower level code handles any
-			 * new dma mappings it needs. our caller always gave
-			 * us dma-safe buffers.
+		/* transfer data.  the lower level code handles any
+		 * new dma mappings it needs. our caller always gave
+		 * us dma-safe buffers.
+		 */
+		if (t->len) {
+			/* REVISIT dma API still needs a designated
+			 * DMA_ADDR_INVALID; ~0 might be better.
 			 */
-			if (t->len) {
-				/* REVISIT dma API still needs a designated
-				 * DMA_ADDR_INVALID; ~0 might be better.
-				 */
-				if (!m->is_dma_mapped)
-					t->rx_dma = t->tx_dma = 0;
-				status = bitbang->txrx_bufs(spi, t);
-			}
-			if (status > 0)
-				m->actual_length += status;
-			if (status != t->len) {
-				/* always report some kind of error */
-				if (status >= 0)
-					status = -EREMOTEIO;
-				break;
-			}
-			status = 0;
+			if (!m->is_dma_mapped)
+				t->rx_dma = t->tx_dma = 0;
+			status = bitbang->txrx_bufs(spi, t);
+		}
 
+		if (status > 0)
+			m->actual_length += status;
+		if (status != t->len) {
+			/* always report some kind of error */
+			if (status >= 0)
+				status = -EREMOTEIO;
+			break;
+		}
+		status = 0;
 			/* protocol tweaks before next transfer */
-			if (t->delay_usecs)
-				udelay(t->delay_usecs);
-
+		if (t->delay_usecs)
+			udelay(t->delay_usecs);
 			if (!cs_change)
 				continue;
-			if (t->transfer_list.next == &m->transfers)
-				break;
-
+		if (t->transfer_list.next == &m->transfers)
+			break;
 			/* sometimes a short mid-message deselect of the chip
-			 * may be needed to terminate a mode or command
-			 */
-			ndelay(nsecs);
-			bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
-			ndelay(nsecs);
-		}
+		 * may be needed to terminate a mode or command
+		 */
+		ndelay(nsecs);
+		bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
+		ndelay(nsecs);
+	}
 
-		m->status = status;
+	m->status = status;
+	if (m->complete)
 		m->complete(m->context);
 
-		/* restore speed and wordsize */
-		if (setup_transfer)
-			setup_transfer(spi, NULL);
+	/* restore speed and wordsize */
+	if (setup_transfer)
+		setup_transfer(spi, NULL);
 
-		/* normally deactivate chipselect ... unless no error and
-		 * cs_change has hinted that the next message will probably
-		 * be for this chip too.
-		 */
-		if (!(status == 0 && cs_change)) {
-			ndelay(nsecs);
-			bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
-			ndelay(nsecs);
-		}
+	/* normally deactivate chipselect ... unless no error and
+	 * cs_change has hinted that the next message will probably
+	 * be for this chip too.
+	 */
+	if (!(status == 0 && cs_change)) {
+		ndelay(nsecs);
+		bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
+		ndelay(nsecs);
+	}
 
+	local_irq_restore(flags);
+
+	return status;
+}
+EXPORT_SYMBOL_GPL(spi_bitbang_transfer_sync);
+
+static void bitbang_work(struct work_struct *work)
+{
+	struct spi_bitbang	*bitbang =
+		container_of(work, struct spi_bitbang, work);
+	unsigned long		flags;
+
+	spin_lock_irqsave(&bitbang->lock, flags);
+	bitbang->busy = 1;
+	while (!list_empty(&bitbang->queue)) {
+		struct spi_message	*m;
+
+		m = container_of(bitbang->queue.next, struct spi_message,
+				queue);
+		list_del_init(&m->queue);
+
+		spin_unlock_irqrestore(&bitbang->lock, flags);
+		spi_bitbang_transfer_sync(m->spi, m);
 		spin_lock_irqsave(&bitbang->lock, flags);
 	}
 	bitbang->busy = 0;
@@ -459,6 +469,9 @@ int spi_bitbang_start(struct spi_bitbang *bitbang)
 
 	if (!bitbang->master->transfer)
 		bitbang->master->transfer = spi_bitbang_transfer;
+	if (!bitbang->master->transfer_sync && bitbang->non_blocking_transfer)
+		bitbang->master->transfer_sync = spi_bitbang_transfer_sync;
+
 	if (!bitbang->txrx_bufs) {
 		bitbang->use_dma = 0;
 		bitbang->txrx_bufs = spi_bitbang_bufs;
diff --git a/include/linux/spi/spi_bitbang.h b/include/linux/spi/spi_bitbang.h
index eed4254..b99ed8d 100644
--- a/include/linux/spi/spi_bitbang.h
+++ b/include/linux/spi/spi_bitbang.h
@@ -31,6 +31,9 @@ struct spi_bitbang {
 	u8			use_dma;
 	u8			flags;		/* extra spi->mode support */
 
+	/* Support for synchronous non blocking transfers */
+	int 			non_blocking_transfer;
+
 	struct spi_master	*master;
 
 	/* setup_transfer() changes clock and/or wordsize to match settings
@@ -60,6 +63,8 @@ struct spi_bitbang {
 extern int spi_bitbang_setup(struct spi_device *spi);
 extern void spi_bitbang_cleanup(struct spi_device *spi);
 extern int spi_bitbang_transfer(struct spi_device *spi, struct spi_message *m);
+extern int spi_bitbang_transfer_sync(struct spi_device *spi,
+				      struct spi_message *m);
 extern int spi_bitbang_setup_transfer(struct spi_device *spi,
 				      struct spi_transfer *t);
 

Re: [PATCH 2/2] spi_bitbang: Add support for non-blocking synchronous transfers

[Simon Kagstrom]

Thanks for taking this up Balaji!

In the comment below I'm assuming the Openmoko accelerometer use of
this functionality, which would include doing SPI transfers in the
interrupt handler.

On Sat, 28 Feb 2009 13:41:17 +0530
Balaji Rao <balajirrao@openmoko.org> wrote:

> +/* Synchronous non blocking transfer */
> +int
> +spi_bitbang_transfer_sync(struct spi_device *spi, struct spi_message
> *m) +{
> [...]
> +		if (setup_transfer) {
> +			status = setup_transfer(spi, t);
> [...]
> +			if (!m->is_dma_mapped)
> +				t->rx_dma = t->tx_dma = 0;
> +			status = bitbang->txrx_bufs(spi, t);

Another thing that we'd need to take care of is that the stuff being
called from the synhronous transfer is actually callable from interrupt
context. Looking at txrx_bufs for s3c24xx (s3c24xx_spi_txrx), it's
using a wait_for_completion which is in turn completed by the s3c24xx
SPI interrupt handler.

Without trying, I'm guessing that that won't be possible from interrupt
context.

// Simon

Re: [PATCH 2/2] spi_bitbang: Add support for non-blocking synchronous transfers

[Balaji Rao]

On Sat, Feb 28, 2009 at 10:09:14AM +0100, Simon Kagstrom wrote:

Hi Simon,

> Thanks for taking this up Balaji!


You are welcome.
 
> In the comment below I'm assuming the Openmoko accelerometer use of
> this functionality, which would include doing SPI transfers in the
> interrupt handler.
> 

> 
> > +/* Synchronous non blocking transfer */
> > +int
> > +spi_bitbang_transfer_sync(struct spi_device *spi, struct spi_message
> > *m) +{
> > [...]
> > +		if (setup_transfer) {
> > +			status = setup_transfer(spi, t);
> > [...]
> > +			if (!m->is_dma_mapped)
> > +				t->rx_dma = t->tx_dma = 0;
> > +			status = bitbang->txrx_bufs(spi, t);
> 
> Another thing that we'd need to take care of is that the stuff being
> called from the synhronous transfer is actually callable from interrupt
> context. Looking at txrx_bufs for s3c24xx (s3c24xx_spi_txrx), it's
> using a wait_for_completion which is in turn completed by the s3c24xx
> SPI interrupt handler.
> 
> Without trying, I'm guessing that that won't be possible from interrupt
> context.
> 

The master is not spi_s3c24xx but spi_s3x24xx_gpio, whose txrx are very
simple code.

Additionally all of this has been tested and found to work. The code,
along with the modified new spi based lis302dl driver is all in
andy-tracking [1].

[1] - git://git.openmoko.org/git/kernel.git andy-tracking

Thanks,
Balaji

Re: [PATCH 2/2] spi_bitbang: Add support for non-blocking synchronous transfers

[Simon Kagstrom]

On Sat, 28 Feb 2009 15:28:48 +0530
Balaji Rao <balajirrao@openmoko.org> wrote:

> The master is not spi_s3c24xx but spi_s3x24xx_gpio, whose txrx are
> very simple code.
> 
> Additionally all of this has been tested and found to work. The code,
> along with the modified new spi based lis302dl driver is all in
> andy-tracking [1].

Oh, I didn't notice that. Nice then.

I see from the git logs that this and some other related patches have
been added now. Another question I have then is about the name: to me 
spi_non_blocking_transfer() sounds like it would do the opposite of
what I guess it does - it would go ahead without blocking on the call.

I guess what the name means is that it will not sleep during the call,
but for pushing it upstream, could it be better to name it something
else? Perhaps spi_sync and then rename the existing API name (which I
think is more than a bit strange), or maybe spi_sync_nowait or
something?

// Simon

Re: [PATCH 2/2] spi_bitbang: Add support for non-blocking synchronous transfers

[Balaji Rao]

On Sat, Feb 28, 2009 at 11:15:24AM +0100, Simon Kagstrom wrote:
> On Sat, 28 Feb 2009 15:28:48 +0530
> Balaji Rao <balajirrao@openmoko.org> wrote:
> 
> > The master is not spi_s3c24xx but spi_s3x24xx_gpio, whose txrx are
> > very simple code.
> > 
> > Additionally all of this has been tested and found to work. The code,
> > along with the modified new spi based lis302dl driver is all in
> > andy-tracking [1].
> 
> Oh, I didn't notice that. Nice then.
> 
> I see from the git logs that this and some other related patches have
> been added now. Another question I have then is about the name: to me 
> spi_non_blocking_transfer() sounds like it would do the opposite of
> what I guess it does - it would go ahead without blocking on the call.
> 

Yes, isn't that what it's supposed to do ? It's going to complete
without putting current to sleep.

> I guess what the name means is that it will not sleep during the call,
> but for pushing it upstream, could it be better to name it something
> else? Perhaps spi_sync and then rename the existing API name (which I
> think is more than a bit strange), or maybe spi_sync_nowait or
> something?

Yes, even I was not terribly happy with 'non_blocking_transfer'.
But I recommend against changing the behaviour of existing functions.
spi_sync_nowait seems good though. 

I hope to get more comments soon. Let's see what other people have to
say.

Thanks,
Balaji

Re: [PATCH 0/2] spi: Add support for non-blocking synchronous transfers

[David Brownell]

Note that $SUBJECT concept is nonsense.
Synchronous calls are by definition blocking ones...


On Saturday 28 February 2009, Balaji Rao wrote:
> During the course of development of an accelerometer driver, we saw the
> necessity to execute spi transfers synchronously within an interrupt handler.

This sounds like a bad design.  How can you know that no other
transfers are going on ... or are queued in front of the transfer
you're requesting?

You'd need to wait for all the other transfers to work their
way through the transfer queue.  There are *much* better things
to do in interrupt handlers.


> When using a workqueue instead, we observed a huge number of overruns
> with very high cpu utlization, which is unacceptable.

Sure, but at least part of that seems to be caused by some
broken design assumptions.

Why are you even trying to touch SPI devices from hardirq
context?  That's never going to be OK; "can't-sleep" contexts
don't mix with "must-sleep" calls.


> This series adds a new interface for this and modifies no existing ones.

NAK on these two patches.

Re: [PATCH 0/2] spi: Add support for non-blocking synchronous transfers

[Balaji Rao]

On Sat, Feb 28, 2009 at 12:33:50PM -0800, David Brownell wrote:
> Note that $SUBJECT concept is nonsense.
> Synchronous calls are by definition blocking ones...
> 
> 

FWIW, it is exactly this that we want to change.

> On Saturday 28 February 2009, Balaji Rao wrote:
> > During the course of development of an accelerometer driver, we saw the
> > necessity to execute spi transfers synchronously within an interrupt handler.
> 
> This sounds like a bad design.  How can you know that no other
> transfers are going on ... or are queued in front of the transfer
> you're requesting?
> 
> You'd need to wait for all the other transfers to work their
> way through the transfer queue.  There are *much* better things
> to do in interrupt handlers.
> 

Please do look at the patches. We *don't* use a transfer queue.
Transfers requested through our proposed function should/will complete the
transfer when it returns without sleeping in between. (Which is the whole
point of this patch).

> 
> > When using a workqueue instead, we observed a huge number of overruns
> > with very high cpu utlization, which is unacceptable.
> 
> Sure, but at least part of that seems to be caused by some
> broken design assumptions.
> 

No, it's not. Read below.

> Why are you even trying to touch SPI devices from hardirq
> context?  That's never going to be OK; "can't-sleep" contexts
> don't mix with "must-sleep" calls.
> 
> 

Accelerometers can produce a huge amount of data and we need to quickly
read them to avoid overruns. Also, scheduling workers for this greatly
increases the number of context switches, unnecessarily.

> > This series adds a new interface for this and modifies no existing ones.
> 
> NAK on these two patches.
> 

Ok, it will be helpful if you please suggest an alternative keeping in
mind the huge amount of data produced by the accelerometer and the need
to read them quickly ?

Thanks,
Balaji

Re: [PATCH 0/2] spi: Add support for non-blocking synchronous transfers

[David Brownell]

On Saturday 28 February 2009, Balaji Rao wrote:
> On Sat, Feb 28, 2009 at 12:33:50PM -0800, David Brownell wrote:
> > Note that $SUBJECT concept is nonsense.
> > Synchronous calls are by definition blocking ones...
> 
> FWIW, it is exactly this that we want to change.

This seems like

#define UP	DOWN
#define GOOD	BAD
#define LEFT	RIGHT
#define 1	2

... etc ...

How can you possibly be synchronous without blocking the
caller?


> > On Saturday 28 February 2009, Balaji Rao wrote:
> > > During the course of development of an accelerometer driver, we saw the
> > > necessity to execute spi transfers synchronously within an interrupt handler.
> > 
> > This sounds like a bad design.  How can you know that no other
> > transfers are going on ... or are queued in front of the transfer
> > you're requesting?
> > 
> > You'd need to wait for all the other transfers to work their
> > way through the transfer queue.  There are *much* better things
> > to do in interrupt handlers.
> > 
> 
> Please do look at the patches. We *don't* use a transfer queue.

Another example of a conceptual bug.  Because SPI is a shared
bus, transfers to some other device may be happening when you
issue a request.  And accordingly, you'd need to wait for that
transfer to complete.  The SPI I/O queue is at least a logical
abstraction for that reality.  I have a hard time imagining any
spi_master driver not having some kind of queue data structure.

Plus, see Documentation/spi/spi-summary:

| SPI requests always go into I/O queues.  Requests for a given SPI device
| are always executed in FIFO order, and complete asynchronously through
| completion callbacks.  There are also some simple synchronous wrappers
| for those calls, including ones for common transaction types like writing
| a command and then reading its response.

Note that the queueing discipline is explicitly unspecified,
except in the context of a given spi_device.  If you want
your spi_master controller to prioritize one device, that's
fine ... but it's not part of the interface used by portable
drivers (it'd be platform-specific).


> Transfers requested through our proposed function should/will complete the
> transfer when it returns without sleeping in between. (Which is the whole
> point of this patch).

So instead of "non-blocking" you mean "non-sleeping".

That leaves un-answered the question of what to do when
the SPI bus is busy performing some other transfer.  I
looked at your [2/2] patch, and saw it ignoring that very
basic issue ... this new call will just poke at the bus,
trashing any transfer that was ongoing.

I hope you understand why that would be bad.  It can
cause data corruption, and in some cases worse (like
hardware damage).

 
> > > When using a workqueue instead, we observed a huge number of overruns
> > > with very high cpu utlization, which is unacceptable.
> > 
> > Sure, but at least part of that seems to be caused by some
> > broken design assumptions.
> > 
> 
> No, it's not. Read below.

I've already identified several broken assumptions you
have made; so "part of that" seems pretty clearly right.

 
> > Why are you even trying to touch SPI devices from hardirq
> > context?  That's never going to be OK; "can't-sleep" contexts
> > don't mix with "must-sleep" calls.
> 
> Accelerometers can produce a huge amount of data and we need to quickly
> read them to avoid overruns. Also, scheduling workers for this greatly
> increases the number of context switches, unnecessarily.

That sounds like a performance issue with the spi_master driver
you're using.  Using the bitbang framework, and worker tasks, is
a good way to get something going quickly.  But if I wanted high
performance, I'd likely use a more traditional driver structure
(with no tasks, IRQ driven, DMA).  Or I might just increase the
priority of the relevant tasks; depends on what bottlenecks turn
up when I measure things.

That might combine with sub-optimal design for your accelerometer
driver or hardware.  Can you keep multiple transfers queued?  Are
you using async messaging intelligently?


> > > This series adds a new interface for this and modifies no existing ones.
> > 
> > NAK on these two patches.
> > 
> 
> Ok, it will be helpful if you please suggest an alternative keeping in
> mind the huge amount of data produced by the accelerometer and the need
> to read them quickly ?

If your spi_master driver isn't using DMA, fix that.  Nothing
else addresses "huge amount of data" well at all.

If some driver -- spi_master, accelerometer, or whatever -- is
introducing context switch delays in critical paths, get rid of
those.  The gap between one DMA transfer and the next can be on
the order of one IRQ latency, using current interfaces, if the
drivers are decently written.

- Dave

Re: [PATCH 0/2] spi: Add support for non-blocking synchronous transfers

[Balaji Rao]

On Sat, Feb 28, 2009 at 03:19:55PM -0800, David Brownell wrote:
> > > > During the course of development of an accelerometer driver, we saw the
> > > > necessity to execute spi transfers synchronously within an interrupt handler.
> > > 
> > > This sounds like a bad design.  How can you know that no other
> > > transfers are going on ... or are queued in front of the transfer
> > > you're requesting?
> > > 
> > > You'd need to wait for all the other transfers to work their
> > > way through the transfer queue.  There are *much* better things
> > > to do in interrupt handlers.
> > > 
> > 
> > Please do look at the patches. We *don't* use a transfer queue.
> 
> Another example of a conceptual bug.  Because SPI is a shared
> bus, transfers to some other device may be happening when you
> issue a request.  And accordingly, you'd need to wait for that
> transfer to complete.  The SPI I/O queue is at least a logical
> abstraction for that reality.  I have a hard time imagining any
> spi_master driver not having some kind of queue data structure.
> 
> Plus, see Documentation/spi/spi-summary:
> 
> | SPI requests always go into I/O queues.  Requests for a given SPI device
> | are always executed in FIFO order, and complete asynchronously through
> | completion callbacks.  There are also some simple synchronous wrappers
> | for those calls, including ones for common transaction types like writing
> | a command and then reading its response.
> 
> Note that the queueing discipline is explicitly unspecified,
> except in the context of a given spi_device.  If you want
> your spi_master controller to prioritize one device, that's
> fine ... but it's not part of the interface used by portable
> drivers (it'd be platform-specific).
> 
> 
> > Transfers requested through our proposed function should/will complete the
> > transfer when it returns without sleeping in between. (Which is the whole
> > point of this patch).
> 
> So instead of "non-blocking" you mean "non-sleeping".
> 
> That leaves un-answered the question of what to do when
> the SPI bus is busy performing some other transfer.  I
> looked at your [2/2] patch, and saw it ignoring that very
> basic issue ... this new call will just poke at the bus,
> trashing any transfer that was ongoing.
> 

We use s3c24xx_gpio as the master, which is a very simple gpio based
bitbang. 

Yes, it is with this intention, interrupts are disabled around the
actual bitbang code, so that it completes without being interrupted.
Doesn't this guarantee atomicity ?


> > > Why are you even trying to touch SPI devices from hardirq
> > > context?  That's never going to be OK; "can't-sleep" contexts
> > > don't mix with "must-sleep" calls.
> > 
> > Accelerometers can produce a huge amount of data and we need to quickly
> > read them to avoid overruns. Also, scheduling workers for this greatly
> > increases the number of context switches, unnecessarily.
> 
> That sounds like a performance issue with the spi_master driver
> you're using.  Using the bitbang framework, and worker tasks, is
> a good way to get something going quickly.  But if I wanted high
> performance, I'd likely use a more traditional driver structure
> (with no tasks, IRQ driven, DMA).  Or I might just increase the
> priority of the relevant tasks; depends on what bottlenecks turn
> up when I measure things.
> 

OK, true. But since the master here is a simple s3c24xx based gpio
bitbang, we can't do DMA, or bitbang is the only way to go.

> That might combine with sub-optimal design for your accelerometer
> driver or hardware.  Can you keep multiple transfers queued?  Are
> you using async messaging intelligently?
> 

No, we can't queue multiple transfers. It would be very helpful if it
were so.

> 
> > > > This series adds a new interface for this and modifies no existing ones.
> > > 
> > > NAK on these two patches.
> > > 
> > 
> > Ok, it will be helpful if you please suggest an alternative keeping in
> > mind the huge amount of data produced by the accelerometer and the need
> > to read them quickly ?
> 
> If your spi_master driver isn't using DMA, fix that.  Nothing
> else addresses "huge amount of data" well at all.
> 
> If some driver -- spi_master, accelerometer, or whatever -- is
> introducing context switch delays in critical paths, get rid of
> those.  The gap between one DMA transfer and the next can be on
> the order of one IRQ latency, using current interfaces, if the
> drivers are decently written.

Since it's a simple gpio bitbang we are using, we cannot do DMA, isn't
it ? Sorry, I'm still not convinced of a way to make it work with
queueable transfers. Do you still say that spi_async is the way to go ?
Please explain.

Thanks a lot for your time.

- Balaji

Re: [PATCH 0/2] spi: Add support for non-blocking synchronous transfers

[David Brownell]

On Saturday 28 February 2009, Balaji Rao wrote:
> > That leaves un-answered the question of what to do when
> > the SPI bus is busy performing some other transfer.  I
> > looked at your [2/2] patch, and saw it ignoring that very
> > basic issue ... this new call will just poke at the bus,
> > trashing any transfer that was ongoing.
> 
> We use s3c24xx_gpio as the master, which is a very simple gpio based
> bitbang. 
> 
> Yes, it is with this intention, interrupts are disabled around the
> actual bitbang code, so that it completes without being interrupted.
> Doesn't this guarantee atomicity ?

Atomicity isn't the issue so much as the fact that if the
bus is in the middle of some transfer to one device,
your patch lets another device trash that transmission.

I don't know how many more times I can say that your
patches introduce DATA CORRUPTION to the system, but
it's surely not many more times.

Re: [PATCH 0/2] spi: Add support for non-blocking synchronous transfers

[Balaji Rao]

On Sun, Mar 01, 2009 at 01:49:19AM -0800, David Brownell wrote:
> On Saturday 28 February 2009, Balaji Rao wrote:
> > > That leaves un-answered the question of what to do when
> > > the SPI bus is busy performing some other transfer.  I
> > > looked at your [2/2] patch, and saw it ignoring that very
> > > basic issue ... this new call will just poke at the bus,
> > > trashing any transfer that was ongoing.
> > 
> > We use s3c24xx_gpio as the master, which is a very simple gpio based
> > bitbang. 
> > 
> > Yes, it is with this intention, interrupts are disabled around the
> > actual bitbang code, so that it completes without being interrupted.
> > Doesn't this guarantee atomicity ?
> 
> Atomicity isn't the issue so much as the fact that if the
> bus is in the middle of some transfer to one device,
> your patch lets another device trash that transmission.
> 
> I don't know how many more times I can say that your
> patches introduce DATA CORRUPTION to the system, but
> it's surely not many more times.

Yes, I get the point now. Sorry for not observing it earlier.

- Balaji

Re: [PATCH 0/2] spi: Add support for non-blocking synchronous transfers

[Andy Green]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Somebody in the thread at some point said:

| Note that $SUBJECT concept is nonsense.
| Synchronous calls are by definition blocking ones...

What's meant here is that it won't sleep you and make you wait for an
asynchronous completion.  So the call itself will perform the bitbang
action before returning.

| On Saturday 28 February 2009, Balaji Rao wrote:
|> During the course of development of an accelerometer driver, we saw the
|> necessity to execute spi transfers synchronously within an interrupt
handler.
|
| This sounds like a bad design.  How can you know that no other
| transfers are going on ... or are queued in front of the transfer
| you're requesting?
|
| You'd need to wait for all the other transfers to work their
| way through the transfer queue.  There are *much* better things
| to do in interrupt handlers.

In our case, the sync and async SPI things are mutually exclusive, you
either talk to your thing with interrupt-lockout protected sync
transfers entirely or use the existing API.  It can be enforced if
that's the concern.

I think it's a little fast to call down the airstrike of "bad design" on
being able to use bitbang SPI inside an ISR.  Clearly, adding this
ability does not replace the existing system and exists parallel but
compatibly with it; but the existing system cannot provide the same
capability as it stands.  With two lis302dl motion sensors in GTA02,
they can spam up to 800 interrupts a second that need servicing each
with a 7-byte bitbang read.  The existing SPI setup in Linux does not
provide a way to deal with that in a CPU-friendly and low latency way
(there's no FIFO in these devices either).

|> When using a workqueue instead, we observed a huge number of overruns
|> with very high cpu utlization, which is unacceptable.
|
| Sure, but at least part of that seems to be caused by some
| broken design assumptions.
|
| Why are you even trying to touch SPI devices from hardirq
| context?  That's never going to be OK; "can't-sleep" contexts
| don't mix with "must-sleep" calls.

With the "sync" API addition it is OK, given the mutually exclusive
aspect of it.

|> This series adds a new interface for this and modifies no existing ones.
|
| NAK on these two patches.

I hope this maybe gives some extra context about why we use this system,
and why it can be an interesting option for others in the same situation.

- -Andy
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iEYEARECAAYFAkmqPdYACgkQOjLpvpq7dMop/ACfUGHL+BilbzlN4E7rACmlC48N
uv0AnjqgzshtVeYdIVz/14OGq4krCn7+
=Qveo
-----END PGP SIGNATURE-----

Re: [PATCH 0/2] spi: Add support for non-blocking synchronous transfers

[David Brownell]

On Saturday 28 February 2009, Andy Green wrote:
> | This sounds like a bad design.  How can you know that no other
> | transfers are going on ... or are queued in front of the transfer
> | you're requesting?
> |
> | You'd need to wait for all the other transfers to work their
> | way through the transfer queue.  There are *much* better things
> | to do in interrupt handlers.
> 
> In our case, the sync and async SPI things are mutually exclusive, you 

Come up with some new terminology for what you're proposing,
instead of trying to repurpose the existing terms.  The
spi_sync() and spi_async() calls already exist, and they
are *NOT* mutually exclusive.

I suggest you talk about "non-sleeping" calls, since that
seems to be close to what you're thinking about.  (Though
it does beg the question of why you're not using the async
calls, if sleeping makes trouble...)


> either talk to your thing with interrupt-lockout protected sync
> transfers entirely or use the existing API.  It can be enforced if
> that's the concern.

The API proposal is a generic one, and the reference
implementation has deep issues as I noted.  At the
level of data corruption.  How would that be "enforced"
generally?


> I think it's a little fast to call down the airstrike of "bad design"
> on being able to use bitbang SPI inside an ISR.

The way to access SPI inside an ISR is spi_async().
No exceptions for bitbang are necessary.

Neither you nor Balaji have mentioned any issue that
prevents using that.  Or, for that matter, that you
even tried using the interface as designed.  You're
defending something that -- so far -- is not defensible;
it introduces data corruption.  I'm not sure what else
to call it but "bad design".  Regardless, the key point
is to call your attention to the fact that you're doing
something quite wrong.  (The nonsensical $SUBJECT was
the first clue...)


> Clearly, adding this 
> ability does not replace the existing system and exists parallel but
> compatibly with it;

No, it's not compatible.  Start by answering the question
I asked above (quoted at the top of this email), and you'll
surely begin to understand.


> but the existing system cannot provide the same 
> capability as it stands.

Erm, the existing system is clearly self-compatible.
I don't think that word means what you think it means.  ;)


> With two lis302dl motion sensors in GTA02, 
> they can spam up to 800 interrupts a second that need servicing each
> with a 7-byte bitbang read.

Hmm, I'm looking at a schematic with a lis302dl hooked up
using I2C ... at 400 KHz max.  So those chips have useful
use cases that don't need much of a data rate at all.

Now, 800 IRQ/sec * 7 bytes * 8 bits/byte == 44800 bits/sec,
which is easy to achieve.  At least, in drivers that are
written sensibly.  (Even if that's 45 Kbit/sec *each*...)


> The existing SPI setup in Linux does not 
> provide a way to deal with that in a CPU-friendly and low latency way
> (there's no FIFO in these devices either).

If you can't get 45 Kbit/second you're doing something
extremely odd.  I've bitbanged SPI at over 2 MHz on
ARM chips less than half the speed of your OpenMoko
hardware.


So to recap:  the $SUBJECT proposal has serious issues;
and simple math suggests that the performance issues you
are seeing are not fundamentally due to the code at or
below the layer being patched.

- Dave