[PATCH v4 0/5] OMAP: mailbox: fixes and enhancements

[PATCH v4 0/5] OMAP: mailbox: fixes and enhancements

[Hari Kanigeri]

Thanks to Benoit and Felipe Balbi for their comments.

Here is the revised set taking into account comments received.
========================

- Added more description for checkpatch warnings
- Created revision field to check for mailbox IP version to use for rx
  interrupt disable functionality.
- Cleaned up description in change full flag per mailbox patch.

References:
http://marc.info/?l=linux-omap&m=129012283514652&w=2
http://marc.info/?l=linux-omap&m=129016767426046&w=2
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg38826.html

The previous versions of this patch set can be found here
=======================

http://www.spinics.net/lists/arm-kernel/msg104544.html
http://www.spinics.net/lists/linux-omap/msg39988.html
http://www.spinics.net/lists/linux-omap/msg39931.html
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg37278.html

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

Hari Kanigeri (4):
  OMAP: mailbox: fix rx interrupt disable in omap4
  OMAP: mailbox: fix checkpatch warnings
  OMAP: mailbox: send message in process context
  OMAP: mailbox: add notification support for multiple readers

 arch/arm/mach-omap2/mailbox.c             |    7 ++-
 arch/arm/plat-omap/include/plat/mailbox.h |   12 ++-
 arch/arm/plat-omap/mailbox.c              |  135 +++++++++++++++++------------
 3 files changed, 96 insertions(+), 58 deletions(-)

[PATCH v4 1/5] OMAP: mailbox: change full flag per mailbox queue instead of global

[Hari Kanigeri]

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

The variable rq_full flag is a global variable, so if there are multiple
mailbox users there will be conflicts. Now there is a full flag per
mailbox queue.

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              |    9 +++++++--
 2 files changed, 8 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..48e161c 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,12 @@ static void mbox_rx_work(struct work_struct *work)
 
 		if (mq->callback)
 			mq->callback((void *)msg);
+		spin_lock_irq(&mq->lock);
+		if (mq->full) {
+			mq->full = false;
+			omap_mbox_enable_irq(mq->mbox, IRQ_RX);
+		}
+		spin_unlock_irq(&mq->lock);
 	}
 }
 
@@ -170,7 +175,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 v4 2/5] OMAP: mailbox: fix rx interrupt disable in omap4

[Hari Kanigeri]

disablign 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.

Defined rev field in mailbox structure to differentiate the mailbox
versions.

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

diff --git a/arch/arm/mach-omap2/mailbox.c b/arch/arm/mach-omap2/mailbox.c
index 42dbfa4..c32058d 100644
--- a/arch/arm/mach-omap2/mailbox.c
+++ b/arch/arm/mach-omap2/mailbox.c
@@ -195,7 +195,12 @@ 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 (mbox->rev == OMAP_MBOX_IP_VERSION_2)
+		l |= bit;
+	else
+		l &= ~bit;
+
 	mbox_write_reg(l, p->irqdisable);
 }
 
diff --git a/arch/arm/plat-omap/include/plat/mailbox.h b/arch/arm/plat-omap/include/plat/mailbox.h
index 13f2ef3..1655c61 100644
--- a/arch/arm/plat-omap/include/plat/mailbox.h
+++ b/arch/arm/plat-omap/include/plat/mailbox.h
@@ -20,6 +20,9 @@ typedef int __bitwise omap_mbox_type_t;
 #define OMAP_MBOX_TYPE1 ((__force omap_mbox_type_t) 1)
 #define OMAP_MBOX_TYPE2 ((__force omap_mbox_type_t) 2)
 
+#define OMAP_MBOX_IP_LEGACY                    0x1
+#define OMAP_MBOX_IP_VERSION_2                 0x2
+
 struct omap_mbox_ops {
 	omap_mbox_type_t	type;
 	int		(*startup)(struct omap_mbox *mbox);
@@ -58,6 +61,7 @@ struct omap_mbox {
 	struct omap_mbox_ops	*ops;
 	struct device		*dev;
 	void			*priv;
+	int			rev;
 };
 
 int omap_mbox_msg_send(struct omap_mbox *, mbox_msg_t msg);
diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index 48e161c..a1c6bd9 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -358,6 +358,10 @@ int omap_mbox_register(struct device *parent, struct omap_mbox **list)
 			ret = PTR_ERR(mbox->dev);
 			goto err_out;
 		}
+		if (cpu_is_omap44xx())
+			mbox->rev = OMAP_MBOX_IP_VERSION_2;
+		else
+			mbox->rev = OMAP_MBOX_IP_LEGACY;
 	}
 	return 0;
 
-- 
1.7.0

[PATCH v4 3/5] OMAP: mailbox: fix checkpatch warnings

[Hari Kanigeri]

Fix the following checkpatch warnings observed in mailbox module.

WARNING: please, no space for starting a line,
                                excluding comments
+ fail_alloc_rxq:$

WARNING: please, no space for starting a line,
                                excluding comments
+ fail_alloc_txq:$

