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

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

[Hari Kanigeri]

Sending the patch set by seperating the clock related patch seperated
out based on Paul Walmsey's comment.
http://www.spinics.net/lists/arm-kernel/msg103692.html
The clock patch will be sent out as a seperate patch.

The previous versions of this patch set can be found here
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             |    5 +-
 arch/arm/plat-omap/include/plat/mailbox.h |    8 +-
 arch/arm/plat-omap/mailbox.c              |  127 +++++++++++++++++------------
 3 files changed, 82 insertions(+), 58 deletions(-)

[PATCH v3 1/5] OMAP: 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 v3 1/5] OMAP: mailbox: change full flag per mailbox queue instead of global

[Cousson, Benoit]

On 11/18/2010 8:15 PM, Kanigeri, Hari wrote:
> 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:

You should not put the patch revisions here, because it will stay in the 
changelog when it will be in GIT.
You'd better put that in the cover-letter or after the ---.

Everything after --- will not be taken into account in the changelog.

Regards,
Benoit

> - 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;
>   		}
>

[PATCH v3 2/5] 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 v3 2/5] OMAP: mailbox: fix rx interrupt disable in omap4

[Cousson, Benoit]

On 11/18/2010 8:15 PM, Hari Kanigeri wrote:
> 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())

Since it is not omap version specific but IP version specific, you 
should not use cpu_is_ to do that. Moreover cpu_is calls should be used 
during init only.
You can use the rev field in hwmod_class in order to detect the IP version.
Smartreflex series for 3630 is already using that kind of mechanism.
You will have to copy that revision information into pdata struct and 
then use that here.

Benoit

> +		l |= bit;
> +	else
> +		l&= ~bit;
>   	mbox_write_reg(l, p->irqdisable);
>   }
>

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

[Kanigeri, Hari]

Benoit,

On Thu, Nov 18, 2010 at 5:28 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
> On 11/18/2010 8:15 PM, Hari Kanigeri wrote:
>>
>> 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())
>
> Since it is not omap version specific but IP version specific, you should
> not use cpu_is_ to do that. Moreover cpu_is calls should be used during init
> only.
> You can use the rev field in hwmod_class in order to detect the IP version.
> Smartreflex series for 3630 is already using that kind of mechanism.
> You will have to copy that revision information into pdata struct and then
> use that here.

I see your point, but since mailbox hwmod patches from Omar are still
under review I didn't find any other option than to enable this
This is critical functionality that I want to include in and not wait
till the hwmod patches are accepted.
Please let me know if there is any other way of approaching this problem ?

Thank you,
Best regards,
Hari

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

[Felipe Balbi]

On Thu, Nov 18, 2010 at 06:07:40PM -0600, Kanigeri, Hari wrote:
>Benoit,
>
>On Thu, Nov 18, 2010 at 5:28 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
>> On 11/18/2010 8:15 PM, Hari Kanigeri wrote:
>>>
>>> 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())
>>
>> Since it is not omap version specific but IP version specific, you should
>> not use cpu_is_ to do that. Moreover cpu_is calls should be used during init
>> only.
>> You can use the rev field in hwmod_class in order to detect the IP version.
>> Smartreflex series for 3630 is already using that kind of mechanism.
>> You will have to copy that revision information into pdata struct and then
>> use that here.
>
>I see your point, but since mailbox hwmod patches from Omar are still
>under review I didn't find any other option than to enable this
>This is critical functionality that I want to include in and not wait
>till the hwmod patches are accepted.
>Please let me know if there is any other way of approaching this problem ?

how about you read the IP revision yourself during probe ? Or pass in a
flag like I said on the other email ?

-- 
balbi

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

[Kanigeri, Hari]

Felipe,

On Fri, Nov 19, 2010 at 2:32 AM, Felipe Balbi <balbi@ti.com> wrote:
> On Thu, Nov 18, 2010 at 06:07:40PM -0600, Kanigeri, Hari wrote:
>>
>> Benoit,
>>
>> On Thu, Nov 18, 2010 at 5:28 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
>>>
>>> On 11/18/2010 8:15 PM, Hari Kanigeri wrote:
>>>>
>>>> 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())
>>>
>>> Since it is not omap version specific but IP version specific, you should
>>> not use cpu_is_ to do that. Moreover cpu_is calls should be used during
>>> init
>>> only.
>>> You can use the rev field in hwmod_class in order to detect the IP
>>> version.
>>> Smartreflex series for 3630 is already using that kind of mechanism.
>>> You will have to copy that revision information into pdata struct and
>>> then
>>> use that here.
>>
>> I see your point, but since mailbox hwmod patches from Omar are still
>> under review I didn't find any other option than to enable this
>> This is critical functionality that I want to include in and not wait
>> till the hwmod patches are accepted.
>> Please let me know if there is any other way of approaching this problem ?
>
> how about you read the IP revision yourself during probe ? Or pass in a
> flag like I said on the other email ?
>

I like your proposal of reading the IP revision in probe. I will send
a revised patch for this.

Benoit, is this ok until we move to hwmod implemenation ?


Thank you,
Best regards,
Hari Kanigeri

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

[Cousson, Benoit]

