Re: [PATCH] mailbox: fix completion order for blocking requests

[Jassi Brar]

On Thu, May 25, 2017 at 10:10 PM, Alexey Klimov  wrote:
> On Sun, Apr 23, 2017 at 03:33:39PM +0530, Jassi Brar wrote:

>
> Sorry for delay -- this is not my main activity so please be patient.
>
No problem.

>> >
>> > Along with this patch you still need at least one patch from Sudeep with 
>> > subject:
>> > "[PATCH 1/3] mailbox: always wait in mbox_send_message for blocking Tx 
>> > mode"
>> >
>> Yes. Just so we are on same page, can you please redo your tests and
>> see if this and Sudeep's patch-1/3 does the trick?
>
> Yes, for such kind of behaviour to make things work I need to apply Sudeep's 
> patches
> and this your (or mine) patch that changes completion behaviour.
> Or do I miss something and have you already applied it?
>
I have already applied three patches from Sudeep. The patch in this
thread is not yet applied, I was hoping to get some tested-by, though
I think it is OK.

Thanks

Re: [PATCH] mailbox: fix completion order for blocking requests

[Jassi Brar]

On Thu, May 25, 2017 at 10:10 PM, Alexey Klimov  wrote:
> On Sun, Apr 23, 2017 at 03:33:39PM +0530, Jassi Brar wrote:

>
> Sorry for delay -- this is not my main activity so please be patient.
>
No problem.

>> >
>> > Along with this patch you still need at least one patch from Sudeep with 
>> > subject:
>> > "[PATCH 1/3] mailbox: always wait in mbox_send_message for blocking Tx 
>> > mode"
>> >
>> Yes. Just so we are on same page, can you please redo your tests and
>> see if this and Sudeep's patch-1/3 does the trick?
>
> Yes, for such kind of behaviour to make things work I need to apply Sudeep's 
> patches
> and this your (or mine) patch that changes completion behaviour.
> Or do I miss something and have you already applied it?
>
I have already applied three patches from Sudeep. The patch in this
thread is not yet applied, I was hoping to get some tested-by, though
I think it is OK.

Thanks

Re: [PATCH] mailbox: fix completion order for blocking requests

[Alexey Klimov]

Hi Jassi,

Sorry for delay -- this is not my main activity so please be patient.

On Sun, Apr 23, 2017 at 03:33:39PM +0530, Jassi Brar wrote:
> On Tue, Apr 11, 2017 at 6:15 PM, Alexey Klimov  wrote:
> > On Thu, Apr 06, 2017 at 10:45:26PM +0530, Jassi Brar wrote:
> >> On 6 April 2017 at 22:28, Alexey Klimov  wrote:
> >> > Hi Jassi/Sudeep,
> >> >
> >> > On Wed, Mar 29, 2017 at 07:01:09PM +0100, Sudeep Holla wrote:
> >> >>
> >> >>
> >> >> On 29/03/17 18:43, Jassi Brar wrote:
> >> ...
> >>
> >> >> > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> >> >> > index 9dfbf7e..e06c50c 100644
> >> >> > --- a/drivers/mailbox/mailbox.c
> >> >> > +++ b/drivers/mailbox/mailbox.c
> >> >> > @@ -41,6 +41,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void 
> >> >> > *mssg)
> >> >> >
> >> >> > idx = chan->msg_free;
> >> >> > chan->msg_data[idx] = mssg;
> >> >> > +   init_completion(>tx_cmpl[idx]);
> >> >>
> >> >> reinit would be better.
> >> >
> >> Of course.
> >>
> >> 
> >> > From: Alexey Klimov 
> >> > Date: Thu, 6 Apr 2017 13:57:02 +0100
> >> > Subject: [RFC][PATCH] mailbox: per-channel arrays with msg data and 
> >> > completion
> >> >  structures
> >> >
> >> > When a mailbox client doesn't serialize sending of the message itself,
> >> > and asks mailbox framework to block on mbox_send_message(), one
> >> > completion structure per channel is not enough. Client can make a few
> >> > mbox_send_message() calls at the same time, and there is no guaranteed
> >> > order of going to sleep on completion.
> >> >
> >> > If mailbox controller acks a message transfer, then tx_tick() wakes up
> >> > the first thread that waits on completion.
> >> > If mailbox controller doesn't ack the transfer and timeout happens, then
> >> > tx_tick() calls complete, and the next caller trying to sleep on
> >> > completion wakes up immediately.
> >> >
> >> > This patch fixes this by changing completion structures to be inserted
> >> > into an array that contains a) pointer to data provided by client and
> >> > b) the completion structure. Thus active_req field tracks the index of
> >> > the current running request that was submitted to mailbox controller.
> >> >
> >> > Signed-off-by: Alexey Klimov 
> >> > ---
> >> >  drivers/mailbox/mailbox.c  | 40 
> >> > +++---
> >> >  drivers/mailbox/pcc.c  | 10 +++---
> >> >  include/linux/mailbox_controller.h | 24 +--
> >> ...
> >> >  3 files changed, 49 insertions(+), 25 deletions(-)
> >> >
> >>  Versus   4 files changed, 17 insertions(+), 8 deletions(-)
> >>
> >> I think we should just keep it simpler if it works just as fine.
> >
> > Along with this patch you still need at least one patch from Sudeep with 
> > subject:
> > "[PATCH 1/3] mailbox: always wait in mbox_send_message for blocking Tx mode"
> >
> Yes. Just so we are on same page, can you please redo your tests and
> see if this and Sudeep's patch-1/3 does the trick?

Yes, for such kind of behaviour to make things work I need to apply Sudeep's 
patches
and this your (or mine) patch that changes completion behaviour.
Or do I miss something and have you already applied it?

Thanks!

Alexey.

Re: [PATCH] mailbox: fix completion order for blocking requests

[Alexey Klimov]

Hi Jassi,

Sorry for delay -- this is not my main activity so please be patient.

