| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <1491499891-30135-1-git-send-email-alexey.klimov@arm.com>
Date: Thu, 6 Apr 2017 18:31:31 +0100
From: Alexey Klimov <alexey.klimov@....com>
To: jassisinghbrar@...il.com, sudeep.holla@....com
Cc: linux-kernel@...r.kernel.org, jaswinder.singh@...aro.org,
alexey.klimov@....com
Subject: [PATCH RFC] mailbox: move controller timer to per-channel timers
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 : [<ffff0000081039f0>] lr : [<ffff00000881b874>] 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 <alexey.klimov@....com>
---
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
Powered by blists - more mailing lists