linux-kernel - Re: [PATCH RFC] mailbox: move controller timer to per-channel timers

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CABb+yY3sodFPTv186zZD462Sd8WojPb=Ukgn22_nvgLqEOmPsA@mail.gmail.com>
Date:   Wed, 31 May 2017 14:08:15 +0530
From:   Jassi Brar <jassisinghbrar@...il.com>
To:     Alexey Klimov <alexey.klimov@....com>
Cc:     Jassi Brar <jaswinder.singh@...aro.org>,
        Sudeep Holla <sudeep.holla@....com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC] mailbox: move controller timer to per-channel timers

On Tue, May 30, 2017 at 9:59 PM, Alexey Klimov <alexey.klimov@....com> wrote:

>
> I was able to come up with new version with only HR-timer spinlock in timer
> callback. That one seems a little bit better.
>
Yes, this is what I meant by simply using a lock. However the lock
around the last_tx_done() for all channels seems clumsy. To save a
lots of posts, I have modified your patch as follows. If it fixes the
issue please submit the revision. Thanks


iff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 4671f8a..e111571 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -23,6 +23,10 @@

 #include "mailbox.h"

+#define HRT_INACTIVE   0
+#define HRT_SCHEDULED  1
+#define HRT_RESCHEDULE 2
+
 static LIST_HEAD(mbox_cons);
 static DEFINE_MUTEX(con_mutex);

@@ -85,9 +89,18 @@ static void msg_submit(struct mbox_chan *chan)
 exit:
        spin_unlock_irqrestore(&chan->lock, flags);

-       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);
+       if (!err && (chan->txdone_method & TXDONE_BY_POLL)) {
+               spin_lock_irqsave(&chan->mbox->hrt_lock, flags);
+               if (chan->mbox->hrt_state == HRT_INACTIVE) {
+                       /* kick start the timer immediately to avoid delays */
+                       hrtimer_start(&chan->mbox->poll_hrt,
+                                       0, HRTIMER_MODE_REL);
+                       chan->mbox->hrt_state = HRT_SCHEDULED;
+               } else {
+                       chan->mbox->hrt_state = HRT_RESCHEDULE;
+               }
+               spin_unlock_irqrestore(&chan->mbox->hrt_lock, flags);
+       }
 }

 static void tx_tick(struct mbox_chan *chan, int r)
@@ -116,6 +129,8 @@ static enum hrtimer_restart txdone_hrtimer(struct
hrtimer *hrtimer)
        struct mbox_controller *mbox =
                container_of(hrtimer, struct mbox_controller, poll_hrt);
        bool txdone, resched = false;
+       enum hrtimer_restart ret;
+       unsigned long flags;
        int i;

        for (i = 0; i < mbox->num_chans; i++) {
@@ -130,11 +145,18 @@ static enum hrtimer_restart
txdone_hrtimer(struct hrtimer *hrtimer)
                }
        }

-       if (resched) {
+       spin_lock_irqsave(&mbox->hrt_lock, flags);
+       if (resched || mbox->hrt_state == HRT_RESCHEDULE) {
                hrtimer_forward_now(hrtimer, ms_to_ktime(mbox->txpoll_period));
-               return HRTIMER_RESTART;
+               mbox->hrt_state = HRT_SCHEDULED;
+               ret = HRTIMER_RESTART;
+       } else {
+               mbox->hrt_state = HRT_INACTIVE;
+               ret = HRTIMER_NORESTART;
        }
-       return HRTIMER_NORESTART;
+       spin_unlock_irqrestore(&mbox->hrt_lock, flags);
+
+       return ret;
 }

 /**
@@ -453,6 +475,8 @@ int mbox_controller_register(struct mbox_controller *mbox)
                txdone = TXDONE_BY_ACK;

        if (txdone == TXDONE_BY_POLL) {
+               mbox->hrt_state = HRT_INACTIVE;
+               spin_lock_init(&mbox->hrt_lock);
                hrtimer_init(&mbox->poll_hrt, CLOCK_MONOTONIC,
                             HRTIMER_MODE_REL);
                mbox->poll_hrt.function = txdone_hrtimer;
diff --git a/include/linux/mailbox_controller.h
b/include/linux/mailbox_controller.h
index 74deadb..c990301 100644
--- a/include/linux/mailbox_controller.h
+++ b/include/linux/mailbox_controller.h
@@ -69,6 +69,8 @@ struct mbox_chan_ops {
  * @of_xlate:          Controller driver specific mapping of channel via DT
  * @poll_hrt:          API private. hrtimer used to poll for TXDONE on all
  *                     channels.
+ * @hrt_state:         API private. Flag for current state of poll_hrt.
+ * @hrt_lock:          API private. Lock for poll_hrt's state machine.
  * @node:              API private. To hook into list of controllers.
  */
 struct mbox_controller {
@@ -83,6 +85,9 @@ struct mbox_controller {
                                      const struct of_phandle_args *sp);
        /* Internal to API */
        struct hrtimer poll_hrt;
+       int hrt_state;
+       spinlock_t hrt_lock;
+
        struct list_head node;
 };