On 11/19/2010 3:22 PM, Kanigeri, Hari wrote:
> Felipe,
>
> On Fri, Nov 19, 2010 at 2:32 AM, Felipe Balbi<balbi@ti.com>  wrote:
>> On Thu, Nov 18, 2010 at 06:07:40PM -0600, Kanigeri, Hari wrote:
>>>
>>> Benoit,
>>>
>>> On Thu, Nov 18, 2010 at 5:28 PM, Cousson, Benoit<b-cousson@ti.com>  wrote:
>>>>
>>>> On 11/18/2010 8:15 PM, Hari Kanigeri wrote:
>>>>>
>>>>> 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())
>>>>
>>>> Since it is not omap version specific but IP version specific, you should
>>>> not use cpu_is_ to do that. Moreover cpu_is calls should be used during
>>>> init
>>>> only.
>>>> You can use the rev field in hwmod_class in order to detect the IP
>>>> version.
>>>> Smartreflex series for 3630 is already using that kind of mechanism.
>>>> You will have to copy that revision information into pdata struct and
>>>> then
>>>> use that here.
>>>
>>> I see your point, but since mailbox hwmod patches from Omar are still
>>> under review I didn't find any other option than to enable this
>>> This is critical functionality that I want to include in and not wait
>>> till the hwmod patches are accepted.

OK, I didn't see that your series was supposed to be done before Omar's one.

>>> Please let me know if there is any other way of approaching this problem ?
>>
>> how about you read the IP revision yourself during probe ? Or pass in a
>> flag like I said on the other email ?
>>
>
> I like your proposal of reading the IP revision in probe. I will send
> a revised patch for this.

Most of the time, we do not want to use the IP revision because it is 
un-accurate and does not reflect the change we'd like to track.
For example some time a minor change in the RTL that will not impact the 
SW at all might trigger a change in the IP revision, whereas on the 
other hand a major bug fix that will impact the SW is not capture in the 
IP revision... yeah, that's bad, but this can happen.

That's why we are relying on a rev field in the hwmod.

Meanwhile, you can do the cpu_is check at init time, then fill a pdata 
attribute with a revision parameter, and use that revision parameter in 
the omap2_mbox_disable_irq.
When hwmod will be there you will just extract that information from the 
hwmod rev field.

Regards,
Benoit

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

[Felipe Balbi]

On Fri, Nov 19, 2010 at 03:50:02PM +0100, Cousson, Benoit wrote:
>Most of the time, we do not want to use the IP revision because it is 
>un-accurate and does not reflect the change we'd like to track.
>For example some time a minor change in the RTL that will not impact 
>the SW at all might trigger a change in the IP revision, whereas on 
>the other hand a major bug fix that will impact the SW is not capture 
>in the IP revision... yeah, that's bad, but this can happen.
>
>That's why we are relying on a rev field in the hwmod.

But then, what's inside this rev field ? Is it some internal revision of
hwmod or do you read from the hw ?

-- 
balbi

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

[Kanigeri, Hari]

On Mon, Nov 22, 2010 at 4:08 AM, Felipe Balbi <balbi@ti.com> wrote:
> On Fri, Nov 19, 2010 at 03:50:02PM +0100, Cousson, Benoit wrote:
>>
>> Most of the time, we do not want to use the IP revision because it is
>> un-accurate and does not reflect the change we'd like to track.
>> For example some time a minor change in the RTL that will not impact the
>> SW at all might trigger a change in the IP revision, whereas on the other
>> hand a major bug fix that will impact the SW is not capture in the IP
>> revision... yeah, that's bad, but this can happen.
>>
>> That's why we are relying on a rev field in the hwmod.
>
> But then, what's inside this rev field ? Is it some internal revision of
> hwmod or do you read from the hw ?
>

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

[Felipe Balbi]

Hi,

On Mon, Nov 22, 2010 at 05:46:53AM -0600, Kanigeri, Hari wrote:
>>From the dmtimer stuff, it looks like the driver defines the version
>types, which hwmod uses to define the rev field.
>
>/* timer ip constants */
>#define OMAP_TIMER_IP_LEGACY			0x1
>#define OMAP_TIMER_IP_VERSION_2			0x2

in that case, it's the same as if you pass in a flag via platform_data.
You might as well call it rev, then the diff when converting to hwmod
will be smaller maybe :-p

-- 
balbi

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

[Kanigeri, Hari]

Felipe,

On Mon, Nov 22, 2010 at 5:51 AM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Mon, Nov 22, 2010 at 05:46:53AM -0600, Kanigeri, Hari wrote:
>>>
>>> From the dmtimer stuff, it looks like the driver defines the version
>>
>> types, which hwmod uses to define the rev field.
>>
>> /* timer ip constants */
>> #define OMAP_TIMER_IP_LEGACY ? ? ? ? ? ? ? ? ? ?0x1
>> #define OMAP_TIMER_IP_VERSION_2 ? ? ? ? ? ? ? ? 0x2
>
> in that case, it's the same as if you pass in a flag via platform_data.
> You might as well call it rev, then the diff when converting to hwmod
> will be smaller maybe :-p

I agree with you, the changes should be minimal converting to hwmod.


Thank you,
Best regards,
Hari Kanigeri

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

[Cousson, Benoit]

On 11/22/2010 12:51 PM, Felipe Balbi wrote:
> Hi,
>
> On Mon, Nov 22, 2010 at 05:46:53AM -0600, Kanigeri, Hari wrote:
>> > From the dmtimer stuff, it looks like the driver defines the version
>> types, which hwmod uses to define the rev field.
>>
>> /* timer ip constants */
>> #define OMAP_TIMER_IP_LEGACY			0x1
>> #define OMAP_TIMER_IP_VERSION_2			0x2
>
> in that case, it's the same as if you pass in a flag via platform_data.
> You might as well call it rev, then the diff when converting to hwmod
> will be smaller maybe :-p

