[PATCH v3 0/4] omap: mailbox: cleanup & simplify

[PATCH v3 0/4] omap: mailbox: cleanup & simplify

[Ohad Ben-Cohen]

This series includes comments from Hiroshi and Tony (thanks!).

Changes since v2:
- add mbox_kfifo_size module parameter sanity checks
- remove (un)likely macros from cold mailbox paths

Thanks,
Ohad.

---
If you want, you can also reach me at < ohadb at ti dot com >.

Ohad Ben-Cohen (4):
  omap: mailbox: convert rwlocks to spinlock
  omap: mailbox cleanup: split MODULE_AUTHOR line
  omap: mailbox: remove (un)likely macros from cold paths
  omap: mailbox: convert block api to kfifo

 arch/arm/mach-omap2/mailbox.c             |    3 +-
 arch/arm/plat-omap/Kconfig                |    9 ++
 arch/arm/plat-omap/include/plat/mailbox.h |    4 +-
 arch/arm/plat-omap/mailbox.c              |  155 ++++++++++++++---------------
 4 files changed, 87 insertions(+), 84 deletions(-)

[PATCH v3 1/4] omap: mailbox: convert rwlocks to spinlock

[Ohad Ben-Cohen]

rwlocks are slower and have potential starvation issues
therefore spinlocks are generally preferred.

see also: http://lwn.net/Articles/364583/

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
Signed-off-by: Kanigeri Hari <h-kanigeri2@ti.com>
---
If you want, you can also reach me at < ohadb at ti dot com >.

 arch/arm/plat-omap/mailbox.c |   30 +++++++++++++++---------------
 1 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index 08a2df7..af3a6ac 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -31,7 +31,7 @@
 
 static struct workqueue_struct *mboxd;
 static struct omap_mbox *mboxes;
-static DEFINE_RWLOCK(mboxes_lock);
+static DEFINE_SPINLOCK(mboxes_lock);
 
 static int mbox_configured;
 
@@ -249,16 +249,16 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
 	struct omap_mbox_queue *mq;
 
 	if (likely(mbox->ops->startup)) {
-		write_lock(&mboxes_lock);
+		spin_lock(&mboxes_lock);
 		if (!mbox_configured)
 			ret = mbox->ops->startup(mbox);
 
 		if (unlikely(ret)) {
-			write_unlock(&mboxes_lock);
+			spin_unlock(&mboxes_lock);
 			return ret;
 		}
 		mbox_configured++;
-		write_unlock(&mboxes_lock);
+		spin_unlock(&mboxes_lock);
 	}
 
 	ret = request_irq(mbox->irq, mbox_interrupt, IRQF_SHARED,
@@ -304,12 +304,12 @@ static void omap_mbox_fini(struct omap_mbox *mbox)
 	free_irq(mbox->irq, mbox);
 
 	if (unlikely(mbox->ops->shutdown)) {
-		write_lock(&mboxes_lock);
+		spin_lock(&mboxes_lock);
 		if (mbox_configured > 0)
 			mbox_configured--;
 		if (!mbox_configured)
 			mbox->ops->shutdown(mbox);
-		write_unlock(&mboxes_lock);
+		spin_unlock(&mboxes_lock);
 	}
 }
 
@@ -330,14 +330,14 @@ struct omap_mbox *omap_mbox_get(const char *name)
 	struct omap_mbox *mbox;
 	int ret;
 
-	read_lock(&mboxes_lock);
+	spin_lock(&mboxes_lock);
 	mbox = *(find_mboxes(name));
 	if (mbox == NULL) {
-		read_unlock(&mboxes_lock);
+		spin_unlock(&mboxes_lock);
 		return ERR_PTR(-ENOENT);
 	}
 
-	read_unlock(&mboxes_lock);
+	spin_unlock(&mboxes_lock);
 
 	ret = omap_mbox_startup(mbox);
 	if (ret)
@@ -363,15 +363,15 @@ int omap_mbox_register(struct device *parent, struct omap_mbox *mbox)
 	if (mbox->next)
 		return -EBUSY;
 
-	write_lock(&mboxes_lock);
+	spin_lock(&mboxes_lock);
 	tmp = find_mboxes(mbox->name);
 	if (*tmp) {
 		ret = -EBUSY;
-		write_unlock(&mboxes_lock);
+		spin_unlock(&mboxes_lock);
 		goto err_find;
 	}
 	*tmp = mbox;
-	write_unlock(&mboxes_lock);
+	spin_unlock(&mboxes_lock);
 
 	return 0;
 
@@ -384,18 +384,18 @@ int omap_mbox_unregister(struct omap_mbox *mbox)
 {
 	struct omap_mbox **tmp;
 
-	write_lock(&mboxes_lock);
+	spin_lock(&mboxes_lock);
 	tmp = &mboxes;
 	while (*tmp) {
 		if (mbox == *tmp) {
 			*tmp = mbox->next;
 			mbox->next = NULL;
-			write_unlock(&mboxes_lock);
+			spin_unlock(&mboxes_lock);
 			return 0;
 		}
 		tmp = &(*tmp)->next;
 	}
-	write_unlock(&mboxes_lock);
+	spin_unlock(&mboxes_lock);
 
 	return -EINVAL;
 }
-- 
1.6.3.3

[PATCH v3 2/4] omap: mailbox cleanup: split MODULE_AUTHOR line

[Ohad Ben-Cohen]

use multiple MODULE_AUTHOR lines for multiple authors

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
If you want, you can also reach me at < ohadb at ti dot com >.

 arch/arm/mach-omap2/mailbox.c |    3 ++-
 arch/arm/plat-omap/mailbox.c  |    3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/mailbox.c b/arch/arm/mach-omap2/mailbox.c
index 318f363..763272c 100644
--- a/arch/arm/mach-omap2/mailbox.c
+++ b/arch/arm/mach-omap2/mailbox.c
@@ -486,5 +486,6 @@ module_exit(omap2_mbox_exit);
 
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("omap mailbox: omap2/3/4 architecture specific functions");
-MODULE_AUTHOR("Hiroshi DOYU <Hiroshi.DOYU@nokia.com>, Paul Mundt");
+MODULE_AUTHOR("Hiroshi DOYU <Hiroshi.DOYU@nokia.com>");
+MODULE_AUTHOR("Paul Mundt");
 MODULE_ALIAS("platform:"DRV_NAME);
diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index af3a6ac..5140efc 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -419,4 +419,5 @@ module_exit(omap_mbox_exit);
 
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("omap mailbox: interrupt driven messaging");
-MODULE_AUTHOR("Toshihiro Kobayashi and Hiroshi DOYU");
+MODULE_AUTHOR("Toshihiro Kobayashi");
+MODULE_AUTHOR("Hiroshi DOYU");
-- 
1.6.3.3

[PATCH v3 3/4] omap: mailbox: remove (un)likely macros from cold paths

[Ohad Ben-Cohen]

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
If you want, you can also reach me at < ohadb at ti dot com >.

 arch/arm/plat-omap/mailbox.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index 5140efc..7c60550 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -248,12 +248,12 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
 	int ret = 0;
 	struct omap_mbox_queue *mq;
 
