Re: [PATCH 2/2] omap:mailbox-provide multiple reader support
Hi Hari,
On Wed, Jul 28, 2010 at 1:02 AM, Ohad Ben-Cohen wrote:
> On Wed, Jul 21, 2010 at 12:41 AM, Hari Kanigeri wrote:
> > This patch provides mutiple readers support for a mailbox
> > instance.
> >
> > Signed-off-by: Hari Kanigeri
> > ---
> > arch/arm/plat-omap/include/plat/mailbox.h | 6 ++-
> > arch/arm/plat-omap/mailbox.c | 63
> >
> > 2 files changed, 40 insertions(+), 29 deletions(-)
> >
> > diff --git a/arch/arm/plat-omap/include/plat/mailbox.h
> > b/arch/arm/plat-omap/include/plat/mailbox.h
> > index 0486d64..c8e47d8 100644
> > --- a/arch/arm/plat-omap/include/plat/mailbox.h
> > +++ b/arch/arm/plat-omap/include/plat/mailbox.h
> > @@ -68,13 +68,15 @@ struct omap_mbox {
> > void *priv;
> >
> > void (*err_notify)(void);
> > + atomic_t 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);
>
> In this context, I'd like to change notifier to support adding a
> cookie which will be passed back to the handler function (unmodified,
> in a similar manner to request_irq's void *dev param).
This can be easily achieved with a simple container_of manipulation,
so please disregard my request : your patch is fine as-is. After it is
accepted, it'd be simple to add this context-get-back to dspbridge.
Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] omap:mailbox-provide multiple reader support
Hi Hari,
On Wed, Jul 21, 2010 at 12:41 AM, Hari Kanigeri wrote:
> This patch provides mutiple readers support for a mailbox
> instance.
>
> Signed-off-by: Hari Kanigeri
> ---
> arch/arm/plat-omap/include/plat/mailbox.h | 6 ++-
> arch/arm/plat-omap/mailbox.c | 63
> 2 files changed, 40 insertions(+), 29 deletions(-)
>
> diff --git a/arch/arm/plat-omap/include/plat/mailbox.h
> b/arch/arm/plat-omap/include/plat/mailbox.h
> index 0486d64..c8e47d8 100644
> --- a/arch/arm/plat-omap/include/plat/mailbox.h
> +++ b/arch/arm/plat-omap/include/plat/mailbox.h
> @@ -68,13 +68,15 @@ struct omap_mbox {
> void *priv;
>
> void (*err_notify)(void);
> + atomic_t 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);
In this context, I'd like to change notifier to support adding a
cookie which will be passed back to the handler function (unmodified,
in a similar manner to request_irq's void *dev param).
This cookie param will be passed to the mailbox callback whenever a
mailbox event is triggered, and will allow drivers to have multiple
instances: instead of maintaining their state in a global variable
(like bridge is doing today), it will be given back every time a
mailbox event is delivered. For syslink it will also help using the
same mbox callback for different devices (e.g. sysm3, appm3).
The changes should be minor, and shouldn't affect existing notifier
users, here's the diff (untested):
diff --git a/include/linux/notifier.h b/include/linux/notifier.h
index 540703b..e740aea 100644
--- a/include/linux/notifier.h
+++ b/include/linux/notifier.h
@@ -51,6 +51,7 @@ struct notifier_block {
int (*notifier_call)(struct notifier_block *, unsigned long, void *);
struct notifier_block *next;
int priority;
+ void *dev_id;
};
struct atomic_notifier_head {
diff --git a/kernel/notifier.c b/kernel/notifier.c
index 2488ba7..050fd9b 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -64,6 +64,7 @@ static int notifier_chain_unregister(struct notifier_block **n
* @nl:Pointer to head of the blocking notifier chain
* @val: Value passed unmodified to notifier function
* @v: Pointer passed unmodified to notifier function
+ * If pointer is NULL, the notifier block's
cookie is passed
* @nr_to_call:Number of notifier functions to be called. Don't care
* value of this parameter is -1.
* @nr_calls: Records the number of notifications sent. Don't care
@@ -90,6 +91,7 @@ static int __kprobes notifier_call_chain(struct notifier_block
continue;
}
#endif
+ v = v ? v : nb->dev_id;
ret = nb->notifier_call(nb, val, v);
if (nr_calls)
And the only change in your mbox patch should be:
@@ -149,8 +149,8 @@ static void mbox_rx_work(struct work_struct *work)
if (unlikely(len != sizeof(msg)))
pr_err("%s: kfifo_out anomaly detected\n", __func__);
- blocking_notifier_call_chain(&mq->mbox->notifier, len,
- (void *)msg);
+ blocking_notifier_call_chain(&mq->mbox->notifier, msg, NULL);
}
}
The result should be us getting both the mailbox msg and the original
cookie, which will, in our case, be the context of the relevant
dspbridge or syslink driver.
What do you think ?
Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] omap:mailbox-provide multiple reader support
Fernando,
Thanks for looking at the patch.
On Tue, Jul 20, 2010 at 4:59 PM, Guzman Lugo, Fernando
wrote:
>
>
> Hi Hari,
>
>>
>> @@ -252,28 +252,30 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
>> }
>> }
>>
>> - ret = request_irq(mbox->irq, mbox_interrupt, IRQF_SHARED,
>> - mbox->name, mbox);
>> - if (unlikely(ret)) {
>> - printk(KERN_ERR
>> - "failed to register mailbox interrupt:%d\n", ret);
>> - goto fail_request_irq;
>> - }
>> + if (atomic_inc_return(&mbox->use_count) == 1) {
>
> What happen if a thread is preempted by other thread which also uses the same
> mailbox after doing the atomic_inc?
>
> The second thread also will call atomic_inc_return and in this case the value
> returned will be 2 and it will not initialize the mbox queues but it will
> return success status, in this point the second thread will think it could
> get the mailbox handle without any issue. However the first thread still can
> fail and not initialize correctly the mailbox leading second thread to not
> work properly or crash.
>
> I think all the initialization should be protected and not just the use_count.
>
-- How about changing mboxes_lock from spinlock to mutex and
protecting the initialization code ? I guess then the variables don't
have to be atomic too. please share your thougts.
> Please let me know what you think.
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] omap:mailbox-provide multiple reader support
Phil,
Thanks for looking at the patch.
On Wed, Jul 21, 2010 at 4:45 AM, wrote:
> Apologies - top posting and not quoting properly due to having to use MS's
> braindead OWA.
>
> It appears that most of the "changes" are simply indentation changes caused
> by the inclusion of a new inner block here:
-- I don't know if we can say most of the changes are simply
indentation changes.
> + if (atomic_inc_return(&mbox->use_count) == 1) {
> rather than just using a goto to skip past the unneeded parts. (Hmmm, is it
> not simply an immediate return now?)
> The goto/return is both more idiomatic in linux, and I'm sure a simpler patch.
-- Sure, I will make the change.
>
> Phil
>
> From: linux-omap-ow...@vger.kernel.org [linux-omap-ow...@vger.kernel.org] On
> Behalf Of ext Hari Kanigeri [h-kanige...@ti.com]
> Sent: 21 July 2010 01:41
> To: Linux Omap; Tony Lindgren; Doyu Hiroshi (Nokia-MS/Helsinki)
> Cc: Ohad Ben-Cohen; Hari Kanigeri
> Subject: [PATCH 2/2] omap:mailbox-provide multiple reader support
>
> This patch provides mutiple readers support for a mailbox
> instance.
>
> Signed-off-by: Hari Kanigeri
> ---
> arch/arm/plat-omap/include/plat/mailbox.h | 6 ++-
> arch/arm/plat-omap/mailbox.c | 63
> 2 files changed, 40 insertions(+), 29 deletions(-)
>
> diff --git a/arch/arm/plat-omap/include/plat/mailbox.h
> b/arch/arm/plat-omap/include/plat/mailbox.h
> index 0486d64..c8e47d8 100644
> --- a/arch/arm/plat-omap/include/plat/mailbox.h
> +++ b/arch/arm/plat-omap/include/plat/mailbox.h
> @@ -68,13 +68,15 @@ struct omap_mbox {
> void *priv;
>
> void (*err_notify)(void);
> + atomic_t 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 *, struct notifier_block *nb);
>
> int omap_mbox_register(struct device *parent, struct omap_mbox *);
> int omap_mbox_unregister(struct omap_mbox *);
> diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
> index baac315..f9f2af4 100644
> --- a/arch/arm/plat-omap/mailbox.c
> +++ b/arch/arm/plat-omap/mailbox.c
> @@ -149,8 +149,8 @@ static void mbox_rx_work(struct work_struct *work)
> if (unlikely(len != sizeof(msg)))
> pr_err("%s: kfifo_out anomaly detected\n", __func__);
>
> - if (mq->callback)
> - mq->callback((void *)msg);
> + blocking_notifier_call_chain(&mq->mbox->notifier, len,
> + (void *)msg);
> }
> }
>
> @@ -252,28 +252,30 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
> }
> }
>
> - ret = request_irq(mbox->irq, mbox_interrupt, IRQF_SHARED,
> - mbox->name, mbox);
> - if (unlikely(ret)) {
> - printk(KERN_ERR
> - "failed to register mailbox interrupt:%d\n", ret);
> - goto fail_request_irq;
> - }
> + if (atomic_inc_return(&mbox->use_count) == 1) {
> + ret = request_irq(mbox->irq, mbox_interrupt, IRQF_SHARED,
> + mbox->name, mbox);
> + if (unlikely(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;
> + 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;
> -
> return 0;
>
> fail_alloc_rxq:
> @@ -281,6 +283,7 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
> fail_alloc_txq:
> free_irq(mbox->irq, mbox);
> fail_request_irq:
> + atomic_dec(&mbox->use_count);
> if
RE: [PATCH 2/2] omap:mailbox-provide multiple reader support
Apologies - top posting and not quoting properly due to having to use MS's
braindead OWA.
It appears that most of the "changes" are simply indentation changes caused by
the inclusion of a new inner block here:
+ if (atomic_inc_return(&mbox->use_count) == 1) {
rather than just using a goto to skip past the unneeded parts. (Hmmm, is it not
simply an immediate return now?)
The goto/return is both more idiomatic in linux, and I'm sure a simpler patch.
Phil
From: linux-omap-ow...@vger.kernel.org [linux-omap-ow...@vger.kernel.org] On
Behalf Of ext Hari Kanigeri [h-kanige...@ti.com]
Sent: 21 July 2010 01:41
To: Linux Omap; Tony Lindgren; Doyu Hiroshi (Nokia-MS/Helsinki)
Cc: Ohad Ben-Cohen; Hari Kanigeri
Subject: [PATCH 2/2] omap:mailbox-provide multiple reader support
This patch provides mutiple readers support for a mailbox
instance.
Signed-off-by: Hari Kanigeri
---
arch/arm/plat-omap/include/plat/mailbox.h |6 ++-
arch/arm/plat-omap/mailbox.c | 63
2 files changed, 40 insertions(+), 29 deletions(-)
diff --git a/arch/arm/plat-omap/include/plat/mailbox.h
b/arch/arm/plat-omap/include/plat/mailbox.h
index 0486d64..c8e47d8 100644
--- a/arch/arm/plat-omap/include/plat/mailbox.h
+++ b/arch/arm/plat-omap/include/plat/mailbox.h
@@ -68,13 +68,15 @@ struct omap_mbox {
void*priv;
void(*err_notify)(void);
+ atomic_tuse_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 *, struct notifier_block *nb);
int omap_mbox_register(struct device *parent, struct omap_mbox *);
int omap_mbox_unregister(struct omap_mbox *);
diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index baac315..f9f2af4 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -149,8 +149,8 @@ static void mbox_rx_work(struct work_struct *work)
if (unlikely(len != sizeof(msg)))
pr_err("%s: kfifo_out anomaly detected\n", __func__);
- if (mq->callback)
- mq->callback((void *)msg);
+ blocking_notifier_call_chain(&mq->mbox->notifier, len,
+ (void *)msg);
}
}
@@ -252,28 +252,30 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
}
}
- ret = request_irq(mbox->irq, mbox_interrupt, IRQF_SHARED,
- mbox->name, mbox);
- if (unlikely(ret)) {
- printk(KERN_ERR
- "failed to register mailbox interrupt:%d\n", ret);
- goto fail_request_irq;
- }
+ if (atomic_inc_return(&mbox->use_count) == 1) {
+ ret = request_irq(mbox->irq, mbox_interrupt, IRQF_SHARED,
+ mbox->name, mbox);
+ if (unlikely(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;
+ 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;
-
return 0;
fail_alloc_rxq:
@@ -281,6 +283,7 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
fail_alloc_txq:
free_irq(mbox->irq, mbox);
fail_request_irq:
+ atomic_dec(&mbox->use_count);
if (likely(mbox->ops->shutdown)) {
if (atomic_dec_return(&mbox_refcount) == 0)
mbox->ops->shutdown(mbox);
@@ -291,10 +294,12 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
static void omap_mbox_fini(struct omap_mbox *mbox)
{
- mbox_queue_free(mbox->txq);
- mbox_queue_free(mbox->rxq);
- free_irq(mbox->irq, mbox);
+ if (atomic_dec_return(&mbox->us
RE: [PATCH 2/2] omap:mailbox-provide multiple reader support
Hi Hari,
> -Original Message-
> From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
> ow...@vger.kernel.org] On Behalf Of Kanigeri, Hari
> Sent: Tuesday, July 20, 2010 4:42 PM
> To: Linux Omap; Tony Lindgren; Hiroshi DOYU
> Cc: Ohad Ben-Cohen; Kanigeri, Hari
> Subject: [PATCH 2/2] omap:mailbox-provide multiple reader support
>
> This patch provides mutiple readers support for a mailbox
> instance.
>
> Signed-off-by: Hari Kanigeri
> ---
> arch/arm/plat-omap/include/plat/mailbox.h |6 ++-
> arch/arm/plat-omap/mailbox.c | 63
>
> 2 files changed, 40 insertions(+), 29 deletions(-)
>
> diff --git a/arch/arm/plat-omap/include/plat/mailbox.h b/arch/arm/plat-
> omap/include/plat/mailbox.h
> index 0486d64..c8e47d8 100644
> --- a/arch/arm/plat-omap/include/plat/mailbox.h
> +++ b/arch/arm/plat-omap/include/plat/mailbox.h
> @@ -68,13 +68,15 @@ struct omap_mbox {
> void*priv;
>
> void(*err_notify)(void);
> + atomic_tuse_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 *, struct notifier_block *nb);
>
> int omap_mbox_register(struct device *parent, struct omap_mbox *);
> int omap_mbox_unregister(struct omap_mbox *);
> diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
> index baac315..f9f2af4 100644
> --- a/arch/arm/plat-omap/mailbox.c
> +++ b/arch/arm/plat-omap/mailbox.c
> @@ -149,8 +149,8 @@ static void mbox_rx_work(struct work_struct *work)
> if (unlikely(len != sizeof(msg)))
> pr_err("%s: kfifo_out anomaly detected\n", __func__);
>
> - if (mq->callback)
> - mq->callback((void *)msg);
> + blocking_notifier_call_chain(&mq->mbox->notifier, len,
> + (void *)msg);
> }
> }
>
> @@ -252,28 +252,30 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
> }
> }
>
> - ret = request_irq(mbox->irq, mbox_interrupt, IRQF_SHARED,
> - mbox->name, mbox);
> - if (unlikely(ret)) {
> - printk(KERN_ERR
> - "failed to register mailbox interrupt:%d\n", ret);
> - goto fail_request_irq;
> - }
> + if (atomic_inc_return(&mbox->use_count) == 1) {
What happen if a thread is preempted by other thread which also uses the same
mailbox after doing the atomic_inc?
The second thread also will call atomic_inc_return and in this case the value
returned will be 2 and it will not initialize the mbox queues but it will
return success status, in this point the second thread will think it could get
the mailbox handle without any issue. However the first thread still can fail
and not initialize correctly the mailbox leading second thread to not work
properly or crash.
I think all the initialization should be protected and not just the use_count.
Please let me know what you think.
Regards,
Fernando.
> + ret = request_irq(mbox->irq, mbox_interrupt, IRQF_SHARED,
> + mbox->name, mbox);
> + if (unlikely(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;
> + 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;
> -
> return 0;
>
> fail_alloc_rxq:
> @@ -281,6 +283,7 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
> fail_alloc_txq:
> free_irq(mbox->irq, mbox);
> fail_request_irq:
> + atomic_dec(&mbox->use_count);
> if (likely(mbox->ops->shutdown)) {
> if (atomic_dec_return(&mbox_refcount) == 0)
> mbox->ops->shutdown(mbo
