On Wed, Jun 9, 2010 at 12:07 AM, Hiroshi DOYU <Hiroshi.DOYU@xxxxxxxxx> wrote:
> diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
> index 87e0cde..1b79b32 100644
> --- a/arch/arm/plat-omap/mailbox.c
> +++ b/arch/arm/plat-omap/mailbox.c
> @@ -188,7 +188,7 @@ static void __mbox_rx_interrupt(struct omap_mbox *mbox)
> /* no more messages in the fifo. clear IRQ source. */
> ack_mbox_irq(mbox, IRQ_RX);
> nomem:
> - queue_work(mboxd, &mbox->rxq->work);
> + mbox->callback(mbox);
> }
I like this !
It will allow us to easily plug in new IPC mechanisms in the future.
(btw, if we do this, and also introduce omap_mbox_msg_send_sync, do we
still need a mailbox queuing mechanism at all ? :)
Having said that, this is not going to solve the lockdep warning
reported by Deepak - that was caused because of dspbridge's sending
context (and not because of the receiving context). To eliminate that
issue, I prefer fixing dspbridge to use work queues rather than using
spin_lock_bh in omap_mbox_msg_send. Disabling system bottom halves
just to send a mbox msg sounds unjustified (unless bridge really needs
to use tasklets instead of work queues, which I slightly doubt). What
do you think ?
Speaking of mailbox I'd like to address some issues that are code related:
* Let's add mailbox API to set the callback pointer (it feels wrong to
let users directly manipulate the mbox structure).
* We can also safely move the callback field to the main mbox
structure, since it has no usage in the TX mq; it's pretty global to
the whole mbox.
* Let's make sure no one accidentally registers two receivers on the
same time (which would result in one overwriting the other's callback
field).
* mbox_configured is a global variable and that breaks support of
multiple concurrent mailbox instances. Let's move this logic into the
instance of mbox.
* If we make sure there are no two user of the same mailbox instance
(see 3rd *), we can eliminate mbox_configured and its locking (not
that doesn't mean there can't be two concurrent sending contexts, it
just means they are somewhat related, they have a common receiver,
which was registered using only a single call to get).
Something like this, pls tell me what you think:
>From 353d36d7fd6ec76c81689893338dfd4d33c8c89a Mon Sep 17 00:00:00 2001
From: Ohad Ben-Cohen <ohad@xxxxxxxxxx>
Date: Sun, 13 Jun 2010 18:32:22 -0500
Subject: [PATCH] mailbox: enforce sane usage
Signed-off-by: Ohad Ben-Cohen <ohad@xxxxxxxxxx>
---
I can also be contacted via < ohadb at ti dot com >
arch/arm/plat-omap/include/plat/mailbox.h | 5 ++-
arch/arm/plat-omap/mailbox.c | 38 +++++++++++++++-------------
2 files changed, 23 insertions(+), 20 deletions(-)
diff --git a/arch/arm/plat-omap/include/plat/mailbox.h
b/arch/arm/plat-omap/include/plat/mailbox.h
index 729166b..0a5bf19 100644
--- a/arch/arm/plat-omap/include/plat/mailbox.h
+++ b/arch/arm/plat-omap/include/plat/mailbox.h
@@ -45,7 +45,6 @@ struct omap_mbox_queue {
struct request_queue *queue;
struct work_struct work;
struct tasklet_struct tasklet;
- int (*callback)(void *);
struct omap_mbox *mbox;
};
@@ -65,12 +64,14 @@ struct omap_mbox {
void *priv;
void (*err_notify)(void);
+ int (*callback)(void *);
+ atomic_t count;
};
int omap_mbox_msg_send(struct omap_mbox *, mbox_msg_t msg);
void omap_mbox_init_seq(struct omap_mbox *);
-struct omap_mbox *omap_mbox_get(const char *);
+struct omap_mbox *omap_mbox_get(const char *, int (*)(void *));
void omap_mbox_put(struct omap_mbox *);
int omap_mbox_register(struct device *parent, struct omap_mbox *);
diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index 08a2df7..5f29ea4 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -33,8 +33,6 @@ static struct workqueue_struct *mboxd;
static struct omap_mbox *mboxes;
static DEFINE_RWLOCK(mboxes_lock);
-static int mbox_configured;
-
/* Mailbox FIFO handle functions */
static inline mbox_msg_t mbox_fifo_read(struct omap_mbox *mbox)
{
@@ -146,7 +144,7 @@ static void mbox_rx_work(struct work_struct *work)
msg = (mbox_msg_t)rq->special;
blk_end_request_all(rq, 0);
- mbox->rxq->callback((void *)msg);
+ mbox->callback((void *)msg);
}
}
@@ -249,16 +247,10 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
struct omap_mbox_queue *mq;
if (likely(mbox->ops->startup)) {
- write_lock(&mboxes_lock);
- if (!mbox_configured)
- ret = mbox->ops->startup(mbox);
-
+ ret = mbox->ops->startup(mbox);
if (unlikely(ret)) {
- write_unlock(&mboxes_lock);
return ret;
}
- mbox_configured++;
- write_unlock(&mboxes_lock);
}
ret = request_irq(mbox->irq, mbox_interrupt, IRQF_SHARED,
@@ -304,13 +296,10 @@ static void omap_mbox_fini(struct omap_mbox *mbox)
free_irq(mbox->irq, mbox);
if (unlikely(mbox->ops->shutdown)) {
- write_lock(&mboxes_lock);
- if (mbox_configured > 0)
- mbox_configured--;
- if (!mbox_configured)
- mbox->ops->shutdown(mbox);
- write_unlock(&mboxes_lock);
+ mbox->ops->shutdown(mbox);
}
+
+ atomic_dec(&mbox->count);
}
static struct omap_mbox **find_mboxes(const char *name)
@@ -325,7 +314,7 @@ static struct omap_mbox **find_mboxes(const char *name)
return p;
}
-struct omap_mbox *omap_mbox_get(const char *name)
+struct omap_mbox *omap_mbox_get(const char *name, int (*callback)(void *))
{
struct omap_mbox *mbox;
int ret;
@@ -339,9 +328,19 @@ struct omap_mbox *omap_mbox_get(const char *name)
read_unlock(&mboxes_lock);
+ if (atomic_inc_return(&mbox->count) > 1) {
+ pr_err("%s: mbox %s already in use\n", __func__, mbox->name);
+ atomic_dec(&mbox->count);
+ return ERR_PTR(-EBUSY);
+ }
+
ret = omap_mbox_startup(mbox);
- if (ret)
+ if (ret) {
+ atomic_dec(&mbox->count);
return ERR_PTR(-ENODEV);
+ }
+
+ mbox->callback = callback;
return mbox;
}
@@ -370,6 +369,9 @@ int omap_mbox_register(struct device *parent,
struct omap_mbox *mbox)
write_unlock(&mboxes_lock);
goto err_find;
}
+
+ atomic_set(&mbox->count, 0);
+
*tmp = mbox;
write_unlock
Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