-	if (likely(mbox->ops->startup)) {
+	if (mbox->ops->startup) {
 		spin_lock(&mboxes_lock);
 		if (!mbox_configured)
 			ret = mbox->ops->startup(mbox);
 
-		if (unlikely(ret)) {
+		if (ret) {
 			spin_unlock(&mboxes_lock);
 			return ret;
 		}
@@ -263,7 +263,7 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
 
 	ret = request_irq(mbox->irq, mbox_interrupt, IRQF_SHARED,
 				mbox->name, mbox);
-	if (unlikely(ret)) {
+	if (ret) {
 		printk(KERN_ERR
 			"failed to register mailbox interrupt:%d\n", ret);
 		goto fail_request_irq;
@@ -290,7 +290,7 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
  fail_alloc_txq:
 	free_irq(mbox->irq, mbox);
  fail_request_irq:
-	if (unlikely(mbox->ops->shutdown))
+	if (mbox->ops->shutdown)
 		mbox->ops->shutdown(mbox);
 
 	return ret;
@@ -303,7 +303,7 @@ static void omap_mbox_fini(struct omap_mbox *mbox)
 
 	free_irq(mbox->irq, mbox);
 
-	if (unlikely(mbox->ops->shutdown)) {
+	if (mbox->ops->shutdown) {
 		spin_lock(&mboxes_lock);
 		if (mbox_configured > 0)
 			mbox_configured--;
-- 
1.6.3.3

[PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Ohad Ben-Cohen]

The underlying buffering implementation of mailbox
is converted from block API to kfifo due to the simplicity
and speed of kfifo.

The default size of the kfifo buffer is set to 256 bytes.
This value is configurable at compile time (via
CONFIG_OMAP_MBOX_KFIFO_SIZE), and can be changed at
runtime (via the mbox_kfifo_size module parameter).

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
---
If you want, you can also reach me at < ohadb at ti dot com >.

 arch/arm/plat-omap/Kconfig                |    9 +++
 arch/arm/plat-omap/include/plat/mailbox.h |    4 +-
 arch/arm/plat-omap/mailbox.c              |  112 +++++++++++++---------------
 3 files changed, 63 insertions(+), 62 deletions(-)

diff --git a/arch/arm/plat-omap/Kconfig b/arch/arm/plat-omap/Kconfig
index 6da796e..f4b0029 100644
--- a/arch/arm/plat-omap/Kconfig
+++ b/arch/arm/plat-omap/Kconfig
@@ -106,6 +106,15 @@ config OMAP_MBOX_FWK
 	  Say Y here if you want to use OMAP Mailbox framework support for
 	  DSP, IVA1.0 and IVA2 in OMAP1/2/3.
 
+config OMAP_MBOX_KFIFO_SIZE
+	int "Mailbox kfifo default buffer size (bytes)"
+	depends on OMAP_MBOX_FWK
+	default 256
+	help
+	  Specify the default size of mailbox's kfifo buffers (bytes).
+	  This can also be changed at runtime (via the mbox_kfifo_size
+	  module parameter).
+
 config OMAP_IOMMU
 	tristate
 
diff --git a/arch/arm/plat-omap/include/plat/mailbox.h b/arch/arm/plat-omap/include/plat/mailbox.h
index 729166b..0c3c4a5 100644
--- a/arch/arm/plat-omap/include/plat/mailbox.h
+++ b/arch/arm/plat-omap/include/plat/mailbox.h
@@ -5,8 +5,8 @@
 
 #include <linux/wait.h>
 #include <linux/workqueue.h>
-#include <linux/blkdev.h>
 #include <linux/interrupt.h>
+#include <linux/kfifo.h>
 
 typedef u32 mbox_msg_t;
 struct omap_mbox;
@@ -42,7 +42,7 @@ struct omap_mbox_ops {
 
 struct omap_mbox_queue {
 	spinlock_t		lock;
-	struct request_queue	*queue;
+	struct kfifo		fifo;
 	struct work_struct	work;
 	struct tasklet_struct	tasklet;
 	int	(*callback)(void *);
diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index 7c60550..908d9d2 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -21,11 +21,14 @@
  *
  */
 
+#include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/interrupt.h>
 #include <linux/device.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
+#include <linux/kfifo.h>
+#include <linux/err.h>
 
 #include <plat/mailbox.h>
 
@@ -35,6 +38,10 @@ static DEFINE_SPINLOCK(mboxes_lock);
 
 static int mbox_configured;
 
+static unsigned int mbox_kfifo_size = CONFIG_OMAP_MBOX_KFIFO_SIZE;
+module_param(mbox_kfifo_size, uint, S_IRUGO);
+MODULE_PARM_DESC(mbox_kfifo_size, "Size of omap's mailbox kfifo (bytes)");
+
 /* Mailbox FIFO handle functions */
 static inline mbox_msg_t mbox_fifo_read(struct omap_mbox *mbox)
 {
@@ -67,7 +74,7 @@ static inline int is_mbox_irq(struct omap_mbox *mbox, omap_mbox_irq_t irq)
 /*
  * message sender
  */
-static int __mbox_msg_send(struct omap_mbox *mbox, mbox_msg_t msg)
+static int __mbox_poll_for_space(struct omap_mbox *mbox)
 {
 	int ret = 0, i = 1000;
 
@@ -78,49 +85,50 @@ static int __mbox_msg_send(struct omap_mbox *mbox, mbox_msg_t msg)
 			return -1;
 		udelay(1);
 	}
-	mbox_fifo_write(mbox, msg);
 	return ret;
 }
 
-
 int omap_mbox_msg_send(struct omap_mbox *mbox, mbox_msg_t msg)
 {
+	struct omap_mbox_queue *mq = mbox->txq;
+	int ret = 0, len;
 
-	struct request *rq;
-	struct request_queue *q = mbox->txq->queue;
+	spin_lock(&mq->lock);
 
-	rq = blk_get_request(q, WRITE, GFP_ATOMIC);
-	if (unlikely(!rq))
-		return -ENOMEM;
+	if (kfifo_avail(&mq->fifo) < sizeof(msg)) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	len = kfifo_in(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
+	WARN_ON(len != sizeof(msg));
 
-	blk_insert_request(q, rq, 0, (void *) msg);
 	tasklet_schedule(&mbox->txq->tasklet);
 
-	return 0;
+out:
+	spin_unlock(&mq->lock);
+	return ret;
 }
 EXPORT_SYMBOL(omap_mbox_msg_send);
 
 static void mbox_tx_tasklet(unsigned long tx_data)
 {
-	int ret;
-	struct request *rq;
 	struct omap_mbox *mbox = (struct omap_mbox *)tx_data;
-	struct request_queue *q = mbox->txq->queue;
-
-	while (1) {
-
-		rq = blk_fetch_request(q);
-
-		if (!rq)
-			break;
+	struct omap_mbox_queue *mq = mbox->txq;
+	mbox_msg_t msg;
+	int ret;
 
-		ret = __mbox_msg_send(mbox, (mbox_msg_t)rq->special);
-		if (ret) {
+	while (kfifo_len(&mq->fifo)) {
+		if (__mbox_poll_for_space(mbox)) {
 			omap_mbox_enable_irq(mbox, IRQ_TX);
-			blk_requeue_request(q, rq);
-			return;
+			break;
 		}
-		blk_end_request_all(rq, 0);
+
+		ret = kfifo_out(&mq->fifo, (unsigned char *)&msg,
+								sizeof(msg));
+		WARN_ON(ret != sizeof(msg));
+
+		mbox_fifo_write(mbox, msg);
 	}
 }
 
@@ -131,36 +139,21 @@ static void mbox_rx_work(struct work_struct *work)
 {
 	struct omap_mbox_queue *mq =
 			container_of(work, struct omap_mbox_queue, work);
-	struct omap_mbox *mbox = mq->queue->queuedata;
-	struct request_queue *q = mbox->rxq->queue;
-	struct request *rq;
 	mbox_msg_t msg;
-	unsigned long flags;
+	int len;
 
-	while (1) {
-		spin_lock_irqsave(q->queue_lock, flags);
-		rq = blk_fetch_request(q);
-		spin_unlock_irqrestore(q->queue_lock, flags);
-		if (!rq)
-			break;
+	while (kfifo_len(&mq->fifo) >= sizeof(msg)) {
+		len = kfifo_out(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
+		WARN_ON(len != sizeof(msg));
 
-		msg = (mbox_msg_t)rq->special;
-		blk_end_request_all(rq, 0);
-		mbox->rxq->callback((void *)msg);
+		if (mq->callback)
+			mq->callback((void *)msg);
 	}
 }
 
 /*
  * Mailbox interrupt handler
  */
-static void mbox_txq_fn(struct request_queue *q)
-{
-}
-
-static void mbox_rxq_fn(struct request_queue *q)
-{
-}
-
 static void __mbox_tx_interrupt(struct omap_mbox *mbox)
 {
 	omap_mbox_disable_irq(mbox, IRQ_TX);
@@ -170,19 +163,19 @@ static void __mbox_tx_interrupt(struct omap_mbox *mbox)
 
 static void __mbox_rx_interrupt(struct omap_mbox *mbox)
 {
-	struct request *rq;
+	struct omap_mbox_queue *mq = mbox->rxq;
 	mbox_msg_t msg;
-	struct request_queue *q = mbox->rxq->queue;
+	int len;
 
 	while (!mbox_fifo_empty(mbox)) {
-		rq = blk_get_request(q, WRITE, GFP_ATOMIC);
-		if (unlikely(!rq))
+		if (unlikely(kfifo_avail(&mq->fifo) < sizeof(msg)))
 			goto nomem;
 
 		msg = mbox_fifo_read(mbox);
 
+		len = kfifo_in(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
+		WARN_ON(len != sizeof(msg));
 
-		blk_insert_request(q, rq, 0, (void *)msg);
 		if (mbox->ops->type == OMAP_MBOX_TYPE1)
 			break;
 	}
@@ -207,11 +200,9 @@ static irqreturn_t mbox_interrupt(int irq, void *p)
 }
 
 static struct omap_mbox_queue *mbox_queue_alloc(struct omap_mbox *mbox,
-					request_fn_proc *proc,
 					void (*work) (struct work_struct *),
 					void (*tasklet)(unsigned long))
 {
-	struct request_queue *q;
 	struct omap_mbox_queue *mq;
 
 	mq = kzalloc(sizeof(struct omap_mbox_queue), GFP_KERNEL);
@@ -220,11 +211,8 @@ static struct omap_mbox_queue *mbox_queue_alloc(struct omap_mbox *mbox,
 
 	spin_lock_init(&mq->lock);
 
-	q = blk_init_queue(proc, &mq->lock);
-	if (!q)
+	if (kfifo_alloc(&mq->fifo, mbox_kfifo_size, GFP_KERNEL))
 		goto error;
-	q->queuedata = mbox;
-	mq->queue = q;
 
 	if (work)
 		INIT_WORK(&mq->work, work);
@@ -239,7 +227,7 @@ error:
 
 static void mbox_queue_free(struct omap_mbox_queue *q)
 {
-	blk_cleanup_queue(q->queue);
+	kfifo_free(&q->fifo);
 	kfree(q);
 }
 
@@ -269,14 +257,14 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
 		goto fail_request_irq;
 	}
 
-	mq = mbox_queue_alloc(mbox, mbox_txq_fn, NULL, mbox_tx_tasklet);
+	mq = mbox_queue_alloc(mbox, NULL, mbox_tx_tasklet);
 	if (!mq) {
 		ret = -ENOMEM;
 		goto fail_alloc_txq;
 	}
 	mbox->txq = mq;
 
-	mq = mbox_queue_alloc(mbox, mbox_rxq_fn, mbox_rx_work, NULL);
+	mq = mbox_queue_alloc(mbox, mbox_rx_work, NULL);
 	if (!mq) {
 		ret = -ENOMEM;
 		goto fail_alloc_rxq;
@@ -407,6 +395,10 @@ static int __init omap_mbox_init(void)
 	if (!mboxd)
 		return -ENOMEM;
 
+	/* kfifo size sanity check: alignment and minimal size */
+	mbox_kfifo_size = ALIGN(mbox_kfifo_size, sizeof(mbox_msg_t));
+	mbox_kfifo_size = max_t(unsigned int, mbox_kfifo_size, sizeof(mbox_msg_t));
+
 	return 0;
 }
 module_init(omap_mbox_init);
-- 
1.6.3.3

Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Deepak Chitriki]

Ohad Ben-Cohen wrote:
> The underlying buffering implementation of mailbox
> is converted from block API to kfifo due to the simplicity
> and speed of kfifo.
>
> The default size of the kfifo buffer is set to 256 bytes.
> This value is configurable at compile time (via
> CONFIG_OMAP_MBOX_KFIFO_SIZE), and can be changed at
> runtime (via the mbox_kfifo_size module parameter).
>
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
> Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
> ---
> If you want, you can also reach me at < ohadb at ti dot com >.
>
>  arch/arm/plat-omap/Kconfig                |    9 +++
>  arch/arm/plat-omap/include/plat/mailbox.h |    4 +-
>  arch/arm/plat-omap/mailbox.c              |  112 +++++++++++++---------------
>  3 files changed, 63 insertions(+), 62 deletions(-)
>
> diff --git a/arch/arm/plat-omap/Kconfig b/arch/arm/plat-omap/Kconfig
> index 6da796e..f4b0029 100644
> --- a/arch/arm/plat-omap/Kconfig
> +++ b/arch/arm/plat-omap/Kconfig
> @@ -106,6 +106,15 @@ config OMAP_MBOX_FWK
>  	  Say Y here if you want to use OMAP Mailbox framework support for
>  	  DSP, IVA1.0 and IVA2 in OMAP1/2/3.
>  
> +config OMAP_MBOX_KFIFO_SIZE
> +	int "Mailbox kfifo default buffer size (bytes)"
> +	depends on OMAP_MBOX_FWK
> +	default 256
> +	help
> +	  Specify the default size of mailbox's kfifo buffers (bytes).
> +	  This can also be changed at runtime (via the mbox_kfifo_size
> +	  module parameter).
> +
>  config OMAP_IOMMU
>  	tristate
>  
> diff --git a/arch/arm/plat-omap/include/plat/mailbox.h b/arch/arm/plat-omap/include/plat/mailbox.h
> index 729166b..0c3c4a5 100644
> --- a/arch/arm/plat-omap/include/plat/mailbox.h
> +++ b/arch/arm/plat-omap/include/plat/mailbox.h
> @@ -5,8 +5,8 @@
>  
>  #include <linux/wait.h>
>  #include <linux/workqueue.h>
> -#include <linux/blkdev.h>
>  #include <linux/interrupt.h>
> +#include <linux/kfifo.h>
>  
>  typedef u32 mbox_msg_t;
>  struct omap_mbox;
> @@ -42,7 +42,7 @@ struct omap_mbox_ops {
>  
>  struct omap_mbox_queue {
>  	spinlock_t		lock;
> -	struct request_queue	*queue;
> +	struct kfifo		fifo;
>  	struct work_struct	work;
>  	struct tasklet_struct	tasklet;
>  	int	(*callback)(void *);
> diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
> index 7c60550..908d9d2 100644
> --- a/arch/arm/plat-omap/mailbox.c
> +++ b/arch/arm/plat-omap/mailbox.c
> @@ -21,11 +21,14 @@
>   *
>   */
>  
> +#include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/interrupt.h>
>  #include <linux/device.h>
>  #include <linux/delay.h>
>  #include <linux/slab.h>
> +#include <linux/kfifo.h>
> +#include <linux/err.h>
>  
>  #include <plat/mailbox.h>
>  
> @@ -35,6 +38,10 @@ static DEFINE_SPINLOCK(mboxes_lock);
>  
>  static int mbox_configured;
>  
> +static unsigned int mbox_kfifo_size = CONFIG_OMAP_MBOX_KFIFO_SIZE;
> +module_param(mbox_kfifo_size, uint, S_IRUGO);
> +MODULE_PARM_DESC(mbox_kfifo_size, "Size of omap's mailbox kfifo (bytes)");
> +
>  /* Mailbox FIFO handle functions */
>  static inline mbox_msg_t mbox_fifo_read(struct omap_mbox *mbox)
>  {
> @@ -67,7 +74,7 @@ static inline int is_mbox_irq(struct omap_mbox *mbox, omap_mbox_irq_t irq)
>  /*
>   * message sender
>   */
> -static int __mbox_msg_send(struct omap_mbox *mbox, mbox_msg_t msg)
> +static int __mbox_poll_for_space(struct omap_mbox *mbox)
>  {
>  	int ret = 0, i = 1000;
>  
> @@ -78,49 +85,50 @@ static int __mbox_msg_send(struct omap_mbox *mbox, mbox_msg_t msg)
>  			return -1;
>  		udelay(1);
>  	}
> -	mbox_fifo_write(mbox, msg);
>  	return ret;
>  }
>  
> -
>  int omap_mbox_msg_send(struct omap_mbox *mbox, mbox_msg_t msg)
>  {
> +	struct omap_mbox_queue *mq = mbox->txq;
> +	int ret = 0, len;
>  
> -	struct request *rq;
> -	struct request_queue *q = mbox->txq->queue;
> +	spin_lock(&mq->lock);
>  
> -	rq = blk_get_request(q, WRITE, GFP_ATOMIC);
> -	if (unlikely(!rq))
> -		return -ENOMEM;
> +	if (kfifo_avail(&mq->fifo) < sizeof(msg)) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	len = kfifo_in(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
> +	WARN_ON(len != sizeof(msg));
>  
> -	blk_insert_request(q, rq, 0, (void *) msg);
>  	tasklet_schedule(&mbox->txq->tasklet);
>  
> -	return 0;
> +out:
> +	spin_unlock(&mq->lock);
> +	return ret;
>  }
>  EXPORT_SYMBOL(omap_mbox_msg_send);
>  
>  static void mbox_tx_tasklet(unsigned long tx_data)
>  {
> -	int ret;
> -	struct request *rq;
>  	struct omap_mbox *mbox = (struct omap_mbox *)tx_data;
> -	struct request_queue *q = mbox->txq->queue;
> -
> -	while (1) {
> -
> -		rq = blk_fetch_request(q);
> -
> -		if (!rq)
> -			break;
> +	struct omap_mbox_queue *mq = mbox->txq;
> +	mbox_msg_t msg;
> +	int ret;
>  
> -		ret = __mbox_msg_send(mbox, (mbox_msg_t)rq->special);
> -		if (ret) {
> +	while (kfifo_len(&mq->fifo)) {
> +		if (__mbox_poll_for_space(mbox)) {
>  			omap_mbox_enable_irq(mbox, IRQ_TX);
> -			blk_requeue_request(q, rq);
> -			return;
> +			break;
>  		}
> -		blk_end_request_all(rq, 0);
> +
> +		ret = kfifo_out(&mq->fifo, (unsigned char *)&msg,
> +								sizeof(msg));
> +		WARN_ON(ret != sizeof(msg));
> +
> +		mbox_fifo_write(mbox, msg);
>  	}
>  }
>  
> @@ -131,36 +139,21 @@ static void mbox_rx_work(struct work_struct *work)
>  {
>  	struct omap_mbox_queue *mq =
>  			container_of(work, struct omap_mbox_queue, work);
> -	struct omap_mbox *mbox = mq->queue->queuedata;
> -	struct request_queue *q = mbox->rxq->queue;
> -	struct request *rq;
>  	mbox_msg_t msg;
> -	unsigned long flags;
> +	int len;
>  
> -	while (1) {
> -		spin_lock_irqsave(q->queue_lock, flags);
> -		rq = blk_fetch_request(q);
> -		spin_unlock_irqrestore(q->queue_lock, flags);
> -		if (!rq)
> -			break;
> +	while (kfifo_len(&mq->fifo) >= sizeof(msg)) {
> +		len = kfifo_out(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
> +		WARN_ON(len != sizeof(msg));
>  
> -		msg = (mbox_msg_t)rq->special;
> -		blk_end_request_all(rq, 0);
> -		mbox->rxq->callback((void *)msg);
> +		if (mq->callback)
> +			mq->callback((void *)msg);
>  	}
>  }
>  
>  /*
>   * Mailbox interrupt handler
>   */
> -static void mbox_txq_fn(struct request_queue *q)
> -{
> -}
> -
> -static void mbox_rxq_fn(struct request_queue *q)
> -{
> -}
> -
>  static void __mbox_tx_interrupt(struct omap_mbox *mbox)
>  {
>  	omap_mbox_disable_irq(mbox, IRQ_TX);
> @@ -170,19 +163,19 @@ static void __mbox_tx_interrupt(struct omap_mbox *mbox)
>  
>  static void __mbox_rx_interrupt(struct omap_mbox *mbox)
>  {
> -	struct request *rq;
> +	struct omap_mbox_queue *mq = mbox->rxq;
>  	mbox_msg_t msg;
> -	struct request_queue *q = mbox->rxq->queue;
> +	int len;
>  
>  	while (!mbox_fifo_empty(mbox)) {
> -		rq = blk_get_request(q, WRITE, GFP_ATOMIC);
> -		if (unlikely(!rq))
> +		if (unlikely(kfifo_avail(&mq->fifo) < sizeof(msg)))
>  			goto nomem;
>  
>  		msg = mbox_fifo_read(mbox);
>  
> +		len = kfifo_in(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
> +		WARN_ON(len != sizeof(msg));
>  
> -		blk_insert_request(q, rq, 0, (void *)msg);
>  		if (mbox->ops->type == OMAP_MBOX_TYPE1)
>  			break;
>  	}
> @@ -207,11 +200,9 @@ static irqreturn_t mbox_interrupt(int irq, void *p)
>  }
>  
>  static struct omap_mbox_queue *mbox_queue_alloc(struct omap_mbox *mbox,
> -					request_fn_proc *proc,
>  					void (*work) (struct work_struct *),
>  					void (*tasklet)(unsigned long))
>  {
> -	struct request_queue *q;
>  	struct omap_mbox_queue *mq;
>  
>  	mq = kzalloc(sizeof(struct omap_mbox_queue), GFP_KERNEL);
> @@ -220,11 +211,8 @@ static struct omap_mbox_queue *mbox_queue_alloc(struct omap_mbox *mbox,
>  
>  	spin_lock_init(&mq->lock);
>  
> -	q = blk_init_queue(proc, &mq->lock);
> -	if (!q)
> +	if (kfifo_alloc(&mq->fifo, mbox_kfifo_size, GFP_KERNEL))
>  		goto error;
> -	q->queuedata = mbox;
> -	mq->queue = q;
>  
>  	if (work)
>  		INIT_WORK(&mq->work, work);
> @@ -239,7 +227,7 @@ error:
>  
>  static void mbox_queue_free(struct omap_mbox_queue *q)
>  {
> -	blk_cleanup_queue(q->queue);
> +	kfifo_free(&q->fifo);
>  	kfree(q);
>  }
>  
> @@ -269,14 +257,14 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
>  		goto fail_request_irq;
>  	}
>  
> -	mq = mbox_queue_alloc(mbox, mbox_txq_fn, NULL, mbox_tx_tasklet);
> +	mq = mbox_queue_alloc(mbox, NULL, mbox_tx_tasklet);
>  	if (!mq) {
>  		ret = -ENOMEM;
>  		goto fail_alloc_txq;
>  	}
>  	mbox->txq = mq;
>  
> -	mq = mbox_queue_alloc(mbox, mbox_rxq_fn, mbox_rx_work, NULL);
> +	mq = mbox_queue_alloc(mbox, mbox_rx_work, NULL);
>  	if (!mq) {
>  		ret = -ENOMEM;
>  		goto fail_alloc_rxq;
> @@ -407,6 +395,10 @@ static int __init omap_mbox_init(void)
>  	if (!mboxd)
>  		return -ENOMEM;
>  
> +	/* kfifo size sanity check: alignment and minimal size */
> +	mbox_kfifo_size = ALIGN(mbox_kfifo_size, sizeof(mbox_msg_t));
> +	mbox_kfifo_size = max_t(unsigned int, mbox_kfifo_size, sizeof(mbox_msg_t));
> +
>  	return 0;
>  }
>  module_init(omap_mbox_init);
>   
Ohad,

With this patch I observed "inconsistent lock state" warning.Please 
refer the below log snippet.But this issue is seen very randomly during 
device bootup with spin lock debug enabled in kernel.

[  105.002593]
[  105.002593] =================================
[  105.008483] [ INFO: inconsistent lock state ]
[  105.012878] 2.6.32.14-11527-g82d84bc #1
[  105.016723] ---------------------------------
[  105.021087] inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
[  105.027160] ksoftirqd/0/3 [HC0[0]:SC1[1]:HE1:SE0] takes:
[  105.032501]  (&mq->lock){+.?...}, at: [<bf096548>] 
omap_mbox_msg_send+0x18/0xa4 [mailbox]
[  105.040771] {SOFTIRQ-ON-W} state was registered at:
[  105.045684]   [<c0090798>] __lock_acquire+0x660/0x1704
[  105.050872]   [<c009189c>] lock_acquire+0x60/0x74
[  105.055603]   [<c038dabc>] _spin_lock+0x40/0x50
[  105.060180]   [<bf096548>] omap_mbox_msg_send+0x18/0xa4 [mailbox]
[  105.066314]   [<bf17a908>] sm_interrupt_dsp+0xe8/0x118 [bridgedriver]
[  105.072998]   [<bf17a4e4>] wake_dsp+0x20/0x30 [bridgedriver]
[  105.078796]   [<bf179170>] bridge_brd_mem_map+0x57c/0x5dc [bridgedriver]
[  105.085632]   [<bf18803c>] proc_map+0xb4/0x158 [bridgedriver]
[  105.091522]   [<bf17e1a8>] procwrap_map+0x40/0x84 [bridgedriver]
[  105.097686]   [<bf17ce70>] wcd_call_dev_io_ctl+0xf8/0x120 [bridgedriver]
[  105.104553]   [<bf18c298>] bridge_ioctl+0x138/0x164 [bridgedriver]
[  105.110900]   [<c00ec0f8>] vfs_ioctl+0x2c/0x8c
[  105.115386]   [<c00ec7a4>] do_vfs_ioctl+0x55c/0x5a0
[  105.120300]   [<c00ec834>] sys_ioctl+0x4c/0x6c
[  105.124786]   [<c003b000>] ret_fast_syscall+0x0/0x38
[  105.129791] irq event stamp: 114029
[  105.133300] hardirqs last  enabled at (114029): [<c006fbc8>] 
tasklet_action+0x20/0xd0
[  105.141204] hardirqs last disabled at (114028): [<c006fbb4>] 
tasklet_action+0xc/0xd0
[  105.148986] softirqs last  enabled at (114021): [<c00704a4>] 
do_softirq+0x4c/0x70
[  105.156524] softirqs last disabled at (114026): [<c00704a4>] 
do_softirq+0x4c/0x70
[  105.164062]
[  105.164062] other info that might help us debug this:
[  105.170654] no locks held by ksoftirqd/0/3.
[  105.174835]
[  105.174865] stack backtrace:
[  105.179260] [<c003f868>] (unwind_backtrace+0x0/0xdc) from 
[<c008eaec>] (print_usage_bug+0x170/0x1b4)
[  105.188446] [<c008eaec>] (print_usage_bug+0x170/0x1b4) from 
[<c008ee8c>] (mark_lock+0x35c/0x628)
[  105.197296] [<c008ee8c>] (mark_lock+0x35c/0x628) from [<c0090708>] 
(__lock_acquire+0x5d0/0x1704)
[  105.206146] [<c0090708>] (__lock_acquire+0x5d0/0x1704) from 
[<c009189c>] (lock_acquire+0x60/0x74)
[  105.215057] [<c009189c>] (lock_acquire+0x60/0x74) from [<c038dabc>] 
(_spin_lock+0x40/0x50)
[  105.223388] [<c038dabc>] (_spin_lock+0x40/0x50) from [<bf096548>] 
(omap_mbox_msg_send+0x18/0xa4 [mailbox])
[  105.233215] [<bf096548>] (omap_mbox_msg_send+0x18/0xa4 [mailbox]) 
from [<bf17a908>] (sm_interrupt_dsp+0xe8/0x118 [bridgedriver])
[  105.245025] [<bf17a908>] (sm_interrupt_dsp+0xe8/0x118 [bridgedriver]) 
from [<bf1774e0>] (io_dpc+0x384/0x6c4 [bridgedriver])
[  105.256317] [<bf1774e0>] (io_dpc+0x384/0x6c4 [bridgedriver]) from 
[<c006fc18>] (tasklet_action+0x70/0xd0)
[  105.265960] [<c006fc18>] (tasklet_action+0x70/0xd0) from [<c0070300>] 
(__do_softirq+0x9c/0x148)
[  105.274719] [<c0070300>] (__do_softirq+0x9c/0x148) from [<c00704a4>] 
(do_softirq+0x4c/0x70)
[  105.283111] [<c00704a4>] (do_softirq+0x4c/0x70) from [<c007053c>] 
(ksoftirqd+0x74/0x170)
[  105.291259] [<c007053c>] (ksoftirqd+0x74/0x170) from [<c007f2bc>] 
(kthread+0x80/0x88)
[  105.299163] [<c007f2bc>] (kthread+0x80/0x88) from [<c003ba04>] 
(kernel_thread_exit+0x0/0x8)

Kfifo is acccessed in omap_mbox_msg_send() and mbox_tx_tasklet() functions.In order to protect this critical section we need to protect by using spin_lock_bh() so that the tasklet cannot preempt omap_mobx_msg_send().With the below change the issue not seen anymore.

@@ -93,7 +93,7 @@ int omap_mbox_msg_send(struct omap_mbox *mbox, mbox_msg_t msg)
        struct omap_mbox_queue *mq = mbox->txq;
        int ret = 0, len;
 
-       spin_lock(&mq->lock);
+       spin_lock_bh(&mq->lock);
 
        if (kfifo_avail(&mq->fifo) < sizeof(msg)) {
                ret = -ENOMEM;
@@ -106,7 +106,7 @@ int omap_mbox_msg_send(struct omap_mbox *mbox, mbox_msg_t msg)
        tasklet_schedule(&mbox->txq->tasklet);
 
 out:
-       spin_unlock(&mq->lock);
+       spin_unlock_bh(&mq->lock);
        return ret;
 }

Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Ohad Ben-Cohen]

Hi Deepak,

On Mon, Jun 7, 2010 at 1:52 PM, Deepak Chitriki <deepak.chitriki@ti.com> wrote:
> With this patch I observed "inconsistent lock state" warning.

Thanks for the report!

> Kfifo is acccessed in omap_mbox_msg_send() and mbox_tx_tasklet()
> functions.In order to protect this critical section we need to protect by
> using spin_lock_bh() so that the tasklet cannot preempt
> omap_mobx_msg_send().

This is actually not the problem: it's ok if mbox_tx_tasklet preempts
omap_mbox_msg_send. In fact, such a use case is even ok if we don't
take a spinlock at all: kfifo is designed in a way that if you have
only 1 consumer and 1 producer, they can both access kfifo
simultaneously without any locking. That's why we don't take the
spinlock in the mbox_tx_tasklet. The reason, btw, that we take a
spinlock in omap_mbox_msg_send is to allow multiple simultaneous
sending contexts (taking a spinlock there ensures serialization of
those multiple simultaneous sending contexts).

The problem here lies in the fact (that I've just learnt) that
dspbridge also sends mbox messages from a tasklet context
(dpc_tasklet). Lockdep identifies that the spinlock is taken in a
softirq context (dspbridge's dpc_tasklet) after it was previously
taken in a softirq-enabled context. Your proposed fix will solve the
lockdep issue here, but:

Do we really want to have tasklets in dspbridge ? Are we that
performance critical ?

In general I'd prefer to switch to workqueues in both dspbridge and
mailbox. I'm really not convinced we have to use tasklets in those
modules, and workqueues are much more system friendly. This way we
would also not have to stop all bottom halves when sending a mailbox
message.

Somewhat relevant note about mailbox performance: omap_mbox_msg_send
often (i.e. when the kfifo is empty) can just send the message
directly, without triggering the tasklet to do that. Applying such a
change will substantially improve the mailbox performance, and will
make the shift to workqueues even more reasonable. I've got a patch
for that, I'll post it soon.

What do you think (looping in Fernando, Omar and Hari) ?

Thanks,
Ohad.

RE: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Guzman Lugo, Fernando]

Hi,

> -----Original Message-----
> From: Ohad Ben-Cohen [mailto:ohad@wizery.com]
> Sent: Monday, June 07, 2010 4:41 PM
> To: Chitriki Rudramuni, Deepak; Guzman Lugo, Fernando; Ramirez Luna, Omar;
> Kanigeri, Hari
> Cc: linux-omap@vger.kernel.org; Hiroshi Doyu
> Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
> 
> Hi Deepak,
> 
> On Mon, Jun 7, 2010 at 1:52 PM, Deepak Chitriki <deepak.chitriki@ti.com>
> wrote:
> > With this patch I observed "inconsistent lock state" warning.
> 
> Thanks for the report!
> 
> > Kfifo is acccessed in omap_mbox_msg_send() and mbox_tx_tasklet()
> > functions.In order to protect this critical section we need to protect
> by
> > using spin_lock_bh() so that the tasklet cannot preempt
> > omap_mobx_msg_send().
> 
> This is actually not the problem: it's ok if mbox_tx_tasklet preempts
> omap_mbox_msg_send. In fact, such a use case is even ok if we don't
> take a spinlock at all: kfifo is designed in a way that if you have
> only 1 consumer and 1 producer, they can both access kfifo
> simultaneously without any locking. That's why we don't take the
> spinlock in the mbox_tx_tasklet. The reason, btw, that we take a
> spinlock in omap_mbox_msg_send is to allow multiple simultaneous
> sending contexts (taking a spinlock there ensures serialization of
> those multiple simultaneous sending contexts).
> 
> The problem here lies in the fact (that I've just learnt) that
> dspbridge also sends mbox messages from a tasklet context
> (dpc_tasklet). Lockdep identifies that the spinlock is taken in a
> softirq context (dspbridge's dpc_tasklet) after it was previously
> taken in a softirq-enabled context. Your proposed fix will solve the
> lockdep issue here, but:

I think the best thing to do here is remove the spinlock, if not, you are preventing that omap_mbox_msg_send be executed from a tasklet or isr context. That maybe in this moment it is not called but it could be needed in the future. The caller of omap_mbox_msg_send should be take care of that function can not be call simultaneously.

> 
> Do we really want to have tasklets in dspbridge ? Are we that
> performance critical ?

We are facing some issues when the DSP send the request for hibernation and we get that message and start processing the request (because it is done in a workqueue and there some latency). I issue had been manage in other ways but maybe we would have that issue it the hibernation request could be handle immediately. Why not remove the workqueue at all and let the mailbox module user be the judge for using tasklet, workqueue or do the job in the same isr (if it is small task and need to be fast)?


> 
> In general I'd prefer to switch to workqueues in both dspbridge and
> mailbox. I'm really not convinced we have to use tasklets in those
> modules, and workqueues are much more system friendly. This way we
> would also not have to stop all bottom halves when sending a mailbox
> message.
> 
> Somewhat relevant note about mailbox performance: omap_mbox_msg_send
> often (i.e. when the kfifo is empty) can just send the message
> directly, without triggering the tasklet to do that. Applying such a
> change will substantially improve the mailbox performance, and will
> make the shift to workqueues even more reasonable. I've got a patch
> for that, I'll post it soon.

What would be really good!

Regards,
Fernando.

> 
> What do you think (looping in Fernando, Omar and Hari) ?
> 
> Thanks,
> Ohad.

Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Ohad Ben-Cohen]

On Mon, Jun 7, 2010 at 6:14 PM, Guzman Lugo, Fernando
<fernando.lugo@ti.com> wrote:
> I think the best thing to do here is remove the spinlock

I'm afraid we can't do that: we need it to support OMAP4 syslink IPC
use cases which have multiple simultaneous sending contexts.


>, if not, you are preventing that omap_mbox_msg_send be executed from a tasklet or isr context.


Right. But this is pretty standard - you can't call every kernel API
from every possible context - the allowed calling context is part of
the API. If later we decide we need to add additional calling
contexts, it's ok - it's just like changing the API.


> Why not remove the workqueue at all and let the mailbox module user be the judge for using tasklet, workqueue or do the job in the same isr (if it is small task and need to be fast)?


hmm I'm not following here - will you please elaborate on this a bit more ?


Thanks,
Ohad.

Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Hiroshi DOYU]

From: ext Ohad Ben-Cohen <ohad@wizery.com>
Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
Date: Tue, 8 Jun 2010 04:54:55 +0200

> On Mon, Jun 7, 2010 at 6:14 PM, Guzman Lugo, Fernando
> <fernando.lugo@ti.com> wrote:
>> I think the best thing to do here is remove the spinlock
> 
> I'm afraid we can't do that: we need it to support OMAP4 syslink IPC
> use cases which have multiple simultaneous sending contexts.
> 
> 
>>, if not, you are preventing that omap_mbox_msg_send be executed from a tasklet or isr context.
> 
> 
> Right. But this is pretty standard - you can't call every kernel API
> from every possible context - the allowed calling context is part of
> the API. If later we decide we need to add additional calling
> contexts, it's ok - it's just like changing the API.
> 
> 
>> Why not remove the workqueue at all and let the mailbox module user be the judge for using tasklet, workqueue or do the job in the same isr (if it is small task and need to be fast)?
> 
> 
> hmm I'm not following here - will you please elaborate on this a bit more ?

The abvoe could be something like below:

	Modified arch/arm/plat-omap/mailbox.c
diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index 87e0cde..1b79b32 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -188,7 +188,7 @@ static void __mbox_rx_interrupt(struct omap_mbox *mbox)
 	/* no more messages in the fifo. clear IRQ source. */
 	ack_mbox_irq(mbox, IRQ_RX);
 nomem:
-	queue_work(mboxd, &mbox->rxq->work);
+	mbox->callback(mbox);
 }
 
 static irqreturn_t mbox_interrupt(int irq, void *p)

The user/client of mbox can register its own callback function in
advance and do its own job(tasklet, workqueue, just func whatever)
later by itself.

It makes more sense to me than the current workqueue as default.

RE: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Guzman Lugo, Fernando]

________________________________________
>From: Hiroshi DOYU [Hiroshi.DOYU@nokia.com]
>Sent: Wednesday, June 09, 2010 12:07 AM
>To: ohad@wizery.com
>Cc: Guzman Lugo, Fernando; Chitriki Rudramuni, Deepak; Ramirez Luna, Omar; Kanigeri, Hari; linux-omap@vger.kernel.org
>Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
>
>From: ext Ohad Ben-Cohen <ohad@wizery.com>
>Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
>Date: Tue, 8 Jun 2010 04:54:55 +0200
>
>> On Mon, Jun 7, 2010 at 6:14 PM, Guzman Lugo, Fernando
>> <fernando.lugo@ti.com> wrote:
>>> I think the best thing to do here is remove the spinlock
>>
>> I'm afraid we can't do that: we need it to support OMAP4 syslink IPC
>> use cases which have multiple simultaneous sending contexts.
>>
>>
>>>, if not, you are preventing that omap_mbox_msg_send be executed from a tasklet or isr context.
>>
>>
>> Right. But this is pretty standard - you can't call every kernel API
>> from every possible context - the allowed calling context is part of
>> the API. If later we decide we need to add additional calling
>> contexts, it's ok - it's just like changing the API.
>>
>>
>>> Why not remove the workqueue at all and let the mailbox module user be the judge for using tasklet, workqueue or do the job in the same isr (if it is small task and need to be fast)?
>>
>>
>> hmm I'm not following here - will you please elaborate on this a bit more ?
>
>The abvoe could be something like below:
>
>        Modified arch/arm/plat-omap/mailbox.c
>diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
>index 87e0cde..1b79b32 100644
>--- a/arch/arm/plat-omap/mailbox.c
>+++ b/arch/arm/plat-omap/mailbox.c
>@@ -188,7 +188,7 @@ static void __mbox_rx_interrupt(struct omap_mbox *mbox)
>        /* no more messages in the fifo. clear IRQ source. */
>        ack_mbox_irq(mbox, IRQ_RX);
> nomem:
>-       queue_work(mboxd, &mbox->rxq->work);
>+       mbox->callback(mbox);
> }
>
> static irqreturn_t mbox_interrupt(int irq, void *p)
>
>The user/client of mbox can register its own callback function in
>advance and do its own job(tasklet, workqueue, just func whatever)
>later by itself.
>
>It makes more sense to me than the current workqueue as default.

Yes, this is exactly what I was talking about.

Regards,
Fernando.

Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Ohad Ben-Cohen]

On Wed, Jun 9, 2010 at 12:07 AM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
> diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
> index 87e0cde..1b79b32 100644
> --- a/arch/arm/plat-omap/mailbox.c
> +++ b/arch/arm/plat-omap/mailbox.c
> @@ -188,7 +188,7 @@ static void __mbox_rx_interrupt(struct omap_mbox *mbox)
>        /* no more messages in the fifo. clear IRQ source. */
>        ack_mbox_irq(mbox, IRQ_RX);
>  nomem:
> -       queue_work(mboxd, &mbox->rxq->work);
> +       mbox->callback(mbox);
>  }

I like this !

It will allow us to easily plug in new IPC mechanisms in the future.

(btw, if we do this, and also introduce omap_mbox_msg_send_sync, do we
still need a mailbox queuing mechanism at all ? :)

Having said that, this is not going to solve the lockdep warning
reported by Deepak - that was caused because of dspbridge's sending
context (and not because of the receiving context). To eliminate that
issue, I prefer fixing dspbridge to use work queues rather than using
spin_lock_bh in omap_mbox_msg_send. Disabling system bottom halves
just to send a mbox msg sounds unjustified (unless bridge really needs
to use tasklets instead of work queues, which I slightly doubt). What
do you think ?

Speaking of mailbox I'd like to address some issues that are code related:

* Let's add mailbox API to set the callback pointer (it feels wrong to
let users directly manipulate the mbox structure).

* We can also safely move the callback field to the main mbox
structure, since it has no usage in the TX mq; it's pretty global to
the whole mbox.

* Let's make sure no one accidentally registers two receivers on the
same time (which would result in one overwriting the other's callback
field).

* mbox_configured is a global variable and that breaks support of
multiple concurrent mailbox instances. Let's move this logic into the
instance of mbox.

* If we make sure there are no two user of the same mailbox instance
(see 3rd *), we can eliminate mbox_configured and its locking (not
that doesn't mean there can't be two concurrent sending contexts, it
just means they are somewhat related, they have a common receiver,
which was registered using only a single call to get).

Something like this, pls tell me what you think:


From 353d36d7fd6ec76c81689893338dfd4d33c8c89a Mon Sep 17 00:00:00 2001
From: Ohad Ben-Cohen <ohad@wizery.com>
Date: Sun, 13 Jun 2010 18:32:22 -0500
Subject: [PATCH] mailbox: enforce sane usage

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
I can also be contacted via <  ohadb  at  ti  dot  com  >

 arch/arm/plat-omap/include/plat/mailbox.h |    5 ++-
 arch/arm/plat-omap/mailbox.c              |   38 +++++++++++++++-------------
 2 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/mailbox.h
b/arch/arm/plat-omap/include/plat/mailbox.h
index 729166b..0a5bf19 100644
--- a/arch/arm/plat-omap/include/plat/mailbox.h
+++ b/arch/arm/plat-omap/include/plat/mailbox.h
@@ -45,7 +45,6 @@ struct omap_mbox_queue {
 	struct request_queue	*queue;
 	struct work_struct	work;
 	struct tasklet_struct	tasklet;
-	int	(*callback)(void *);
 	struct omap_mbox	*mbox;
 };

@@ -65,12 +64,14 @@ struct omap_mbox {
 	void			*priv;

 	void			(*err_notify)(void);
+	int			(*callback)(void *);
+	atomic_t		count;
 };

 int omap_mbox_msg_send(struct omap_mbox *, mbox_msg_t msg);
 void omap_mbox_init_seq(struct omap_mbox *);

-struct omap_mbox *omap_mbox_get(const char *);
+struct omap_mbox *omap_mbox_get(const char *, int (*)(void *));
 void omap_mbox_put(struct omap_mbox *);

 int omap_mbox_register(struct device *parent, struct omap_mbox *);
diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index 08a2df7..5f29ea4 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -33,8 +33,6 @@ static struct workqueue_struct *mboxd;
 static struct omap_mbox *mboxes;
 static DEFINE_RWLOCK(mboxes_lock);

-static int mbox_configured;
-
 /* Mailbox FIFO handle functions */
 static inline mbox_msg_t mbox_fifo_read(struct omap_mbox *mbox)
 {
@@ -146,7 +144,7 @@ static void mbox_rx_work(struct work_struct *work)

 		msg = (mbox_msg_t)rq->special;
 		blk_end_request_all(rq, 0);
-		mbox->rxq->callback((void *)msg);
+		mbox->callback((void *)msg);
 	}
 }

@@ -249,16 +247,10 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
 	struct omap_mbox_queue *mq;

 	if (likely(mbox->ops->startup)) {
-		write_lock(&mboxes_lock);
-		if (!mbox_configured)
-			ret = mbox->ops->startup(mbox);
-
+		ret = mbox->ops->startup(mbox);
 		if (unlikely(ret)) {
-			write_unlock(&mboxes_lock);
 			return ret;
 		}
-		mbox_configured++;
-		write_unlock(&mboxes_lock);
 	}

 	ret = request_irq(mbox->irq, mbox_interrupt, IRQF_SHARED,
@@ -304,13 +296,10 @@ static void omap_mbox_fini(struct omap_mbox *mbox)
 	free_irq(mbox->irq, mbox);

 	if (unlikely(mbox->ops->shutdown)) {
-		write_lock(&mboxes_lock);
-		if (mbox_configured > 0)
-			mbox_configured--;
-		if (!mbox_configured)
-			mbox->ops->shutdown(mbox);
-		write_unlock(&mboxes_lock);
+		mbox->ops->shutdown(mbox);
 	}
+
+	atomic_dec(&mbox->count);
 }

 static struct omap_mbox **find_mboxes(const char *name)
@@ -325,7 +314,7 @@ static struct omap_mbox **find_mboxes(const char *name)
 	return p;
 }

-struct omap_mbox *omap_mbox_get(const char *name)
+struct omap_mbox *omap_mbox_get(const char *name, int (*callback)(void *))
 {
 	struct omap_mbox *mbox;
 	int ret;
@@ -339,9 +328,19 @@ struct omap_mbox *omap_mbox_get(const char *name)

 	read_unlock(&mboxes_lock);

+	if (atomic_inc_return(&mbox->count) > 1) {
+		pr_err("%s: mbox %s already in use\n", __func__, mbox->name);
+		atomic_dec(&mbox->count);
+		return ERR_PTR(-EBUSY);
+	}
+
 	ret = omap_mbox_startup(mbox);
-	if (ret)
+	if (ret) {
+		atomic_dec(&mbox->count);
 		return ERR_PTR(-ENODEV);
+	}
+
+	mbox->callback = callback;

 	return mbox;
 }
@@ -370,6 +369,9 @@ int omap_mbox_register(struct device *parent,
struct omap_mbox *mbox)
 		write_unlock(&mboxes_lock);
 		goto err_find;
 	}
+
+	atomic_set(&mbox->count, 0);
+
 	*tmp = mbox;
 	write_unlock



Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Hiroshi DOYU]

Hi Ohad,

From: ext Ohad Ben-Cohen <ohad@wizery.com>
Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
Date: Mon, 14 Jun 2010 01:52:16 +0200

> On Wed, Jun 9, 2010 at 12:07 AM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
>> diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
>> index 87e0cde..1b79b32 100644
>> --- a/arch/arm/plat-omap/mailbox.c
>> +++ b/arch/arm/plat-omap/mailbox.c
>> @@ -188,7 +188,7 @@ static void __mbox_rx_interrupt(struct omap_mbox *mbox)
>>        /* no more messages in the fifo. clear IRQ source. */
>>        ack_mbox_irq(mbox, IRQ_RX);
>>  nomem:
>> -       queue_work(mboxd, &mbox->rxq->work);
>> +       mbox->callback(mbox);
>>  }
> 
> I like this !
>
> It will allow us to easily plug in new IPC mechanisms in the future.

Agree.

> (btw, if we do this, and also introduce omap_mbox_msg_send_sync, do we
> still need a mailbox queuing mechanism at all ? :)

I think that "queuing"(can be called as S/W fifo/buffer?) is basically
necessary it can compensate the shortage of H/W fifo slots under high
load or emergency case. It has only 4 slots. I guess that, DSP usually
responds quickly and 4 H/W slots may be enough, but it might be
safer/more robust to avoid the assumption which depends on other
entity/DSP. From latecy perspective, "s/w fifo" + "tasklet" would be
enough short.

omap_mbox_msg_send_sync() can handle the case for a special message,
like PM, which has to respond at the higher priority than the normal
ones.

> Having said that, this is not going to solve the lockdep warning
> reported by Deepak - that was caused because of dspbridge's sending
> context (and not because of the receiving context). To eliminate that

Does dspbridge really need its own defered work for sending mailbox
messages?

For me, the problem here is the unneccesary duplication of tasklet, or
can be said, the unnecessary use of tasklet for _sending_ mailbox
messages in dspbridge.

http://marc.info/?l=linux-omap&m=127601655325416&w=2

I thought that bridge_msg_put()/bridge_msg_get() can use omap mbox
APIs directly, with getting rid of its use of its own defered
work/tasklet as pointed out in the above link. Fernando?

For recieving, its defered work(tasklet) can be trigered directly in
the above proposed callback, that callback can triger its own
workqueue if necessary, then. I think that, for recieving, some PM
command may has to be sent back immedieately inside of
tasklet. omap_mbox_msg_send_sync() may handle this case.

What do you think?

> issue, I prefer fixing dspbridge to use work queues rather than using
> spin_lock_bh in omap_mbox_msg_send. Disabling system bottom halves
> just to send a mbox msg sounds unjustified (unless bridge really needs
> to use tasklets instead of work queues, which I slightly doubt). What
> do you think ?

I think that workqueue is only necessary when it has to sleep,
otherwise tasklet is prefered. For _sending_ a message inside of
dspbridge, I haven't found any reasonable reason to use any defered
work(softirq, tasklet, workqueue) so far.

> Speaking of mailbox I'd like to address some issues that are code related:
> 
> * Let's add mailbox API to set the callback pointer (it feels wrong to
> let users directly manipulate the mbox structure).
> 
> * We can also safely move the callback field to the main mbox
> structure, since it has no usage in the TX mq; it's pretty global to
> the whole mbox.
> 
> * Let's make sure no one accidentally registers two receivers on the
> same time (which would result in one overwriting the other's callback
> field).

I'd like to allow mutiple listners in some way, as the flexibility of
mailbox functionalty.

> * mbox_configured is a global variable and that breaks support of
> multiple concurrent mailbox instances. Let's move this logic into the
> instance of mbox.
> 
> * If we make sure there are no two user of the same mailbox instance
> (see 3rd *), we can eliminate mbox_configured and its locking (not
> that doesn't mean there can't be two concurrent sending contexts, it
> just means they are somewhat related, they have a common receiver,
> which was registered using only a single call to get).
>
> Something like this, pls tell me what you think:

For the above 5 proposals, excpet the multiple listeners issue, they
seems ok.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[C.A, Subramaniam]

Hi Ohad/Hiroshi,

Good to see the new mailbox requirements. While we are at it can we add some more :)?

* Make the mailbox as a generic driver. 
	Meaning, users can request for a pair of mailbox rather than using a set of pre-defined ones. For eg: Instead of doing an omap_mbox_get("mailbox-1") we can have the user specify the mailbox pair that he needs ( omap_mbox_get(int tx_mbox, int rx_mbox) or similar API ). This also means that we remove the bulk of the data structures like omap2_mbox_1_priv and mbox_1_info and so on. Additional checks needs to be done so that consequtive requests to the driver does not re-configure the rx-tx pairs.

Rationale: 
The pre-configured structures does make sense in case of DSP Bridge, since it is the only user of mailbox. However, for Syslink for example, (or for any other IPC or user of mailbox) it would be good for the user of the mailbox to request the pair (or just tx/rx) from user/kernel side.

* Provide debug support for the mailboxes (relevant for OMAP4)
	Since OMAP4 has support to read the RAW registers we might as well add an API for the user to read the status from RAW registers.

Please provide your feedback!


Thank you and Regards
Subbu
 

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org 
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Hiroshi DOYU
> Sent: Monday, June 14, 2010 3:58 AM
> To: Guzman Lugo, Fernando; ohad@wizery.com
> Cc: Chitriki Rudramuni, Deepak; Ramirez Luna, Omar; Kanigeri, 
> Hari; linux-omap@vger.kernel.org
> Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
> 
> Hi Ohad,
> 
> From: ext Ohad Ben-Cohen <ohad@wizery.com>
> Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
> Date: Mon, 14 Jun 2010 01:52:16 +0200
> 
> > On Wed, Jun 9, 2010 at 12:07 AM, Hiroshi DOYU 
> <Hiroshi.DOYU@nokia.com> wrote:
> >> diff --git a/arch/arm/plat-omap/mailbox.c 
> b/arch/arm/plat-omap/mailbox.c
> >> index 87e0cde..1b79b32 100644
> >> --- a/arch/arm/plat-omap/mailbox.c
> >> +++ b/arch/arm/plat-omap/mailbox.c
> >> @@ -188,7 +188,7 @@ static void __mbox_rx_interrupt(struct 
> omap_mbox *mbox)
> >>        /* no more messages in the fifo. clear IRQ source. */
> >>        ack_mbox_irq(mbox, IRQ_RX);
> >>  nomem:
> >> -       queue_work(mboxd, &mbox->rxq->work);
> >> +       mbox->callback(mbox);
> >>  }
> > 
> > I like this !
> >
> > It will allow us to easily plug in new IPC mechanisms in the future.
> 
> Agree.
> 
> > (btw, if we do this, and also introduce 
> omap_mbox_msg_send_sync, do we
> > still need a mailbox queuing mechanism at all ? :)
> 
> I think that "queuing"(can be called as S/W fifo/buffer?) is basically
> necessary it can compensate the shortage of H/W fifo slots under high
> load or emergency case. It has only 4 slots. I guess that, DSP usually
> responds quickly and 4 H/W slots may be enough, but it might be
> safer/more robust to avoid the assumption which depends on other
> entity/DSP. From latecy perspective, "s/w fifo" + "tasklet" would be
> enough short.
> 
> omap_mbox_msg_send_sync() can handle the case for a special message,
> like PM, which has to respond at the higher priority than the normal
> ones.
> 
> > Having said that, this is not going to solve the lockdep warning
> > reported by Deepak - that was caused because of dspbridge's sending
> > context (and not because of the receiving context). To 
> eliminate that
> 
> Does dspbridge really need its own defered work for sending mailbox
> messages?
> 
> For me, the problem here is the unneccesary duplication of tasklet, or
> can be said, the unnecessary use of tasklet for _sending_ mailbox
> messages in dspbridge.
> 
> http://marc.info/?l=linux-omap&m=127601655325416&w=2
> 
> I thought that bridge_msg_put()/bridge_msg_get() can use omap mbox
> APIs directly, with getting rid of its use of its own defered
> work/tasklet as pointed out in the above link. Fernando?
> 
> For recieving, its defered work(tasklet) can be trigered directly in
> the above proposed callback, that callback can triger its own
> workqueue if necessary, then. I think that, for recieving, some PM
> command may has to be sent back immedieately inside of
> tasklet. omap_mbox_msg_send_sync() may handle this case.
> 
> What do you think?
> 
> > issue, I prefer fixing dspbridge to use work queues rather 
> than using
> > spin_lock_bh in omap_mbox_msg_send. Disabling system bottom halves
> > just to send a mbox msg sounds unjustified (unless bridge 
> really needs
> > to use tasklets instead of work queues, which I slightly 
> doubt). What
> > do you think ?
> 
> I think that workqueue is only necessary when it has to sleep,
> otherwise tasklet is prefered. For _sending_ a message inside of
> dspbridge, I haven't found any reasonable reason to use any defered
> work(softirq, tasklet, workqueue) so far.
> 
> > Speaking of mailbox I'd like to address some issues that 
> are code related:
> > 
> > * Let's add mailbox API to set the callback pointer (it 
> feels wrong to
> > let users directly manipulate the mbox structure).
> > 
> > * We can also safely move the callback field to the main mbox
> > structure, since it has no usage in the TX mq; it's pretty global to
> > the whole mbox.
> > 
> > * Let's make sure no one accidentally registers two receivers on the
> > same time (which would result in one overwriting the 
> other's callback
> > field).
> 
> I'd like to allow mutiple listners in some way, as the flexibility of
> mailbox functionalty.
> 
> > * mbox_configured is a global variable and that breaks support of
> > multiple concurrent mailbox instances. Let's move this 
> logic into the
> > instance of mbox.
> > 
> > * If we make sure there are no two user of the same mailbox instance
> > (see 3rd *), we can eliminate mbox_configured and its locking (not
> > that doesn't mean there can't be two concurrent sending contexts, it
> > just means they are somewhat related, they have a common receiver,
> > which was registered using only a single call to get).
> >
> > Something like this, pls tell me what you think:
> 
> For the above 5 proposals, excpet the multiple listeners issue, they
> seems ok.
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Ohad Ben-Cohen]

Hi Subbu,

On Mon, Jun 14, 2010 at 10:56 AM, C.A, Subramaniam
<subramaniam.ca@ti.com> wrote:
> Hi Ohad/Hiroshi,
>
> Good to see the new mailbox requirements. While we are at it can we add some more :)?
>
...
> Please provide your feedback!


Feel free to send the patches ;)


Thanks,
Ohad.

Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Hiroshi DOYU]

From: "ext C.A, Subramaniam" <subramaniam.ca@ti.com>
Subject: RE: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
Date: Mon, 14 Jun 2010 17:56:35 +0200

> Hi Ohad/Hiroshi,
> 
> Good to see the new mailbox requirements. While we are at it can we add some more :)?
> 
> * Make the mailbox as a generic driver. 
> 	Meaning, users can request for a pair of mailbox rather than
> using a set of pre-defined ones. For eg: Instead of doing an
> omap_mbox_get("mailbox-1") we can have the user specify the mailbox
> pair that he needs ( omap_mbox_get(int tx_mbox, int rx_mbox) or
> similar API ). This also means that we remove the bulk of the data
> structures like omap2_mbox_1_priv and mbox_1_info and so
> on. Additional checks needs to be done so that consequtive requests
> to the driver does not re-configure the rx-tx pairs.
> 
> Rationale: 
> The pre-configured structures does make sense in case of DSP Bridge,
> since it is the only user of mailbox. However, for Syslink for
> example, (or for any other IPC or user of mailbox) it would be good
> for the user of the mailbox to request the pair (or just tx/rx) from
> user/kernel side.

Fair enough.

> * Provide debug support for the mailboxes (relevant for OMAP4)
> 	Since OMAP4 has support to read the RAW registers we might as
> well add an API for the user to read the status from RAW registers.

This could be debugfs.

RE: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Sapiens, Rene]

Hi Hiroshi

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Hiroshi DOYU
> Sent: Monday, June 14, 2010 3:58 AM
> To: Guzman Lugo, Fernando; ohad@wizery.com
> Cc: Chitriki Rudramuni, Deepak; Ramirez Luna, Omar; Kanigeri, Hari; linux-
> omap@vger.kernel.org
> Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
> 
> Hi Ohad,
> 
> From: ext Ohad Ben-Cohen <ohad@wizery.com>
> Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
> Date: Mon, 14 Jun 2010 01:52:16 +0200
> 
> > On Wed, Jun 9, 2010 at 12:07 AM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
> wrote:
> >> diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-
> omap/mailbox.c
> >> index 87e0cde..1b79b32 100644
> >> --- a/arch/arm/plat-omap/mailbox.c
> >> +++ b/arch/arm/plat-omap/mailbox.c
> >> @@ -188,7 +188,7 @@ static void __mbox_rx_interrupt(struct omap_mbox
> *mbox)
> >>        /* no more messages in the fifo. clear IRQ source. */
> >>        ack_mbox_irq(mbox, IRQ_RX);
> >>  nomem:
> >> -       queue_work(mboxd, &mbox->rxq->work);
> >> +       mbox->callback(mbox);
> >>  }
> >
> > I like this !
> >
> > It will allow us to easily plug in new IPC mechanisms in the future.
> 
> Agree.
> 
> > (btw, if we do this, and also introduce omap_mbox_msg_send_sync, do we
> > still need a mailbox queuing mechanism at all ? :)
> 
> I think that "queuing"(can be called as S/W fifo/buffer?) is basically
> necessary it can compensate the shortage of H/W fifo slots under high
> load or emergency case. It has only 4 slots. I guess that, DSP usually
> responds quickly and 4 H/W slots may be enough, but it might be
> safer/more robust to avoid the assumption which depends on other
> entity/DSP. From latecy perspective, "s/w fifo" + "tasklet" would be
> enough short.
> 
> omap_mbox_msg_send_sync() can handle the case for a special message,
> like PM, which has to respond at the higher priority than the normal
> ones.
> 
> > Having said that, this is not going to solve the lockdep warning
> > reported by Deepak - that was caused because of dspbridge's sending
> > context (and not because of the receiving context). To eliminate that
> 
> Does dspbridge really need its own defered work for sending mailbox
> messages?
> 
> For me, the problem here is the unneccesary duplication of tasklet, or
> can be said, the unnecessary use of tasklet for _sending_ mailbox
> messages in dspbridge.
> 
> http://marc.info/?l=linux-omap&m=127601655325416&w=2
> 
> I thought that bridge_msg_put()/bridge_msg_get() can use omap mbox
> APIs directly, with getting rid of its use of its own defered
> work/tasklet as pointed out in the above link. Fernando?
> 

Actually the intention of that deferred work is not to send the Mbox messages. Bridge shares a section of physical memory with the DSP and communication is established through this one. Messages (not mailbox messages) coming from users go into bridge_msg_put(), those are more significant messages containing more arguments that are understandable by a DSP task. If the Shared memory(SHM) is occupied by a previous message, messages are queued inside bridge_msg_put() and the tasklet to send them is scheduled (so that we don't loose any) due that there can only be a single message going out and one coming back at a time in this Shared memory. The usage of the Mailbox messages is to notify to the DSP (actually to interrupt it and make it go to check the SHM) that there is a message with more information in that shared memory. The tasklet is needed to serialize the upper communication that is done in the SHM level and every time that we want to send out a message previously queued we need to interrupt the DSP (use the Mailbox messages).

On the other hand PM messages are sent directly by using the Mailbox messages where we directly call omap_mbox_msg_send().

> For recieving, its defered work(tasklet) can be trigered directly in
> the above proposed callback, that callback can triger its own
> workqueue if necessary, then. I think that, for recieving, some PM
> command may has to be sent back immedieately inside of
> tasklet. omap_mbox_msg_send_sync() may handle this case.
> 
> What do you think?
> 
> > issue, I prefer fixing dspbridge to use work queues rather than using
> > spin_lock_bh in omap_mbox_msg_send. Disabling system bottom halves
> > just to send a mbox msg sounds unjustified (unless bridge really needs
> > to use tasklets instead of work queues, which I slightly doubt). What
> > do you think ?
> 
> I think that workqueue is only necessary when it has to sleep,
> otherwise tasklet is prefered. For _sending_ a message inside of
> dspbridge, I haven't found any reasonable reason to use any defered
> work(softirq, tasklet, workqueue) so far.
> 
> > Speaking of mailbox I'd like to address some issues that are code
> related:
> >
> > * Let's add mailbox API to set the callback pointer (it feels wrong to
> > let users directly manipulate the mbox structure).
> >
> > * We can also safely move the callback field to the main mbox
> > structure, since it has no usage in the TX mq; it's pretty global to
> > the whole mbox.
> >
> > * Let's make sure no one accidentally registers two receivers on the
> > same time (which would result in one overwriting the other's callback
> > field).
> 
> I'd like to allow mutiple listners in some way, as the flexibility of
> mailbox functionalty.
> 
> > * mbox_configured is a global variable and that breaks support of
> > multiple concurrent mailbox instances. Let's move this logic into the
> > instance of mbox.
> >
> > * If we make sure there are no two user of the same mailbox instance
> > (see 3rd *), we can eliminate mbox_configured and its locking (not
> > that doesn't mean there can't be two concurrent sending contexts, it
> > just means they are somewhat related, they have a common receiver,
> > which was registered using only a single call to get).
> >
> > Something like this, pls tell me what you think:
> 
> For the above 5 proposals, excpet the multiple listeners issue, they
> seems ok.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Felipe Contreras]

On Mon, Jun 14, 2010 at 11:58 AM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
> From: ext Ohad Ben-Cohen <ohad@wizery.com>
>> * Let's make sure no one accidentally registers two receivers on the
>> same time (which would result in one overwriting the other's callback
>> field).
>
> I'd like to allow mutiple listners in some way, as the flexibility of
> mailbox functionalty.

Can you explain a use case? If there's no good argument for
complexity, I would rather go for simplicity.

-- 
Felipe Contreras

Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Ohad Ben-Cohen]

Hi Hiroshi,

On Mon, Jun 14, 2010 at 3:58 AM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
> Does dspbridge really need its own defered work for sending mailbox
> messages?

We do, according to

http://permalink.gmane.org/gmane.linux.ports.arm.omap/38240

(thanks Rene for the explanation).

> For recieving, its defered work(tasklet) can be trigered directly in
> the above proposed callback, that callback can triger its own
> workqueue if necessary, then.

Agree

> I think that, for recieving, some PM
> command may has to be sent back immedieately inside of
> tasklet. omap_mbox_msg_send_sync() may handle this case.

Not sure how much QoS is an issue with mailbox (do we really have
latency issues ?), but I guess adding such interface can't hurt.

> I think that workqueue is only necessary when it has to sleep,
> otherwise tasklet is prefered.

I must say I disagree; tasklets are really not friendly to the whole
system's responsiveness and should be used only if really needed
(there was even an attempt to remove them entirely in the past:
http://lwn.net/Articles/239633/).

> I'd like to allow mutiple listners in some way, as the flexibility of
> mailbox functionalty.

This can be achieved using notifier chains of multiple listeners
callback, but I agree with Felipe: do we really want this
functionality ? I just rather not add code that no one is going to
use.

What do you say we start with enforcing only 1 listener and later, if
the use case of several listeners shows up, we'd add that support
(promise! :) ?

Thanks,
Ohad.

Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Hiroshi DOYU]

Hi Ohad,

From: ext Ohad Ben-Cohen <ohad@wizery.com>
Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
Date: Tue, 15 Jun 2010 06:43:21 +0200

> Hi Hiroshi,
> 
> On Mon, Jun 14, 2010 at 3:58 AM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
>> Does dspbridge really need its own defered work for sending mailbox
>> messages?
> 
> We do, according to
> 
> http://permalink.gmane.org/gmane.linux.ports.arm.omap/38240

I am wondering that this defered work may not be necessary just for
the serialization.

> (thanks Rene for the explanation).
> 
>> For recieving, its defered work(tasklet) can be trigered directly in
>> the above proposed callback, that callback can triger its own
>> workqueue if necessary, then.
> 
> Agree
> 
>> I think that, for recieving, some PM
>> command may has to be sent back immedieately inside of
>> tasklet. omap_mbox_msg_send_sync() may handle this case.
> 
> Not sure how much QoS is an issue with mailbox (do we really have
> latency issues ?), but I guess adding such interface can't hurt.

This solves the one case of lockdep issue.

>> I think that workqueue is only necessary when it has to sleep,
>> otherwise tasklet is prefered.
> 
> I must say I disagree; tasklets are really not friendly to the whole
> system's responsiveness and should be used only if really needed
> (there was even an attempt to remove them entirely in the past:
> http://lwn.net/Articles/239633/).

Evoking a workqueue may be overkilling just for putting several bytes
into registers since large part of contents is usually passed through
shared memory.

>> I'd like to allow mutiple listners in some way, as the flexibility of
>> mailbox functionalty.
> 
> This can be achieved using notifier chains of multiple listeners
> callback, but I agree with Felipe: do we really want this
> functionality ? I just rather not add code that no one is going to
> use.
> 
> What do you say we start with enforcing only 1 listener and later, if
> the use case of several listeners shows up, we'd add that support
> (promise! :) ?

Fair enough.

Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Ohad Ben-Cohen]

On Tue, Jun 15, 2010 at 3:04 AM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
> Fair enough.

Thanks, I'll prepare them and resubmit

Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Hiroshi DOYU]

Hi Ohad,

From: ext Ohad Ben-Cohen <ohad@wizery.com>
Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
Date: Wed, 16 Jun 2010 07:09:13 +0200

> On Tue, Jun 15, 2010 at 3:04 AM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
>> Fair enough.
> 
> Thanks, I'll prepare them and resubmit

You can use the following branch which has accumulateed unmerged
mailbox patches.

git://gitorious.org/~doyu/lk/mainline.git v2.6.35-rc3-mailbox

Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Ohad Ben-Cohen]

