On Thu 16 Nov 09:06 PST 2017, Jassi Brar wrote: > On Thu, Nov 16, 2017 at 10:21 PM, Jassi Brar <[email protected]> wrote: > > On Thu, Nov 16, 2017 at 11:01 AM, Bjorn Andersson > > <[email protected]> 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