Yes, this is exactly what I was proposing to Hari.
For the moment create the pdata field that will be populated at init 
time using the cpu_is.
During the hwmod migration, the value will be extracted from the hwmod 
rev field.

Benoit

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

[Cousson, Benoit]

On 11/22/2010 11:08 AM, Balbi, Felipe wrote:
> On Fri, Nov 19, 2010 at 03:50:02PM +0100, Cousson, Benoit wrote:
>> Most of the time, we do not want to use the IP revision because it is
>> un-accurate and does not reflect the change we'd like to track.
>> For example some time a minor change in the RTL that will not impact
>> the SW at all might trigger a change in the IP revision, whereas on
>> the other hand a major bug fix that will impact the SW is not capture
>> in the IP revision... yeah, that's bad, but this can happen.
>>
>> That's why we are relying on a rev field in the hwmod.
>
> But then, what's inside this rev field ? Is it some internal revision of
> hwmod or do you read from the hw ?

So far we are using a artificial SW IP revision that reflect the 
difference we'd like to highlight v1, v2.

Benoit

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

[Felipe Balbi]

On Mon, Nov 22, 2010 at 03:55:47PM +0100, Cousson, Benoit wrote:
>So far we are using a artificial SW IP revision that reflect the 
>difference we'd like to highlight v1, v2.

but what's the big deal in reading that from HW itself ? The only change
will in the comparisson operator. Instead of doing:

if (rev == REV_V1)

you'll be doing:

if (rev >= REV_V1_X_Y_Z)

no ??

-- 
balbi

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

[Felipe Balbi]

On Thu, Nov 18, 2010 at 01:15:39PM -0600, Hari Kanigeri wrote:
>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.

How nasty :-p

>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;

you should not use cpu_is_* checks on drivers. Even though this is
located under mach-omap2, it's still a normal driver and you should not
use those checks. Pass a flag like "has_twisted_rx_irq_disable" via
platform_data and use that instead.

-- 
balbi

[PATCH v3 3/5] 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..abfc495 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 v3 3/5] OMAP: mailbox: fix checkpatch warnings

[Felipe Balbi]

Hi,

On Thu, Nov 18, 2010 at 01:15:40PM -0600, Hari Kanigeri wrote:
>Fix the checkpatch warnings observed in mailbox module

you should put which warnings are those here :-)

>Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>

-- 
balbi

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

[Kanigeri, Hari]

Felipe,

On Fri, Nov 19, 2010 at 2:33 AM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Thu, Nov 18, 2010 at 01:15:40PM -0600, Hari Kanigeri wrote:
>>
>> Fix the checkpatch warnings observed in mailbox module
>
> you should put which warnings are those here :-)
>

Sure, will add some descriptions in next patch revision.


Thank you,
Best regards,
Hari Kanigeri

[PATCH v3 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 abfc495..b14bc34 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 v3 4/5] OMAP: mailbox: send message in process context

[Felipe Balbi]

Hi,

On Thu, Nov 18, 2010 at 01:15:41PM -0600, Hari Kanigeri 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
		     ^kfifo

-- 
balbi

[PATCH v3 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              |  102 ++++++++++++++++-------------
 2 files changed, 60 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..cc3921e 100644
--- a/arch/arm/plat-omap/include/plat/mailbox.h
+++ b/arch/arm/plat-omap/include/plat/mailbox.h
@@ -46,7 +46,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 +57,15 @@ 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 b14bc34..b1f8f2c 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);
@@ -247,41 +248,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:
@@ -291,29 +291,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;
@@ -332,12 +337,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);
@@ -361,6 +370,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 v3 5/5] OMAP: mailbox: add notification support for multiple readers

[Felipe Balbi]

Hi,

On Thu, Nov 18, 2010 at 01:15:42PM -0600, Hari Kanigeri 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>

Personally, I don't like this patch. I think it's much better to have a
per-message callback then a "global" notification which all users will
listen to.

What will happen is that every user will have to check if every message
belongs to them based on the notifications.

Imagine a hypothetical situation where you have 100 users each with 100
messages. You will have 100 * 100 (10.000)
"does-this-message-belongs-to-me" checks.

Rather than doing it this way, I would, as the easiest path, add a
"callback" parameter to omap_mbox_request_send() and then, internally,
allocate a e.g. struct omap_mbox_request and add that to a list of pending
messages. Something like:

struct omap_mbox_request {
	struct omap_mbox	*mbox;
	int			(*complete)(void *context);
	void			*context;
	void			*buf;
	unsigned		len;
	struct list_head	list;
};

[...]

int omap_mbox_request_send(struct omap_mbox *mbox, mbox_msg_t msg, int
		(*complete)(void *context), void *context, gfp_t flags)
{
	struct omap_mbox_queue	*mq = mbox->txq;
	struct omap_mbox_request	*req;
	int			ret = 0;
	int			len;

	req = kzalloc(sizeof(*req), flags);
	if (!req) {
		[...]
	}

	memcpy(req->buf, &msg, sizeof(msg));
	req->len = sizeof(msg));
	req->mbox = msg;
	req->complete = complete;
	req->context = context;
	INIT_LIST_HEAD(&req->list);

	list_add_tail(&req->list, &mq->req_list);

	/* kick the tasklet here */

	return 0;
}

then on your tasklet, you simply iterate over the list and start sending
one by one and calling callback when it completes. You would be giving
your users a more asynchronous API and you wouldn't need this notifier
which, IMO, isn't a good solution at all.

But hey, since you'd be doing so many changes, you might as well provide
a:

omap_mbox_alloc_req();

to allocate a struct omap_mbox_request and a