On Sun, Apr 23, 2017 at 03:33:39PM +0530, Jassi Brar wrote:
> On Tue, Apr 11, 2017 at 6:15 PM, Alexey Klimov  wrote:
> > On Thu, Apr 06, 2017 at 10:45:26PM +0530, Jassi Brar wrote:
> >> On 6 April 2017 at 22:28, Alexey Klimov  wrote:
> >> > Hi Jassi/Sudeep,
> >> >
> >> > On Wed, Mar 29, 2017 at 07:01:09PM +0100, Sudeep Holla wrote:
> >> >>
> >> >>
> >> >> On 29/03/17 18:43, Jassi Brar wrote:
> >> ...
> >>
> >> >> > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> >> >> > index 9dfbf7e..e06c50c 100644
> >> >> > --- a/drivers/mailbox/mailbox.c
> >> >> > +++ b/drivers/mailbox/mailbox.c
> >> >> > @@ -41,6 +41,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void 
> >> >> > *mssg)
> >> >> >
> >> >> > idx = chan->msg_free;
> >> >> > chan->msg_data[idx] = mssg;
> >> >> > +   init_completion(>tx_cmpl[idx]);
> >> >>
> >> >> reinit would be better.
> >> >
> >> Of course.
> >>
> >> 
> >> > From: Alexey Klimov 
> >> > Date: Thu, 6 Apr 2017 13:57:02 +0100
> >> > Subject: [RFC][PATCH] mailbox: per-channel arrays with msg data and 
> >> > completion
> >> >  structures
> >> >
> >> > When a mailbox client doesn't serialize sending of the message itself,
> >> > and asks mailbox framework to block on mbox_send_message(), one
> >> > completion structure per channel is not enough. Client can make a few
> >> > mbox_send_message() calls at the same time, and there is no guaranteed
> >> > order of going to sleep on completion.
> >> >
> >> > If mailbox controller acks a message transfer, then tx_tick() wakes up
> >> > the first thread that waits on completion.
> >> > If mailbox controller doesn't ack the transfer and timeout happens, then
> >> > tx_tick() calls complete, and the next caller trying to sleep on
> >> > completion wakes up immediately.
> >> >
> >> > This patch fixes this by changing completion structures to be inserted
> >> > into an array that contains a) pointer to data provided by client and
> >> > b) the completion structure. Thus active_req field tracks the index of
> >> > the current running request that was submitted to mailbox controller.
> >> >
> >> > Signed-off-by: Alexey Klimov 
> >> > ---
> >> >  drivers/mailbox/mailbox.c  | 40 
> >> > +++---
> >> >  drivers/mailbox/pcc.c  | 10 +++---
> >> >  include/linux/mailbox_controller.h | 24 +--
> >> ...
> >> >  3 files changed, 49 insertions(+), 25 deletions(-)
> >> >
> >>  Versus   4 files changed, 17 insertions(+), 8 deletions(-)
> >>
> >> I think we should just keep it simpler if it works just as fine.
> >
> > Along with this patch you still need at least one patch from Sudeep with 
> > subject:
> > "[PATCH 1/3] mailbox: always wait in mbox_send_message for blocking Tx mode"
> >
> Yes. Just so we are on same page, can you please redo your tests and
> see if this and Sudeep's patch-1/3 does the trick?

Yes, for such kind of behaviour to make things work I need to apply Sudeep's 
patches
and this your (or mine) patch that changes completion behaviour.
Or do I miss something and have you already applied it?

Thanks!

Alexey.

Re: [PATCH] mailbox: fix completion order for blocking requests

[Jassi Brar]

On Tue, Apr 11, 2017 at 6:15 PM, Alexey Klimov  wrote:
> On Thu, Apr 06, 2017 at 10:45:26PM +0530, Jassi Brar wrote:
>> On 6 April 2017 at 22:28, Alexey Klimov  wrote:
>> > Hi Jassi/Sudeep,
>> >
>> > On Wed, Mar 29, 2017 at 07:01:09PM +0100, Sudeep Holla wrote:
>> >>
>> >>
>> >> On 29/03/17 18:43, Jassi Brar wrote:
>> ...
>>
>> >> > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>> >> > index 9dfbf7e..e06c50c 100644
>> >> > --- a/drivers/mailbox/mailbox.c
>> >> > +++ b/drivers/mailbox/mailbox.c
>> >> > @@ -41,6 +41,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void 
>> >> > *mssg)
>> >> >
>> >> > idx = chan->msg_free;
>> >> > chan->msg_data[idx] = mssg;
>> >> > +   init_completion(>tx_cmpl[idx]);
>> >>
>> >> reinit would be better.
>> >
>> Of course.
>>
>> 
>> > From: Alexey Klimov 
>> > Date: Thu, 6 Apr 2017 13:57:02 +0100
>> > Subject: [RFC][PATCH] mailbox: per-channel arrays with msg data and 
>> > completion
>> >  structures
>> >
>> > When a mailbox client doesn't serialize sending of the message itself,
>> > and asks mailbox framework to block on mbox_send_message(), one
>> > completion structure per channel is not enough. Client can make a few
>> > mbox_send_message() calls at the same time, and there is no guaranteed
>> > order of going to sleep on completion.
>> >
>> > If mailbox controller acks a message transfer, then tx_tick() wakes up
>> > the first thread that waits on completion.
>> > If mailbox controller doesn't ack the transfer and timeout happens, then
>> > tx_tick() calls complete, and the next caller trying to sleep on
>> > completion wakes up immediately.
>> >
>> > This patch fixes this by changing completion structures to be inserted
>> > into an array that contains a) pointer to data provided by client and
>> > b) the completion structure. Thus active_req field tracks the index of
>> > the current running request that was submitted to mailbox controller.
>> >
>> > Signed-off-by: Alexey Klimov 
>> > ---
>> >  drivers/mailbox/mailbox.c  | 40 
>> > +++---
>> >  drivers/mailbox/pcc.c  | 10 +++---
>> >  include/linux/mailbox_controller.h | 24 +--
>> ...
>> >  3 files changed, 49 insertions(+), 25 deletions(-)
>> >
>>  Versus   4 files changed, 17 insertions(+), 8 deletions(-)
>>
>> I think we should just keep it simpler if it works just as fine.
>
> Along with this patch you still need at least one patch from Sudeep with 
> subject:
> "[PATCH 1/3] mailbox: always wait in mbox_send_message for blocking Tx mode"
>
Yes. Just so we are on same page, can you please redo your tests and
see if this and Sudeep's patch-1/3 does the trick?

Thanks

Re: [PATCH] mailbox: fix completion order for blocking requests

[Jassi Brar]

