Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934382AbdDFRcE (ORCPT ); Thu, 6 Apr 2017 13:32:04 -0400 Received: from foss.arm.com ([217.140.101.70]:46722 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934259AbdDFRb4 (ORCPT ); Thu, 6 Apr 2017 13:31:56 -0400 From: Alexey Klimov To: jassisinghbrar@gmail.com, sudeep.holla@arm.com Cc: linux-kernel@vger.kernel.org, jaswinder.singh@linaro.org, alexey.klimov@arm.com Subject: [PATCH RFC] mailbox: move controller timer to per-channel timers Date: Thu, 6 Apr 2017 18:31:31 +0100 Message-Id: <1491499891-30135-1-git-send-email-alexey.klimov@arm.com> X-Mailer: git-send-email 1.9.1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6711 Lines: 186 When mailbox controller provides two or more channels and they are actively used by mailbox client(s) it's very easy to trigger the warning in hrtimer_forward(): [ 247.853060] WARNING: CPU: 6 PID: 0 at kernel/time/hrtimer.c:805 hrtimer_forward+0x88/0xd8 [ 247.853549] Modules linked in: [ 247.853907] CPU: 6 PID: 0 Comm: swapper/6 Tainted: G W 4.11.0-rc2-00362-g93afaa4513bb-dirty #13 [ 247.854472] Hardware name: linux,dummy-virt (DT) [ 247.854699] task: ffff80001d89d780 task.stack: ffff80001d8c4000 [ 247.854999] PC is at hrtimer_forward+0x88/0xd8 [ 247.855280] LR is at txdone_hrtimer+0xd4/0xf8 [ 247.855551] pc : [] lr : [] pstate: 200001c5 [ 247.855857] sp : ffff80001efbdeb0 [ 247.856072] x29: ffff80001efbdeb0 x28: ffff80001efc3140 [ 247.856358] x27: ffff00000881b7a0 x26: 00000039ac93e8b6 [ 247.856604] x25: ffff000008e756be x24: ffff80001c4a1348 [ 247.856882] x23: 0000000000000001 x22: 00000000000000f8 [ 247.857189] x21: ffff80001c4a1318 x20: ffff80001d327110 [ 247.857509] x19: 00000000000f4240 x18: 0000000000000030 [ 247.857808] x17: 0000ffffaecdf370 x16: ffff0000081ccc80 [ 247.858000] x15: 0000000000000010 x14: 00000000fffffff0 [ 247.858186] x13: ffff000008f488e0 x12: 000000000002e3eb [ 247.858381] x11: ffff000008979690 x10: 0000000000000000 [ 247.858573] x9 : 0000000000000001 x8 : ffff80001efc66e0 [ 247.858758] x7 : ffff80001efc6708 x6 : 00000005be7732f2 [ 247.858943] x5 : 0000000000000001 x4 : ffff80001c4a1348 [ 247.859130] x3 : 00000039ac94952a x2 : 00000000000f4240 [ 247.859315] x1 : 00000039ac98243c x0 : 0000000000038f12 [ 247.859582] ---[ end trace d61812426ec3c30b ]--- To fix this current patch migrates hr timers to be per-channel instead of using only one timer per-controller. The racy reading of chan->active_req is removed from timer callback since it's not done under spinlock and it seems that timer-based polling logic shouldn't rely on this. Timer is started on the channel when new message is submitted to controller and timer will continue to reschedule itself until it detects that controller acked a message by using ->last_tx_done(), after acknowledge from controller timer callback will trigger mailbox tx state machine. Signed-off-by: Alexey Klimov --- Hi Jassi and Sudeep, could you please take a look at this? The only thing that I don't know how to fix here is that if controller reports timers-based polling and client supports acknowledgement of message transfer then the scenario looks like it theoretically possible to call tx_tick() from two points: from timer callback and from a client. This is setup in mbox_request_channel() in such lines: if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone) chan->txdone_method |= TXDONE_BY_ACK; Thanks, Alexey drivers/mailbox/mailbox.c | 45 ++++++++++++++++++-------------------- include/linux/mailbox_controller.h | 2 +- 2 files changed, 22 insertions(+), 25 deletions(-) diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c index 4671f8a12872..124d2a64de83 100644 --- a/drivers/mailbox/mailbox.c +++ b/drivers/mailbox/mailbox.c @@ -87,7 +87,7 @@ static void msg_submit(struct mbox_chan *chan) 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); + hrtimer_start(&chan->poll_hrt, 0, HRTIMER_MODE_REL); } static void tx_tick(struct mbox_chan *chan, int r) @@ -113,25 +113,21 @@ static void tx_tick(struct mbox_chan *chan, int r) static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer) { - struct mbox_controller *mbox = - container_of(hrtimer, struct mbox_controller, poll_hrt); + struct mbox_chan *chan = + container_of(hrtimer, struct mbox_chan, poll_hrt); bool txdone, resched = false; - int i; - - for (i = 0; i < mbox->num_chans; i++) { - struct mbox_chan *chan = &mbox->chans[i]; - if (chan->active_req && chan->cl) { - txdone = chan->mbox->ops->last_tx_done(chan); - if (txdone) - tx_tick(chan, 0); - else - resched = true; - } + if (chan->cl) { + txdone = chan->mbox->ops->last_tx_done(chan); + if (txdone) + tx_tick(chan, 0); + else + resched = true; } if (resched) { - hrtimer_forward_now(hrtimer, ms_to_ktime(mbox->txpoll_period)); + hrtimer_forward_now(hrtimer, + ms_to_ktime(chan->mbox->txpoll_period)); return HRTIMER_RESTART; } return HRTIMER_NORESTART; @@ -350,6 +346,12 @@ struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index) if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone) chan->txdone_method |= TXDONE_BY_ACK; + if (chan->txdone_method == TXDONE_BY_POLL) { + hrtimer_init(&chan->poll_hrt, CLOCK_MONOTONIC, + HRTIMER_MODE_REL); + chan->poll_hrt.function = txdone_hrtimer; + } + spin_unlock_irqrestore(&chan->lock, flags); ret = chan->mbox->ops->startup(chan); @@ -411,6 +413,10 @@ 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_POLL) + hrtimer_cancel(&chan->poll_hrt); + if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK)) chan->txdone_method = TXDONE_BY_POLL; @@ -452,12 +458,6 @@ int mbox_controller_register(struct mbox_controller *mbox) else /* It has to be ACK then */ txdone = TXDONE_BY_ACK; - if (txdone == TXDONE_BY_POLL) { - hrtimer_init(&mbox->poll_hrt, CLOCK_MONOTONIC, - HRTIMER_MODE_REL); - mbox->poll_hrt.function = txdone_hrtimer; - } - for (i = 0; i < mbox->num_chans; i++) { struct mbox_chan *chan = &mbox->chans[i]; @@ -496,9 +496,6 @@ void mbox_controller_unregister(struct mbox_controller *mbox) for (i = 0; i < mbox->num_chans; i++) mbox_free_channel(&mbox->chans[i]); - if (mbox->txdone_poll) - hrtimer_cancel(&mbox->poll_hrt); - mutex_unlock(&con_mutex); } EXPORT_SYMBOL_GPL(mbox_controller_unregister); diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h index 74deadb42d76..f5e5a6e8ccfc 100644 --- a/include/linux/mailbox_controller.h +++ b/include/linux/mailbox_controller.h @@ -82,7 +82,6 @@ struct mbox_controller { struct mbox_chan *(*of_xlate)(struct mbox_controller *mbox, const struct of_phandle_args *sp); /* Internal to API */ - struct hrtimer poll_hrt; struct list_head node; }; @@ -123,6 +122,7 @@ struct mbox_chan { unsigned msg_count, msg_free; void *msg_data[MBOX_TX_QUEUE_LEN]; spinlock_t lock; /* Serialise access to the channel */ + struct hrtimer poll_hrt; void *con_priv; }; -- 2.11.0