On Wed, Jun 16, 2010 at 8:50 AM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
> From: ext Ohad Ben-Cohen <ohad@wizery.com>
>> Thanks, I'll prepare them and resubmit
>
> You can use the following branch which has accumulateed unmerged
> mailbox patches.
>
> git://gitorious.org/~doyu/lk/mainline.git v2.6.35-rc3-mailbox


Done.


>
>
>
>

Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Hiroshi DOYU]

On Wed, 23 Jun 2010 02:29:00 +0200
ext Ohad Ben-Cohen <ohad@wizery.com> wrote:

> On Wed, Jun 16, 2010 at 8:50 AM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
> > From: ext Ohad Ben-Cohen <ohad@wizery.com>
> >> Thanks, I'll prepare them and resubmit
> >
> > You can use the following branch which has accumulateed unmerged
> > mailbox patches.
> >
> > git://gitorious.org/~doyu/lk/mainline.git v2.6.35-rc3-mailbox
> 
> 
> Done.

Thansk.
[PATCH 1-3/4] are queued on the above branch and will be pulled soon.

The following should be taken care of dspbridge:

"[PATCH 4/4] dspbridge: use mailbox API to set rx callback"

Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Hiroshi DOYU]

From: "ext Guzman Lugo, Fernando" <fernando.lugo@ti.com>
Subject: RE: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
Date: Tue, 8 Jun 2010 01:14:41 +0200