On Tue, Apr 11, 2017 at 6:15 PM, Alexey Klimov  wrote:
> On Thu, Apr 06, 2017 at 10:45:26PM +0530, Jassi Brar wrote:
>> On 6 April 2017 at 22:28, Alexey Klimov  wrote:
>> > Hi Jassi/Sudeep,
>> >
>> > On Wed, Mar 29, 2017 at 07:01:09PM +0100, Sudeep Holla wrote:
>> >>
>> >>
>> >> On 29/03/17 18:43, Jassi Brar wrote:
>> ...
>>
>> >> > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>> >> > index 9dfbf7e..e06c50c 100644
>> >> > --- a/drivers/mailbox/mailbox.c
>> >> > +++ b/drivers/mailbox/mailbox.c
>> >> > @@ -41,6 +41,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void 
>> >> > *mssg)
>> >> >
>> >> > idx = chan->msg_free;
>> >> > chan->msg_data[idx] = mssg;
>> >> > +   init_completion(>tx_cmpl[idx]);
>> >>
>> >> reinit would be better.
>> >
>> Of course.
>>
>> 
>> > From: Alexey Klimov 
>> > Date: Thu, 6 Apr 2017 13:57:02 +0100
>> > Subject: [RFC][PATCH] mailbox: per-channel arrays with msg data and 
>> > completion
>> >  structures
>> >
>> > When a mailbox client doesn't serialize sending of the message itself,
>> > and asks mailbox framework to block on mbox_send_message(), one
>> > completion structure per channel is not enough. Client can make a few
>> > mbox_send_message() calls at the same time, and there is no guaranteed
>> > order of going to sleep on completion.
>> >
>> > If mailbox controller acks a message transfer, then tx_tick() wakes up
>> > the first thread that waits on completion.
>> > If mailbox controller doesn't ack the transfer and timeout happens, then
>> > tx_tick() calls complete, and the next caller trying to sleep on
>> > completion wakes up immediately.
>> >
>> > This patch fixes this by changing completion structures to be inserted
>> > into an array that contains a) pointer to data provided by client and
>> > b) the completion structure. Thus active_req field tracks the index of
>> > the current running request that was submitted to mailbox controller.
>> >
>> > Signed-off-by: Alexey Klimov 
>> > ---
>> >  drivers/mailbox/mailbox.c  | 40 
>> > +++---
>> >  drivers/mailbox/pcc.c  | 10 +++---
>> >  include/linux/mailbox_controller.h | 24 +--
>> ...
>> >  3 files changed, 49 insertions(+), 25 deletions(-)
>> >
>>  Versus   4 files changed, 17 insertions(+), 8 deletions(-)
>>
>> I think we should just keep it simpler if it works just as fine.
>
> Along with this patch you still need at least one patch from Sudeep with 
> subject:
> "[PATCH 1/3] mailbox: always wait in mbox_send_message for blocking Tx mode"
>
Yes. Just so we are on same page, can you please redo your tests and
see if this and Sudeep's patch-1/3 does the trick?

Thanks

Re: [PATCH] mailbox: fix completion order for blocking requests

[Alexey Klimov]

On Thu, Apr 06, 2017 at 10:45:26PM +0530, Jassi Brar wrote:
> On 6 April 2017 at 22:28, Alexey Klimov  wrote:
> > Hi Jassi/Sudeep,
> >
> > On Wed, Mar 29, 2017 at 07:01:09PM +0100, Sudeep Holla wrote:
> >>
> >>
> >> On 29/03/17 18:43, Jassi Brar wrote:
> ...
> 
> >> > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> >> > index 9dfbf7e..e06c50c 100644
> >> > --- a/drivers/mailbox/mailbox.c
> >> > +++ b/drivers/mailbox/mailbox.c
> >> > @@ -41,6 +41,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void 
> >> > *mssg)
> >> >
> >> > idx = chan->msg_free;
> >> > chan->msg_data[idx] = mssg;
> >> > +   init_completion(>tx_cmpl[idx]);
> >>
> >> reinit would be better.
> >
> Of course.
> 
> 
> > From: Alexey Klimov 
> > Date: Thu, 6 Apr 2017 13:57:02 +0100
> > Subject: [RFC][PATCH] mailbox: per-channel arrays with msg data and 
> > completion
> >  structures
> >
> > When a mailbox client doesn't serialize sending of the message itself,
> > and asks mailbox framework to block on mbox_send_message(), one
> > completion structure per channel is not enough. Client can make a few
> > mbox_send_message() calls at the same time, and there is no guaranteed
> > order of going to sleep on completion.
> >
> > If mailbox controller acks a message transfer, then tx_tick() wakes up
> > the first thread that waits on completion.
> > If mailbox controller doesn't ack the transfer and timeout happens, then
> > tx_tick() calls complete, and the next caller trying to sleep on
> > completion wakes up immediately.
> >
> > This patch fixes this by changing completion structures to be inserted
> > into an array that contains a) pointer to data provided by client and
> > b) the completion structure. Thus active_req field tracks the index of
> > the current running request that was submitted to mailbox controller.
> >
> > Signed-off-by: Alexey Klimov 
> > ---
> >  drivers/mailbox/mailbox.c  | 40 
> > +++---
> >  drivers/mailbox/pcc.c  | 10 +++---
> >  include/linux/mailbox_controller.h | 24 +--
> ...
> >  3 files changed, 49 insertions(+), 25 deletions(-)
> >
>  Versus   4 files changed, 17 insertions(+), 8 deletions(-)
> 
> I think we should just keep it simpler if it works just as fine.

Along with this patch you still need at least one patch from Sudeep with 
subject:
"[PATCH 1/3] mailbox: always wait in mbox_send_message for blocking Tx mode"

Decision to block or not shouldn't depend on racy reading of active_req field.

Best regards,
Alexey Klimov.

Re: [PATCH] mailbox: fix completion order for blocking requests

[Alexey Klimov]

On Thu, Apr 06, 2017 at 10:45:26PM +0530, Jassi Brar wrote:
> On 6 April 2017 at 22:28, Alexey Klimov  wrote:
> > Hi Jassi/Sudeep,
> >
> > On Wed, Mar 29, 2017 at 07:01:09PM +0100, Sudeep Holla wrote:
> >>
> >>
> >> On 29/03/17 18:43, Jassi Brar wrote:
> ...
> 
> >> > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> >> > index 9dfbf7e..e06c50c 100644
> >> > --- a/drivers/mailbox/mailbox.c
> >> > +++ b/drivers/mailbox/mailbox.c
> >> > @@ -41,6 +41,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void 
> >> > *mssg)
> >> >
> >> > idx = chan->msg_free;
> >> > chan->msg_data[idx] = mssg;
> >> > +   init_completion(>tx_cmpl[idx]);
> >>
> >> reinit would be better.
> >
> Of course.
> 
> 
> > From: Alexey Klimov 
> > Date: Thu, 6 Apr 2017 13:57:02 +0100
> > Subject: [RFC][PATCH] mailbox: per-channel arrays with msg data and 
> > completion
> >  structures
> >
> > When a mailbox client doesn't serialize sending of the message itself,
> > and asks mailbox framework to block on mbox_send_message(), one
> > completion structure per channel is not enough. Client can make a few
> > mbox_send_message() calls at the same time, and there is no guaranteed
> > order of going to sleep on completion.
> >
> > If mailbox controller acks a message transfer, then tx_tick() wakes up
> > the first thread that waits on completion.
> > If mailbox controller doesn't ack the transfer and timeout happens, then
> > tx_tick() calls complete, and the next caller trying to sleep on
> > completion wakes up immediately.
> >
> > This patch fixes this by changing completion structures to be inserted
> > into an array that contains a) pointer to data provided by client and
> > b) the completion structure. Thus active_req field tracks the index of
> > the current running request that was submitted to mailbox controller.
> >
> > Signed-off-by: Alexey Klimov 
> > ---
> >  drivers/mailbox/mailbox.c  | 40 
> > +++---
> >  drivers/mailbox/pcc.c  | 10 +++---
> >  include/linux/mailbox_controller.h | 24 +--
> ...
> >  3 files changed, 49 insertions(+), 25 deletions(-)
> >
>  Versus   4 files changed, 17 insertions(+), 8 deletions(-)
> 
> I think we should just keep it simpler if it works just as fine.

