Re: [PATCH] mailbox: txdone_method shouldn't always be reset

[Bjorn Andersson <bjorn.andersson@linaro.org>]

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