WARNING: please, no space for starting a line,
                                excluding comments
+ fail_request_irq:$

WARNING: line over 80 characters
+       mbox_kfifo_size = max_t(unsigned int, mbox_kfifo_size, sizeof(mbox_msg_t));

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 a1c6bd9..13698ab 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -281,11 +281,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);
 
@@ -400,7 +400,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 v4 4/5] OMAP: mailbox: send message in process context

[Hari Kanigeri]

Schedule the Tasklet to send only when mailbox fifo is full and there are
pending messages in kifo, else send the message directly 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 13698ab..f4177df 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 (kfifo_is_empty(&mq->fifo) && !__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 v4 5/5] 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 |    7 +-
 arch/arm/plat-omap/mailbox.c              |  104 ++++++++++++++++-------------
 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 1655c61..839c6c3 100644
--- a/arch/arm/plat-omap/include/plat/mailbox.h
+++ b/arch/arm/plat-omap/include/plat/mailbox.h
@@ -49,7 +49,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;
 };
@@ -62,13 +61,15 @@ struct omap_mbox {
 	struct device		*dev;
 	void			*priv;
 	int			rev;
+	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 f4177df..05d1c9c 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);
 		spin_lock_irq(&mq->lock);
 		if (mq->full) {
 			mq->full = false;
@@ -249,41 +250,40 @@ 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;
-		}
-		mbox_configured++;
-		mutex_unlock(&mbox_configured_lock);
+			if (unlikely(ret))
+				goto fail_startup;
+		} else
+			goto fail_startup;
 	}
 
-	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:
@@ -293,29 +293,34 @@ 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);
+	mutex_lock(&mbox_configured_lock);
+
+	if (!--mbox->use_count) {
+		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)
+	if (likely(mbox->ops->shutdown)) {
+		if (!--mbox_configured)
 			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;
@@ -334,12 +339,16 @@ struct omap_mbox *omap_mbox_get(const char *name)
 	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);
@@ -363,10 +372,13 @@ int omap_mbox_register(struct device *parent, struct omap_mbox **list)
 			ret = PTR_ERR(mbox->dev);
 			goto err_out;
 		}
+
 		if (cpu_is_omap44xx())
 			mbox->rev = OMAP_MBOX_IP_VERSION_2;
 		else
 			mbox->rev = OMAP_MBOX_IP_LEGACY;
+
+		BLOCKING_INIT_NOTIFIER_HEAD(&mbox->notifier);
 	}
 	return 0;
 
-- 
1.7.0

[PATCH v4 2/5] OMAP: mailbox: fix rx interrupt disable in omap4

[Varadarajan, Charulatha]

On Wed, Nov 24, 2010 at 02:56, Hari Kanigeri <h-kanigeri2@ti.com> wrote:

Should the subject be like this?
OMAP4: mailbox: fix rx interrupt disable

> disablign rx interrupt on omap4 is different than its pre-decessors.

Typo ->disablign

> The bit in OMAP4_MAILBOX_IRQENABLE_CLR should be set to disable the
> interrupts instead of clearing the bit.
>
> Defined rev field in mailbox structure to differentiate the mailbox
> versions.
>
> Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
> ---
> ?arch/arm/mach-omap2/mailbox.c ? ? ? ? ? ? | ? ?7 ++++++-
> ?arch/arm/plat-omap/include/plat/mailbox.h | ? ?4 ++++
> ?arch/arm/plat-omap/mailbox.c ? ? ? ? ? ? ?| ? ?4 ++++
> ?3 files changed, 14 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/mailbox.c b/arch/arm/mach-omap2/mailbox.c
> index 42dbfa4..c32058d 100644
> --- a/arch/arm/mach-omap2/mailbox.c
> +++ b/arch/arm/mach-omap2/mailbox.c
> @@ -195,7 +195,12 @@ 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 (mbox->rev == OMAP_MBOX_IP_VERSION_2)
> + ? ? ? ? ? ? ? l |= bit;
> + ? ? ? else
> + ? ? ? ? ? ? ? l &= ~bit;
> +
> ? ? ? ?mbox_write_reg(l, p->irqdisable);
> ?}
>
> diff --git a/arch/arm/plat-omap/include/plat/mailbox.h b/arch/arm/plat-omap/include/plat/mailbox.h
> index 13f2ef3..1655c61 100644
> --- a/arch/arm/plat-omap/include/plat/mailbox.h
> +++ b/arch/arm/plat-omap/include/plat/mailbox.h
> @@ -20,6 +20,9 @@ typedef int __bitwise omap_mbox_type_t;
> ?#define OMAP_MBOX_TYPE1 ((__force omap_mbox_type_t) 1)
> ?#define OMAP_MBOX_TYPE2 ((__force omap_mbox_type_t) 2)
>
> +#define OMAP_MBOX_IP_LEGACY ? ? ? ? ? ? ? ? ? ?0x1
> +#define OMAP_MBOX_IP_VERSION_2 ? ? ? ? ? ? ? ? 0x2
> +
> ?struct omap_mbox_ops {
> ? ? ? ?omap_mbox_type_t ? ? ? ?type;
> ? ? ? ?int ? ? ? ? ? ? (*startup)(struct omap_mbox *mbox);
> @@ -58,6 +61,7 @@ struct omap_mbox {
> ? ? ? ?struct omap_mbox_ops ? ?*ops;
> ? ? ? ?struct device ? ? ? ? ? *dev;
> ? ? ? ?void ? ? ? ? ? ? ? ? ? ?*priv;
> + ? ? ? int ? ? ? ? ? ? ? ? ? ? rev;
> ?};
>
> ?int omap_mbox_msg_send(struct omap_mbox *, mbox_msg_t msg);
> diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
> index 48e161c..a1c6bd9 100644
> --- a/arch/arm/plat-omap/mailbox.c
> +++ b/arch/arm/plat-omap/mailbox.c
> @@ -358,6 +358,10 @@ int omap_mbox_register(struct device *parent, struct omap_mbox **list)
> ? ? ? ? ? ? ? ? ? ? ? ?ret = PTR_ERR(mbox->dev);
> ? ? ? ? ? ? ? ? ? ? ? ?goto err_out;
> ? ? ? ? ? ? ? ?}
> + ? ? ? ? ? ? ? if (cpu_is_omap44xx())