Along with this patch you still need at least one patch from Sudeep with 
subject:
"[PATCH 1/3] mailbox: always wait in mbox_send_message for blocking Tx mode"

Decision to block or not shouldn't depend on racy reading of active_req field.

Best regards,
Alexey Klimov.

Re: [PATCH] mailbox: fix completion order for blocking requests

[Jassi Brar]

On 6 April 2017 at 22:28, Alexey Klimov  wrote:
> Hi Jassi/Sudeep,
>
> On Wed, Mar 29, 2017 at 07:01:09PM +0100, Sudeep Holla wrote:
>>
>>
>> On 29/03/17 18:43, Jassi Brar wrote:
...

>> > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>> > index 9dfbf7e..e06c50c 100644
>> > --- a/drivers/mailbox/mailbox.c
>> > +++ b/drivers/mailbox/mailbox.c
>> > @@ -41,6 +41,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void 
>> > *mssg)
>> >
>> > idx = chan->msg_free;
>> > chan->msg_data[idx] = mssg;
>> > +   init_completion(>tx_cmpl[idx]);
>>
>> reinit would be better.
>
Of course.


> From: Alexey Klimov 
> Date: Thu, 6 Apr 2017 13:57:02 +0100
> Subject: [RFC][PATCH] mailbox: per-channel arrays with msg data and completion
>  structures
>
> When a mailbox client doesn't serialize sending of the message itself,
> and asks mailbox framework to block on mbox_send_message(), one
> completion structure per channel is not enough. Client can make a few
> mbox_send_message() calls at the same time, and there is no guaranteed
> order of going to sleep on completion.
>
> If mailbox controller acks a message transfer, then tx_tick() wakes up
> the first thread that waits on completion.
> If mailbox controller doesn't ack the transfer and timeout happens, then
> tx_tick() calls complete, and the next caller trying to sleep on
> completion wakes up immediately.
>
> This patch fixes this by changing completion structures to be inserted
> into an array that contains a) pointer to data provided by client and
> b) the completion structure. Thus active_req field tracks the index of
> the current running request that was submitted to mailbox controller.
>
> Signed-off-by: Alexey Klimov 
> ---
>  drivers/mailbox/mailbox.c  | 40 
> +++---
>  drivers/mailbox/pcc.c  | 10 +++---
>  include/linux/mailbox_controller.h | 24 +--
...
>  3 files changed, 49 insertions(+), 25 deletions(-)
>
 Versus   4 files changed, 17 insertions(+), 8 deletions(-)

I think we should just keep it simpler if it works just as fine.

Thanks.

Re: [PATCH] mailbox: fix completion order for blocking requests

[Jassi Brar]

On 6 April 2017 at 22:28, Alexey Klimov  wrote:
> Hi Jassi/Sudeep,
>
> On Wed, Mar 29, 2017 at 07:01:09PM +0100, Sudeep Holla wrote:
>>
>>
>> On 29/03/17 18:43, Jassi Brar wrote:
...

>> > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>> > index 9dfbf7e..e06c50c 100644
>> > --- a/drivers/mailbox/mailbox.c
>> > +++ b/drivers/mailbox/mailbox.c
>> > @@ -41,6 +41,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void 
>> > *mssg)
>> >
>> > idx = chan->msg_free;
>> > chan->msg_data[idx] = mssg;
>> > +   init_completion(>tx_cmpl[idx]);
>>
>> reinit would be better.
>
Of course.


> From: Alexey Klimov 
> Date: Thu, 6 Apr 2017 13:57:02 +0100
> Subject: [RFC][PATCH] mailbox: per-channel arrays with msg data and completion
>  structures
>
> When a mailbox client doesn't serialize sending of the message itself,
> and asks mailbox framework to block on mbox_send_message(), one
> completion structure per channel is not enough. Client can make a few
> mbox_send_message() calls at the same time, and there is no guaranteed
> order of going to sleep on completion.
>
> If mailbox controller acks a message transfer, then tx_tick() wakes up
> the first thread that waits on completion.
> If mailbox controller doesn't ack the transfer and timeout happens, then
> tx_tick() calls complete, and the next caller trying to sleep on
> completion wakes up immediately.
>
> This patch fixes this by changing completion structures to be inserted
> into an array that contains a) pointer to data provided by client and
> b) the completion structure. Thus active_req field tracks the index of
> the current running request that was submitted to mailbox controller.
>
> Signed-off-by: Alexey Klimov 
> ---
>  drivers/mailbox/mailbox.c  | 40 
> +++---
>  drivers/mailbox/pcc.c  | 10 +++---
>  include/linux/mailbox_controller.h | 24 +--
...
>  3 files changed, 49 insertions(+), 25 deletions(-)
>
 Versus   4 files changed, 17 insertions(+), 8 deletions(-)

I think we should just keep it simpler if it works just as fine.

Thanks.

Re: [PATCH] mailbox: fix completion order for blocking requests

[Alexey Klimov]

Hi Jassi/Sudeep,

On Wed, Mar 29, 2017 at 07:01:09PM +0100, Sudeep Holla wrote:
> 
> 
> On 29/03/17 18:43, Jassi Brar wrote:
> > Currently two threads, wait on blocking requests, could wake up for
> > completion of request of each other as ...
> > 
> > Thread#1(T1)   Thread#2(T2)
> >  mbox_send_message   mbox_send_message
> > |   |
> > V   |
> > add_to_rbuf(M1) V
> > | add_to_rbuf(M2)
> > |   |
> > |   V
> > V  msg_submit(picks M1)
> > msg_submit  |
> > |   V
> > V   wait_for_completion(on M2)
> >  wait_for_completion(on M1) |  (1st in waitQ)
> > |   (2nd in waitQ)  V
> > V   wake_up(on completion of M1)<--incorrect
> > 
> >  Fix this situaion by assigning completion structures to each queued
> > request, so that the threads could wait on their own completions.
> > 
> 
> Alexey came up with exact similar solution. I didn't like:

Sorry for delay.

Let me attach it, just in case. It's inserted in the of the email at [1].
It has some issues with naming of structure maybe and thing that
Sudeep pointed out.
-1 is used for active request field which doesn't look good too.
 
> 1. the static array just bloats the structure with equal no. of
>completion which may be useless for !TXDONE_BY_POLL
> 
> 2. We have client drivers already doing something similar. I wanted
>to fix/move those along with this fix. Or at-least see the feasibiliy
> 
> > Reported-by: Alexey Klimov 
> > Signed-off-by: Jassi Brar 
> > ---
> >  drivers/mailbox/mailbox.c  | 15 +++
> >  drivers/mailbox/omap-mailbox.c |  2 +-
> >  drivers/mailbox/pcc.c  |  2 +-
> >  include/linux/mailbox_controller.h |  6 --
> >  4 files changed, 17 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> > index 9dfbf7e..e06c50c 100644
> > --- a/drivers/mailbox/mailbox.c
> > +++ b/drivers/mailbox/mailbox.c
> > @@ -41,6 +41,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
> >  
> > idx = chan->msg_free;
> > chan->msg_data[idx] = mssg;
> > +   init_completion(>tx_cmpl[idx]);
> 
> reinit would be better.