> 
> 
> Hi,
> 
>> -----Original Message-----
>> From: Ohad Ben-Cohen [mailto:ohad@wizery.com]
>> Sent: Monday, June 07, 2010 4:41 PM
>> To: Chitriki Rudramuni, Deepak; Guzman Lugo, Fernando; Ramirez Luna, Omar;
>> Kanigeri, Hari
>> Cc: linux-omap@vger.kernel.org; Hiroshi Doyu
>> Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
>> 
>> Hi Deepak,
>> 
>> On Mon, Jun 7, 2010 at 1:52 PM, Deepak Chitriki <deepak.chitriki@ti.com>
>> wrote:
>> > With this patch I observed "inconsistent lock state" warning.
>> 
>> Thanks for the report!
>> 
>> > Kfifo is acccessed in omap_mbox_msg_send() and mbox_tx_tasklet()
>> > functions.In order to protect this critical section we need to protect
>> by
>> > using spin_lock_bh() so that the tasklet cannot preempt
>> > omap_mobx_msg_send().
>> 
>> This is actually not the problem: it's ok if mbox_tx_tasklet preempts
>> omap_mbox_msg_send. In fact, such a use case is even ok if we don't
>> take a spinlock at all: kfifo is designed in a way that if you have
>> only 1 consumer and 1 producer, they can both access kfifo
>> simultaneously without any locking. That's why we don't take the
>> spinlock in the mbox_tx_tasklet. The reason, btw, that we take a
>> spinlock in omap_mbox_msg_send is to allow multiple simultaneous
>> sending contexts (taking a spinlock there ensures serialization of
>> those multiple simultaneous sending contexts).
>> 
>> The problem here lies in the fact (that I've just learnt) that
>> dspbridge also sends mbox messages from a tasklet context
>> (dpc_tasklet). Lockdep identifies that the spinlock is taken in a
>> softirq context (dspbridge's dpc_tasklet) after it was previously
>> taken in a softirq-enabled context. Your proposed fix will solve the
>> lockdep issue here, but:
> 
> I think the best thing to do here is remove the spinlock, if not,
> you are preventing that omap_mbox_msg_send be executed from a
> tasklet or isr context. That maybe in this moment it is not called
> but it could be needed in the future. The caller of
> omap_mbox_msg_send should be take care of that function can not be
> call simultaneously.

What about adding another function, omap_mbox_msg_send_sync()?

I don't want to limit the ability of having multiple senders. The
above would send the message with bypassing tasklet.

Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Felipe Contreras]

On Tue, Jun 8, 2010 at 6:46 AM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
> From: "ext Guzman Lugo, Fernando" <fernando.lugo@ti.com>
>> I think the best thing to do here is remove the spinlock, if not,
>> you are preventing that omap_mbox_msg_send be executed from a
>> tasklet or isr context. That maybe in this moment it is not called
>> but it could be needed in the future. The caller of
>> omap_mbox_msg_send should be take care of that function can not be
>> call simultaneously.
>
> What about adding another function, omap_mbox_msg_send_sync()?

That's exactly what I was thinking about when I heard the problem.
Would this function send all the msgs already queued, and then wait
for the current one actually be sent?

-- 
Felipe Contreras

Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Hiroshi DOYU]