omap_mbox_queue();

to add a request to the list (simply rename omap_mbox_send to
omap_mbox_queue() and make the necessary changes, like changing the
prototype to omap_mbox_queue(struct omap_mbux *mbox, struct
omap_mbox_request *req);)

In any case, even though I don't like the solution, it's Hiroshi's
decision to take or not this patch :-p

-- 
balbi

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

[Kanigeri, Hari]

Felipe,

On Fri, Nov 19, 2010 at 2:50 AM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Thu, Nov 18, 2010 at 01:15:42PM -0600, Hari Kanigeri 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>
>
> Personally, I don't like this patch. I think it's much better to have a
> per-message callback then a "global" notification which all users will
> listen to.
>

Thanks for your comments :).
I think handling of per-message callback should be handled at one
level above mailbox, like in IPC modules such as dspbridge,
syslink..etc.
Mailbox module should be free of any protocols and should take care of
just writing to the mailbox fifo and delivering the message to the
interested listener(s).


> What will happen is that every user will have to check if every message
> belongs to them based on the notifications.
>
> Imagine a hypothetical situation where you have 100 users each with 100
> messages. You will have 100 * 100 (10.000)
> "does-this-message-belongs-to-me" checks.
>

Ideally one shouldn't design their IPC this way. They should have a
IPC driver and the knowledge of notification to multiple users should
be routed from there.

What my patch is addressing is ensure that the users of mailbox don't
mess with the mailbox's internal pointer, and also provide the
flexibility of multiple listeners. Designers of IPC should make their
own judgment whether to use the flexibility option based on their use
cases.

> Rather than doing it this way, I would, as the easiest path, add a
> "callback" parameter to omap_mbox_request_send() and then, internally,
> allocate a e.g. struct omap_mbox_request and add that to a list of pending
> messages. Something like:
>
> struct omap_mbox_request {
> ? ? ? ?struct omap_mbox ? ? ? ?*mbox;
> ? ? ? ?int ? ? ? ? ? ? ? ? ? ? (*complete)(void *context);
> ? ? ? ?void ? ? ? ? ? ? ? ? ? ?*context;
> ? ? ? ?void ? ? ? ? ? ? ? ? ? ?*buf;
> ? ? ? ?unsigned ? ? ? ? ? ? ? ?len;
> ? ? ? ?struct list_head ? ? ? ?list;
> };
>
> [...]
>
> int omap_mbox_request_send(struct omap_mbox *mbox, mbox_msg_t msg, int
> ? ? ? ? ? ? ? ?(*complete)(void *context), void *context, gfp_t flags)
> {
> ? ? ? ?struct omap_mbox_queue ?*mq = mbox->txq;
> ? ? ? ?struct omap_mbox_request ? ? ? ?*req;
> ? ? ? ?int ? ? ? ? ? ? ? ? ? ? ret = 0;
> ? ? ? ?int ? ? ? ? ? ? ? ? ? ? len;
>
> ? ? ? ?req = kzalloc(sizeof(*req), flags);
> ? ? ? ?if (!req) {
> ? ? ? ? ? ? ? ?[...]
> ? ? ? ?}
>
> ? ? ? ?memcpy(req->buf, &msg, sizeof(msg));
> ? ? ? ?req->len = sizeof(msg));
> ? ? ? ?req->mbox = msg;
> ? ? ? ?req->complete = complete;
> ? ? ? ?req->context = context;
> ? ? ? ?INIT_LIST_HEAD(&req->list);
>
> ? ? ? ?list_add_tail(&req->list, &mq->req_list);
>
> ? ? ? ?/* kick the tasklet here */
>
> ? ? ? ?return 0;
> }
>
> then on your tasklet, you simply iterate over the list and start sending
> one by one and calling callback when it completes. You would be giving
> your users a more asynchronous API and you wouldn't need this notifier
> which, IMO, isn't a good solution at all.
>

Your ideas are good. But this type of intelligence is already inbuilt
in IPC drivers such as dspbridge and syslink. And I think mailbox
driver should be free of protocols.

Thank you,
Best regards,
Hari

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

[Felipe Balbi]

On Fri, Nov 19, 2010 at 05:50:19AM -0600, Kanigeri, Hari wrote:
>Thanks for your comments :).

np.

>I think handling of per-message callback should be handled at one
>level above mailbox, like in IPC modules such as dspbridge,
>syslink..etc.

then you'll have duplication of functionality :-)

>Mailbox module should be free of any protocols and should take care of

it's not a protocol that I'm proposing. It's just one way to give
mailbox users a more async API.

>just writing to the mailbox fifo and delivering the message to the
>interested listener(s).

exactly. Only to the interested listener. With your patch, you'll be
delivering the message to all listeners and listeners have to check if
that particular message belongs to them.

>> What will happen is that every user will have to check if every message
>> belongs to them based on the notifications.
>>
>> Imagine a hypothetical situation where you have 100 users each with 100
>> messages. You will have 100 * 100 (10.000)
>> "does-this-message-belongs-to-me" checks.
>>
>
>Ideally one shouldn't design their IPC this way. They should have a
>IPC driver and the knowledge of notification to multiple users should
>be routed from there.

only if the message is interesting for N listeners. But if each
listeners will have its own message towards the other side of the link,
then mailbox should only give the result to one listener.

>What my patch is addressing is ensure that the users of mailbox don't
>mess with the mailbox's internal pointer, and also provide the
>flexibility of multiple listeners. Designers of IPC should make their
>own judgment whether to use the flexibility option based on their use
>cases.

