[PATCH 0/7] omap:mailbox-enhancements and fixes

[PATCH 0/7] omap:mailbox-enhancements and fixes

[Hari Kanigeri]

My apologies for spamming linux omap.
Resending again with the patch set copying correct linux arm mailing list.
please ignore the previous patchsets.
[http://www.spinics.net/lists/linux-omap/msg38782.html]
[http://www.mail-archive.com/linux-omap@vger.kernel.org/msg37214.html]

This patch set includes following mailbox enhancements and fixes.
        
- Fix in RX interrupt disable mechanism for OMAP4
- Fix checkpatch warnings
- Resolve multiple receiver problem in OMAP4 where there is
  only one interrupt line shared between Ducati and Tesla.
- Protect the Mailbox's internal callback function pointer from being
  set directly by Users. Added option for multiple listeners on a
  mailbox instance.
- Send Mailbox message in Process context to avoid the latency
  involved in scheduling Tasklet for every Mailbox message. Schedule
  Tasklet only when the mailbox fifo is full
- There is no explicit mailbox configuration register to enable mailbox
  clocks. Define a dummy clock for mailbox to avoid addign cpu check for
  omap4 in mailbox driver.
- The patch from Fernando was sent to LO, but looks like it
  didn't get merged, resending the patch after revising and rebasing.
  https://patchwork.kernel.org/patch/105650/

Fernando Guzman Lugo (1):
  mailbox: change full flag per mailbox queue instead of global

Hari Kanigeri (6):
  omap:mailbox: fix rx interrupt disable in omap4
  omap:mailbox-fix checkpatch warnings
  omap:mailbox-send message in process context
  omap:mailbox-resolve multiple receiver problem
  omap:mailbox-add notification support for multiple readers
  omap:clocks44x-add dummy clock for mailbox

 arch/arm/mach-omap2/clock44xx_data.c      |    1 +
 arch/arm/mach-omap2/mailbox.c             |    5 +-
 arch/arm/plat-omap/include/plat/mailbox.h |   10 ++-
 arch/arm/plat-omap/mailbox.c              |  148 +++++++++++++++++------------
 4 files changed, 98 insertions(+), 66 deletions(-)

[PATCH 0/7] omap:mailbox-enhancements and fixes

[Hari Kanigeri]

My apologies for spamming linux omap.
Resending again with the patch set copying correct linux arm mailing list.
please ignore the previous patchsets.
[http://www.spinics.net/lists/linux-omap/msg38782.html]
[http://www.mail-archive.com/linux-omap at vger.kernel.org/msg37214.html]

This patch set includes following mailbox enhancements and fixes.
        
- Fix in RX interrupt disable mechanism for OMAP4
- Fix checkpatch warnings
- Resolve multiple receiver problem in OMAP4 where there is
  only one interrupt line shared between Ducati and Tesla.
- Protect the Mailbox's internal callback function pointer from being
  set directly by Users. Added option for multiple listeners on a
  mailbox instance.
- Send Mailbox message in Process context to avoid the latency
  involved in scheduling Tasklet for every Mailbox message. Schedule
  Tasklet only when the mailbox fifo is full
- There is no explicit mailbox configuration register to enable mailbox
  clocks. Define a dummy clock for mailbox to avoid addign cpu check for
  omap4 in mailbox driver.
- The patch from Fernando was sent to LO, but looks like it
  didn't get merged, resending the patch after revising and rebasing.
  https://patchwork.kernel.org/patch/105650/

Fernando Guzman Lugo (1):
  mailbox: change full flag per mailbox queue instead of global

Hari Kanigeri (6):
  omap:mailbox: fix rx interrupt disable in omap4
  omap:mailbox-fix checkpatch warnings
  omap:mailbox-send message in process context
  omap:mailbox-resolve multiple receiver problem
  omap:mailbox-add notification support for multiple readers
  omap:clocks44x-add dummy clock for mailbox

 arch/arm/mach-omap2/clock44xx_data.c      |    1 +
 arch/arm/mach-omap2/mailbox.c             |    5 +-
 arch/arm/plat-omap/include/plat/mailbox.h |   10 ++-
 arch/arm/plat-omap/mailbox.c              |  148 +++++++++++++++++------------
 4 files changed, 98 insertions(+), 66 deletions(-)

[PATCH 1/7] mailbox: change full flag per mailbox queue instead of global

[Hari Kanigeri]

From: Fernando Guzman Lugo <x0095840@ti.com>

As pointed by Ohad Ben-Cohen, the variable rq_full flag is a
global variable, so if there are multiple mailbox users
there will be conflics. Now there is a full flag per
mailbox queue.

Version 2:
- Rebase to the latest.
Version 3:
- Remove spin_lock protection. When the full flag is
true the interrupt for that mailbox is disabled. So there
is no race condition if full flag is modified before
calling omap_mbox_enable_irq.

Reported-by: Ohad Ben-Cohen <ohad@wizery.com>
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
---
 arch/arm/plat-omap/include/plat/mailbox.h |    1 +
 arch/arm/plat-omap/mailbox.c              |    7 +++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/mailbox.h b/arch/arm/plat-omap/include/plat/mailbox.h
index 9976565..13f2ef3 100644
--- a/arch/arm/plat-omap/include/plat/mailbox.h
+++ b/arch/arm/plat-omap/include/plat/mailbox.h
@@ -48,6 +48,7 @@ struct omap_mbox_queue {
 	struct tasklet_struct	tasklet;
 	int	(*callback)(void *);
 	struct omap_mbox	*mbox;
+	bool full;
 };
 
 struct omap_mbox {
diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index d2fafb8..9ce3570 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -33,7 +33,6 @@
 
 static struct workqueue_struct *mboxd;
 static struct omap_mbox **mboxes;
-static bool rq_full;
 
 static int mbox_configured;
 static DEFINE_MUTEX(mbox_configured_lock);
@@ -148,6 +147,10 @@ static void mbox_rx_work(struct work_struct *work)
 
 		if (mq->callback)
 			mq->callback((void *)msg);
+		if (mq->full) {
+			mq->full = false;
+			omap_mbox_enable_irq(mq->mbox, IRQ_RX);
+		}
 	}
 }
 
@@ -170,7 +173,7 @@ static void __mbox_rx_interrupt(struct omap_mbox *mbox)
 	while (!mbox_fifo_empty(mbox)) {
 		if (unlikely(kfifo_avail(&mq->fifo) < sizeof(msg))) {
 			omap_mbox_disable_irq(mbox, IRQ_RX);
-			rq_full = true;
+			mq->full = true;
 			goto nomem;
 		}
 
-- 
1.7.0

[PATCH 1/7] mailbox: change full flag per mailbox queue instead of global

[Hari Kanigeri]

From: Fernando Guzman Lugo <x0095840@ti.com>

As pointed by Ohad Ben-Cohen, the variable rq_full flag is a
global variable, so if there are multiple mailbox users
there will be conflics. Now there is a full flag per
mailbox queue.

Version 2:
- Rebase to the latest.
Version 3:
- Remove spin_lock protection. When the full flag is
true the interrupt for that mailbox is disabled. So there
is no race condition if full flag is modified before
calling omap_mbox_enable_irq.

Reported-by: Ohad Ben-Cohen <ohad@wizery.com>
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
---
 arch/arm/plat-omap/include/plat/mailbox.h |    1 +
 arch/arm/plat-omap/mailbox.c              |    7 +++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/mailbox.h b/arch/arm/plat-omap/include/plat/mailbox.h
index 9976565..13f2ef3 100644
--- a/arch/arm/plat-omap/include/plat/mailbox.h
+++ b/arch/arm/plat-omap/include/plat/mailbox.h
@@ -48,6 +48,7 @@ struct omap_mbox_queue {
 	struct tasklet_struct	tasklet;
 	int	(*callback)(void *);
 	struct omap_mbox	*mbox;
+	bool full;
 };
 
 struct omap_mbox {
diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index d2fafb8..9ce3570 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -33,7 +33,6 @@
 
 static struct workqueue_struct *mboxd;
 static struct omap_mbox **mboxes;
-static bool rq_full;
 
 static int mbox_configured;
 static DEFINE_MUTEX(mbox_configured_lock);
@@ -148,6 +147,10 @@ static void mbox_rx_work(struct work_struct *work)
 
 		if (mq->callback)
 			mq->callback((void *)msg);
+		if (mq->full) {
+			mq->full = false;
+			omap_mbox_enable_irq(mq->mbox, IRQ_RX);
+		}
 	}
 }
 
@@ -170,7 +173,7 @@ static void __mbox_rx_interrupt(struct omap_mbox *mbox)
 	while (!mbox_fifo_empty(mbox)) {
 		if (unlikely(kfifo_avail(&mq->fifo) < sizeof(msg))) {
 			omap_mbox_disable_irq(mbox, IRQ_RX);
-			rq_full = true;
+			mq->full = true;
 			goto nomem;
 		}
 
-- 
1.7.0

[PATCH 2/7] omap:mailbox: fix rx interrupt disable in omap4

[Hari Kanigeri]

disabling rx interrupt on omap4 is different than its pre-decessors.
The bit in OMAP4_MAILBOX_IRQENABLE_CLR should be set to disable the
interrupts instead of clearing the bit.

Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
---
 arch/arm/mach-omap2/mailbox.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/mailbox.c b/arch/arm/mach-omap2/mailbox.c
index 42dbfa4..82b5ced 100644
--- a/arch/arm/mach-omap2/mailbox.c
+++ b/arch/arm/mach-omap2/mailbox.c
@@ -195,7 +195,10 @@ static void omap2_mbox_disable_irq(struct omap_mbox *mbox,
 	struct omap_mbox2_priv *p = (struct omap_mbox2_priv *)mbox->priv;
 	u32 l, bit = (irq == IRQ_TX) ? p->notfull_bit : p->newmsg_bit;
 	l = mbox_read_reg(p->irqdisable);
-	l &= ~bit;
+	if (cpu_is_omap44xx())
+		l |= bit;
+	else
+		l &= ~bit;
 	mbox_write_reg(l, p->irqdisable);
 }
 
-- 
1.7.0

[PATCH 2/7] omap:mailbox: fix rx interrupt disable in omap4

[Hari Kanigeri]

disabling rx interrupt on omap4 is different than its pre-decessors.
The bit in OMAP4_MAILBOX_IRQENABLE_CLR should be set to disable the
interrupts instead of clearing the bit.

Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
---
 arch/arm/mach-omap2/mailbox.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/mailbox.c b/arch/arm/mach-omap2/mailbox.c
index 42dbfa4..82b5ced 100644
--- a/arch/arm/mach-omap2/mailbox.c
+++ b/arch/arm/mach-omap2/mailbox.c
@@ -195,7 +195,10 @@ static void omap2_mbox_disable_irq(struct omap_mbox *mbox,
 	struct omap_mbox2_priv *p = (struct omap_mbox2_priv *)mbox->priv;
 	u32 l, bit = (irq == IRQ_TX) ? p->notfull_bit : p->newmsg_bit;
 	l = mbox_read_reg(p->irqdisable);
-	l &= ~bit;
+	if (cpu_is_omap44xx())
+		l |= bit;
+	else
+		l &= ~bit;
 	mbox_write_reg(l, p->irqdisable);
 }
 
-- 
1.7.0

[PATCH 3/7] omap:mailbox-fix checkpatch warnings

[Hari Kanigeri]

Fix the checkpatch warnings observed in mailbox module

Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
---
 arch/arm/plat-omap/mailbox.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index 9ce3570..ed960c1 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -279,11 +279,11 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
 
 	return 0;
 
- fail_alloc_rxq:
+fail_alloc_rxq:
 	mbox_queue_free(mbox->txq);
- fail_alloc_txq:
+fail_alloc_txq:
 	free_irq(mbox->irq, mbox);
- fail_request_irq:
+fail_request_irq:
 	if (mbox->ops->shutdown)
 		mbox->ops->shutdown(mbox);
 
@@ -394,7 +394,8 @@ static int __init omap_mbox_init(void)
 
 	/* 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));
+	mbox_kfifo_size = max_t(unsigned int, mbox_kfifo_size,
+						sizeof(mbox_msg_t));
 
 	return 0;
 }
-- 
1.7.0

[PATCH 3/7] omap:mailbox-fix checkpatch warnings

[Hari Kanigeri]

Fix the checkpatch warnings observed in mailbox module

Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
---
 arch/arm/plat-omap/mailbox.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index 9ce3570..ed960c1 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -279,11 +279,11 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
 
 	return 0;
 
- fail_alloc_rxq:
+fail_alloc_rxq:
 	mbox_queue_free(mbox->txq);
- fail_alloc_txq:
+fail_alloc_txq:
 	free_irq(mbox->irq, mbox);
- fail_request_irq:
+fail_request_irq:
 	if (mbox->ops->shutdown)
 		mbox->ops->shutdown(mbox);
 
@@ -394,7 +394,8 @@ static int __init omap_mbox_init(void)
 
 	/* 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));
+	mbox_kfifo_size = max_t(unsigned int, mbox_kfifo_size,
+						sizeof(mbox_msg_t));
 
 	return 0;
 }
-- 
1.7.0

[PATCH 4/7] omap:mailbox-send message in process context

[Hari Kanigeri]

Schedule the Tasklet to send only when mailbox fifo is full, else
send the message in the Process context. This would avoid
needless scheduling of Tasklet for every message transfer

Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
---
 arch/arm/plat-omap/mailbox.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index ed960c1..a4170c7 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -92,20 +92,25 @@ 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;
 		goto out;
 	}
 
+	if (!__mbox_poll_for_space(mbox)) {
+		mbox_fifo_write(mbox, msg);
+		goto out;
+	}
+
 	len = kfifo_in(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
 	WARN_ON(len != sizeof(msg));
 
 	tasklet_schedule(&mbox->txq->tasklet);
 
 out:
-	spin_unlock(&mq->lock);
+	spin_unlock_bh(&mq->lock);
 	return ret;
 }
 EXPORT_SYMBOL(omap_mbox_msg_send);
-- 
1.7.0

[PATCH 4/7] omap:mailbox-send message in process context

[Hari Kanigeri]

Schedule the Tasklet to send only when mailbox fifo is full, else
send the message in the Process context. This would avoid
needless scheduling of Tasklet for every message transfer

Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
---
 arch/arm/plat-omap/mailbox.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index ed960c1..a4170c7 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -92,20 +92,25 @@ 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;
 		goto out;
 	}
 
+	if (!__mbox_poll_for_space(mbox)) {
+		mbox_fifo_write(mbox, msg);
+		goto out;
+	}
+
 	len = kfifo_in(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
 	WARN_ON(len != sizeof(msg));
 
 	tasklet_schedule(&mbox->txq->tasklet);
 
 out:
-	spin_unlock(&mq->lock);
+	spin_unlock_bh(&mq->lock);
 	return ret;
 }
 EXPORT_SYMBOL(omap_mbox_msg_send);
-- 
1.7.0

[PATCH 5/7] omap:mailbox-resolve multiple receiver problem

[Hari Kanigeri]

OMAP4 shares one interrupt line for all the mailbox instances.
The ISR is handling only the mailbox instance that was registered last.
So if both mailbox instances are running at the same time, the first mailbox
that registered wouldn't get the mailbox message. The same issue is present
in Transmit Interrupt case too. Only the last registered mailbox is handled.

The fix is to iterate through the list of mailboxes that were registered
checking for the mailbox TX and RX interrupt source.

Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
Signed-off-by: Ramesh Gupta <grgupta@ti.com>
Signed-off-by: Subramaniam C.A <subramaniam.ca@ti.com>
---
 arch/arm/plat-omap/mailbox.c |   21 +++++++++++++--------
 1 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index a4170c7..1727548 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -174,6 +174,7 @@ static void __mbox_rx_interrupt(struct omap_mbox *mbox)
 	struct omap_mbox_queue *mq = mbox->rxq;
 	mbox_msg_t msg;
 	int len;
+	bool msg_rx = false;
 
 	while (!mbox_fifo_empty(mbox)) {
 		if (unlikely(kfifo_avail(&mq->fifo) < sizeof(msg))) {
@@ -181,7 +182,7 @@ static void __mbox_rx_interrupt(struct omap_mbox *mbox)
 			mq->full = true;
 			goto nomem;
 		}
-
+		msg_rx = true;
 		msg = mbox_fifo_read(mbox);
 
 		len = kfifo_in(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
@@ -192,21 +193,25 @@ static void __mbox_rx_interrupt(struct omap_mbox *mbox)
 	}
 
 	/* no more messages in the fifo. clear IRQ source. */
-	ack_mbox_irq(mbox, IRQ_RX);
+	if (msg_rx)
+		ack_mbox_irq(mbox, IRQ_RX);
 nomem:
-	queue_work(mboxd, &mbox->rxq->work);
+	if (msg_rx)
+		queue_work(mboxd, &mbox->rxq->work);
 }
 
 static irqreturn_t mbox_interrupt(int irq, void *p)
 {
 	struct omap_mbox *mbox = p;
 
-	if (is_mbox_irq(mbox, IRQ_TX))
-		__mbox_tx_interrupt(mbox);
-
-	if (is_mbox_irq(mbox, IRQ_RX))
-		__mbox_rx_interrupt(mbox);
+	for (i = 0; mboxes[i]; i++)  {
+		struct omap_mbox *mbox = mboxes[i];
+		if (is_mbox_irq(mbox, IRQ_TX))
+			__mbox_tx_interrupt(mbox);
 
+		if (is_mbox_irq(mbox, IRQ_RX))
+			__mbox_rx_interrupt(mbox);
+	}
 	return IRQ_HANDLED;
 }
 
-- 
1.7.0

[PATCH 5/7] omap:mailbox-resolve multiple receiver problem

[Hari Kanigeri]

OMAP4 shares one interrupt line for all the mailbox instances.
The ISR is handling only the mailbox instance that was registered last.
So if both mailbox instances are running at the same time, the first mailbox
that registered wouldn't get the mailbox message. The same issue is present
in Transmit Interrupt case too. Only the last registered mailbox is handled.

The fix is to iterate through the list of mailboxes that were registered
checking for the mailbox TX and RX interrupt source.

Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
Signed-off-by: Ramesh Gupta <grgupta@ti.com>
Signed-off-by: Subramaniam C.A <subramaniam.ca@ti.com>
---
 arch/arm/plat-omap/mailbox.c |   21 +++++++++++++--------
 1 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index a4170c7..1727548 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -174,6 +174,7 @@ static void __mbox_rx_interrupt(struct omap_mbox *mbox)
 	struct omap_mbox_queue *mq = mbox->rxq;
 	mbox_msg_t msg;
 	int len;
+	bool msg_rx = false;
 
 	while (!mbox_fifo_empty(mbox)) {
 		if (unlikely(kfifo_avail(&mq->fifo) < sizeof(msg))) {
@@ -181,7 +182,7 @@ static void __mbox_rx_interrupt(struct omap_mbox *mbox)
 			mq->full = true;
 			goto nomem;
 		}
-
+		msg_rx = true;
 		msg = mbox_fifo_read(mbox);
 
 		len = kfifo_in(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
@@ -192,21 +193,25 @@ static void __mbox_rx_interrupt(struct omap_mbox *mbox)
 	}
 
 	/* no more messages in the fifo. clear IRQ source. */
-	ack_mbox_irq(mbox, IRQ_RX);
+	if (msg_rx)
+		ack_mbox_irq(mbox, IRQ_RX);
 nomem:
-	queue_work(mboxd, &mbox->rxq->work);
+	if (msg_rx)
+		queue_work(mboxd, &mbox->rxq->work);
 }
 
 static irqreturn_t mbox_interrupt(int irq, void *p)
 {
 	struct omap_mbox *mbox = p;
 
-	if (is_mbox_irq(mbox, IRQ_TX))
-		__mbox_tx_interrupt(mbox);
-
-	if (is_mbox_irq(mbox, IRQ_RX))
-		__mbox_rx_interrupt(mbox);
+	for (i = 0; mboxes[i]; i++)  {
+		struct omap_mbox *mbox = mboxes[i];
+		if (is_mbox_irq(mbox, IRQ_TX))
+			__mbox_tx_interrupt(mbox);
 
+		if (is_mbox_irq(mbox, IRQ_RX))
+			__mbox_rx_interrupt(mbox);
+	}
 	return IRQ_HANDLED;
 }
 
-- 
1.7.0

[PATCH 6/7] omap:mailbox-add notification support for multiple readers

[Hari Kanigeri]

In the current mailbox driver, the mailbox internal pointer for
callback can be directly manipulated by the Users, so a second
User can easily corrupt the first user's callback pointer.
The initial effort to correct this issue can be referred here:
https://patchwork.kernel.org/patch/107520/

Along with fixing the above stated issue, this patch  adds the
flexibility option to register notifications from
multiple readers to the events received on a mailbox instance.
The discussion regarding this can be referred here.
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg30671.html

Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
 arch/arm/plat-omap/include/plat/mailbox.h |    9 ++-
 arch/arm/plat-omap/mailbox.c              |  102 ++++++++++++++++-------------
 2 files changed, 62 insertions(+), 49 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/mailbox.h b/arch/arm/plat-omap/include/plat/mailbox.h
index 13f2ef3..4c35654 100644
--- a/arch/arm/plat-omap/include/plat/mailbox.h
+++ b/arch/arm/plat-omap/include/plat/mailbox.h
@@ -8,6 +8,7 @@
 #include <linux/interrupt.h>
 #include <linux/device.h>
 #include <linux/kfifo.h>
+#include <linux/notifier.h>
 
 typedef u32 mbox_msg_t;
 struct omap_mbox;
@@ -46,7 +47,6 @@ struct omap_mbox_queue {
 	struct kfifo		fifo;
 	struct work_struct	work;
 	struct tasklet_struct	tasklet;
-	int	(*callback)(void *);
 	struct omap_mbox	*mbox;
 	bool full;
 };
@@ -58,13 +58,16 @@ struct omap_mbox {
 	struct omap_mbox_ops	*ops;
 	struct device		*dev;
 	void			*priv;
+
+	int			use_count;
+	struct blocking_notifier_head	notifier;
 };
 
 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 *);
-void omap_mbox_put(struct omap_mbox *);
+struct omap_mbox *omap_mbox_get(const char *, struct notifier_block *nb);
+void omap_mbox_put(struct omap_mbox *mbox, struct notifier_block *nb);
 
 int omap_mbox_register(struct device *parent, struct omap_mbox **);
 int omap_mbox_unregister(void);
diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index 1727548..35956f5 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -28,6 +28,7 @@
 #include <linux/slab.h>
 #include <linux/kfifo.h>
 #include <linux/err.h>
+#include <linux/notifier.h>
 
 #include <plat/mailbox.h>
 
@@ -150,8 +151,8 @@ static void mbox_rx_work(struct work_struct *work)
 		len = kfifo_out(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
 		WARN_ON(len != sizeof(msg));
 
-		if (mq->callback)
-			mq->callback((void *)msg);
+		blocking_notifier_call_chain(&mq->mbox->notifier, len,
+							(void *)msg);
 		if (mq->full) {
 			mq->full = false;
 			omap_mbox_enable_irq(mq->mbox, IRQ_RX);
@@ -202,7 +203,7 @@ nomem:
 
 static irqreturn_t mbox_interrupt(int irq, void *p)
 {
-	struct omap_mbox *mbox = p;
+	int i;
 
 	for (i = 0; mboxes[i]; i++)  {
 		struct omap_mbox *mbox = mboxes[i];
@@ -252,41 +253,39 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
 	int ret = 0;
 	struct omap_mbox_queue *mq;
 
-	if (mbox->ops->startup) {
-		mutex_lock(&mbox_configured_lock);
-		if (!mbox_configured)
+	mutex_lock(&mbox_configured_lock);
+	if (!mbox_configured++) {
+		if (likely(mbox->ops->startup)) {
 			ret = mbox->ops->startup(mbox);
-
-		if (ret) {
-			mutex_unlock(&mbox_configured_lock);
-			return ret;
+			if (unlikely(ret))
+				goto fail_startup;
 		}
-		mbox_configured++;
-		mutex_unlock(&mbox_configured_lock);
 	}
 
-	ret = request_irq(mbox->irq, mbox_interrupt, IRQF_SHARED,
-				mbox->name, mbox);
-	if (ret) {
-		printk(KERN_ERR
-			"failed to register mailbox interrupt:%d\n", ret);
-		goto fail_request_irq;
-	}
-
-	mq = mbox_queue_alloc(mbox, NULL, mbox_tx_tasklet);
-	if (!mq) {
-		ret = -ENOMEM;
-		goto fail_alloc_txq;
-	}
-	mbox->txq = mq;
+	if (!mbox->use_count++) {
+		ret = request_irq(mbox->irq, mbox_interrupt, IRQF_SHARED,
+					mbox->name, mbox);
+		if (unlikely(ret)) {
+			pr_err("failed to register mailbox interrupt:%d\n",
+									ret);
+			goto fail_request_irq;
+		}
+		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_rx_work, NULL);
-	if (!mq) {
-		ret = -ENOMEM;
-		goto fail_alloc_rxq;
+		mq = mbox_queue_alloc(mbox, mbox_rx_work, NULL);
+		if (!mq) {
+			ret = -ENOMEM;
+			goto fail_alloc_rxq;
+		}
+		mbox->rxq = mq;
+		mq->mbox = mbox;
 	}
-	mbox->rxq = mq;
-
+	mutex_unlock(&mbox_configured_lock);
 	return 0;
 
 fail_alloc_rxq:
@@ -296,29 +295,36 @@ fail_alloc_txq:
 fail_request_irq:
 	if (mbox->ops->shutdown)
 		mbox->ops->shutdown(mbox);
-
+	mbox->use_count--;
+fail_startup:
+	mbox_configured--;
+	mutex_unlock(&mbox_configured_lock);
 	return ret;
 }
 
 static void omap_mbox_fini(struct omap_mbox *mbox)
 {
-	free_irq(mbox->irq, mbox);
-	tasklet_kill(&mbox->txq->tasklet);
-	flush_work(&mbox->rxq->work);
-	mbox_queue_free(mbox->txq);
-	mbox_queue_free(mbox->rxq);
 
-	if (mbox->ops->shutdown) {
-		mutex_lock(&mbox_configured_lock);
-		if (mbox_configured > 0)
-			mbox_configured--;
-		if (!mbox_configured)
+	mutex_lock(&mbox_configured_lock);
+
+	if (!--mbox->use_count) {
+		tasklet_kill(&mbox->txq->tasklet);
+		flush_work(&mbox->rxq->work);
+		mbox_queue_free(mbox->txq);
+		mbox_queue_free(mbox->rxq);
+	}
+
+	if (likely(mbox->ops->shutdown)) {
+		if (!--mbox_configured) {
+			free_irq(mbox->irq, mbox);
 			mbox->ops->shutdown(mbox);
-		mutex_unlock(&mbox_configured_lock);
+		}
 	}
+
+	mutex_unlock(&mbox_configured_lock);
 }
 
-struct omap_mbox *omap_mbox_get(const char *name)
+struct omap_mbox *omap_mbox_get(const char *name, struct notifier_block *nb)
 {
 	struct omap_mbox *mbox;
 	int ret;
@@ -336,13 +342,16 @@ struct omap_mbox *omap_mbox_get(const char *name)
 	ret = omap_mbox_startup(mbox);
 	if (ret)
 		return ERR_PTR(-ENODEV);
+	if (nb)
+		blocking_notifier_chain_register(&mbox->notifier, nb);
 
 	return mbox;
 }
 EXPORT_SYMBOL(omap_mbox_get);
 
-void omap_mbox_put(struct omap_mbox *mbox)
+void omap_mbox_put(struct omap_mbox *mbox, struct notifier_block *nb)
 {
+	blocking_notifier_chain_unregister(&mbox->notifier, nb);
 	omap_mbox_fini(mbox);
 }
 EXPORT_SYMBOL(omap_mbox_put);
@@ -366,6 +375,7 @@ int omap_mbox_register(struct device *parent, struct omap_mbox **list)
 			ret = PTR_ERR(mbox->dev);
 			goto err_out;
 		}
+		BLOCKING_INIT_NOTIFIER_HEAD(&mbox->notifier);
 	}
 	return 0;
 
-- 
1.7.0

[PATCH 6/7] omap:mailbox-add notification support for multiple readers

[Hari Kanigeri]

In the current mailbox driver, the mailbox internal pointer for
callback can be directly manipulated by the Users, so a second
User can easily corrupt the first user's callback pointer.
The initial effort to correct this issue can be referred here:
https://patchwork.kernel.org/patch/107520/

Along with fixing the above stated issue, this patch  adds the
flexibility option to register notifications from
multiple readers to the events received on a mailbox instance.
The discussion regarding this can be referred here.
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg30671.html

Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
 arch/arm/plat-omap/include/plat/mailbox.h |    9 ++-
 arch/arm/plat-omap/mailbox.c              |  102 ++++++++++++++++-------------
 2 files changed, 62 insertions(+), 49 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/mailbox.h b/arch/arm/plat-omap/include/plat/mailbox.h
index 13f2ef3..4c35654 100644
--- a/arch/arm/plat-omap/include/plat/mailbox.h
+++ b/arch/arm/plat-omap/include/plat/mailbox.h
@@ -8,6 +8,7 @@
 #include <linux/interrupt.h>
 #include <linux/device.h>
 #include <linux/kfifo.h>
+#include <linux/notifier.h>
 
 typedef u32 mbox_msg_t;
 struct omap_mbox;
@@ -46,7 +47,6 @@ struct omap_mbox_queue {
 	struct kfifo		fifo;
 	struct work_struct	work;
 	struct tasklet_struct	tasklet;
-	int	(*callback)(void *);
 	struct omap_mbox	*mbox;
 	bool full;
 };
@@ -58,13 +58,16 @@ struct omap_mbox {
 	struct omap_mbox_ops	*ops;
 	struct device		*dev;
 	void			*priv;
+
+	int			use_count;
+	struct blocking_notifier_head	notifier;
 };
 
 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 *);
-void omap_mbox_put(struct omap_mbox *);
+struct omap_mbox *omap_mbox_get(const char *, struct notifier_block *nb);
+void omap_mbox_put(struct omap_mbox *mbox, struct notifier_block *nb);
 
 int omap_mbox_register(struct device *parent, struct omap_mbox **);
 int omap_mbox_unregister(void);
diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index 1727548..35956f5 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -28,6 +28,7 @@
 #include <linux/slab.h>
 #include <linux/kfifo.h>
 #include <linux/err.h>
+#include <linux/notifier.h>
 
 #include <plat/mailbox.h>
 
@@ -150,8 +151,8 @@ static void mbox_rx_work(struct work_struct *work)
 		len = kfifo_out(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
 		WARN_ON(len != sizeof(msg));
 
-		if (mq->callback)
-			mq->callback((void *)msg);
+		blocking_notifier_call_chain(&mq->mbox->notifier, len,
+							(void *)msg);
 		if (mq->full) {
 			mq->full = false;
 			omap_mbox_enable_irq(mq->mbox, IRQ_RX);
@@ -202,7 +203,7 @@ nomem:
 
 static irqreturn_t mbox_interrupt(int irq, void *p)
 {
-	struct omap_mbox *mbox = p;
+	int i;
 
 	for (i = 0; mboxes[i]; i++)  {
 		struct omap_mbox *mbox = mboxes[i];
@@ -252,41 +253,39 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
 	int ret = 0;
 	struct omap_mbox_queue *mq;
 
-	if (mbox->ops->startup) {
-		mutex_lock(&mbox_configured_lock);
-		if (!mbox_configured)
+	mutex_lock(&mbox_configured_lock);
+	if (!mbox_configured++) {
+		if (likely(mbox->ops->startup)) {
 			ret = mbox->ops->startup(mbox);
-
-		if (ret) {
-			mutex_unlock(&mbox_configured_lock);
-			return ret;
+			if (unlikely(ret))
+				goto fail_startup;
 		}
-		mbox_configured++;
-		mutex_unlock(&mbox_configured_lock);
 	}
 
-	ret = request_irq(mbox->irq, mbox_interrupt, IRQF_SHARED,
-				mbox->name, mbox);
-	if (ret) {
-		printk(KERN_ERR
-			"failed to register mailbox interrupt:%d\n", ret);
-		goto fail_request_irq;
-	}
-
-	mq = mbox_queue_alloc(mbox, NULL, mbox_tx_tasklet);
-	if (!mq) {
-		ret = -ENOMEM;
-		goto fail_alloc_txq;
-	}
-	mbox->txq = mq;
+	if (!mbox->use_count++) {
+		ret = request_irq(mbox->irq, mbox_interrupt, IRQF_SHARED,
+					mbox->name, mbox);
+		if (unlikely(ret)) {
+			pr_err("failed to register mailbox interrupt:%d\n",
+									ret);
+			goto fail_request_irq;
+		}
+		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_rx_work, NULL);
-	if (!mq) {
-		ret = -ENOMEM;
-		goto fail_alloc_rxq;
+		mq = mbox_queue_alloc(mbox, mbox_rx_work, NULL);
+		if (!mq) {
+			ret = -ENOMEM;
+			goto fail_alloc_rxq;
+		}
+		mbox->rxq = mq;
+		mq->mbox = mbox;
 	}
-	mbox->rxq = mq;
-
+	mutex_unlock(&mbox_configured_lock);
 	return 0;
 
 fail_alloc_rxq:
@@ -296,29 +295,36 @@ fail_alloc_txq:
 fail_request_irq:
 	if (mbox->ops->shutdown)
 		mbox->ops->shutdown(mbox);
-
+	mbox->use_count--;
+fail_startup:
+	mbox_configured--;
+	mutex_unlock(&mbox_configured_lock);
 	return ret;
 }
 
 static void omap_mbox_fini(struct omap_mbox *mbox)
 {
-	free_irq(mbox->irq, mbox);
-	tasklet_kill(&mbox->txq->tasklet);
-	flush_work(&mbox->rxq->work);
-	mbox_queue_free(mbox->txq);
-	mbox_queue_free(mbox->rxq);
 
-	if (mbox->ops->shutdown) {
-		mutex_lock(&mbox_configured_lock);
-		if (mbox_configured > 0)
-			mbox_configured--;
-		if (!mbox_configured)
+	mutex_lock(&mbox_configured_lock);
+
+	if (!--mbox->use_count) {
+		tasklet_kill(&mbox->txq->tasklet);
+		flush_work(&mbox->rxq->work);
+		mbox_queue_free(mbox->txq);
+		mbox_queue_free(mbox->rxq);
+	}
+
+	if (likely(mbox->ops->shutdown)) {
+		if (!--mbox_configured) {
+			free_irq(mbox->irq, mbox);
 			mbox->ops->shutdown(mbox);
-		mutex_unlock(&mbox_configured_lock);
+		}
 	}
+
+	mutex_unlock(&mbox_configured_lock);
 }
 
-struct omap_mbox *omap_mbox_get(const char *name)
+struct omap_mbox *omap_mbox_get(const char *name, struct notifier_block *nb)
 {
 	struct omap_mbox *mbox;
 	int ret;
@@ -336,13 +342,16 @@ struct omap_mbox *omap_mbox_get(const char *name)
 	ret = omap_mbox_startup(mbox);
 	if (ret)
 		return ERR_PTR(-ENODEV);
+	if (nb)
+		blocking_notifier_chain_register(&mbox->notifier, nb);
 
 	return mbox;
 }
 EXPORT_SYMBOL(omap_mbox_get);
 
-void omap_mbox_put(struct omap_mbox *mbox)
+void omap_mbox_put(struct omap_mbox *mbox, struct notifier_block *nb)
 {
+	blocking_notifier_chain_unregister(&mbox->notifier, nb);
 	omap_mbox_fini(mbox);
 }
 EXPORT_SYMBOL(omap_mbox_put);
@@ -366,6 +375,7 @@ int omap_mbox_register(struct device *parent, struct omap_mbox **list)
 			ret = PTR_ERR(mbox->dev);
 			goto err_out;
 		}
+		BLOCKING_INIT_NOTIFIER_HEAD(&mbox->notifier);
 	}
 	return 0;
 
-- 
1.7.0

[PATCH 7/7] omap:clocks44x-add dummy clock for mailbox

[Hari Kanigeri]

In omap4, there is no explicit configuration register to enable mailbox clocks.
Defining dummy clock for mailbox clock module to keep the mailbox driver
backward compatible with previous omaps.

Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
---
 arch/arm/mach-omap2/clock44xx_data.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/clock44xx_data.c b/arch/arm/mach-omap2/clock44xx_data.c
index e10db7a..421ae7f 100644
--- a/arch/arm/mach-omap2/clock44xx_data.c
+++ b/arch/arm/mach-omap2/clock44xx_data.c
@@ -2687,6 +2687,7 @@ static struct omap_clk omap44xx_clks[] = {
 	CLK(NULL,	"uart3_ick",			&dummy_ck,	CK_443X),
 	CLK(NULL,	"uart4_ick",			&dummy_ck,	CK_443X),
 	CLK("omap_wdt",	"ick",				&dummy_ck,	CK_443X),
+	CLK(NULL,	"mailboxes_ick",		&dummy_ck,	CK_443X),
 };
 
 int __init omap4xxx_clk_init(void)
-- 
1.7.0

[PATCH 7/7] omap:clocks44x-add dummy clock for mailbox

[Hari Kanigeri]

In omap4, there is no explicit configuration register to enable mailbox clocks.
Defining dummy clock for mailbox clock module to keep the mailbox driver
backward compatible with previous omaps.

Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
---
 arch/arm/mach-omap2/clock44xx_data.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/clock44xx_data.c b/arch/arm/mach-omap2/clock44xx_data.c
index e10db7a..421ae7f 100644
--- a/arch/arm/mach-omap2/clock44xx_data.c
+++ b/arch/arm/mach-omap2/clock44xx_data.c
@@ -2687,6 +2687,7 @@ static struct omap_clk omap44xx_clks[] = {
 	CLK(NULL,	"uart3_ick",			&dummy_ck,	CK_443X),
 	CLK(NULL,	"uart4_ick",			&dummy_ck,	CK_443X),
 	CLK("omap_wdt",	"ick",				&dummy_ck,	CK_443X),
+	CLK(NULL,	"mailboxes_ick",		&dummy_ck,	CK_443X),
 };
 
 int __init omap4xxx_clk_init(void)
-- 
1.7.0

RE: [PATCH 4/7] omap:mailbox-send message in process context

[Sapiens, Rene]

Hi Hari,

On Thursday, October 14, 2010 9:13 PM Kanigeri, Hari wrote:
> Schedule the Tasklet to send only when mailbox fifo is full, else
> send the message in the Process context. This would avoid
> needless scheduling of Tasklet for every message transfer
> 
> Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
> ---
>  arch/arm/plat-omap/mailbox.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
> index ed960c1..a4170c7 100644
> --- a/arch/arm/plat-omap/mailbox.c
> +++ b/arch/arm/plat-omap/mailbox.c
> @@ -92,20 +92,25 @@ 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);
> 

Please check if this scenario looks valid to you, as discussed and depicted
with Fernando:

Supposing that at this point the hw fifo is full and there are messages in
the kfifo.

>  	if (kfifo_avail(&mq->fifo) < sizeof(msg)) {
>  		ret = -ENOMEM;
>  		goto out;
>  	}
> 

We reach this point.

In this scenario, the DSP or other Core reads a message from the mbox hw fifo.
The next happens:
1.- The not full interrupt is triggered to the MPU.
2.- The mbox's ISR schedules the tasklet to write the message.
3.- The ISR is left and returns to here (since we still have the bh lock the
    tasklet won't run).

> +	if (!__mbox_poll_for_space(mbox)) {\

4.- We check for mbox_fifo_full and we have space. so the message is written.

> +		mbox_fifo_write(mbox, msg);

At this point it looks that the FIFO order is lost. We write this message
before the ones in the kfifo.

A solution for this could be checking if there are messages in the kfifo
before trying to write directly to the hw fifo, if so, just continue
scheduling the tasklet.

> +		goto out;
> +	}
> +
>  	len = kfifo_in(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
>  	WARN_ON(len != sizeof(msg));
> 
>  	tasklet_schedule(&mbox->txq->tasklet);
> 
>  out:
> -	spin_unlock(&mq->lock);
> +	spin_unlock_bh(&mq->lock);
>  	return ret;
>  }
>  EXPORT_SYMBOL(omap_mbox_msg_send);
> --
> 1.7.0

[PATCH 4/7] omap:mailbox-send message in process context

[Sapiens, Rene]

Hi Hari,

On Thursday, October 14, 2010 9:13 PM Kanigeri, Hari wrote:
> Schedule the Tasklet to send only when mailbox fifo is full, else
> send the message in the Process context. This would avoid
> needless scheduling of Tasklet for every message transfer
> 
> Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
> ---
>  arch/arm/plat-omap/mailbox.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
> index ed960c1..a4170c7 100644
> --- a/arch/arm/plat-omap/mailbox.c
> +++ b/arch/arm/plat-omap/mailbox.c
> @@ -92,20 +92,25 @@ 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);
> 

Please check if this scenario looks valid to you, as discussed and depicted
with Fernando:

Supposing that at this point the hw fifo is full and there are messages in
the kfifo.

>  	if (kfifo_avail(&mq->fifo) < sizeof(msg)) {
>  		ret = -ENOMEM;
>  		goto out;
>  	}
> 

We reach this point.

In this scenario, the DSP or other Core reads a message from the mbox hw fifo.
The next happens:
1.- The not full interrupt is triggered to the MPU.
2.- The mbox's ISR schedules the tasklet to write the message.
3.- The ISR is left and returns to here (since we still have the bh lock the
    tasklet won't run).

> +	if (!__mbox_poll_for_space(mbox)) {\

4.- We check for mbox_fifo_full and we have space. so the message is written.

> +		mbox_fifo_write(mbox, msg);

At this point it looks that the FIFO order is lost. We write this message
before the ones in the kfifo.

A solution for this could be checking if there are messages in the kfifo
before trying to write directly to the hw fifo, if so, just continue
scheduling the tasklet.

> +		goto out;
> +	}
> +
>  	len = kfifo_in(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
>  	WARN_ON(len != sizeof(msg));
> 
>  	tasklet_schedule(&mbox->txq->tasklet);
> 
>  out:
> -	spin_unlock(&mq->lock);
> +	spin_unlock_bh(&mq->lock);
>  	return ret;
>  }
>  EXPORT_SYMBOL(omap_mbox_msg_send);
> --
> 1.7.0

Re: [PATCH 4/7] omap:mailbox-send message in process context

[Hari Kanigeri]

Rene,

Thanks for your comment.


>> @@ -92,20 +92,25 @@ 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);
>>
>
> Please check if this scenario looks valid to you, as discussed and depicted
> with Fernando:
>
> Supposing that at this point the hw fifo is full and there are messages in
> the kfifo.
>
>>       if (kfifo_avail(&mq->fifo) < sizeof(msg)) {
>>               ret = -ENOMEM;
>>               goto out;
>>       }
>>
>
> We reach this point.
>
> In this scenario, the DSP or other Core reads a message from the mbox hw fifo.
> The next happens:
> 1.- The not full interrupt is triggered to the MPU.
> 2.- The mbox's ISR schedules the tasklet to write the message.
> 3.- The ISR is left and returns to here (since we still have the bh lock the
>    tasklet won't run).
>
>> +     if (!__mbox_poll_for_space(mbox)) {\
>
> 4.- We check for mbox_fifo_full and we have space. so the message is written.
>
>> +             mbox_fifo_write(mbox, msg);
>
> At this point it looks that the FIFO order is lost. We write this message
> before the ones in the kfifo.
>

Good point. I agree.

> A solution for this could be checking if there are messages in the kfifo
> before trying to write directly to the hw fifo, if so, just continue
> scheduling the tasklet.

A change something like this ?

-       if (!__mbox_poll_for_space(mbox)) {
+       if (kfifo_is_empty() && !__mbox_poll_for_space(mbox)) {
                mbox_fifo_write(mbox, msg);

Thank you,
Best regards,
Hari
--
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

[PATCH 4/7] omap:mailbox-send message in process context

[Hari Kanigeri]

Rene,

Thanks for your comment.


>> @@ -92,20 +92,25 @@ 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);
>>
>
> Please check if this scenario looks valid to you, as discussed and depicted
> with Fernando:
>
> Supposing that at this point the hw fifo is full and there are messages in
> the kfifo.
>
>> ? ? ? if (kfifo_avail(&mq->fifo) < sizeof(msg)) {
>> ? ? ? ? ? ? ? ret = -ENOMEM;
>> ? ? ? ? ? ? ? goto out;
>> ? ? ? }
>>
>
> We reach this point.
>
> In this scenario, the DSP or other Core reads a message from the mbox hw fifo.
> The next happens:
> 1.- The not full interrupt is triggered to the MPU.
> 2.- The mbox's ISR schedules the tasklet to write the message.
> 3.- The ISR is left and returns to here (since we still have the bh lock the
> ? ?tasklet won't run).
>
>> + ? ? if (!__mbox_poll_for_space(mbox)) {\
>
> 4.- We check for mbox_fifo_full and we have space. so the message is written.
>
>> + ? ? ? ? ? ? mbox_fifo_write(mbox, msg);
>
> At this point it looks that the FIFO order is lost. We write this message
> before the ones in the kfifo.
>

Good point. I agree.

> A solution for this could be checking if there are messages in the kfifo
> before trying to write directly to the hw fifo, if so, just continue
> scheduling the tasklet.

A change something like this ?

-       if (!__mbox_poll_for_space(mbox)) {
+       if (kfifo_is_empty() && !__mbox_poll_for_space(mbox)) {
                mbox_fifo_write(mbox, msg);

Thank you,
Best regards,
Hari

RE: [PATCH 4/7] omap:mailbox-send message in process context

[Sapiens, Rene]

Hari,

On Thursday, October 21, 2010 3:49 PM Hari Kanigeri wrote:
> Rene,
> 
> Thanks for your comment.
> 
> 
>>> @@ -92,20 +92,25 @@ 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);
>>> 
>> 
>> Please check if this scenario looks valid to you, as discussed and depicted
>> with Fernando:
>> 
>> Supposing that at this point the hw fifo is full and there are messages in
>> the kfifo.
>> 
>>>       if (kfifo_avail(&mq->fifo) < sizeof(msg)) {
>>>               ret = -ENOMEM;
>>>               goto out;
>>>       }
>>> 
>> 
>> We reach this point.
>> 
>> In this scenario, the DSP or other Core reads a message from the mbox hw
>> fifo. The next happens:
>> 1.- The not full interrupt is triggered to the MPU.
>> 2.- The mbox's ISR schedules the tasklet to write the message.
>> 3.- The ISR is left and returns to here (since we still have the bh lock
>> the    tasklet won't run).
>> 
>>> +     if (!__mbox_poll_for_space(mbox)) {\
>> 
>> 4.- We check for mbox_fifo_full and we have space. so the message is
>> written. 
>> 
>>> +             mbox_fifo_write(mbox, msg);
>> 
>> At this point it looks that the FIFO order is lost. We write this message
>> before the ones in the kfifo.
>> 
> 
> Good point. I agree.
> 
>> A solution for this could be checking if there are messages in the kfifo
>> before trying to write directly to the hw fifo, if so, just continue
>> scheduling the tasklet.
> 
> A change something like this ?
> 
> -       if (!__mbox_poll_for_space(mbox)) {
> +       if (kfifo_is_empty() && !__mbox_poll_for_space(mbox)) {

Yes, this looks good to me.

>                 mbox_fifo_write(mbox, msg);

Regards,
Rene--
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

[PATCH 4/7] omap:mailbox-send message in process context

[Sapiens, Rene]

Hari,

On Thursday, October 21, 2010 3:49 PM Hari Kanigeri wrote:
> Rene,
> 
> Thanks for your comment.
> 
> 
>>> @@ -92,20 +92,25 @@ 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);
>>> 
>> 
>> Please check if this scenario looks valid to you, as discussed and depicted
>> with Fernando:
>> 
>> Supposing that at this point the hw fifo is full and there are messages in
>> the kfifo.
>> 
>>> ? ? ? if (kfifo_avail(&mq->fifo) < sizeof(msg)) {
>>> ? ? ? ? ? ? ? ret = -ENOMEM;
>>> ? ? ? ? ? ? ? goto out;
>>> ? ? ? }
>>> 
>> 
>> We reach this point.
>> 
>> In this scenario, the DSP or other Core reads a message from the mbox hw
>> fifo. The next happens:
>> 1.- The not full interrupt is triggered to the MPU.
>> 2.- The mbox's ISR schedules the tasklet to write the message.
>> 3.- The ISR is left and returns to here (since we still have the bh lock
>> the ? ?tasklet won't run).
>> 
>>> + ? ? if (!__mbox_poll_for_space(mbox)) {\
>> 
>> 4.- We check for mbox_fifo_full and we have space. so the message is
>> written. 
>> 
>>> + ? ? ? ? ? ? mbox_fifo_write(mbox, msg);
>> 
>> At this point it looks that the FIFO order is lost. We write this message
>> before the ones in the kfifo.
>> 
> 
> Good point. I agree.
> 
>> A solution for this could be checking if there are messages in the kfifo
>> before trying to write directly to the hw fifo, if so, just continue
>> scheduling the tasklet.
> 
> A change something like this ?
> 
> -       if (!__mbox_poll_for_space(mbox)) {
> +       if (kfifo_is_empty() && !__mbox_poll_for_space(mbox)) {

Yes, this looks good to me.

>                 mbox_fifo_write(mbox, msg);

Regards,
Rene

Re: [PATCH 6/7] omap:mailbox-add notification support for multiple readers

[Omar Ramirez Luna]

Hi,

On 10/14/2010 9:13 PM, Hari Kanigeri wrote:
> @@ -252,41 +253,39 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
...
> +	if (!mbox->use_count++) {
> +		ret = request_irq(mbox->irq, mbox_interrupt, IRQF_SHARED,
> +					mbox->name, mbox);
...
> @@ -296,29 +295,36 @@ fail_alloc_txq:
...
>   static void omap_mbox_fini(struct omap_mbox *mbox)
>   {
> +	if (!--mbox->use_count) {
> +		tasklet_kill(&mbox->txq->tasklet);
> +		flush_work(&mbox->rxq->work);
> +		mbox_queue_free(mbox->txq);
> +		mbox_queue_free(mbox->rxq);
> +	}
> +
> +	if (likely(mbox->ops->shutdown)) {
> +		if (!--mbox_configured) {
> +			free_irq(mbox->irq, mbox);

Above hunks will create an imbalance of free_irq, as request_irq can be 
called per registered mailbox and free_irq is only done for the last 
caller releasing the mailbox handle.

e.g.: mbox-1, mbox-N will request a shared irq on the same interrupt 
line, but only the last caller of omap_mbox_put will free its irq, 
leaving the other one there.

This can be fixed if the free is moved to be executed within the 
following block:

	if (!--mbox->use_count) {
		...
		free_irq(mbox->irq, mbox);
	}

Regards,

Omar

[PATCH 6/7] omap:mailbox-add notification support for multiple readers

[Omar Ramirez Luna]

Hi,

On 10/14/2010 9:13 PM, Hari Kanigeri wrote:
> @@ -252,41 +253,39 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
...
> +	if (!mbox->use_count++) {
> +		ret = request_irq(mbox->irq, mbox_interrupt, IRQF_SHARED,
> +					mbox->name, mbox);
...
> @@ -296,29 +295,36 @@ fail_alloc_txq:
...
>   static void omap_mbox_fini(struct omap_mbox *mbox)
>   {
> +	if (!--mbox->use_count) {
> +		tasklet_kill(&mbox->txq->tasklet);
> +		flush_work(&mbox->rxq->work);
> +		mbox_queue_free(mbox->txq);
> +		mbox_queue_free(mbox->rxq);
> +	}
> +
> +	if (likely(mbox->ops->shutdown)) {
> +		if (!--mbox_configured) {
> +			free_irq(mbox->irq, mbox);

Above hunks will create an imbalance of free_irq, as request_irq can be 
called per registered mailbox and free_irq is only done for the last 
caller releasing the mailbox handle.

e.g.: mbox-1, mbox-N will request a shared irq on the same interrupt 
line, but only the last caller of omap_mbox_put will free its irq, 
leaving the other one there.

This can be fixed if the free is moved to be executed within the 
following block:

	if (!--mbox->use_count) {
		...
		free_irq(mbox->irq, mbox);
	}

Regards,

Omar

Re: [PATCH 5/7] omap:mailbox-resolve multiple receiver problem

[Omar Ramirez Luna]

On 10/14/2010 9:13 PM, Kanigeri, Hari wrote:
> OMAP4 shares one interrupt line for all the mailbox instances.
> The ISR is handling only the mailbox instance that was registered last.

This shouldn't be needed, request_irq is being called with IRQF_SHARED 
flag and different device ids, so if a message arrives it fires an 
interrupt handler for each of the callers to request_irq and since the 
device id is actually a pointer to a mbox struct, the different users 
can be detected and signaled without looping through the "mboxes" list.

Also using "mboxes" list, will try to check for all registered mailboxes 
during probe, which might not be the same as the actual users (the ones 
that have called omap_mbox_get) and then unnecesary check their irq 
statuses if an interrupt arrives.

I think this patch can be dropped.

Regards,

Omar

[PATCH 5/7] omap:mailbox-resolve multiple receiver problem

[Omar Ramirez Luna]

On 10/14/2010 9:13 PM, Kanigeri, Hari wrote:
> OMAP4 shares one interrupt line for all the mailbox instances.
> The ISR is handling only the mailbox instance that was registered last.

This shouldn't be needed, request_irq is being called with IRQF_SHARED 
flag and different device ids, so if a message arrives it fires an 
interrupt handler for each of the callers to request_irq and since the 
device id is actually a pointer to a mbox struct, the different users 
can be detected and signaled without looping through the "mboxes" list.

Also using "mboxes" list, will try to check for all registered mailboxes 
during probe, which might not be the same as the actual users (the ones 
that have called omap_mbox_get) and then unnecesary check their irq 
statuses if an interrupt arrives.

I think this patch can be dropped.

Regards,

Omar

Re: [PATCH 6/7] omap:mailbox-add notification support for multiple readers

[Hari Kanigeri]

Omar,

>>
>>  static void omap_mbox_fini(struct omap_mbox *mbox)
>>  {
>> +       if (!--mbox->use_count) {
>> +               tasklet_kill(&mbox->txq->tasklet);
>> +               flush_work(&mbox->rxq->work);
>> +               mbox_queue_free(mbox->txq);
>> +               mbox_queue_free(mbox->rxq);
>> +       }
>> +
>> +       if (likely(mbox->ops->shutdown)) {
>> +               if (!--mbox_configured) {
>> +                       free_irq(mbox->irq, mbox);
>
> Above hunks will create an imbalance of free_irq, as request_irq can be
> called per registered mailbox and free_irq is only done for the last caller
> releasing the mailbox handle.
>
> e.g.: mbox-1, mbox-N will request a shared irq on the same interrupt line,
> but only the last caller of omap_mbox_put will free its irq, leaving the
> other one there.
>
> This can be fixed if the free is moved to be executed within the following
> block:
>
>        if (!--mbox->use_count) {
>                ...
>                free_irq(mbox->irq, mbox);
>        }
>

Good catch. I will make the changes in next revision.

Thank you,
Best regards,
Hari
--
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

[PATCH 6/7] omap:mailbox-add notification support for multiple readers

[Hari Kanigeri]

Omar,

>>
>> ?static void omap_mbox_fini(struct omap_mbox *mbox)
>> ?{
>> + ? ? ? if (!--mbox->use_count) {
>> + ? ? ? ? ? ? ? tasklet_kill(&mbox->txq->tasklet);
>> + ? ? ? ? ? ? ? flush_work(&mbox->rxq->work);
>> + ? ? ? ? ? ? ? mbox_queue_free(mbox->txq);
>> + ? ? ? ? ? ? ? mbox_queue_free(mbox->rxq);
>> + ? ? ? }
>> +
>> + ? ? ? if (likely(mbox->ops->shutdown)) {
>> + ? ? ? ? ? ? ? if (!--mbox_configured) {
>> + ? ? ? ? ? ? ? ? ? ? ? free_irq(mbox->irq, mbox);
>
> Above hunks will create an imbalance of free_irq, as request_irq can be
> called per registered mailbox and free_irq is only done for the last caller
> releasing the mailbox handle.
>
> e.g.: mbox-1, mbox-N will request a shared irq on the same interrupt line,
> but only the last caller of omap_mbox_put will free its irq, leaving the
> other one there.
>
> This can be fixed if the free is moved to be executed within the following
> block:
>
> ? ? ? ?if (!--mbox->use_count) {
> ? ? ? ? ? ? ? ?...
> ? ? ? ? ? ? ? ?free_irq(mbox->irq, mbox);
> ? ? ? ?}
>

Good catch. I will make the changes in next revision.

Thank you,
Best regards,
Hari

Re: [PATCH 5/7] omap:mailbox-resolve multiple receiver problem

[Hari Kanigeri]

Omar,


>> OMAP4 shares one interrupt line for all the mailbox instances.
>> The ISR is handling only the mailbox instance that was registered last.
>
> This shouldn't be needed, request_irq is being called with IRQF_SHARED flag
> and different device ids, so if a message arrives it fires an interrupt
> handler for each of the callers to request_irq and since the device id is
> actually a pointer to a mbox struct, the different users can be detected and
> signaled without looping through the "mboxes" list.
>
> Also using "mboxes" list, will try to check for all registered mailboxes
> during probe, which might not be the same as the actual users (the ones that
> have called omap_mbox_get) and then unnecesary check their irq statuses if
> an interrupt arrives.
>
> I think this patch can be dropped.
>

Agree, I will drop the patch and send the rebased patches.

Thank you,
Best regards,
Hari

[PATCH 5/7] omap:mailbox-resolve multiple receiver problem

[Hari Kanigeri]

Omar,


>> OMAP4 shares one interrupt line for all the mailbox instances.
>> The ISR is handling only the mailbox instance that was registered last.
>
> This shouldn't be needed, request_irq is being called with IRQF_SHARED flag
> and different device ids, so if a message arrives it fires an interrupt
> handler for each of the callers to request_irq and since the device id is
> actually a pointer to a mbox struct, the different users can be detected and
> signaled without looping through the "mboxes" list.
>
> Also using "mboxes" list, will try to check for all registered mailboxes
> during probe, which might not be the same as the actual users (the ones that
> have called omap_mbox_get) and then unnecesary check their irq statuses if
> an interrupt arrives.
>
> I think this patch can be dropped.
>

Agree, I will drop the patch and send the rebased patches.

Thank you,
Best regards,
Hari

[PATCH 0/7] omap:mailbox-enhancements and fixes

[Hari Kanigeri]

Resending the patch set copying linux arm mailing list.
please ignore the previous patchset.
[http://www.mail-archive.com/linux-omap@vger.kernel.org/msg37214.html]

This patch set includes following mailbox enhancements and fixes.
        
- Fix in RX interrupt disable mechanism for OMAP4
- Fix checkpatch warnings
- Resolve multiple receiver problem in OMAP4 where there is
  only one interrupt line shared between Ducati and Tesla.
- Protect the Mailbox's internal callback function pointer from being
  set directly by Users. Added option for multiple listeners on a
  mailbox instance.
- Send Mailbox message in Process context to avoid the latency
  involved in scheduling Tasklet for every Mailbox message. Schedule
  Tasklet only when the mailbox fifo is full
- There is no explicit mailbox configuration register to enable mailbox
  clocks. Define a dummy clock for mailbox to avoid addign cpu check for
  omap4 in mailbox driver.
- The patch from Fernando was sent to LO, but looks like it
  didn't get merged, resending the patch after revising and rebasing.
  https://patchwork.kernel.org/patch/105650/

Fernando Guzman Lugo (1):
  mailbox: change full flag per mailbox queue instead of global

Hari Kanigeri (6):
  omap:mailbox: fix rx interrupt disable in omap4
  omap:mailbox-fix checkpatch warnings
  omap:mailbox-send message in process context
  omap:mailbox-resolve multiple receiver problem
  omap:mailbox-add notification support for multiple readers
  omap:clocks44x-add dummy clock for mailbox

 arch/arm/mach-omap2/clock44xx_data.c      |    1 +
 arch/arm/mach-omap2/mailbox.c             |    5 +-
 arch/arm/plat-omap/include/plat/mailbox.h |   10 ++-
 arch/arm/plat-omap/mailbox.c              |  148 +++++++++++++++++------------
 4 files changed, 98 insertions(+), 66 deletions(-)