From: ext Felipe Contreras <felipe.contreras@gmail.com>
Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
Date: Tue, 8 Jun 2010 11:43:28 +0200

> On Tue, Jun 8, 2010 at 6:46 AM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
>> From: "ext Guzman Lugo, Fernando" <fernando.lugo@ti.com>
>>> I think the best thing to do here is remove the spinlock, if not,
>>> you are preventing that omap_mbox_msg_send be executed from a
>>> tasklet or isr context. That maybe in this moment it is not called
>>> but it could be needed in the future. The caller of
>>> omap_mbox_msg_send should be take care of that function can not be
>>> call simultaneously.
>>
>> What about adding another function, omap_mbox_msg_send_sync()?
> 
> That's exactly what I was thinking about when I heard the problem.
> Would this function send all the msgs already queued, and then wait
> for the current one actually be sent?

Could be as you said. I haven't thought that much.

What I just thought was to bypass kfifo and tasklet and to put a
message directly to H/W fifo. Maily just for the above case which
Fernando described above.

Fernando, what do you think on Felipes?

RE: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Guzman Lugo, Fernando]

> -----Original Message-----
> From: Hiroshi DOYU [mailto:Hiroshi.DOYU@nokia.com]
> Sent: Tuesday, June 08, 2010 4:56 AM
> To: felipe.contreras@gmail.com
> Cc: Guzman Lugo, Fernando; ohad@wizery.com; Chitriki Rudramuni, Deepak;
> Ramirez Luna, Omar; Kanigeri, Hari; linux-omap@vger.kernel.org
> Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
> 
> From: ext Felipe Contreras <felipe.contreras@gmail.com>
> Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
> Date: Tue, 8 Jun 2010 11:43:28 +0200
> 
> > On Tue, Jun 8, 2010 at 6:46 AM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
> wrote:
> >> From: "ext Guzman Lugo, Fernando" <fernando.lugo@ti.com>
> >>> I think the best thing to do here is remove the spinlock, if not,
> >>> you are preventing that omap_mbox_msg_send be executed from a
> >>> tasklet or isr context. That maybe in this moment it is not called
> >>> but it could be needed in the future. The caller of
> >>> omap_mbox_msg_send should be take care of that function can not be
> >>> call simultaneously.
> >>
> >> What about adding another function, omap_mbox_msg_send_sync()?
> >
> > That's exactly what I was thinking about when I heard the problem.
> > Would this function send all the msgs already queued, and then wait
> > for the current one actually be sent?