but with your patch, you are delivering the same message to all
listeners. You give the oportunity even for some attacks. I could fiddle
with other listeners' message just by attaching a notifier_block and
checking what's the content of every message.

>> Rather than doing it this way, I would, as the easiest path, add a
>> "callback" parameter to omap_mbox_request_send() and then, internally,
>> allocate a e.g. struct omap_mbox_request and add that to a list of pending
>> messages. Something like:
>>
>> struct omap_mbox_request {
>> ? ? ? ?struct omap_mbox ? ? ? ?*mbox;
>> ? ? ? ?int ? ? ? ? ? ? ? ? ? ? (*complete)(void *context);
>> ? ? ? ?void ? ? ? ? ? ? ? ? ? ?*context;
>> ? ? ? ?void ? ? ? ? ? ? ? ? ? ?*buf;
>> ? ? ? ?unsigned ? ? ? ? ? ? ? ?len;
>> ? ? ? ?struct list_head ? ? ? ?list;
>> };
>>
>> [...]
>>
>> int omap_mbox_request_send(struct omap_mbox *mbox, mbox_msg_t msg, int
>> ? ? ? ? ? ? ? ?(*complete)(void *context), void *context, gfp_t flags)
>> {
>> ? ? ? ?struct omap_mbox_queue ?*mq = mbox->txq;
>> ? ? ? ?struct omap_mbox_request ? ? ? ?*req;
>> ? ? ? ?int ? ? ? ? ? ? ? ? ? ? ret = 0;
>> ? ? ? ?int ? ? ? ? ? ? ? ? ? ? len;
>>
>> ? ? ? ?req = kzalloc(sizeof(*req), flags);
>> ? ? ? ?if (!req) {
>> ? ? ? ? ? ? ? ?[...]
>> ? ? ? ?}
>>
>> ? ? ? ?memcpy(req->buf, &msg, sizeof(msg));
>> ? ? ? ?req->len = sizeof(msg));
>> ? ? ? ?req->mbox = msg;
>> ? ? ? ?req->complete = complete;
>> ? ? ? ?req->context = context;
>> ? ? ? ?INIT_LIST_HEAD(&req->list);
>>
>> ? ? ? ?list_add_tail(&req->list, &mq->req_list);
>>
>> ? ? ? ?/* kick the tasklet here */
>>
>> ? ? ? ?return 0;
>> }
>>
>> then on your tasklet, you simply iterate over the list and start sending
>> one by one and calling callback when it completes. You would be giving
>> your users a more asynchronous API and you wouldn't need this notifier
>> which, IMO, isn't a good solution at all.
>>
>
>Your ideas are good. But this type of intelligence is already inbuilt
>in IPC drivers such as dspbridge and syslink. And I think mailbox
>driver should be free of protocols.

Like I said before, this is not a protocol. And if both dspbridge and
syslink have the same kind of thing, don't you think it's a duplication?
Don't you, also, think common features should be done at framework
levels ? Since we don't have an IPC framework, we consider the mailbox
driver a framework for using the OMAP mailbox, then the API I proposed
is just one way to address the problem you described. There's nothing
like a protocol here.

In any case, you are the one taking care of those drivers and it's your
call how to handle multiple users, but keep in mind you'll be allowing
anyone to see all messages that are going through mailbox just by
attaching a notifier block :-) If that's ok for you, if you don't see
any troubles with that, go for it. If you also don't see any troubles
with having hundreds of users each with hundreds of messages and you
notifying all of them about all messages as a problem, go for it :-p

-- 
balbi

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

[Kanigeri, Hari]

Felipe,

>
>> I think handling of per-message callback should be handled at one
>> level above mailbox, like in IPC modules such as dspbridge,
>> syslink..etc.
>
> then you'll have duplication of functionality :-)
>
yes in mailbox :). One solution doesn't fit all and this should be
handled at IPC driver.

>> Mailbox module should be free of any protocols and should take care of
>
> it's not a protocol that I'm proposing. It's just one way to give
> mailbox users a more async API.
>
>> just writing to the mailbox fifo and delivering the message to the
>> interested listener(s).
>
> exactly. Only to the interested listener. With your patch, you'll be
> delivering the message to all listeners and listeners have to check if
> that particular message belongs to them.
>
>>> What will happen is that every user will have to check if every message
>>> belongs to them based on the notifications.
>>>
>>> Imagine a hypothetical situation where you have 100 users each with 100
>>> messages. You will have 100 * 100 (10.000)
>>> "does-this-message-belongs-to-me" checks.
>>>
>>
>> Ideally one shouldn't design their IPC this way. They should have a
>> IPC driver and the knowledge of notification to multiple users should
>> be routed from there.
>
> only if the message is interesting for N listeners. But if each
> listeners will have its own message towards the other side of the link,
> then mailbox should only give the result to one listener.
>
>> What my patch is addressing is ensure that the users of mailbox don't
>> mess with the mailbox's internal pointer, and also provide the
>> flexibility of multiple listeners. Designers of IPC should make their
>> own judgment whether to use the flexibility option based on their use
>> cases.
> but with your patch, you are delivering the same message to all
> listeners. You give the oportunity even for some attacks. I could fiddle
> with other listeners' message just by attaching a notifier_block and
> checking what's the content of every message.

IMHO, an kernel API have a scope to be misused if the Users misuse it,
and I think the users of the Kernel APIs should know what they are
doing before using an API.

