diff mbox series

mailbox: txdone_method shouldn't always be reset

Message ID 20171116053126.28640-1-bjorn.andersson@linaro.org
State New
Headers show
Series mailbox: txdone_method shouldn't always be reset | expand

Commit Message

Bjorn Andersson Nov. 16, 2017, 5:31 a.m. UTC 
A client that knows how to drive txdone would temporarily "upgrade" the
method to TXDONE_BY_ACK. But with the introduction of commit 33cd7123ac0ba
("mailbox: reset txdone_method TXDONE_BY_POLL if client knows_txdone")
there is no longer a distinction between a channel in "upgraded" state
or a channel for a controller that only supports TXDONE_BY_ACK. So upon
freeing the channel it will be "downgraded" to TXDONE_BY_POLL.

But a channel that operates with the txdone method of TXDONE_BY_POLL
requires that the controller implements the last_tx_done callback and
that the associated hrtimer was initialized when the controller was
registered.

So the core now relies on the fact that subsequent calls to
mbox_request_channel() "upgrades" the channel to TXDONE_BY_ACK or it
will dereference the non-initialized hrtimer.

The intention of commit 33cd7123ac0ba ("mailbox: reset txdone_method
TXDONE_BY_POLL if client knows_txdone") is to not restart the hrtimer
when the channel is in an "upgraded" state. So this patch reverts the
commit, in order to never leave the channel is a invalid state, and
instead only start the timer when we're in the "non-upgraded" POLL
state.

Fixes: 33cd7123ac0ba ("mailbox: reset txdone_method TXDONE_BY_POLL if client knows_txdone")
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

---
 drivers/mailbox/mailbox.c | 6 +++---
 drivers/mailbox/pcc.c     | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

-- 
2.15.0

Comments

Jassi Brar Nov. 16, 2017, 4:51 p.m. UTC | #1 
On Thu, Nov 16, 2017 at 11:01 AM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> A client that knows how to drive txdone would temporarily "upgrade" the

> method to TXDONE_BY_ACK. But with the introduction of commit 33cd7123ac0ba

> ("mailbox: reset txdone_method TXDONE_BY_POLL if client knows_txdone")

> there is no longer a distinction between a channel in "upgraded" state

> or a channel for a controller that only supports TXDONE_BY_ACK. So upon

> freeing the channel it will be "downgraded" to TXDONE_BY_POLL.

>

> But a channel that operates with the txdone method of TXDONE_BY_POLL

> requires that the controller implements the last_tx_done callback and

> that the associated hrtimer was initialized when the controller was

> registered.

>

> So the core now relies on the fact that subsequent calls to

> mbox_request_channel() "upgrades" the channel to TXDONE_BY_ACK or it

> will dereference the non-initialized hrtimer.

>

I think we just need avoid the channel not running under POLL

Does this fix your issue?

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 674b35f..672e3ab 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -124,7 +124,8 @@ static enum hrtimer_restart txdone_hrtimer(struct
hrtimer *hrtimer)
        for (i = 0; i < mbox->num_chans; i++) {
                struct mbox_chan *chan = &mbox->chans[i];

-               if (chan->active_req && chan->cl) {
+               if (chan->active_req && chan->cl &&
+                   chan->txdone_method == TXDONE_BY_POLL) {
                        txdone = chan->mbox->ops->last_tx_done(chan);
                        if (txdone)
                                tx_tick(chan, 0);



Thanks.
Jassi Brar Nov. 16, 2017, 5:06 p.m. UTC | #2 
On Thu, Nov 16, 2017 at 10:21 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Thu, Nov 16, 2017 at 11:01 AM, Bjorn Andersson

> <bjorn.andersson@linaro.org> wrote:

>> A client that knows how to drive txdone would temporarily "upgrade" the

>> method to TXDONE_BY_ACK. But with the introduction of commit 33cd7123ac0ba

>> ("mailbox: reset txdone_method TXDONE_BY_POLL if client knows_txdone")

>> there is no longer a distinction between a channel in "upgraded" state

>> or a channel for a controller that only supports TXDONE_BY_ACK. So upon

>> freeing the channel it will be "downgraded" to TXDONE_BY_POLL.

>>

>> But a channel that operates with the txdone method of TXDONE_BY_POLL

>> requires that the controller implements the last_tx_done callback and

>> that the associated hrtimer was initialized when the controller was

>> registered.

>>

>> So the core now relies on the fact that subsequent calls to

>> mbox_request_channel() "upgrades" the channel to TXDONE_BY_ACK or it

>> will dereference the non-initialized hrtimer.

>>

> I think we just need avoid the channel not running under POLL

>

And also detect if the channel was originally driven under POLL before
mbox_request_channel.
So this is finally what I think we need.

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 674b35f..95e480e 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -124,7 +124,8 @@ static enum hrtimer_restart txdone_hrtimer(struct
hrtimer *hrtimer)
        for (i = 0; i < mbox->num_chans; i++) {
                struct mbox_chan *chan = &mbox->chans[i];

-               if (chan->active_req && chan->cl) {
+               if (chan->active_req && chan->cl &&
+                   chan->txdone_method == TXDONE_BY_POLL) {
                        txdone = chan->mbox->ops->last_tx_done(chan);
                        if (txdone)
                                tx_tick(chan, 0);
@@ -418,7 +419,7 @@ void mbox_free_channel(struct mbox_chan *chan)
        spin_lock_irqsave(&chan->lock, flags);
        chan->cl = NULL;
        chan->active_req = NULL;
-       if (chan->txdone_method == TXDONE_BY_ACK)
+       if (chan->txdone_method == TXDONE_BY_ACK && chan->mbox->txdone_poll)
                chan->txdone_method = TXDONE_BY_POLL;

        module_put(chan->mbox->dev->driver->owner);
Sudeep Holla Nov. 16, 2017, 5:24 p.m. UTC | #3 
(- Remove  Alexey Klimov's ARM id as he has left the company, adding his
personal id instead)

On 16/11/17 16:51, Jassi Brar wrote:
> On Thu, Nov 16, 2017 at 11:01 AM, Bjorn Andersson

> <bjorn.andersson@linaro.org> wrote:

>> A client that knows how to drive txdone would temporarily "upgrade" the

>> method to TXDONE_BY_ACK. But with the introduction of commit 33cd7123ac0ba

>> ("mailbox: reset txdone_method TXDONE_BY_POLL if client knows_txdone")

>> there is no longer a distinction between a channel in "upgraded" state

>> or a channel for a controller that only supports TXDONE_BY_ACK. So upon

>> freeing the channel it will be "downgraded" to TXDONE_BY_POLL.

>>

>> But a channel that operates with the txdone method of TXDONE_BY_POLL

>> requires that the controller implements the last_tx_done callback and

>> that the associated hrtimer was initialized when the controller was

>> registered.

>>

>> So the core now relies on the fact that subsequent calls to

>> mbox_request_channel() "upgrades" the channel to TXDONE_BY_ACK or it

>> will dereference the non-initialized hrtimer.

>>

> I think we just need avoid the channel not running under POLL

> 

> Does this fix your issue?

> 


Unfortunately only Alexey had fast reproducer of the issue reported with
original commit. I will try to run on my setup, but it was hard to
reproduce(mainly lack of firmware support for multiple virtual channels).
I will try my best to test this.

Sorry for not considering temporary "upgrade" use case.

-- 
Regards,
Sudeep
Bjorn Andersson Nov. 16, 2017, 5:42 p.m. UTC | #4 
On Thu 16 Nov 09:06 PST 2017, Jassi Brar wrote:

> On Thu, Nov 16, 2017 at 10:21 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:

> > On Thu, Nov 16, 2017 at 11:01 AM, Bjorn Andersson

> > <bjorn.andersson@linaro.org> wrote:

> >> A client that knows how to drive txdone would temporarily "upgrade" the

> >> method to TXDONE_BY_ACK. But with the introduction of commit 33cd7123ac0ba

> >> ("mailbox: reset txdone_method TXDONE_BY_POLL if client knows_txdone")

> >> there is no longer a distinction between a channel in "upgraded" state

> >> or a channel for a controller that only supports TXDONE_BY_ACK. So upon

> >> freeing the channel it will be "downgraded" to TXDONE_BY_POLL.

> >>

> >> But a channel that operates with the txdone method of TXDONE_BY_POLL

> >> requires that the controller implements the last_tx_done callback and

> >> that the associated hrtimer was initialized when the controller was

> >> registered.

> >>

> >> So the core now relies on the fact that subsequent calls to

> >> mbox_request_channel() "upgrades" the channel to TXDONE_BY_ACK or it

> >> will dereference the non-initialized hrtimer.

> >>

> > I think we just need avoid the channel not running under POLL

> >

> And also detect if the channel was originally driven under POLL before

> mbox_request_channel.

> So this is finally what I think we need.

> 

> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c

> index 674b35f..95e480e 100644

> --- a/drivers/mailbox/mailbox.c

> +++ b/drivers/mailbox/mailbox.c

> @@ -124,7 +124,8 @@ static enum hrtimer_restart txdone_hrtimer(struct

> hrtimer *hrtimer)

>         for (i = 0; i < mbox->num_chans; i++) {

>                 struct mbox_chan *chan = &mbox->chans[i];

> 

> -               if (chan->active_req && chan->cl) {

> +               if (chan->active_req && chan->cl &&

> +                   chan->txdone_method == TXDONE_BY_POLL) {


The hrtimer code will crash before reaching this point if the channel
wasn't TXDONE_BY_POLL when it was created, so this part is not needed.

>                         txdone = chan->mbox->ops->last_tx_done(chan);

>                         if (txdone)

>                                 tx_tick(chan, 0);

> @@ -418,7 +419,7 @@ void mbox_free_channel(struct mbox_chan *chan)

>         spin_lock_irqsave(&chan->lock, flags);

>         chan->cl = NULL;

>         chan->active_req = NULL;

> -       if (chan->txdone_method == TXDONE_BY_ACK)

> +       if (chan->txdone_method == TXDONE_BY_ACK && chan->mbox->txdone_poll)


This should be enough, but the logic here is not obvious. This relies on
the fact that a mbox with txdone_poll would have been initialized as
txdone = POLL and that txdone now being ACK would imply that it was
"upgraded" in the request path.

So at least this would have to be captured in a comment.

(Using a bitmask with both POLL | ACK set did capture this in a more
direct way, but obviously wasn't clear to Sudeep)

>                 chan->txdone_method = TXDONE_BY_POLL;

> 

>         module_put(chan->mbox->dev->driver->owner);


Regards,
Bjorn
Bjorn Andersson Nov. 17, 2017, 7:04 a.m. UTC | #5 
On Thu 16 Nov 22:47 PST 2017, Jassi Brar wrote:
> On 16 Nov 2017 23:12, "Bjorn Andersson" <bjorn.andersson@linaro.org> wrote:

> On Thu 16 Nov 09:06 PST 2017, Jassi Brar wrote:

> > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c

> > index 674b35f..95e480e 100644

> > --- a/drivers/mailbox/mailbox.c

> > +++ b/drivers/mailbox/mailbox.c

> > @@ -124,7 +124,8 @@ static enum hrtimer_restart txdone_hrtimer(struct

> > hrtimer *hrtimer)

> >         for (i = 0; i < mbox->num_chans; i++) {

> >                 struct mbox_chan *chan = &mbox->chans[i];

> >

> > -               if (chan->active_req && chan->cl) {

> > +               if (chan->active_req && chan->cl &&

> > +                   chan->txdone_method == TXDONE_BY_POLL) {

> 

> The hrtimer code will crash before reaching this point if the channel

> wasn't TXDONE_BY_POLL when it was created, so this part is not needed.

> 

> 

> We have one timer for all channels of a controller. While this channel may

> be run by ACK, some other might need to be POLLed. And we want to avoid

> polling this channel.

> 


Oh, you're right.

But the fact that the timer function will poll channels that are
"upgraded" to ACK is a separate issue.

> 

> >                         txdone = chan->mbox->ops->last_tx_done(chan);

> >                         if (txdone)

> >                                 tx_tick(chan, 0);

> > @@ -418,7 +419,7 @@ void mbox_free_channel(struct mbox_chan *chan)

> >         spin_lock_irqsave(&chan->lock, flags);

> >         chan->cl = NULL;

> >         chan->active_req = NULL;

> > -       if (chan->txdone_method == TXDONE_BY_ACK)

> > +       if (chan->txdone_method == TXDONE_BY_ACK &&

> chan->mbox->txdone_poll)

> 

> This should be enough, but the logic here is not obvious. This relies on

> the fact that a mbox with txdone_poll would have been initialized as

> txdone = POLL and that txdone now being ACK would imply that it was

> "upgraded" in the request path.

> 

> So at least this would have to be captured in a comment.

> 

> 

> Sure I'll add a comment.

> 


Thanks

Regards,
Bjorn
Sudeep Holla Nov. 17, 2017, 9:51 a.m. UTC | #6 
On 17/11/17 07:04, Bjorn Andersson wrote:
> On Thu 16 Nov 22:47 PST 2017, Jassi Brar wrote:

>> On 16 Nov 2017 23:12, "Bjorn Andersson" <bjorn.andersson@linaro.org> wrote:

>> On Thu 16 Nov 09:06 PST 2017, Jassi Brar wrote:

>>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c

>>> index 674b35f..95e480e 100644

>>> --- a/drivers/mailbox/mailbox.c

>>> +++ b/drivers/mailbox/mailbox.c

>>> @@ -124,7 +124,8 @@ static enum hrtimer_restart txdone_hrtimer(struct

>>> hrtimer *hrtimer)

>>>         for (i = 0; i < mbox->num_chans; i++) {

>>>                 struct mbox_chan *chan = &mbox->chans[i];

>>>

>>> -               if (chan->active_req && chan->cl) {

>>> +               if (chan->active_req && chan->cl &&

>>> +                   chan->txdone_method == TXDONE_BY_POLL) {

>>

>> The hrtimer code will crash before reaching this point if the channel

>> wasn't TXDONE_BY_POLL when it was created, so this part is not needed.

>>

>>

>> We have one timer for all channels of a controller. While this channel may

>> be run by ACK, some other might need to be POLLed. And we want to avoid

>> polling this channel.

>>

> 

> Oh, you're right.

> 

> But the fact that the timer function will poll channels that are

> "upgraded" to ACK is a separate issue.

> 


Ah right, I recall now that's the reason Alexey had patch introducing
timer per channel.

-- 
Regards,
Sudeep
Jassi Brar Nov. 17, 2017, 12:40 p.m. UTC | #7 
On Fri, Nov 17, 2017 at 12:34 PM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Thu 16 Nov 22:47 PST 2017, Jassi Brar wrote:

>> On 16 Nov 2017 23:12, "Bjorn Andersson" <bjorn.andersson@linaro.org> wrote:

>> On Thu 16 Nov 09:06 PST 2017, Jassi Brar wrote:

>> > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c

>> > index 674b35f..95e480e 100644

>> > --- a/drivers/mailbox/mailbox.c

>> > +++ b/drivers/mailbox/mailbox.c

>> > @@ -124,7 +124,8 @@ static enum hrtimer_restart txdone_hrtimer(struct

>> > hrtimer *hrtimer)

>> >         for (i = 0; i < mbox->num_chans; i++) {

>> >                 struct mbox_chan *chan = &mbox->chans[i];

>> >

>> > -               if (chan->active_req && chan->cl) {

>> > +               if (chan->active_req && chan->cl &&

>> > +                   chan->txdone_method == TXDONE_BY_POLL) {

>>

>> The hrtimer code will crash before reaching this point if the channel

>> wasn't TXDONE_BY_POLL when it was created, so this part is not needed.

>>

>>

>> We have one timer for all channels of a controller. While this channel may

>> be run by ACK, some other might need to be POLLed. And we want to avoid

>> polling this channel.

>>

>

> Oh, you're right.

>

> But the fact that the timer function will poll channels that are

> "upgraded" to ACK is a separate issue.

>

After adding that extra check, the timer function never polls such a channel.

thnx
diff mbox series

Patch

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 674b35f402f5..da8d666a8c56 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -85,7 +85,7 @@  static void msg_submit(struct mbox_chan *chan)
 exit:
 	spin_unlock_irqrestore(&chan->lock, flags);
 
-	if (!err && (chan->txdone_method & TXDONE_BY_POLL))
+	if (!err && chan->txdone_method == TXDONE_BY_POLL)
 		/* kick start the timer immediately to avoid delays */
 		hrtimer_start(&chan->mbox->poll_hrt, 0, HRTIMER_MODE_REL);
 }
@@ -351,7 +351,7 @@  struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index)
 	init_completion(&chan->tx_complete);
 
 	if (chan->txdone_method	== TXDONE_BY_POLL && cl->knows_txdone)
-		chan->txdone_method = TXDONE_BY_ACK;
+		chan->txdone_method |= TXDONE_BY_ACK;
 
 	spin_unlock_irqrestore(&chan->lock, flags);
 
@@ -418,7 +418,7 @@  void mbox_free_channel(struct mbox_chan *chan)
 	spin_lock_irqsave(&chan->lock, flags);
 	chan->cl = NULL;
 	chan->active_req = NULL;
-	if (chan->txdone_method == TXDONE_BY_ACK)
+	if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
 		chan->txdone_method = TXDONE_BY_POLL;
 
 	module_put(chan->mbox->dev->driver->owner);
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 3ef7f036ceea..e5a69679cfa2 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -265,7 +265,7 @@  struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl,
 	init_completion(&chan->tx_complete);
 
 	if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
-		chan->txdone_method = TXDONE_BY_ACK;
+		chan->txdone_method |= TXDONE_BY_ACK;
 
 	spin_unlock_irqrestore(&chan->lock, flags);
 
@@ -311,7 +311,7 @@  void pcc_mbox_free_channel(struct mbox_chan *chan)
 	spin_lock_irqsave(&chan->lock, flags);
 	chan->cl = NULL;
 	chan->active_req = NULL;
-	if (chan->txdone_method == TXDONE_BY_ACK)
+	if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
 		chan->txdone_method = TXDONE_BY_POLL;
 
 	spin_unlock_irqrestore(&chan->lock, flags);