A function like that could be helpful in some cases I think. Maybe and other one to flush (discard) all pending messages would be good too. So that I can clean the mailbox and would not be needed to do a mbox_put and mbox_get.

> 
> Could be as you said. I haven't thought that much.
> 
> What I just thought was to bypass kfifo and tasklet and to put a
> message directly to H/W fifo. Maily just for the above case which
> Fernando described above.
> 
> Fernando, what do you think on Felipes?

Why don't just bypass the tasklet? I think the kfifo is ok in order to not discard the message in case of full hw fifo. I was thinking in the omap_mbox_msg_send just check if we have messajes in the kfifo or if the hw fifo is full. If so, store the message en the kfifo and enable not full interrupt. Otherwise write the message to hw fifo.


	if (kfifo_len(&mq->fifo) || mbox_fifo_full(mbox)) {
		kfifo_in(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
		omap_mbox_enable_irq(mbox, IRQ_TX);
	} else
		mbox_fifo_write(mbox, msg);


and in the tx isr just send the messages store in the kfifo, reding the messages from the kfifo is very fast and at last we will be able to send 4 messages (most of the cases we will be able to send 1 message), which I think is even faster than just doing the tasklet schedule.


	while (kfifo_len(&mq->fifo) && !mbox_fifo_full(mbox)) {
		kfifo_out(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
		mbox_fifo_write(mbox, msg);
	}
	If (!mbox_fifo_full(mbox))
		omap_mbox_disable_irq(mbox, IRQ_TX);



Regards,
Fernando.

Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Deepak Chitriki]

Ohad Ben-Cohen wrote:
> Hi Deepak,
>
> On Mon, Jun 7, 2010 at 1:52 PM, Deepak Chitriki <deepak.chitriki@ti.com> wrote:
>   
>> With this patch I observed "inconsistent lock state" warning.
>>     
>
> Thanks for the report!
>
>   
>> Kfifo is acccessed in omap_mbox_msg_send() and mbox_tx_tasklet()
>> functions.In order to protect this critical section we need to protect by
>> using spin_lock_bh() so that the tasklet cannot preempt
>> omap_mobx_msg_send().
>>     
>
> This is actually not the problem: it's ok if mbox_tx_tasklet preempts
> omap_mbox_msg_send. In fact, such a use case is even ok if we don't
> take a spinlock at all: kfifo is designed in a way that if you have
> only 1 consumer and 1 producer, they can both access kfifo
> simultaneously without any locking. That's why we don't take the
> spinlock in the mbox_tx_tasklet. The reason, btw, that we take a
> spinlock in omap_mbox_msg_send is to allow multiple simultaneous
> sending contexts (taking a spinlock there ensures serialization of
> those multiple simultaneous sending contexts).
>
> The problem here lies in the fact (that I've just learnt) that
> dspbridge also sends mbox messages from a tasklet context
> (dpc_tasklet). Lockdep identifies that the spinlock is taken in a
> softirq context (dspbridge's dpc_tasklet) after it was previously
> taken in a softirq-enabled context. Your proposed fix will solve the
> lockdep issue here, but:
>
> Do we really want to have tasklets in dspbridge ? Are we that
> performance critical ?
>
> In general I'd prefer to switch to workqueues in both dspbridge and
> mailbox. I'm really not convinced we have to use tasklets in those
> modules, and workqueues are much more system friendly. This way we
> would also not have to stop all bottom halves when sending a mailbox
> message.
>
> Somewhat relevant note about mailbox performance: omap_mbox_msg_send
> often (i.e. when the kfifo is empty) can just send the message
> directly, without triggering the tasklet to do that. Applying such a
> change will substantially improve the mailbox performance, and will
> make the shift to workqueues even more reasonable. I've got a patch
> for that, I'll post it soon.
>   
tasklet is run in atomic context,so I wonder how mailbox performance 
will increase if you switch from tasklet to workqueue?

> What do you think (looping in Fernando, Omar and Hari) ?
>
> Thanks,
> Ohad.
>   

Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Ohad Ben-Cohen]

Hi Deepak,

On Mon, Jun 7, 2010 at 6:27 PM, Deepak Chitriki <deepak.chitriki@ti.com> wrote:
>> Ohad Ben-Cohen wrote:
>> Somewhat relevant note about mailbox performance: omap_mbox_msg_send
>> often (i.e. when the kfifo is empty) can just send the message
>> directly, without triggering the tasklet to do that. Applying such a
>> change will substantially improve the mailbox performance, and will
>> make the shift to workqueues even more reasonable. I've got a patch
>> for that, I'll post it soon.
>>
>
> tasklet is run in atomic context,so I wonder how mailbox performance will
> increase if you switch from tasklet to workqueue?

In general moving from tasklet to workqueues will probably increase
some scheduling latencies.

Nevertheless, if we'll send mailbox messages directly whenever
possible, without triggering any deferred context, we'll be improving
mailbox performance on average, because most of the time we will get
rid of the scheduling latencies entirely.

Thanks,
Ohad.

Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Hiroshi DOYU]

From: ext Ohad Ben-Cohen <ohad@wizery.com>
Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
Date: Tue, 8 Jun 2010 05:11:50 +0200

> Hi Deepak,
> 
> On Mon, Jun 7, 2010 at 6:27 PM, Deepak Chitriki <deepak.chitriki@ti.com> wrote:
>>> Ohad Ben-Cohen wrote:
>>> Somewhat relevant note about mailbox performance: omap_mbox_msg_send
>>> often (i.e. when the kfifo is empty) can just send the message
>>> directly, without triggering the tasklet to do that. Applying such a
>>> change will substantially improve the mailbox performance, and will
>>> make the shift to workqueues even more reasonable. I've got a patch
>>> for that, I'll post it soon.
>>>
>>
>> tasklet is run in atomic context,so I wonder how mailbox performance will
>> increase if you switch from tasklet to workqueue?
> 
> In general moving from tasklet to workqueues will probably increase
> some scheduling latencies.
> 
> Nevertheless, if we'll send mailbox messages directly whenever
> possible, without triggering any deferred context, we'll be improving
> mailbox performance on average, because most of the time we will get
> rid of the scheduling latencies entirely.

I think that this depends on the situation.

1) too many messages are coming, buffering and tasklet works
   efficiently at onece, for good throughput.