Agree.
Also, reinit_completion can be moved to mbox_send_message() under
"if" that checks if it's a blocking request or not.
 
> > chan->msg_count++;
> >  
> > if (idx == MBOX_TX_QUEUE_LEN - 1)
> > @@ -73,6 +74,7 @@ static void msg_submit(struct mbox_chan *chan)
> > idx += MBOX_TX_QUEUE_LEN - count;
> >  
> > data = chan->msg_data[idx];
> > +   chan->tx_complete = >tx_cmpl[idx];
> >  
> > if (chan->cl->tx_prepare)
> > chan->cl->tx_prepare(chan->cl, data);
> > @@ -81,7 +83,8 @@ static void msg_submit(struct mbox_chan *chan)
> > if (!err) {
> > chan->active_req = data;
> > chan->msg_count--;
> > -   }
> > +   } else
> > +   chan->tx_complete = NULL;
> >  exit:
> > spin_unlock_irqrestore(>lock, flags);
> >  
> > @@ -92,12 +95,15 @@ static void msg_submit(struct mbox_chan *chan)
> >  
> >  static void tx_tick(struct mbox_chan *chan, int r)
> >  {
> > +   struct completion *tx_complete;
> > unsigned long flags;
> > void *mssg;
> >  
> > spin_lock_irqsave(>lock, flags);
> > mssg = chan->active_req;
> > +   tx_complete = chan->tx_complete;
> > chan->active_req = NULL;
> > +   chan->tx_complete = NULL;
> > spin_unlock_irqrestore(>lock, flags);
> >  
> > /* Submit next message */
> > @@ -111,7 +117,7 @@ static void tx_tick(struct mbox_chan *chan, int r)
> > chan->cl->tx_done(chan->cl, mssg, r);
> >  
> > if (r != -ETIME && chan->cl->tx_block)
> > -   complete(>tx_complete);
> > +   complete(tx_complete);
> >  }
> >  
> >  static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
> > @@ -272,7 +278,7 @@ int mbox_send_message(struct mbox_chan *chan, void 
> > *mssg)
> > else
> > wait = msecs_to_jiffies(chan->cl->tx_tout);
> >  
> > -   ret = wait_for_completion_timeout(>tx_complete, wait);
> > +   ret = wait_for_completion_timeout(>tx_cmpl[t], wait);
> > if (ret == 0) {
> > t = -ETIME;
> > tx_tick(chan, t);
> > @@ -348,7 +354,7 @@ struct mbox_chan *mbox_request_channel(struct 
> > mbox_client *cl, int index)
> > chan->msg_count = 0;
> > chan->active_req = NULL;
> > chan->cl = cl;
> > -   init_completion(>tx_complete);
> > +   chan->tx_complete = NULL;
> >  
> > if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
> > 

Re: [PATCH] mailbox: fix completion order for blocking requests

[Alexey Klimov]

Hi Jassi/Sudeep,

On Wed, Mar 29, 2017 at 07:01:09PM +0100, Sudeep Holla wrote:
> 
> 
> On 29/03/17 18:43, Jassi Brar wrote:
> > Currently two threads, wait on blocking requests, could wake up for
> > completion of request of each other as ...
> > 
> > Thread#1(T1)   Thread#2(T2)
> >  mbox_send_message   mbox_send_message
> > |   |
> > V   |
> > add_to_rbuf(M1) V
> > | add_to_rbuf(M2)
> > |   |
> > |   V
> > V  msg_submit(picks M1)
> > msg_submit  |
> > |   V
> > V   wait_for_completion(on M2)
> >  wait_for_completion(on M1) |  (1st in waitQ)
> > |   (2nd in waitQ)  V
> > V   wake_up(on completion of M1)<--incorrect
> > 
> >  Fix this situaion by assigning completion structures to each queued
> > request, so that the threads could wait on their own completions.
> > 
> 
> Alexey came up with exact similar solution. I didn't like:

Sorry for delay.

Let me attach it, just in case. It's inserted in the of the email at [1].
It has some issues with naming of structure maybe and thing that
Sudeep pointed out.
-1 is used for active request field which doesn't look good too.
 
> 1. the static array just bloats the structure with equal no. of
>completion which may be useless for !TXDONE_BY_POLL
> 
> 2. We have client drivers already doing something similar. I wanted
>to fix/move those along with this fix. Or at-least see the feasibiliy
> 
> > Reported-by: Alexey Klimov 
> > Signed-off-by: Jassi Brar 
> > ---
> >  drivers/mailbox/mailbox.c  | 15 +++
> >  drivers/mailbox/omap-mailbox.c |  2 +-
> >  drivers/mailbox/pcc.c  |  2 +-
> >  include/linux/mailbox_controller.h |  6 --
> >  4 files changed, 17 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> > index 9dfbf7e..e06c50c 100644
> > --- a/drivers/mailbox/mailbox.c
> > +++ b/drivers/mailbox/mailbox.c
> > @@ -41,6 +41,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
> >  
> > idx = chan->msg_free;
> > chan->msg_data[idx] = mssg;
> > +   init_completion(>tx_cmpl[idx]);
> 
> reinit would be better.

Agree.
Also, reinit_completion can be moved to mbox_send_message() under
"if" that checks if it's a blocking request or not.
 
> > chan->msg_count++;
> >  
> > if (idx == MBOX_TX_QUEUE_LEN - 1)
> > @@ -73,6 +74,7 @@ static void msg_submit(struct mbox_chan *chan)
> > idx += MBOX_TX_QUEUE_LEN - count;
> >  
> > data = chan->msg_data[idx];
> > +   chan->tx_complete = >tx_cmpl[idx];
> >  
> > if (chan->cl->tx_prepare)
> > chan->cl->tx_prepare(chan->cl, data);
> > @@ -81,7 +83,8 @@ static void msg_submit(struct mbox_chan *chan)
> > if (!err) {
> > chan->active_req = data;
> > chan->msg_count--;
> > -   }
> > +   } else
> > +   chan->tx_complete = NULL;
> >  exit:
> > spin_unlock_irqrestore(>lock, flags);
> >  
> > @@ -92,12 +95,15 @@ static void msg_submit(struct mbox_chan *chan)
> >  
> >  static void tx_tick(struct mbox_chan *chan, int r)
> >  {
> > +   struct completion *tx_complete;
> > unsigned long flags;
> > void *mssg;
> >  
> > spin_lock_irqsave(>lock, flags);
> > mssg = chan->active_req;
> > +   tx_complete = chan->tx_complete;
> > chan->active_req = NULL;
> > +   chan->tx_complete = NULL;
> > spin_unlock_irqrestore(>lock, flags);
> >  
> > /* Submit next message */
> > @@ -111,7 +117,7 @@ static void tx_tick(struct mbox_chan *chan, int r)
> > chan->cl->tx_done(chan->cl, mssg, r);
> >  
> > if (r != -ETIME && chan->cl->tx_block)
> > -   complete(>tx_complete);
> > +   complete(tx_complete);
> >  }
> >  
> >  static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
> > @@ -272,7 +278,7 @@ int mbox_send_message(struct mbox_chan *chan, void 
> > *mssg)
> > else
> > wait = msecs_to_jiffies(chan->cl->tx_tout);
> >  
> > -   ret = wait_for_completion_timeout(>tx_complete, wait);
> > +   ret = wait_for_completion_timeout(>tx_cmpl[t], wait);
> > if (ret == 0) {
> > t = -ETIME;
> > tx_tick(chan, t);
> > @@ -348,7 +354,7 @@ struct mbox_chan *mbox_request_channel(struct 
> > mbox_client *cl, int index)
> > chan->msg_count = 0;
> > chan->active_req = NULL;
> > chan->cl = cl;
> > -   init_completion(>tx_complete);
> > +   chan->tx_complete = NULL;
> >  
> > if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
> > chan->txdone_method |= TXDONE_BY_ACK;
> > @@ 

Re: [PATCH] mailbox: fix completion order for blocking requests

[Sudeep Holla]

On 29/03/17 18:43, Jassi Brar wrote:
> Currently two threads, wait on blocking requests, could wake up for
> completion of request of each other as ...
> 
> Thread#1(T1)   Thread#2(T2)
>  mbox_send_message   mbox_send_message
> |   |
> V   |
> add_to_rbuf(M1) V
> | add_to_rbuf(M2)
> |   |
> |   V
> V  msg_submit(picks M1)
> msg_submit  |
> |   V
> V   wait_for_completion(on M2)
>  wait_for_completion(on M1) |  (1st in waitQ)
> |   (2nd in waitQ)  V
> V   wake_up(on completion of M1)<--incorrect
> 
>  Fix this situaion by assigning completion structures to each queued
> request, so that the threads could wait on their own completions.
> 

Alexey came up with exact similar solution. I didn't like:

1. the static array just bloats the structure with equal no. of
   completion which may be useless for !TXDONE_BY_POLL

2. We have client drivers already doing something similar. I wanted
   to fix/move those along with this fix. Or at-least see the feasibiliy

> Reported-by: Alexey Klimov 
> Signed-off-by: Jassi Brar 
> ---
>  drivers/mailbox/mailbox.c  | 15 +++
>  drivers/mailbox/omap-mailbox.c |  2 +-
>  drivers/mailbox/pcc.c  |  2 +-
>  include/linux/mailbox_controller.h |  6 --
>  4 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> index 9dfbf7e..e06c50c 100644
> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -41,6 +41,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
>  
>   idx = chan->msg_free;
>   chan->msg_data[idx] = mssg;
> + init_completion(>tx_cmpl[idx]);

reinit would be better.

>   chan->msg_count++;
>  
>   if (idx == MBOX_TX_QUEUE_LEN - 1)
> @@ -73,6 +74,7 @@ static void msg_submit(struct mbox_chan *chan)
>   idx += MBOX_TX_QUEUE_LEN - count;
>  
>   data = chan->msg_data[idx];
> + chan->tx_complete = >tx_cmpl[idx];
>  
>   if (chan->cl->tx_prepare)
>   chan->cl->tx_prepare(chan->cl, data);
> @@ -81,7 +83,8 @@ static void msg_submit(struct mbox_chan *chan)
>   if (!err) {
>   chan->active_req = data;
>   chan->msg_count--;
> - }
> + } else
> + chan->tx_complete = NULL;
>  exit:
>   spin_unlock_irqrestore(>lock, flags);
>  
> @@ -92,12 +95,15 @@ static void msg_submit(struct mbox_chan *chan)
>  
>  static void tx_tick(struct mbox_chan *chan, int r)
>  {
> + struct completion *tx_complete;
>   unsigned long flags;
>   void *mssg;
>  
>   spin_lock_irqsave(>lock, flags);
>   mssg = chan->active_req;
> + tx_complete = chan->tx_complete;
>   chan->active_req = NULL;
> + chan->tx_complete = NULL;
>   spin_unlock_irqrestore(>lock, flags);
>  
>   /* Submit next message */
> @@ -111,7 +117,7 @@ static void tx_tick(struct mbox_chan *chan, int r)
>   chan->cl->tx_done(chan->cl, mssg, r);
>  
>   if (r != -ETIME && chan->cl->tx_block)
> - complete(>tx_complete);
> + complete(tx_complete);
>  }
>  
>  static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
> @@ -272,7 +278,7 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg)
>   else
>   wait = msecs_to_jiffies(chan->cl->tx_tout);
>  
> - ret = wait_for_completion_timeout(>tx_complete, wait);
> + ret = wait_for_completion_timeout(>tx_cmpl[t], wait);
>   if (ret == 0) {
>   t = -ETIME;
>   tx_tick(chan, t);
> @@ -348,7 +354,7 @@ struct mbox_chan *mbox_request_channel(struct mbox_client 
> *cl, int index)
>   chan->msg_count = 0;
>   chan->active_req = NULL;
>   chan->cl = cl;
> - init_completion(>tx_complete);
> + chan->tx_complete = NULL;
>  
>   if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
>   chan->txdone_method |= TXDONE_BY_ACK;
> @@ -414,6 +420,7 @@ void mbox_free_channel(struct mbox_chan *chan)
>   spin_lock_irqsave(>lock, flags);
>   chan->cl = NULL;
>   chan->active_req = NULL;
> + chan->tx_complete = NULL;
>   if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
>   chan->txdone_method = TXDONE_BY_POLL;
>  
> diff --git a/drivers/mailbox/omap-mailbox.c b/drivers/mailbox/omap-mailbox.c
> index c5e8b9c..99b0841 100644
> --- a/drivers/mailbox/omap-mailbox.c
> +++ b/drivers/mailbox/omap-mailbox.c
> @@ -449,7 +449,7 @@ struct mbox_chan 

Re: [PATCH] mailbox: fix completion order for blocking requests

[Sudeep Holla]

On 29/03/17 18:43, Jassi Brar wrote:
> Currently two threads, wait on blocking requests, could wake up for
> completion of request of each other as ...
> 
> Thread#1(T1)   Thread#2(T2)
>  mbox_send_message   mbox_send_message
> |   |
> V   |
> add_to_rbuf(M1) V
> | add_to_rbuf(M2)
> |   |
> |   V
> V  msg_submit(picks M1)
> msg_submit  |
> |   V
> V   wait_for_completion(on M2)
>  wait_for_completion(on M1) |  (1st in waitQ)
> |   (2nd in waitQ)  V
> V   wake_up(on completion of M1)<--incorrect
> 
>  Fix this situaion by assigning completion structures to each queued
> request, so that the threads could wait on their own completions.
> 

Alexey came up with exact similar solution. I didn't like:

1. the static array just bloats the structure with equal no. of
   completion which may be useless for !TXDONE_BY_POLL

2. We have client drivers already doing something similar. I wanted
   to fix/move those along with this fix. Or at-least see the feasibiliy

> Reported-by: Alexey Klimov 
> Signed-off-by: Jassi Brar 
> ---
>  drivers/mailbox/mailbox.c  | 15 +++
>  drivers/mailbox/omap-mailbox.c |  2 +-
>  drivers/mailbox/pcc.c  |  2 +-
>  include/linux/mailbox_controller.h |  6 --
>  4 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> index 9dfbf7e..e06c50c 100644
> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -41,6 +41,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
>  
>   idx = chan->msg_free;
>   chan->msg_data[idx] = mssg;
> + init_completion(>tx_cmpl[idx]);

reinit would be better.

>   chan->msg_count++;
>  
>   if (idx == MBOX_TX_QUEUE_LEN - 1)
> @@ -73,6 +74,7 @@ static void msg_submit(struct mbox_chan *chan)
>   idx += MBOX_TX_QUEUE_LEN - count;
>  
>   data = chan->msg_data[idx];
> + chan->tx_complete = >tx_cmpl[idx];
>  
>   if (chan->cl->tx_prepare)
>   chan->cl->tx_prepare(chan->cl, data);
> @@ -81,7 +83,8 @@ static void msg_submit(struct mbox_chan *chan)
>   if (!err) {
>   chan->active_req = data;
>   chan->msg_count--;
> - }
> + } else
> + chan->tx_complete = NULL;
>  exit:
>   spin_unlock_irqrestore(>lock, flags);
>  
> @@ -92,12 +95,15 @@ static void msg_submit(struct mbox_chan *chan)
>  
>  static void tx_tick(struct mbox_chan *chan, int r)
>  {
> + struct completion *tx_complete;
>   unsigned long flags;
>   void *mssg;
>  
>   spin_lock_irqsave(>lock, flags);
>   mssg = chan->active_req;
> + tx_complete = chan->tx_complete;
>   chan->active_req = NULL;
> + chan->tx_complete = NULL;
>   spin_unlock_irqrestore(>lock, flags);
>  
>   /* Submit next message */
> @@ -111,7 +117,7 @@ static void tx_tick(struct mbox_chan *chan, int r)
>   chan->cl->tx_done(chan->cl, mssg, r);
>  
>   if (r != -ETIME && chan->cl->tx_block)
> - complete(>tx_complete);
> + complete(tx_complete);
>  }
>  
>  static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
> @@ -272,7 +278,7 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg)
>   else
>   wait = msecs_to_jiffies(chan->cl->tx_tout);
>  
> - ret = wait_for_completion_timeout(>tx_complete, wait);
> + ret = wait_for_completion_timeout(>tx_cmpl[t], wait);
>   if (ret == 0) {
>   t = -ETIME;
>   tx_tick(chan, t);
> @@ -348,7 +354,7 @@ struct mbox_chan *mbox_request_channel(struct mbox_client 
> *cl, int index)
>   chan->msg_count = 0;
>   chan->active_req = NULL;
>   chan->cl = cl;
> - init_completion(>tx_complete);
> + chan->tx_complete = NULL;
>  
>   if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
>   chan->txdone_method |= TXDONE_BY_ACK;
> @@ -414,6 +420,7 @@ void mbox_free_channel(struct mbox_chan *chan)
>   spin_lock_irqsave(>lock, flags);
>   chan->cl = NULL;
>   chan->active_req = NULL;
> + chan->tx_complete = NULL;
>   if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
>   chan->txdone_method = TXDONE_BY_POLL;
>  
> diff --git a/drivers/mailbox/omap-mailbox.c b/drivers/mailbox/omap-mailbox.c
> index c5e8b9c..99b0841 100644
> --- a/drivers/mailbox/omap-mailbox.c
> +++ b/drivers/mailbox/omap-mailbox.c
> @@ -449,7 +449,7 @@ struct mbox_chan *omap_mbox_request_channel(struct 
> mbox_client *cl,
>   

[PATCH] mailbox: fix completion order for blocking requests

[Jassi Brar]

Currently two threads, wait on blocking requests, could wake up for
completion of request of each other as ...

Thread#1(T1)   Thread#2(T2)
 mbox_send_message   mbox_send_message
|   |
V   |
add_to_rbuf(M1) V
| add_to_rbuf(M2)
|   |
|   V
V  msg_submit(picks M1)
msg_submit  |
|   V
V   wait_for_completion(on M2)
 wait_for_completion(on M1) |  (1st in waitQ)
|   (2nd in waitQ)  V
V   wake_up(on completion of M1)<--incorrect

 Fix this situaion by assigning completion structures to each queued
request, so that the threads could wait on their own completions.

Reported-by: Alexey Klimov 
Signed-off-by: Jassi Brar 
---
 drivers/mailbox/mailbox.c  | 15 +++
 drivers/mailbox/omap-mailbox.c |  2 +-
 drivers/mailbox/pcc.c  |  2 +-
 include/linux/mailbox_controller.h |  6 --
 4 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 9dfbf7e..e06c50c 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -41,6 +41,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
 
idx = chan->msg_free;
chan->msg_data[idx] = mssg;
+   init_completion(>tx_cmpl[idx]);
chan->msg_count++;
 
if (idx == MBOX_TX_QUEUE_LEN - 1)
@@ -73,6 +74,7 @@ static void msg_submit(struct mbox_chan *chan)
idx += MBOX_TX_QUEUE_LEN - count;
 
data = chan->msg_data[idx];
+   chan->tx_complete = >tx_cmpl[idx];
 
if (chan->cl->tx_prepare)
chan->cl->tx_prepare(chan->cl, data);
@@ -81,7 +83,8 @@ static void msg_submit(struct mbox_chan *chan)
if (!err) {
chan->active_req = data;
chan->msg_count--;
-   }
+   } else
+   chan->tx_complete = NULL;
 exit:
spin_unlock_irqrestore(>lock, flags);
 
@@ -92,12 +95,15 @@ static void msg_submit(struct mbox_chan *chan)
 
 static void tx_tick(struct mbox_chan *chan, int r)
 {
+   struct completion *tx_complete;
unsigned long flags;
void *mssg;
 
spin_lock_irqsave(>lock, flags);
mssg = chan->active_req;
+   tx_complete = chan->tx_complete;
chan->active_req = NULL;
+   chan->tx_complete = NULL;
spin_unlock_irqrestore(>lock, flags);
 
/* Submit next message */
@@ -111,7 +117,7 @@ static void tx_tick(struct mbox_chan *chan, int r)
chan->cl->tx_done(chan->cl, mssg, r);
 
if (r != -ETIME && chan->cl->tx_block)
-   complete(>tx_complete);
+   complete(tx_complete);
 }
 
 static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
@@ -272,7 +278,7 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg)
else
wait = msecs_to_jiffies(chan->cl->tx_tout);
 
