From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936134AbdKPRmW (ORCPT ); Thu, 16 Nov 2017 12:42:22 -0500 Received: from mail-pg0-f45.google.com ([74.125.83.45]:48175 "EHLO mail-pg0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S935369AbdKPRmO (ORCPT ); Thu, 16 Nov 2017 12:42:14 -0500 X-Google-Smtp-Source: AGs4zMY35LHD3jmpmDnYnncYByyDbnFV3Yh+GBrBvAanJ5d6AdTq49IDmHBHluHg51q15dc6mii8Dw== Date: Thu, 16 Nov 2017 09:42:11 -0800 From: Bjorn Andersson To: Jassi Brar Cc: Sudeep Holla , Linux Kernel Mailing List , Alexey Klimov Subject: Re: [PATCH] mailbox: txdone_method shouldn't always be reset Message-ID: <20171116174211.GQ28761@minitux> References: <20171116053126.28640-1-bjorn.andersson@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu 16 Nov 09:06 PST 2017, Jassi Brar wrote: > On Thu, Nov 16, 2017 at 10:21 PM, Jassi Brar wrote: > > On Thu, Nov 16, 2017 at 11:01 AM, Bjorn Andersson > > 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