>
>>> Rather than doing it this way, I would, as the easiest path, add a
>>> "callback" parameter to omap_mbox_request_send() and then, internally,
>>> allocate a e.g. struct omap_mbox_request and add that to a list of
>>> pending
>>> messages. Something like:
>>>
>>> struct omap_mbox_request {
>>> ? ? ? ?struct omap_mbox ? ? ? ?*mbox;
>>> ? ? ? ?int ? ? ? ? ? ? ? ? ? ? (*complete)(void *context);
>>> ? ? ? ?void ? ? ? ? ? ? ? ? ? ?*context;
>>> ? ? ? ?void ? ? ? ? ? ? ? ? ? ?*buf;
>>> ? ? ? ?unsigned ? ? ? ? ? ? ? ?len;
>>> ? ? ? ?struct list_head ? ? ? ?list;
>>> };
>>>
>>> [...]
>>>
>>> int omap_mbox_request_send(struct omap_mbox *mbox, mbox_msg_t msg, int
>>> ? ? ? ? ? ? ? ?(*complete)(void *context), void *context, gfp_t flags)
>>> {
>>> ? ? ? ?struct omap_mbox_queue ?*mq = mbox->txq;
>>> ? ? ? ?struct omap_mbox_request ? ? ? ?*req;
>>> ? ? ? ?int ? ? ? ? ? ? ? ? ? ? ret = 0;
>>> ? ? ? ?int ? ? ? ? ? ? ? ? ? ? len;
>>>
>>> ? ? ? ?req = kzalloc(sizeof(*req), flags);
>>> ? ? ? ?if (!req) {
>>> ? ? ? ? ? ? ? ?[...]
>>> ? ? ? ?}
>>>
>>> ? ? ? ?memcpy(req->buf, &msg, sizeof(msg));
>>> ? ? ? ?req->len = sizeof(msg));
>>> ? ? ? ?req->mbox = msg;
>>> ? ? ? ?req->complete = complete;
>>> ? ? ? ?req->context = context;
>>> ? ? ? ?INIT_LIST_HEAD(&req->list);
>>>
>>> ? ? ? ?list_add_tail(&req->list, &mq->req_list);
>>>
>>> ? ? ? ?/* kick the tasklet here */
>>>
>>> ? ? ? ?return 0;
>>> }
>>>
>>> then on your tasklet, you simply iterate over the list and start sending
>>> one by one and calling callback when it completes. You would be giving

How do you know that a response is received for a particular sender ?
By reading mailbox payload or by reading some shared memory ? I think
this itself would constitute building up protocol in mailbox driver.

>>> your users a more asynchronous API and you wouldn't need this notifier
>>> which, IMO, isn't a good solution at all.
>>>
>>
>> Your ideas are good. But this type of intelligence is already inbuilt
>> in IPC drivers such as dspbridge and syslink. And I think mailbox
>> driver should be free of protocols.
>
> Like I said before, this is not a protocol. And if both dspbridge and
> syslink have the same kind of thing, don't you think it's a duplication?
> Don't you, also, think common features should be done at framework
> levels ? Since we don't have an IPC framework, we consider the mailbox
> driver a framework for using the OMAP mailbox, then the API I proposed
> is just one way to address the problem you described. There's nothing
> like a protocol here.

I fully agree with you about common framework, and the proposed
solution for the common IPC framework is Syslink. This was discussed
during the recent LPC meeting to come up with a common IPC framework
that is currently missing in Kernel.
I can share with you the details if you are interested.

>
> In any case, you are the one taking care of those drivers and it's your
> call how to handle multiple users, but keep in mind you'll be allowing
> anyone to see all messages that are going through mailbox just by
> attaching a notifier block :-) If that's ok for you, if you don't see
> any troubles with that, go for it. If you also don't see any troubles
> with having hundreds of users each with hundreds of messages and you
> notifying all of them about all messages as a problem, go for it :-p

As I said if some one does that then it is a misuse on their part :).
The messages should be routed through IPC driver. And let's not forget
the main motivation of this patch as well.


Thank you,
Best regards,
Hari Kanigeri

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

[Felipe Balbi]

Hi Hari,

On Fri, Nov 19, 2010 at 06:29:39AM -0600, Kanigeri, Hari wrote:
>How do you know that a response is received for a particular sender ?

think of it as FIFO. So the first completed message is the first in your
list of requests. Ain't that easy ? :-)

>By reading mailbox payload or by reading some shared memory ? I think
>this itself would constitute building up protocol in mailbox driver.

I believe you're thinking too complicated :-p

>I fully agree with you about common framework, and the proposed
>solution for the common IPC framework is Syslink. This was discussed
>during the recent LPC meeting to come up with a common IPC framework
>that is currently missing in Kernel.
>I can share with you the details if you are interested.

Don't bother, I know all I wanted to know so far :-p

>As I said if some one does that then it is a misuse on their part :).
>The messages should be routed through IPC driver. And let's not forget
>the main motivation of this patch as well.

Still it doesn't the fact that currently you allow for that. There are
malware writers everywhere. But hey, it would be cool to have mailbox
appear on securityfocus.com :-p

-- 
balbi

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

[Kanigeri, Hari]

Felipe,

On Fri, Nov 19, 2010 at 6:53 AM, Felipe Balbi <balbi@ti.com> wrote:
> Hi Hari,
>
> On Fri, Nov 19, 2010 at 06:29:39AM -0600, Kanigeri, Hari wrote:
>>
>> How do you know that a response is received for a particular sender ?
>
> think of it as FIFO. So the first completed message is the first in your
> list of requests. Ain't that easy ? :-)

Actually it isn't that easy ;) because there is no guarantee that the
completed message is the first one in the list of requests.
Responses could be asynchronous and we cannot depend on serialized
handling of requests on BIOS...even in some cases you might not even
get a response for particular request in which case you have to handle
that in IPC module.