-   ret = wait_for_completion_timeout(>tx_complete, wait);
+   ret = wait_for_completion_timeout(>tx_cmpl[t], wait);
if (ret == 0) {
t = -ETIME;
tx_tick(chan, t);
@@ -348,7 +354,7 @@ struct mbox_chan *mbox_request_channel(struct mbox_client 
*cl, int index)
chan->msg_count = 0;
chan->active_req = NULL;
chan->cl = cl;
-   init_completion(>tx_complete);
+   chan->tx_complete = NULL;
 
if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
chan->txdone_method |= TXDONE_BY_ACK;
@@ -414,6 +420,7 @@ void mbox_free_channel(struct mbox_chan *chan)
spin_lock_irqsave(>lock, flags);
chan->cl = NULL;
chan->active_req = NULL;
+   chan->tx_complete = NULL;
if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
chan->txdone_method = TXDONE_BY_POLL;
 
diff --git a/drivers/mailbox/omap-mailbox.c b/drivers/mailbox/omap-mailbox.c
index c5e8b9c..99b0841 100644
--- a/drivers/mailbox/omap-mailbox.c
+++ b/drivers/mailbox/omap-mailbox.c
@@ -449,7 +449,7 @@ struct mbox_chan *omap_mbox_request_channel(struct 
mbox_client *cl,
chan->msg_count = 0;
chan->active_req = NULL;
chan->cl = cl;
-   init_completion(>tx_complete);
+   chan->tx_complete = NULL;
spin_unlock_irqrestore(>lock, flags);
 
ret = chan->mbox->ops->startup(chan);
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index dd9ecd35..b26cc9c 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -263,7 +263,7 @@ struct mbox_chan *pcc_mbox_request_channel(struct 

[PATCH] mailbox: fix completion order for blocking requests

[Jassi Brar]

Currently two threads, wait on blocking requests, could wake up for
completion of request of each other as ...

Thread#1(T1)   Thread#2(T2)
 mbox_send_message   mbox_send_message
|   |
V   |
add_to_rbuf(M1) V
| add_to_rbuf(M2)
|   |
|   V
V  msg_submit(picks M1)
msg_submit  |
|   V
V   wait_for_completion(on M2)
 wait_for_completion(on M1) |  (1st in waitQ)
|   (2nd in waitQ)  V
V   wake_up(on completion of M1)<--incorrect

 Fix this situaion by assigning completion structures to each queued
request, so that the threads could wait on their own completions.

Reported-by: Alexey Klimov 
Signed-off-by: Jassi Brar 
---
 drivers/mailbox/mailbox.c  | 15 +++
 drivers/mailbox/omap-mailbox.c |  2 +-
 drivers/mailbox/pcc.c  |  2 +-
 include/linux/mailbox_controller.h |  6 --
 4 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 9dfbf7e..e06c50c 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -41,6 +41,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
 
idx = chan->msg_free;
chan->msg_data[idx] = mssg;
+   init_completion(>tx_cmpl[idx]);
chan->msg_count++;
 
if (idx == MBOX_TX_QUEUE_LEN - 1)
@@ -73,6 +74,7 @@ static void msg_submit(struct mbox_chan *chan)
idx += MBOX_TX_QUEUE_LEN - count;
 
data = chan->msg_data[idx];
+   chan->tx_complete = >tx_cmpl[idx];
 
if (chan->cl->tx_prepare)
chan->cl->tx_prepare(chan->cl, data);
@@ -81,7 +83,8 @@ static void msg_submit(struct mbox_chan *chan)
if (!err) {
chan->active_req = data;
chan->msg_count--;
-   }
+   } else
+   chan->tx_complete = NULL;
 exit:
spin_unlock_irqrestore(>lock, flags);
 
@@ -92,12 +95,15 @@ static void msg_submit(struct mbox_chan *chan)
 
 static void tx_tick(struct mbox_chan *chan, int r)
 {
+   struct completion *tx_complete;
unsigned long flags;
void *mssg;
 
spin_lock_irqsave(>lock, flags);
mssg = chan->active_req;
+   tx_complete = chan->tx_complete;
chan->active_req = NULL;
+   chan->tx_complete = NULL;
spin_unlock_irqrestore(>lock, flags);
 
/* Submit next message */
@@ -111,7 +117,7 @@ static void tx_tick(struct mbox_chan *chan, int r)
chan->cl->tx_done(chan->cl, mssg, r);
 
if (r != -ETIME && chan->cl->tx_block)
-   complete(>tx_complete);
+   complete(tx_complete);
 }
 
 static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
@@ -272,7 +278,7 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg)
else
wait = msecs_to_jiffies(chan->cl->tx_tout);
 
-   ret = wait_for_completion_timeout(>tx_complete, wait);
+   ret = wait_for_completion_timeout(>tx_cmpl[t], wait);
if (ret == 0) {
t = -ETIME;
tx_tick(chan, t);
@@ -348,7 +354,7 @@ struct mbox_chan *mbox_request_channel(struct mbox_client 
*cl, int index)
chan->msg_count = 0;
chan->active_req = NULL;
chan->cl = cl;
-   init_completion(>tx_complete);
+   chan->tx_complete = NULL;
 
if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
chan->txdone_method |= TXDONE_BY_ACK;
@@ -414,6 +420,7 @@ void mbox_free_channel(struct mbox_chan *chan)
spin_lock_irqsave(>lock, flags);
chan->cl = NULL;
chan->active_req = NULL;
+   chan->tx_complete = NULL;
if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
chan->txdone_method = TXDONE_BY_POLL;
 
diff --git a/drivers/mailbox/omap-mailbox.c b/drivers/mailbox/omap-mailbox.c
index c5e8b9c..99b0841 100644
--- a/drivers/mailbox/omap-mailbox.c
+++ b/drivers/mailbox/omap-mailbox.c
@@ -449,7 +449,7 @@ struct mbox_chan *omap_mbox_request_channel(struct 
mbox_client *cl,
chan->msg_count = 0;
chan->active_req = NULL;
chan->cl = cl;
-   init_completion(>tx_complete);
+   chan->tx_complete = NULL;
spin_unlock_irqrestore(>lock, flags);
 
ret = chan->mbox->ops->startup(chan);
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index dd9ecd35..b26cc9c 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -263,7 +263,7 @@ struct mbox_chan *pcc_mbox_request_channel(struct 
mbox_client *cl,
chan->msg_count = 0;