2) too few messages, the latency won't be a problem? Or some PM case
   may be concerned.

I guess that most of the case, we just care about the throughput of
encoding and decoding, but I am afraid that data themselves are sent
via shared memory and the traffic of mailbox message may not be so
big. I'd like to see the actual data for practical usecases.

Does TI have some typical test pattern to measure throughput? Or Felipe?

Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Felipe Contreras]

On Tue, Jun 8, 2010 at 6:55 AM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
> From: ext Ohad Ben-Cohen <ohad@wizery.com>
> Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
> Date: Tue, 8 Jun 2010 05:11:50 +0200
>
>> Hi Deepak,
>>
>> On Mon, Jun 7, 2010 at 6:27 PM, Deepak Chitriki <deepak.chitriki@ti.com> wrote:
>>>> Ohad Ben-Cohen wrote:
>>>> Somewhat relevant note about mailbox performance: omap_mbox_msg_send
>>>> often (i.e. when the kfifo is empty) can just send the message
>>>> directly, without triggering the tasklet to do that. Applying such a
>>>> change will substantially improve the mailbox performance, and will
>>>> make the shift to workqueues even more reasonable. I've got a patch
>>>> for that, I'll post it soon.
>>>>
>>>
>>> tasklet is run in atomic context,so I wonder how mailbox performance will
>>> increase if you switch from tasklet to workqueue?
>>
>> In general moving from tasklet to workqueues will probably increase
>> some scheduling latencies.
>>
>> Nevertheless, if we'll send mailbox messages directly whenever
>> possible, without triggering any deferred context, we'll be improving
>> mailbox performance on average, because most of the time we will get
>> rid of the scheduling latencies entirely.
>
> I think that this depends on the situation.
>
> 1) too many messages are coming, buffering and tasklet works
>   efficiently at onece, for good throughput.
> 2) too few messages, the latency won't be a problem? Or some PM case
>   may be concerned.
>
> I guess that most of the case, we just care about the throughput of
> encoding and decoding, but I am afraid that data themselves are sent
> via shared memory and the traffic of mailbox message may not be so
> big. I'd like to see the actual data for practical usecases.
>
> Does TI have some typical test pattern to measure throughput? Or Felipe?