Do not use cpu_is* checks in plat-omap/*

> + ? ? ? ? ? ? ? ? ? ? ? mbox->rev = OMAP_MBOX_IP_VERSION_2;
> + ? ? ? ? ? ? ? else
> + ? ? ? ? ? ? ? ? ? ? ? mbox->rev = OMAP_MBOX_IP_LEGACY;
> ? ? ? ?}
> ? ? ? ?return 0;
>
> --
> 1.7.0
>

[PATCH v4 1/5] OMAP: mailbox: change full flag per mailbox queue instead of global

[Varadarajan, Charulatha]

On Wed, Nov 24, 2010 at 02:56, Hari Kanigeri <h-kanigeri2@ti.com> wrote:
> From: Fernando Guzman Lugo <x0095840@ti.com>
>
> The variable rq_full flag is a global variable, so if there are multiple
> mailbox users there will be conflicts. Now there is a full flag per
> mailbox queue.
>
> 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 ? ? ? ? ? ? ?| ? ?9 +++++++--
> ?2 files changed, 8 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..48e161c 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,12 @@ static void mbox_rx_work(struct work_struct *work)
>
> ? ? ? ? ? ? ? ?if (mq->callback)
> ? ? ? ? ? ? ? ? ? ? ? ?mq->callback((void *)msg);
> + ? ? ? ? ? ? ? spin_lock_irq(&mq->lock);
> + ? ? ? ? ? ? ? if (mq->full) {
> + ? ? ? ? ? ? ? ? ? ? ? mq->full = false;
> + ? ? ? ? ? ? ? ? ? ? ? omap_mbox_enable_irq(mq->mbox, IRQ_RX);
> + ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? spin_unlock_irq(&mq->lock);
> ? ? ? ?}
> ?}
>
> @@ -170,7 +175,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;

Should this also be inside spin_lock_irq?

> ? ? ? ? ? ? ? ? ? ? ? ?goto nomem;
> ? ? ? ? ? ? ? ?}
>
> --
> 1.7.0
>

[PATCH v4 4/5] OMAP: mailbox: send message in process context

[Varadarajan, Charulatha]

On Wed, Nov 24, 2010 at 02:56, Hari Kanigeri <h-kanigeri2@ti.com> wrote:
> Schedule the Tasklet to send only when mailbox fifo is full and there are
> pending messages in kifo, else send the message directly in the Process

Typo -> kifo

> 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 13698ab..f4177df 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 (kfifo_is_empty(&mq->fifo) && !__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 v4 5/5] OMAP: mailbox: add notification support for multiple readers

[Varadarajan, Charulatha]

On Wed, Nov 24, 2010 at 02:56, Hari Kanigeri <h-kanigeri2@ti.com> wrote:
> 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 | ? ?7 +-
> ?arch/arm/plat-omap/mailbox.c ? ? ? ? ? ? ?| ?104 ++++++++++++++++-------------
> ?2 files changed, 62 insertions(+), 49 deletions(-)
>

<<snip>>

> @@ -363,10 +372,13 @@ int omap_mbox_register(struct device *parent, struct omap_mbox **list)
> ? ? ? ? ? ? ? ? ? ? ? ?ret = PTR_ERR(mbox->dev);
> ? ? ? ? ? ? ? ? ? ? ? ?goto err_out;
> ? ? ? ? ? ? ? ?}
> +

Do this change in the patch where the below code was added.

> ? ? ? ? ? ? ? ?if (cpu_is_omap44xx())
> ? ? ? ? ? ? ? ? ? ? ? ?mbox->rev = OMAP_MBOX_IP_VERSION_2;
> ? ? ? ? ? ? ? ?else
> ? ? ? ? ? ? ? ? ? ? ? ?mbox->rev = OMAP_MBOX_IP_LEGACY;
> +
> + ? ? ? ? ? ? ? BLOCKING_INIT_NOTIFIER_HEAD(&mbox->notifier);
> ? ? ? ?}
> ? ? ? ?return 0;
>
> --
> 1.7.0
>

[PATCH v4 2/5] OMAP: mailbox: fix rx interrupt disable in omap4

[Felipe Balbi]

On Wed, Nov 24, 2010 at 10:46:04AM +0530, Varadarajan, Charulatha wrote:
>> diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
>> index 48e161c..a1c6bd9 100644
>> --- a/arch/arm/plat-omap/mailbox.c
>> +++ b/arch/arm/plat-omap/mailbox.c
>> @@ -358,6 +358,10 @@ int omap_mbox_register(struct device *parent, struct omap_mbox **list)
>> ? ? ? ? ? ? ? ? ? ? ? ?ret = PTR_ERR(mbox->dev);
>> ? ? ? ? ? ? ? ? ? ? ? ?goto err_out;
>> ? ? ? ? ? ? ? ?}
>> + ? ? ? ? ? ? ? if (cpu_is_omap44xx())
>
>Do not use cpu_is* checks in plat-omap/*

see the previous thread.

-- 
balbi

[PATCH v4 2/5] OMAP: mailbox: fix rx interrupt disable in omap4

[Varadarajan, Charulatha]

On Wed, Nov 24, 2010 at 13:52, Felipe Balbi <balbi@ti.com> wrote:
> On Wed, Nov 24, 2010 at 10:46:04AM +0530, Varadarajan, Charulatha wrote:
>>>
>>> diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
>>> index 48e161c..a1c6bd9 100644
>>> --- a/arch/arm/plat-omap/mailbox.c
>>> +++ b/arch/arm/plat-omap/mailbox.c
>>> @@ -358,6 +358,10 @@ int omap_mbox_register(struct device *parent, struct
>>> omap_mbox **list)
>>> ? ? ? ? ? ? ? ? ? ? ? ?ret = PTR_ERR(mbox->dev);
>>> ? ? ? ? ? ? ? ? ? ? ? ?goto err_out;
>>> ? ? ? ? ? ? ? ?}
>>> + ? ? ? ? ? ? ? if (cpu_is_omap44xx())
>>
>> Do not use cpu_is* checks in plat-omap/*
>
> see the previous thread.

Referring to [1], I do not find why cpu_is* checks is used in plat-omap and
why it can't be avoided.

In [1], it was suggested to create the pdata field that will be
populated at init
time using the cpu_is* check. But in this version, I am finding that
this is done
in plat-omap. This can be handled in mach-omap layer itself and can be passed
as a pdata field which can be extracted during probe.

[1] https://patchwork.kernel.org/patch/337131/

[PATCH v4 2/5] OMAP: mailbox: fix rx interrupt disable in omap4

[Felipe Balbi]

On Wed, Nov 24, 2010 at 02:20:40PM +0530, Varadarajan, Charulatha wrote:
>On Wed, Nov 24, 2010 at 13:52, Felipe Balbi <balbi@ti.com> wrote:
>> On Wed, Nov 24, 2010 at 10:46:04AM +0530, Varadarajan, Charulatha wrote:
>>>>
>>>> diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
>>>> index 48e161c..a1c6bd9 100644
>>>> --- a/arch/arm/plat-omap/mailbox.c
>>>> +++ b/arch/arm/plat-omap/mailbox.c
>>>> @@ -358,6 +358,10 @@ int omap_mbox_register(struct device *parent, struct
>>>> omap_mbox **list)
>>>> ? ? ? ? ? ? ? ? ? ? ? ?ret = PTR_ERR(mbox->dev);
>>>> ? ? ? ? ? ? ? ? ? ? ? ?goto err_out;
>>>> ? ? ? ? ? ? ? ?}
>>>> + ? ? ? ? ? ? ? if (cpu_is_omap44xx())
>>>
>>> Do not use cpu_is* checks in plat-omap/*
>>
>> see the previous thread.
>
>Referring to [1], I do not find why cpu_is* checks is used in plat-omap and
>why it can't be avoided.
>
>In [1], it was suggested to create the pdata field that will be
>populated at init
>time using the cpu_is* check. But in this version, I am finding that
>this is done
>in plat-omap. This can be handled in mach-omap layer itself and can be passed
>as a pdata field which can be extracted during probe.
>
>[1] https://patchwork.kernel.org/patch/337131/

true, since I saw mbox->rev, I confused that with something that was
coming from pdata, sorry. You're right, this is not the right way to do
it.

-- 
balbi

[PATCH v4 1/5] OMAP: mailbox: change full flag per mailbox queue instead of global

[Kanigeri, Hari]

On Tue, Nov 23, 2010 at 11:21 PM, Varadarajan, Charulatha <charu@ti.com> wrote:
> On Wed, Nov 24, 2010 at 02:56, Hari Kanigeri <h-kanigeri2@ti.com> wrote:
>> From: Fernando Guzman Lugo <x0095840@ti.com>
>>
>> The variable rq_full flag is a global variable, so if there are multiple
>> mailbox users there will be conflicts. Now there is a full flag per
>> mailbox queue.
>>
>> 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 ? ? ? ? ? ? ?| ? ?9 +++++++--
>> ?2 files changed, 8 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..48e161c 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,12 @@ static void mbox_rx_work(struct work_struct *work)
>>
>> ? ? ? ? ? ? ? ?if (mq->callback)
>> ? ? ? ? ? ? ? ? ? ? ? ?mq->callback((void *)msg);
>> + ? ? ? ? ? ? ? spin_lock_irq(&mq->lock);
>> + ? ? ? ? ? ? ? if (mq->full) {
>> + ? ? ? ? ? ? ? ? ? ? ? mq->full = false;
>> + ? ? ? ? ? ? ? ? ? ? ? omap_mbox_enable_irq(mq->mbox, IRQ_RX);
>> + ? ? ? ? ? ? ? }
>> + ? ? ? ? ? ? ? spin_unlock_irq(&mq->lock);
>> ? ? ? ?}
>> ?}
>>
>> @@ -170,7 +175,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;
>
> Should this also be inside spin_lock_irq?
>

Not needed. this is already run from interrupt context.

Thank you,
Best regards,
Hari

[PATCH v4 2/5] OMAP: mailbox: fix rx interrupt disable in omap4

[Kanigeri, Hari]

On Wed, Nov 24, 2010 at 2:50 AM, Varadarajan, Charulatha <charu@ti.com> wrote:
> On Wed, Nov 24, 2010 at 13:52, Felipe Balbi <balbi@ti.com> wrote:
>> On Wed, Nov 24, 2010 at 10:46:04AM +0530, Varadarajan, Charulatha wrote:
>>>>
>>>> diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
>>>> index 48e161c..a1c6bd9 100644
>>>> --- a/arch/arm/plat-omap/mailbox.c
>>>> +++ b/arch/arm/plat-omap/mailbox.c
>>>> @@ -358,6 +358,10 @@ int omap_mbox_register(struct device *parent, struct
>>>> omap_mbox **list)
>>>> ? ? ? ? ? ? ? ? ? ? ? ?ret = PTR_ERR(mbox->dev);
>>>> ? ? ? ? ? ? ? ? ? ? ? ?goto err_out;
>>>> ? ? ? ? ? ? ? ?}
>>>> + ? ? ? ? ? ? ? if (cpu_is_omap44xx())
>>>
>>> Do not use cpu_is* checks in plat-omap/*
>>
>> see the previous thread.
>
> Referring to [1], I do not find why cpu_is* checks is used in plat-omap and
> why it can't be avoided.
>
> In [1], it was suggested to create the pdata field that will be
> populated at init
> time using the cpu_is* check. But in this version, I am finding that
> this is done
> in plat-omap. This can be handled in mach-omap layer itself and can be passed
> as a pdata field which can be extracted during probe.
>
> [1] https://patchwork.kernel.org/patch/337131/
>

Here are these  reasons why I did this way.

- The function omap_mbox_register is called only once during probe
time, and I thought it was ok to call cpu check once during probe
time. Since each mbox instance needs to be aware of the rev field this
was the right place to add since this function iterates through the
list during probe time. There are already calls to cpu_is_omap44xx in
mailbox probe function.

- platform data is not present for mailbox module. I could add it for
revision sake but I would prefer not to do that since this will be a
throw away code once the hwmod infrastructure is ready (note: mailbox
hwmod patches are under review), and amount of mailbox driver rework
might be considerable.

- I could wait till the hwmod patches are ready to include this
change, but don't want to put the dependency with hwmod since this is
a critical fix and want to make it available in the mainline kernel.

Please let me know what you suggest.

Thank you,
Best regards,
Hari Kanigeri

[PATCH v4 1/5] OMAP: mailbox: change full flag per mailbox queue instead of global

[Varadarajan, Charulatha]

On Wed, Nov 24, 2010 at 18:17, Kanigeri, Hari <h-kanigeri2@ti.com> wrote:
> On Tue, Nov 23, 2010 at 11:21 PM, Varadarajan, Charulatha <charu@ti.com> wrote:
>> On Wed, Nov 24, 2010 at 02:56, Hari Kanigeri <h-kanigeri2@ti.com> wrote:
>>> From: Fernando Guzman Lugo <x0095840@ti.com>
>>>
>>> The variable rq_full flag is a global variable, so if there are multiple
>>> mailbox users there will be conflicts. Now there is a full flag per
>>> mailbox queue.
>>>
>>> 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 ? ? ? ? ? ? ?| ? ?9 +++++++--
>>> ?2 files changed, 8 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..48e161c 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,12 @@ static void mbox_rx_work(struct work_struct *work)
>>>
>>> ? ? ? ? ? ? ? ?if (mq->callback)
>>> ? ? ? ? ? ? ? ? ? ? ? ?mq->callback((void *)msg);
>>> + ? ? ? ? ? ? ? spin_lock_irq(&mq->lock);
>>> + ? ? ? ? ? ? ? if (mq->full) {
>>> + ? ? ? ? ? ? ? ? ? ? ? mq->full = false;
>>> + ? ? ? ? ? ? ? ? ? ? ? omap_mbox_enable_irq(mq->mbox, IRQ_RX);
>>> + ? ? ? ? ? ? ? }
>>> + ? ? ? ? ? ? ? spin_unlock_irq(&mq->lock);
>>> ? ? ? ?}
>>> ?}
>>>
>>> @@ -170,7 +175,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;
>>
>> Should this also be inside spin_lock_irq?
>>
>
> Not needed. this is already run from interrupt context.

oh ok.

>
> Thank you,
> Best regards,
> Hari
>

[PATCH v4 5/5] OMAP: mailbox: add notification support for multiple readers

[Kanigeri, Hari]

On Tue, Nov 23, 2010 at 11:29 PM, Varadarajan, Charulatha <charu@ti.com> wrote:
> On Wed, Nov 24, 2010 at 02:56, Hari Kanigeri <h-kanigeri2@ti.com> wrote:
>> 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 | ? ?7 +-
>> ?arch/arm/plat-omap/mailbox.c ? ? ? ? ? ? ?| ?104 ++++++++++++++++-------------
>> ?2 files changed, 62 insertions(+), 49 deletions(-)
>>
>
> <<snip>>
>
>> @@ -363,10 +372,13 @@ int omap_mbox_register(struct device *parent, struct omap_mbox **list)
>> ? ? ? ? ? ? ? ? ? ? ? ?ret = PTR_ERR(mbox->dev);
>> ? ? ? ? ? ? ? ? ? ? ? ?goto err_out;
>> ? ? ? ? ? ? ? ?}
>> +
>
> Do this change in the patch where the below code was added.

I am sorry I didn't get you. you mean BLOCKING_INIT_NOTIFIER_HEAD line
? yes, it is needed for this patch.
>
>> ? ? ? ? ? ? ? ?if (cpu_is_omap44xx())
>> ? ? ? ? ? ? ? ? ? ? ? ?mbox->rev = OMAP_MBOX_IP_VERSION_2;
>> ? ? ? ? ? ? ? ?else
>> ? ? ? ? ? ? ? ? ? ? ? ?mbox->rev = OMAP_MBOX_IP_LEGACY;
>> +
>> + ? ? ? ? ? ? ? BLOCKING_INIT_NOTIFIER_HEAD(&mbox->notifier);
>> ? ? ? ?}
>> ? ? ? ?return 0;
>>
>> --
>> 1.7.0
>>
>



-- 
Thank you,
Best regards,
Hari Kanigeri

[PATCH v4 5/5] OMAP: mailbox: add notification support for multiple readers

[Varadarajan, Charulatha]

On Wed, Nov 24, 2010 at 18:39, Kanigeri, Hari <h-kanigeri2@ti.com> wrote:
> On Tue, Nov 23, 2010 at 11:29 PM, Varadarajan, Charulatha <charu@ti.com> wrote:
>> On Wed, Nov 24, 2010 at 02:56, Hari Kanigeri <h-kanigeri2@ti.com> wrote:
>>> 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 | ? ?7 +-
>>> ?arch/arm/plat-omap/mailbox.c ? ? ? ? ? ? ?| ?104 ++++++++++++++++-------------
>>> ?2 files changed, 62 insertions(+), 49 deletions(-)
>>>
>>
>> <<snip>>
>>
>>> @@ -363,10 +372,13 @@ int omap_mbox_register(struct device *parent, struct omap_mbox **list)
>>> ? ? ? ? ? ? ? ? ? ? ? ?ret = PTR_ERR(mbox->dev);
>>> ? ? ? ? ? ? ? ? ? ? ? ?goto err_out;
>>> ? ? ? ? ? ? ? ?}
>>> +
>>
>> Do this change in the patch where the below code was added.
>
> I am sorry I didn't get you. you mean BLOCKING_INIT_NOTIFIER_HEAD line
> ? yes, it is needed for this patch.

No. I was saying that adding a new line could be done in the patch where this
piece of code (below 4 lines) was introduced. Sorry if that was not clear :-(

>>
>>> ? ? ? ? ? ? ? ?if (cpu_is_omap44xx())
>>> ? ? ? ? ? ? ? ? ? ? ? ?mbox->rev = OMAP_MBOX_IP_VERSION_2;
>>> ? ? ? ? ? ? ? ?else
>>> ? ? ? ? ? ? ? ? ? ? ? ?mbox->rev = OMAP_MBOX_IP_LEGACY;
>>> +
>>> + ? ? ? ? ? ? ? BLOCKING_INIT_NOTIFIER_HEAD(&mbox->notifier);
>>> ? ? ? ?}
>>> ? ? ? ?return 0;
>>>
>>> --
>>> 1.7.0
>>>
>>
>
>
>
> --
> Thank you,
> Best regards,
> Hari Kanigeri
>

[PATCH v4 4/5] OMAP: mailbox: send message in process context

[Kanigeri, Hari]

On Tue, Nov 23, 2010 at 11:26 PM, Varadarajan, Charulatha <charu@ti.com> wrote:
> On Wed, Nov 24, 2010 at 02:56, Hari Kanigeri <h-kanigeri2@ti.com> wrote:
>> Schedule the Tasklet to send only when mailbox fifo is full and there are
>> pending messages in kifo, else send the message directly in the Process
>
> Typo -> kifo

Thanks :). My bad I I think overlooked Felipe's comment on my previous patch.

Thank you,
Best regards,
Hari Kanigeri

[PATCH v4 5/5] OMAP: mailbox: add notification support for multiple readers

[Kanigeri, Hari]

On Wed, Nov 24, 2010 at 7:12 AM, Varadarajan, Charulatha <charu@ti.com> wrote:
> On Wed, Nov 24, 2010 at 18:39, Kanigeri, Hari <h-kanigeri2@ti.com> wrote:
>> On Tue, Nov 23, 2010 at 11:29 PM, Varadarajan, Charulatha <charu@ti.com> wrote:
>>> On Wed, Nov 24, 2010 at 02:56, Hari Kanigeri <h-kanigeri2@ti.com> wrote:
>>>> 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 | ? ?7 +-
>>>> ?arch/arm/plat-omap/mailbox.c ? ? ? ? ? ? ?| ?104 ++++++++++++++++-------------
>>>> ?2 files changed, 62 insertions(+), 49 deletions(-)
>>>>
>>>
>>> <<snip>>
>>>
>>>> @@ -363,10 +372,13 @@ int omap_mbox_register(struct device *parent, struct omap_mbox **list)
>>>> ? ? ? ? ? ? ? ? ? ? ? ?ret = PTR_ERR(mbox->dev);
>>>> ? ? ? ? ? ? ? ? ? ? ? ?goto err_out;
>>>> ? ? ? ? ? ? ? ?}
>>>> +
>>>
>>> Do this change in the patch where the below code was added.
>>
>> I am sorry I didn't get you. you mean BLOCKING_INIT_NOTIFIER_HEAD line
>> ? yes, it is needed for this patch.
>
> No. I was saying that adding a new line could be done in the patch where this
> piece of code (below 4 lines) was introduced. Sorry if that was not clear :-(

Got it. extra line can be removed.

Thank you,
Best regards,
Hari Kanigeri

[PATCH v4 2/5] OMAP: mailbox: fix rx interrupt disable in omap4

[Varadarajan, Charulatha]

Hari,

On Wed, Nov 24, 2010 at 18:31, Kanigeri, Hari <h-kanigeri2@ti.com> wrote:
> On Wed, Nov 24, 2010 at 2:50 AM, Varadarajan, Charulatha <charu@ti.com> wrote:
>> On Wed, Nov 24, 2010 at 13:52, Felipe Balbi <balbi@ti.com> wrote:
>>> On Wed, Nov 24, 2010 at 10:46:04AM +0530, Varadarajan, Charulatha wrote:
>>>>>
>>>>> diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
>>>>> index 48e161c..a1c6bd9 100644
>>>>> --- a/arch/arm/plat-omap/mailbox.c
>>>>> +++ b/arch/arm/plat-omap/mailbox.c
>>>>> @@ -358,6 +358,10 @@ int omap_mbox_register(struct device *parent, struct
>>>>> omap_mbox **list)
>>>>> ? ? ? ? ? ? ? ? ? ? ? ?ret = PTR_ERR(mbox->dev);
>>>>> ? ? ? ? ? ? ? ? ? ? ? ?goto err_out;
>>>>> ? ? ? ? ? ? ? ?}
>>>>> + ? ? ? ? ? ? ? if (cpu_is_omap44xx())
>>>>
>>>> Do not use cpu_is* checks in plat-omap/*
>>>
>>> see the previous thread.
>>
>> Referring to [1], I do not find why cpu_is* checks is used in plat-omap and
>> why it can't be avoided.
>>
>> In [1], it was suggested to create the pdata field that will be
>> populated at init
>> time using the cpu_is* check. But in this version, I am finding that
>> this is done
>> in plat-omap. This can be handled in mach-omap layer itself and can be passed
>> as a pdata field which can be extracted during probe.
>>
>> [1] https://patchwork.kernel.org/patch/337131/
>>
>
> Here are these ?reasons why I did this way.
>
> - The function omap_mbox_register is called only once during probe
> time, and I thought it was ok to call cpu check once during probe
> time.

AFAIK it is not okay. Patches are not accepted by maintainers if cpu_is*
checks are newly added in plat-omap/*
Moreover, the plat-omap is for common code between omap1 and omap2+
If any specific info is required due to unavoidable reasons, they should be
managed using pdata fld/ by some other means.

> Since each mbox instance needs to be aware of the rev field this
> was the right place to add since this function iterates through the
> list during probe time. There are already calls to cpu_is_omap44xx in
> mailbox probe function.

Infact , there are some more drivers like that. But some of them are getting
cleaned up slowly (work under progress). But old code having cpu_is* checks
does not justify adding a new cpu_is* check. As I mentioned, it is repeatedly
being insisted in LO mailing list to avoid cpu_is* checks in plat-omap and to
clean up the drivers to avoid these checks.

>
> - platform data is not present for mailbox module. I could add it for
> revision sake but I would prefer not to do that since this will be a
> throw away code once the hwmod infrastructure is ready (note: mailbox
> hwmod patches are under review), and amount of mailbox driver rework
> might be considerable.

I would leave this to Tony/ Felipe/ Benoit to decide on this. If agreed, I don't
see any issue. But there should be atleast a "TODO" mentioning that this
needs to be cleaned up.

>
> - I could wait till the hwmod patches are ready to include this
> change, but don't want to put the dependency with hwmod since this is
> a critical fix and want to make it available in the mainline kernel.
>
> Please let me know what you suggest.
>
> Thank you,
> Best regards,
> Hari Kanigeri
>

[PATCH v4 2/5] OMAP: mailbox: fix rx interrupt disable in omap4

[Hari Kanigeri]

On Thu, Nov 25, 2010 at 1:04 AM, Varadarajan, Charulatha <charu@ti.com> wrote:
> Hari,
>
> On Wed, Nov 24, 2010 at 18:31, Kanigeri, Hari <h-kanigeri2@ti.com> wrote:
>> On Wed, Nov 24, 2010 at 2:50 AM, Varadarajan, Charulatha <charu@ti.com> wrote:
>>> On Wed, Nov 24, 2010 at 13:52, Felipe Balbi <balbi@ti.com> wrote:
>>>> On Wed, Nov 24, 2010 at 10:46:04AM +0530, Varadarajan, Charulatha wrote:
>>>>>>
>>>>>> diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
>>>>>> index 48e161c..a1c6bd9 100644
>>>>>> --- a/arch/arm/plat-omap/mailbox.c
>>>>>> +++ b/arch/arm/plat-omap/mailbox.c
>>>>>> @@ -358,6 +358,10 @@ int omap_mbox_register(struct device *parent, struct
>>>>>> omap_mbox **list)
>>>>>> ? ? ? ? ? ? ? ? ? ? ? ?ret = PTR_ERR(mbox->dev);
>>>>>> ? ? ? ? ? ? ? ? ? ? ? ?goto err_out;
>>>>>> ? ? ? ? ? ? ? ?}
>>>>>> + ? ? ? ? ? ? ? if (cpu_is_omap44xx())
>>>>>
>>>>> Do not use cpu_is* checks in plat-omap/*
>>>>
>>>> see the previous thread.
>>>
>>> Referring to [1], I do not find why cpu_is* checks is used in plat-omap and
>>> why it can't be avoided.
>>>
>>> In [1], it was suggested to create the pdata field that will be
>>> populated at init
>>> time using the cpu_is* check. But in this version, I am finding that
>>> this is done
>>> in plat-omap. This can be handled in mach-omap layer itself and can be passed
>>> as a pdata field which can be extracted during probe.
>>>
>>> [1] https://patchwork.kernel.org/patch/337131/
>>>
>>
>> Here are these ?reasons why I did this way.
>>
>> - The function omap_mbox_register is called only once during probe
>> time, and I thought it was ok to call cpu check once during probe
>> time.
>
> AFAIK it is not okay. Patches are not accepted by maintainers if cpu_is*
> checks are newly added in plat-omap/*
> Moreover, the plat-omap is for common code between omap1 and omap2+
> If any specific info is required due to unavoidable reasons, they should be
> managed using pdata fld/ by some other means.
>
>> Since each mbox instance needs to be aware of the rev field this
>> was the right place to add since this function iterates through the
>> list during probe time. There are already calls to cpu_is_omap44xx in
>> mailbox probe function.
>
> Infact , there are some more drivers like that. But some of them are getting
> cleaned up slowly (work under progress). But old code having cpu_is* checks
> does not justify adding a new cpu_is* check. As I mentioned, it is repeatedly
> being insisted in LO mailing list to avoid cpu_is* checks in plat-omap and to
> clean up the drivers to avoid these checks.
>
>>
>> - platform data is not present for mailbox module. I could add it for
>> revision sake but I would prefer not to do that since this will be a
>> throw away code once the hwmod infrastructure is ready (note: mailbox
>> hwmod patches are under review), and amount of mailbox driver rework
>> might be considerable.
>
> I would leave this to Tony/ Felipe/ Benoit to decide on this. If agreed, I don't
> see any issue. But there should be atleast a "TODO" mentioning that this
> needs to be cleaned up.
>

fair enough. I am dropping this patch from the patch set and will send
it with Omar's hwmod patches.
I will send the revised patch set excluding this patch.

Thank you,
Best regards,
Hari