eg: Process A sends a message to M3 core to kick off some translation
in some codec, the thread handling this request running on BIOS would
kick off the translation and wait for the completion before sending
the response. In the mean time BIOS will be handling other requests
coming to it, and shouldn't be blocked until the completion of first
request.


>
> Still it doesn't the fact that currently you allow for that. There are
> malware writers everywhere. But hey, it would be cool to have mailbox
> appear on securityfocus.com :-p
>

Agree about malware writers. Add PM interfaces as well to the list as
one can misuse them as well ;)


Thank you,
Best regards,
Hari Kanigeri

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

[Felipe Balbi]

Hi, (at home already),

On Fri, 2010-11-19 at 07:57 -0600, Kanigeri, Hari wrote:
> > think of it as FIFO. So the first completed message is the first in your
> > list of requests. Ain't that easy ? :-)
> 
> Actually it isn't that easy ;) because there is no guarantee that the
> completed message is the first one in the list of requests.
> Responses could be asynchronous and we cannot depend on serialized
> handling of requests on BIOS...even in some cases you might not even
> get a response for particular request in which case you have to handle
> that in IPC module.

you did not get what I said then.

users will queue a request, which is simply a list_add_tail() to the
mailbox's request list.

Then you kick the transfers which will:

request = list_first_entry(mbox->req_list);
setup_correct_registers();
enable_irq();
kick_transfer();

return;

then on IRQ handler, when the message is completed you do:

request = list_first_entry(mbox->req_list);
list_del(request->list);

/* unlock */
request->complete();
/* lock again */

you will always one message at a time, so you know that when you get the
IRQ, the first message on your list is what was just completed.

> eg: Process A sends a message to M3 core to kick off some translation
> in some codec, the thread handling this request running on BIOS would
> kick off the translation and wait for the completion before sending
> the response. In the mean time BIOS will be handling other requests
> coming to it, and shouldn't be blocked until the completion of first
> request.

why you're involving BIOS here ? mailbox should not have to know what's
on the other side. All it needs to know is how to get the message to the
other side, no matter _what_ it is.

So until BIOS sends the response, you would not start another transfer
on mailbox neither you would any IRQ, right ?

> > Still it doesn't the fact that currently you allow for that. There are
> > malware writers everywhere. But hey, it would be cool to have mailbox
> > appear on securityfocus.com :-p
> >
> 
> Agree about malware writers. Add PM interfaces as well to the list as
> one can misuse them as well ;)

for PM interfaces it's a bit more difficult and you cannot tamper with
another device's PM (well, after all are converted to hwmod, that is)
and on top of that, the biggest trouble you will run into, is your
battery dying quickly. With mailbox you have actual useful data coming
back and forth, no ?

-- 
balbi

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

[Kanigeri, Hari]

Felipe,


On Fri, Nov 19, 2010 at 8:25 AM, Felipe Balbi <me@felipebalbi.com> wrote:
> Hi, (at home already),
>
> On Fri, 2010-11-19 at 07:57 -0600, Kanigeri, Hari wrote:
>> > think of it as FIFO. So the first completed message is the first in your
>> > list of requests. Ain't that easy ? :-)
>>
>> Actually it isn't that easy ;) because there is no guarantee that the
>> completed message is the first one in the list of requests.
>> Responses could be asynchronous and we cannot depend on serialized
>> handling of requests on BIOS...even in some cases you might not even
>> get a response for particular request in which case you have to handle
>> that in IPC module.
>
> you did not get what I said then.

Not really :). Please let me know if if I am wrong, what you
addressing is getting the confirmation that a message is sent and what
I am addressing with the patch is that a response is received from
M3/DSP.

>
> Then you kick the transfers which will:
>
> request = list_first_entry(mbox->req_list);
> setup_correct_registers();
> enable_irq();
> kick_transfer();

Writing to the mailbox fifo delivers the message to other side, and if
the fifo is full the messages are queued up in mbox kfifo, which are
then deliverd in the order they are received.
I don't see a use case where the senders need to know that their
message is actually written to mbox fifo, if there is one we can look
into it.
You enable TX irq only when the mbox fifo is full.

>
> return;
>
> then on IRQ handler, when the message is completed you do:
>
> request = list_first_entry(mbox->req_list);
> list_del(request->list);
>
> /* unlock */
> request->complete();
> /* lock again */
>
> you will always one message at a time, so you know that when you get the
> IRQ, the first message on your list is what was just completed.
>
>> eg: Process A sends a message to M3 core to kick off some translation
>> in some codec, the thread handling this request running on BIOS would
>> kick off the translation and wait for the completion before sending
>> the response. In the mean time BIOS will be handling other requests
>> coming to it, and shouldn't be blocked until the completion of first
>> request.
>
> why you're involving BIOS here ? mailbox should not have to know what's
> on the other side. All it needs to know is how to get the message to the
> other side, no matter _what_ it is.
>
> So until BIOS sends the response, you would not start another transfer
> on mailbox neither you would any IRQ, right ?

That's not true. Even if BIOS doesn't respond to a request, you could
still keep sending the messages.


Thank you,
Best regards,
Hari Kanigeri

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

[Felipe Balbi]

Hi Hari,

On Fri, 2010-11-19 at 08:44 -0600, Kanigeri, Hari wrote:
> Not really :). Please let me know if if I am wrong, what you
> addressing is getting the confirmation that a message is sent and what
> I am addressing with the patch is that a response is received from
> M3/DSP.