What exactly are you interested in? The amount of messages being sent
on typical video encoding/decoding scenarios? My guess is that it
would be one or two messages per frame (60 msgs/s tops).

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

Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Hiroshi DOYU]

From: ext Ohad Ben-Cohen <ohad@wizery.com>
Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
Date: Mon, 7 Jun 2010 23:40:34 +0200

> Hi Deepak,
> 
> On Mon, Jun 7, 2010 at 1:52 PM, Deepak Chitriki <deepak.chitriki@ti.com> wrote:
>> With this patch I observed "inconsistent lock state" warning.
> 
> Thanks for the report!
> 
>> Kfifo is acccessed in omap_mbox_msg_send() and mbox_tx_tasklet()
>> functions.In order to protect this critical section we need to protect by
>> using spin_lock_bh() so that the tasklet cannot preempt
>> omap_mobx_msg_send().
> 
> This is actually not the problem: it's ok if mbox_tx_tasklet preempts
> omap_mbox_msg_send. In fact, such a use case is even ok if we don't
> take a spinlock at all: kfifo is designed in a way that if you have
> only 1 consumer and 1 producer, they can both access kfifo
> simultaneously without any locking. That's why we don't take the
> spinlock in the mbox_tx_tasklet. The reason, btw, that we take a
> spinlock in omap_mbox_msg_send is to allow multiple simultaneous
> sending contexts (taking a spinlock there ensures serialization of
> those multiple simultaneous sending contexts).
> 
> The problem here lies in the fact (that I've just learnt) that
> dspbridge also sends mbox messages from a tasklet context
> (dpc_tasklet). Lockdep identifies that the spinlock is taken in a
> softirq context (dspbridge's dpc_tasklet) after it was previously
> taken in a softirq-enabled context. Your proposed fix will solve the
> lockdep issue here, but:
> 
> Do we really want to have tasklets in dspbridge ? Are we that
> performance critical ?

Can't a whole contents of bridge_msg_get()/bridge_msg_put() be
replaced with omap mailbox APIs?

It seems that dspbridge does the same message queueing and deferred
execution as omap mailbox does in the above 2 functions. At least
bridge_msg_put() looks similar to omap_mbox_msg_send(). If this works,
we can reduce the size of dspbridge again;)

RE: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo

[Guzman Lugo, Fernando]

> -----Original Message-----
> From: Hiroshi DOYU [mailto:Hiroshi.DOYU@nokia.com]
> Sent: Monday, June 07, 2010 10:40 PM
> To: ohad@wizery.com
> Cc: Chitriki Rudramuni, Deepak; Guzman Lugo, Fernando; Ramirez Luna, Omar;
> Kanigeri, Hari; linux-omap@vger.kernel.org
> Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
> 
> From: ext Ohad Ben-Cohen <ohad@wizery.com>
> Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
> Date: Mon, 7 Jun 2010 23:40:34 +0200
> 
> > Hi Deepak,
> >
> > On Mon, Jun 7, 2010 at 1:52 PM, Deepak Chitriki <deepak.chitriki@ti.com>
> wrote:
> >> With this patch I observed "inconsistent lock state" warning.
> >
> > Thanks for the report!
> >
> >> Kfifo is acccessed in omap_mbox_msg_send() and mbox_tx_tasklet()
> >> functions.In order to protect this critical section we need to protect
> by
> >> using spin_lock_bh() so that the tasklet cannot preempt
> >> omap_mobx_msg_send().
> >
> > This is actually not the problem: it's ok if mbox_tx_tasklet preempts
> > omap_mbox_msg_send. In fact, such a use case is even ok if we don't
> > take a spinlock at all: kfifo is designed in a way that if you have
> > only 1 consumer and 1 producer, they can both access kfifo
> > simultaneously without any locking. That's why we don't take the
> > spinlock in the mbox_tx_tasklet. The reason, btw, that we take a
> > spinlock in omap_mbox_msg_send is to allow multiple simultaneous
> > sending contexts (taking a spinlock there ensures serialization of
> > those multiple simultaneous sending contexts).
> >
> > The problem here lies in the fact (that I've just learnt) that
> > dspbridge also sends mbox messages from a tasklet context
> > (dpc_tasklet). Lockdep identifies that the spinlock is taken in a
> > softirq context (dspbridge's dpc_tasklet) after it was previously
> > taken in a softirq-enabled context. Your proposed fix will solve the
> > lockdep issue here, but:
> >
> > Do we really want to have tasklets in dspbridge ? Are we that
> > performance critical ?
> 
> Can't a whole contents of bridge_msg_get()/bridge_msg_put() be
> replaced with omap mailbox APIs?
> 
> It seems that dspbridge does the same message queueing and deferred
> execution as omap mailbox does in the above 2 functions. At least
> bridge_msg_put() looks similar to omap_mbox_msg_send(). If this works,
> we can reduce the size of dspbridge again;)

Yes, it has a pretty similar method to send the messages. However the messages send by bridge_msg_put() is a shared memory message, which has a cmd fiel and 2 arguments fields and the bridge used a mailbox message just to indicate to the DSP that it has some pending shared memory messages.


Regards,
Fernando.

Re: [PATCH v3 0/4] omap: mailbox: cleanup & simplify

[Hiroshi DOYU]

From: ext Ohad Ben-Cohen <ohad@wizery.com>
Subject: [PATCH v3 0/4] omap: mailbox: cleanup & simplify
Date: Wed, 5 May 2010 17:33:05 +0200

> This series includes comments from Hiroshi and Tony (thanks!).
> 
> Changes since v2:
> - add mbox_kfifo_size module parameter sanity checks
> - remove (un)likely macros from cold mailbox paths

Acked-by: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>

> 
> Thanks,
> Ohad.
> 
> ---
> If you want, you can also reach me at < ohadb at ti dot com >.
> 
> Ohad Ben-Cohen (4):
>   omap: mailbox: convert rwlocks to spinlock
>   omap: mailbox cleanup: split MODULE_AUTHOR line
>   omap: mailbox: remove (un)likely macros from cold paths
>   omap: mailbox: convert block api to kfifo
> 
>  arch/arm/mach-omap2/mailbox.c             |    3 +-
>  arch/arm/plat-omap/Kconfig                |    9 ++
>  arch/arm/plat-omap/include/plat/mailbox.h |    4 +-
>  arch/arm/plat-omap/mailbox.c              |  155 ++++++++++++++---------------
>  4 files changed, 87 insertions(+), 84 deletions(-)
>