You got it wrong. My proposal addresses the same what you say, but in a
different and, IMO, better fashion. There's no need to add a blocking
notifier which you can't be sure when that'll be scheduled. Have you
measured possible worst case scenario of this patch ? What's the latency
added by the blocking notifier ? Imagine user cpu is highly busy, and it
takes a long time to call the blocking notifier, is that acceptable ?

> > Then you kick the transfers which will:
> >
> > request = list_first_entry(mbox->req_list);
> > setup_correct_registers();
> > enable_irq();
> > kick_transfer();
> 
> Writing to the mailbox fifo delivers the message to other side, and if
> the fifo is full the messages are queued up in mbox kfifo, which are
> then deliverd in the order they are received.

isn't that the same as what I suggested ? Messages are queued in the
ordered they are received and sent to BIOS at the same order.

> I don't see a use case where the senders need to know that their
> message is actually written to mbox fifo, if there is one we can look
> into it.

I never said there's such usecase :-)

> That's not true. Even if BIOS doesn't respond to a request, you could
> still keep sending the messages.

but then when do you consider a message "completed" ? When it gets sent
or when you receive a response from BIOS ?

-- 
balbi

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

[Kanigeri, Hari]

Felipe,

On Fri, Nov 19, 2010 at 5:07 PM, Felipe Balbi <me@felipebalbi.com> wrote:
> Hi Hari,
>
> On Fri, 2010-11-19 at 08:44 -0600, Kanigeri, Hari wrote:
>> Not really :). Please let me know if if I am wrong, what you
>> addressing is getting the confirmation that a message is sent and what
>> I am addressing with the patch is that a response is received from
>> M3/DSP.
>
> You got it wrong. My proposal addresses the same what you say, but in a
> different and, IMO, better fashion. There's no need to add a blocking
> notifier which you can't be sure when that'll be scheduled.

hmm, it is called in the work queue context after a mailbox interrupt
is received. Blocking notifier just calls all the registered
callbacks, and in this case only one callback is registered so it is
same as today where a function pointer in mailbox is set to callback
function.

> Have you
> measured possible worst case scenario of this patch ? What's the latency
> added by the blocking notifier ?

Of course :), profiling was done before releasing this code and no
difference observed with or without blocking notifier. All the OMAP4
use cases are exercising this code. Just curious , are you doubting
the blocking notifier mechanism ?

> Imagine user cpu is highly busy, and it
> takes a long time to call the blocking notifier, is that acceptable ?

The mailbox rx interrupt schedules the RX work queue, from where the
blocking Notifier is called to notify the interested clients of the
response.
In previous implementation, the work queue was calling the callback
that was registered by setting the mailbox's internal function
pointer, and the only change I did is to change the function pointer
to Notifier call so that the users don't muck with the driver's
internal pointer. Latency wise there is no difference between calling
the function directly or through blocking Notifier, since blocking
notifier mechanism just calls the registered callbacks.

>
>> > Then you kick the transfers which will:
>> >
>> > request = list_first_entry(mbox->req_list);
>> > setup_correct_registers();
>> > enable_irq();
>> > kick_transfer();
>>
>> Writing to the mailbox fifo delivers the message to other side, and if
>> the fifo is full the messages are queued up in mbox kfifo, which are
>> then deliverd in the order they are received.
>
> isn't that the same as what I suggested ? Messages are queued in the
> ordered they are received and sent to BIOS at the same order.
>
This is already handled with the Kfifo implementation in the mailbox driver.

>> I don't see a use case where the senders need to know that their
>> message is actually written to mbox fifo, if there is one we can look
>> into it.
>
> I never said there's such usecase :-)

I agree :), was just asking to get the confirmation since we are miles apart ;)

>
>> That's not true. Even if BIOS doesn't respond to a request, you could
>> still keep sending the messages.
>
> but then when do you consider a message "completed" ? When it gets sent
> or when you receive a response from BIOS ?

The application after exercising the mailbox put,  for him the message
is sent out, and will/will not wait on the asynchronous response from
M3/DSP, which could happen at any later time. And in some cases, he
might not even expect a response. But as I mentioned earlier this kind
of intelligence whether to wait/not wait will be in the IPC driver and
not in the low level driver such as mailbox.

Thank you,
Best regards,
Hari Kanigeri

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

[Felipe Balbi]

Hi Hari,

On Fri, 2010-11-19 at 22:01 -0600, Kanigeri, Hari wrote:
> Of course :), profiling was done before releasing this code and no
> difference observed with or without blocking notifier. All the OMAP4

would you share some numbers ?

> use cases are exercising this code. Just curious , are you doubting
> the blocking notifier mechanism ?

a little bit. Yeah. It added about 600ms of time spent on musb's probe
when we were using blocking notifier for charger detection. When we
moved to atomic notifier, that was solved. Anyway, you can never be sure
when that will be scheduled and if cpu is really busy, it might pose a
great deal.

-- 
balbi

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

[Kanigeri, Hari]

Felipe,

On Sat, Nov 20, 2010 at 5:31 AM, Felipe Balbi <me@felipebalbi.com> wrote:
> Hi Hari,
>
> On Fri, 2010-11-19 at 22:01 -0600, Kanigeri, Hari wrote:
>> Of course :), profiling was done before releasing this code and no
>> difference observed with or without blocking notifier. All the OMAP4
>
> would you share some numbers ?

Sure. On an average, 135 us for round trip message from A9 userspace
Process to a thread running on M3 that handles this message.
The test is with the entire IPC stack on both ends.

Thank you,
Best regards,
Hari