[PATCH 1/2] mailbox: switch to hrtimer for tx_complete polling

[PATCH 1/2] mailbox: switch to hrtimer for tx_complete polling

[Sudeep Holla]

After submitting the message via msg_submit in mbox_send_message, we
wait until it has been transmitted and the remote acknowledges it using
some status register which controller can read(i.e. TXDONE_BY_POLL).

However, since the timer used here to handle that polling for the
transmit completion polling is jiffy based, we might end-up waiting
for atleast a jiffy even though the response for that message from the
remote is received via interrupt and processed in relatively smaller
time granularity.

Since some mailbox controller can operate at much lesser granularity
compared to jiffy, switch to the hrtimer in the mailbox core.

Cc: Jassi Brar <jassisinghbrar@gmail.com>
Reported-and-suggested-by: Juri Lelli <Juri.Lelli@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/mailbox.c          | 27 +++++++++++++++------------
 include/linux/mailbox_controller.h |  7 ++++---
 2 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index c7fdb57fd166..6a4811f85705 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -26,8 +26,6 @@
 static LIST_HEAD(mbox_cons);
 static DEFINE_MUTEX(con_mutex);
 
-static void poll_txdone(unsigned long data);
-
 static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
 {
 	int idx;
@@ -88,7 +86,9 @@ static void msg_submit(struct mbox_chan *chan)
 	spin_unlock_irqrestore(&chan->lock, flags);
 
 	if (!err && (chan->txdone_method & TXDONE_BY_POLL))
-		poll_txdone((unsigned long)chan->mbox);
+		/* kick start the timer immediately to avoid delays */
+		hrtimer_start(&chan->mbox->poll_hrt, ktime_set(0, 0),
+			      HRTIMER_MODE_REL);
 }
 
 static void tx_tick(struct mbox_chan *chan, int r)
@@ -112,9 +112,10 @@ static void tx_tick(struct mbox_chan *chan, int r)
 		complete(&chan->tx_complete);
 }
 
-static void poll_txdone(unsigned long data)
+static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
 {
-	struct mbox_controller *mbox = (struct mbox_controller *)data;
+	struct mbox_controller *mbox =
+		container_of(hrtimer, struct mbox_controller, poll_hrt);
 	bool txdone, resched = false;
 	int i;
 
@@ -130,9 +131,11 @@ static void poll_txdone(unsigned long data)
 		}
 	}
 
-	if (resched)
-		mod_timer(&mbox->poll, jiffies +
-				msecs_to_jiffies(mbox->txpoll_period));
+	if (resched) {
+		hrtimer_forward_now(hrtimer, ms_to_ktime(mbox->txpoll_period));
+		return HRTIMER_RESTART;
+	}
+	return HRTIMER_NORESTART;
 }
 
 /**
@@ -451,9 +454,9 @@ int mbox_controller_register(struct mbox_controller *mbox)
 		txdone = TXDONE_BY_ACK;
 
 	if (txdone == TXDONE_BY_POLL) {
-		mbox->poll.function = &poll_txdone;
-		mbox->poll.data = (unsigned long)mbox;
-		init_timer(&mbox->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++) {
@@ -495,7 +498,7 @@ void mbox_controller_unregister(struct mbox_controller *mbox)
 		mbox_free_channel(&mbox->chans[i]);
 
 	if (mbox->txdone_poll)
-		del_timer_sync(&mbox->poll);
+		hrtimer_cancel(&mbox->poll_hrt);
 
 	mutex_unlock(&con_mutex);
 }
diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
index 68c42454439b..74deadb42d76 100644
--- a/include/linux/mailbox_controller.h
+++ b/include/linux/mailbox_controller.h
@@ -9,7 +9,7 @@
 
 #include <linux/of.h>
 #include <linux/types.h>
-#include <linux/timer.h>
+#include <linux/hrtimer.h>
 #include <linux/device.h>
 #include <linux/completion.h>
 
@@ -67,7 +67,8 @@ struct mbox_chan_ops {
  * @txpoll_period:	If 'txdone_poll' is in effect, the API polls for
  *			last TX's status after these many millisecs
  * @of_xlate:		Controller driver specific mapping of channel via DT
- * @poll:		API private. Used to poll for TXDONE on all channels.
+ * @poll_hrt:		API private. hrtimer used to poll for TXDONE on all
+ *			channels.
  * @node:		API private. To hook into list of controllers.
  */
 struct mbox_controller {
@@ -81,7 +82,7 @@ struct mbox_controller {
 	struct mbox_chan *(*of_xlate)(struct mbox_controller *mbox,
 				      const struct of_phandle_args *sp);
 	/* Internal to API */
-	struct timer_list poll;
+	struct hrtimer poll_hrt;
 	struct list_head node;
 };
 
-- 
1.9.1

[PATCH 2/2] mailbox: arm_mhu: reduce txpoll_period from 10ms to 1 ms

[Sudeep Holla]

Since the mailbox core users hrtimers now, it can handle much higher
resolutions. We can reduce the txpoll_period to 1 ms as the transmit
usually takes just few microseconds.

Cc: Jassi Brar <jassisinghbrar@gmail.com>
Reported-and-suggested-by: Juri Lelli <Juri.Lelli@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/arm_mhu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c
index d9e99f981aa9..438c2896ef22 100644
--- a/drivers/mailbox/arm_mhu.c
+++ b/drivers/mailbox/arm_mhu.c
@@ -148,7 +148,7 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 	mhu->mbox.ops = &mhu_ops;
 	mhu->mbox.txdone_irq = false;
 	mhu->mbox.txdone_poll = true;
-	mhu->mbox.txpoll_period = 10;
+	mhu->mbox.txpoll_period = 1;
 
 	amba_set_drvdata(adev, mhu);
 
-- 
1.9.1

Re: [PATCH 1/2] mailbox: switch to hrtimer for tx_complete polling

[Jassi Brar]

On Wed, Jul 22, 2015 at 5:58 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:

> we might end-up waiting
> for atleast a jiffy even though the response for that message from the
> remote is received via interrupt and processed in relatively smaller
> time granularity.
>
That is wrong.

 If the controller supports TX interrupt it should set txdone_irq,
which prevents polling i.e, controller driver calls mbox_chan_txdone.

 If the controller doesn't support TX interrupt but the client
receives some ack packet, then the client should set knows_txdone and
call mbox_client_txdone. Again you don't have to wait on polling.

 If there's neither TX interrupt nor some ack packet, only then it has
to rely on polling. In which case, I doubt if we can desire some
functionality that requires sub-jiffy notification of TX_done.

Thanks.

Re: [PATCH 1/2] mailbox: switch to hrtimer for tx_complete polling

[Sudeep Holla]

On 24/07/15 06:02, Jassi Brar wrote:
> On Wed, Jul 22, 2015 at 5:58 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>> we might end-up waiting
>> for atleast a jiffy even though the response for that message from the
>> remote is received via interrupt and processed in relatively smaller
>> time granularity.
>>
> That is wrong.
>

No see below.

>   If the controller supports TX interrupt it should set txdone_irq,
> which prevents polling i.e, controller driver calls mbox_chan_txdone.
>
>   If the controller doesn't support TX interrupt but the client
> receives some ack packet, then the client should set knows_txdone and
> call mbox_client_txdone. Again you don't have to wait on polling.
>

Sorry if I was not clear in the commit message, but I thought I did
mention TXDONE_BY_POLL. The case I am referring is definitely not
TXDONE_BY_IRQ or TXDONE_BY_ACK.

>   If there's neither TX interrupt nor some ack packet, only then it has
> to rely on polling. In which case, I doubt if we can desire some
> functionality that requires sub-jiffy notification of TX_done.
>

Can you elaborate on why do you think we can't desire some functionality
that requires sub-jiffy notification of TX_done ?

Few reasons I have in mind as why we need that:

1. The remote processor is capable of dealing with requests in orders of
    milliseconds. E.g. on Juno it reports the DVFS transition latency is
    1.2ms. Now if we report that to CPUFreq governor, we need to ensure
    that. With current jiffy based timer we see latencies > 10ms.

2. Because of that, under stress testing with multiple clients active at
    a time, I am seeing the mailbox buffer overflows quite easily just
    because it's blocked on Tx polling(almost 10x slower) and doesn't
    process new requests though the remote can handle.

3. Also there are efforts for scheduler-driven cpu frequency selection
    where the desired latency should be as low as possible. Juri(cc-ed)
    is working on that and reported this issue with mailbox core.

Hope this clarifies the reasons for switching to hrtimer.

Regards,
Sudeep

Re: [PATCH 1/2] mailbox: switch to hrtimer for tx_complete polling

[Jassi Brar]

On Fri, Jul 24, 2015 at 2:17 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> On 24/07/15 06:02, Jassi Brar wrote:
>>
>> On Wed, Jul 22, 2015 at 5:58 PM, Sudeep Holla <sudeep.holla@arm.com>
>> wrote:
>>
>>> we might end-up waiting
>>> for atleast a jiffy even though the response for that message from the
>>> remote is received via interrupt and processed in relatively smaller
>>> time granularity.
>>>
>> That is wrong.
>
> No see below.
>
>>   If the controller supports TX interrupt it should set txdone_irq,
>> which prevents polling i.e, controller driver calls mbox_chan_txdone.
>>
>>   If the controller doesn't support TX interrupt but the client
>> receives some ack packet, then the client should set knows_txdone and
>> call mbox_client_txdone. Again you don't have to wait on polling.
>>
>
> Sorry if I was not clear in the commit message, but I thought I did
> mention TXDONE_BY_POLL. The case I am referring is definitely not
> TXDONE_BY_IRQ or TXDONE_BY_ACK.
>
That statement is still wrong. The TXDONE_BY_POLL modifier does't make it right.

Anyways, I see you meant the 3rd case of neither IRQ nor ACK.

> Few reasons I have in mind as why we need that:
>
> 1. The remote processor is capable of dealing with requests in orders of
>    milliseconds. E.g. on Juno it reports the DVFS transition latency is
>    1.2ms. Now if we report that to CPUFreq governor, we need to ensure
>    that. With current jiffy based timer we see latencies > 10ms.
>
Can I see the code somewhere?
It seems your remote doesn't send some protocol level 'ack' packet
replying if the command was successfully executed or not. That means
Linux can't differentiate successful execution of the command from a
silent failure (remote still has to set the TX_done flag to make way
for next messages). From Linux' POV our best bet seems to be to submit
the request and simply assume it done. So probably do async rather
than blocking.

> 2. Because of that, under stress testing with multiple clients active at
>    a time, I am seeing the mailbox buffer overflows quite easily just
>    because it's blocked on Tx polling(almost 10x slower) and doesn't
>    process new requests though the remote can handle.
>
Yeah this situation may arise. The following fix is needed regardless, I think.

============================
diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 07fd507..a1dded9 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -64,7 +64,12 @@ static void msg_submit(struct mbox_chan *chan)

  spin_lock_irqsave(&chan->lock, flags);

- if (!chan->msg_count || chan->active_req)
+ if (chan->active_req) {
+ err = 0;
+ goto exit;
+ }
+
+ if (!chan->msg_count)
  goto exit;

  count = chan->msg_count;
============================

> 3. Also there are efforts for scheduler-driven cpu frequency selection
>    where the desired latency should be as low as possible. Juri(cc-ed)
>    is working on that and reported this issue with mailbox core.
>
Sure, latency should be as low as possible. Sounds similar to (1). Was
it reported on some mailing list?

> Hope this clarifies the reasons for switching to hrtimer.
>
I am not against using hrtimer, just need to make sure we don't simply
suppress the symptoms of wrong implementation.

Thanks.

Re: [PATCH 1/2] mailbox: switch to hrtimer for tx_complete polling

[Sudeep Holla]

On 27/07/15 04:26, Jassi Brar wrote:
> On Fri, Jul 24, 2015 at 2:17 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> On 24/07/15 06:02, Jassi Brar wrote:
>>>
>>> On Wed, Jul 22, 2015 at 5:58 PM, Sudeep Holla <sudeep.holla@arm.com>
>>> wrote:
>>>
>>>> we might end-up waiting
>>>> for atleast a jiffy even though the response for that message from the
>>>> remote is received via interrupt and processed in relatively smaller
>>>> time granularity.
>>>>
>>> That is wrong.
>>
>> No see below.
>>
>>>    If the controller supports TX interrupt it should set txdone_irq,
>>> which prevents polling i.e, controller driver calls mbox_chan_txdone.
>>>
>>>    If the controller doesn't support TX interrupt but the client
>>> receives some ack packet, then the client should set knows_txdone and
>>> call mbox_client_txdone. Again you don't have to wait on polling.
>>>
>>
>> Sorry if I was not clear in the commit message, but I thought I did
>> mention TXDONE_BY_POLL. The case I am referring is definitely not
>> TXDONE_BY_IRQ or TXDONE_BY_ACK.
>>
> That statement is still wrong. The TXDONE_BY_POLL modifier does't make it right.
>

I am fine to modify/clarify that statement.

> Anyways, I see you meant the 3rd case of neither IRQ nor ACK.
>

Yes the remote indicates by setting a flag in status register.

>> Few reasons I have in mind as why we need that:
>>
>> 1. The remote processor is capable of dealing with requests in orders of
>>     milliseconds. E.g. on Juno it reports the DVFS transition latency is
>>     1.2ms. Now if we report that to CPUFreq governor, we need to ensure
>>     that. With current jiffy based timer we see latencies > 10ms.
>>
> Can I see the code somewhere?

Part of Juno SCPI support series[1].

> It seems your remote doesn't send some protocol level 'ack' packet
> replying if the command was successfully executed or not. That means
> Linux can't differentiate successful execution of the command from a
> silent failure (remote still has to set the TX_done flag to make way
> for next messages).

Agreed and again I confirm the remote processor in question just sets
the flag always and correctly and doesn't use a protocol ACK. So I am
not sure the context of the above statement for this particular issue
under discussion.

> From Linux' POV our best bet seems to be to submit
> the request and simply assume it done. So probably do async rather
> than blocking.
>

How if it's not Tx is not based on protocol ACK packet. The timer is
only solution IMO for the case where Tx done is indicated by a status flag.

>> 2. Because of that, under stress testing with multiple clients active at
>>     a time, I am seeing the mailbox buffer overflows quite easily just
>>     because it's blocked on Tx polling(almost 10x slower) and doesn't
>>     process new requests though the remote can handle.
>>
> Yeah this situation may arise. The following fix is needed regardless, I think.
>

IIUC it just triggers poll_txdone when chan->active_req is set, but that
will anyway happen through timer, no ?

Anyway it's not the main topic of discussion here and can be taken up
separately.

> ============================
> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> index 07fd507..a1dded9 100644
> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -64,7 +64,12 @@ static void msg_submit(struct mbox_chan *chan)
>
>    spin_lock_irqsave(&chan->lock, flags);
>
> - if (!chan->msg_count || chan->active_req)
> + if (chan->active_req) {
> + err = 0;
> + goto exit;
> + }
> +
> + if (!chan->msg_count)
>    goto exit;
>
>    count = chan->msg_count;
> ============================
>
>> 3. Also there are efforts for scheduler-driven cpu frequency selection
>>     where the desired latency should be as low as possible. Juri(cc-ed)
>>     is working on that and reported this issue with mailbox core.
>>
> Sure, latency should be as low as possible. Sounds similar to (1). Was
> it reported on some mailing list?
>

No, I am reporting it now in the form of this patch (with the solution).

>> Hope this clarifies the reasons for switching to hrtimer.
>>
> I am not against using hrtimer, just need to make sure we don't simply
> suppress the symptoms of wrong implementation.

Agreed, and that's a valid concern. So far based on the testing and
benchmarking done so far, we don't think this patch is suppressing
anything incorrectly.

If you still have concerns with this solution, please explain them here
so that we can discuss and come to conclusion and the issue is fixed.

Regards,
Sudeep

[1] https://lkml.org/lkml/2015/7/23/270

Re: [PATCH 1/2] mailbox: switch to hrtimer for tx_complete polling

[Jassi Brar]

On Mon, Jul 27, 2015 at 3:18 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> On 27/07/15 04:26, Jassi Brar wrote:
>>
>>>>
>>>>> we might end-up waiting
>>>>> for atleast a jiffy even though the response for that message from the
>>>>> remote is received via interrupt and processed in relatively smaller
>>>>> time granularity.
>>>>>
>>>> That is wrong.
>>>
>>>
>>> No see below.
>>>
>>>>    If the controller supports TX interrupt it should set txdone_irq,
>>>> which prevents polling i.e, controller driver calls mbox_chan_txdone.
>>>>
>>>>    If the controller doesn't support TX interrupt but the client
>>>> receives some ack packet, then the client should set knows_txdone and
>>>> call mbox_client_txdone. Again you don't have to wait on polling.
>>>>
>>>
>>> Sorry if I was not clear in the commit message, but I thought I did
>>> mention TXDONE_BY_POLL. The case I am referring is definitely not
>>> TXDONE_BY_IRQ or TXDONE_BY_ACK.
>>>
>> That statement is still wrong. The TXDONE_BY_POLL modifier does't make it
>> right.
>>
>
> I am fine to modify/clarify that statement.
>
>> Anyways, I see you meant the 3rd case of neither IRQ nor ACK.
>>
>
> Yes the remote indicates by setting a flag in status register.
>
However, looking at the arm_scpi.c the protocol does support
TXDONE_BY_ACK that is, every command has a reply packet telling if the
command was successful or failure. When you receive a reply, obviously
the command has already been received by the remote. Which is
mbox_client.knows_txdone or TXDONE_BY_ACK.

>> It seems your remote doesn't send some protocol level 'ack' packet
>> replying if the command was successfully executed or not. That means
>> Linux can't differentiate successful execution of the command from a
>> silent failure (remote still has to set the TX_done flag to make way
>> for next messages).
>
> Agreed and again I confirm the remote processor in question just sets
> the flag always and correctly and doesn't use a protocol ACK.
>
As I note above, the arm_scpi.c tells a different story.

>>> 2. Because of that, under stress testing with multiple clients active at
>>>     a time, I am seeing the mailbox buffer overflows quite easily just
>>>     because it's blocked on Tx polling(almost 10x slower) and doesn't
>>>     process new requests though the remote can handle.
>>>
>> Yeah this situation may arise. The following fix is needed regardless, I
>> think.
>>
>
> IIUC it just triggers poll_txdone when chan->active_req is set, but that
> will anyway happen through timer, no ?
>
No. It polls whenever a new message is queued so that we are not
waiting on a completed but sleeping task.

> Anyway it's not the main topic of discussion here and can be taken up
> separately.
>
>> ============================
>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>> index 07fd507..a1dded9 100644
>> --- a/drivers/mailbox/mailbox.c
>> +++ b/drivers/mailbox/mailbox.c
>> @@ -64,7 +64,12 @@ static void msg_submit(struct mbox_chan *chan)
>>
>>    spin_lock_irqsave(&chan->lock, flags);
>>
>> - if (!chan->msg_count || chan->active_req)
>> + if (chan->active_req) {
>> + err = 0;
>> + goto exit;
>> + }
>> +
>> + if (!chan->msg_count)
>>    goto exit;
>>
>>    count = chan->msg_count;
>> ============================
>>


>>> Hope this clarifies the reasons for switching to hrtimer.
>>>
>> I am not against using hrtimer, just need to make sure we don't simply
>> suppress the symptoms of wrong implementation.
>
> Agreed, and that's a valid concern. So far based on the testing and
> benchmarking done so far, we don't think this patch is suppressing
> anything incorrectly.
>
> If you still have concerns with this solution, please explain them here
> so that we can discuss and come to conclusion and the issue is fixed.
>
I just replied on the patch where you set
     cl->knows_txdone = false;
and which is not the case.

We may use hrtimer for polling, but your platform doesn't have to rely on that.

-jassi

Re: [PATCH 1/2] mailbox: switch to hrtimer for tx_complete polling

[Sudeep Holla]

On 29/07/15 09:33, Jassi Brar wrote:
> On Mon, Jul 27, 2015 at 3:18 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> On 27/07/15 04:26, Jassi Brar wrote:
>>>
>>>>>
>>>>>> we might end-up waiting
>>>>>> for atleast a jiffy even though the response for that message from the
>>>>>> remote is received via interrupt and processed in relatively smaller
>>>>>> time granularity.
>>>>>>
>>>>> That is wrong.
>>>>
>>>>
>>>> No see below.
>>>>
>>>>>     If the controller supports TX interrupt it should set txdone_irq,
>>>>> which prevents polling i.e, controller driver calls mbox_chan_txdone.
>>>>>
>>>>>     If the controller doesn't support TX interrupt but the client
>>>>> receives some ack packet, then the client should set knows_txdone and
>>>>> call mbox_client_txdone. Again you don't have to wait on polling.
>>>>>
>>>>
>>>> Sorry if I was not clear in the commit message, but I thought I did
>>>> mention TXDONE_BY_POLL. The case I am referring is definitely not
>>>> TXDONE_BY_IRQ or TXDONE_BY_ACK.
>>>>
>>> That statement is still wrong. The TXDONE_BY_POLL modifier does't make it
>>> right.
>>>
>>
>> I am fine to modify/clarify that statement.
>>
>>> Anyways, I see you meant the 3rd case of neither IRQ nor ACK.
>>>
>>
>> Yes the remote indicates by setting a flag in status register.
>>
> However, looking at the arm_scpi.c the protocol does support
> TXDONE_BY_ACK that is, every command has a reply packet telling if the
> command was successful or failure. When you receive a reply, obviously
> the command has already been received by the remote. Which is
> mbox_client.knows_txdone or TXDONE_BY_ACK.
>

I do understand TXDONE_BY_ACK, but SCPI protocol doesn't support that.
You can verify the SCPI specification document.

>>> It seems your remote doesn't send some protocol level 'ack' packet
>>> replying if the command was successfully executed or not. That means
>>> Linux can't differentiate successful execution of the command from a
>>> silent failure (remote still has to set the TX_done flag to make way
>>> for next messages).
>>
>> Agreed and again I confirm the remote processor in question just sets
>> the flag always and correctly and doesn't use a protocol ACK.
>>
> As I note above, the arm_scpi.c tells a different story.
>

You are just concluding this from my stupid comment.

[..]

>>>> Hope this clarifies the reasons for switching to hrtimer.
>>>>
>>> I am not against using hrtimer, just need to make sure we don't simply
>>> suppress the symptoms of wrong implementation.
>>
>> Agreed, and that's a valid concern. So far based on the testing and
>> benchmarking done so far, we don't think this patch is suppressing
>> anything incorrectly.
>>
>> If you still have concerns with this solution, please explain them here
>> so that we can discuss and come to conclusion and the issue is fixed.
>>
> I just replied on the patch where you set
>       cl->knows_txdone = false;
> and which is not the case.
>
> We may use hrtimer for polling, but your platform doesn't have to rely on that.
>

Again, sorry for misleading comment, we do need hrtimer as replied on
scpi thread. Any other concern with this patch ?

Regards,
Sudeep

Re: [PATCH 1/2] mailbox: switch to hrtimer for tx_complete polling

[Jassi Brar]

On Wed, Jul 29, 2015 at 2:18 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 29/07/15 09:33, Jassi Brar wrote:
>
>> On Mon, Jul 27, 2015 at 3:18 PM, Sudeep Holla <sudeep.holla@arm.com>
>> wrote:
>>>
>>> On 27/07/15 04:26, Jassi Brar wrote:
>>>>
>>>>
>>>>>>
>>>>>>> we might end-up waiting
>>>>>>> for atleast a jiffy even though the response for that message from
>>>>>>> the
>>>>>>> remote is received via interrupt and processed in relatively smaller
>>>>>>> time granularity.
>>>>>>>
>>>>>> That is wrong.
>>>>>
>>>>>
>>>>>
>>>>> No see below.
>>>>>
>>>>>>     If the controller supports TX interrupt it should set txdone_irq,
>>>>>> which prevents polling i.e, controller driver calls mbox_chan_txdone.
>>>>>>
>>>>>>     If the controller doesn't support TX interrupt but the client
>>>>>> receives some ack packet, then the client should set knows_txdone and
>>>>>> call mbox_client_txdone. Again you don't have to wait on polling.
>>>>>>
>>>>>
>>>>> Sorry if I was not clear in the commit message, but I thought I did
>>>>> mention TXDONE_BY_POLL. The case I am referring is definitely not
>>>>> TXDONE_BY_IRQ or TXDONE_BY_ACK.
>>>>>
>>>> That statement is still wrong. The TXDONE_BY_POLL modifier does't make
>>>> it
>>>> right.
>>>>
>>>
>>> I am fine to modify/clarify that statement.
>>>
>>>> Anyways, I see you meant the 3rd case of neither IRQ nor ACK.
>>>>
>>>
>>> Yes the remote indicates by setting a flag in status register.
>>>
>> However, looking at the arm_scpi.c the protocol does support
>> TXDONE_BY_ACK that is, every command has a reply packet telling if the
>> command was successful or failure. When you receive a reply, obviously
>> the command has already been received by the remote. Which is
>> mbox_client.knows_txdone or TXDONE_BY_ACK.
>>
>
> I do understand TXDONE_BY_ACK, but SCPI protocol doesn't support that.
> You can verify the SCPI specification document.
>
>>>> It seems your remote doesn't send some protocol level 'ack' packet
>>>> replying if the command was successfully executed or not. That means
>>>> Linux can't differentiate successful execution of the command from a
>>>> silent failure (remote still has to set the TX_done flag to make way
>>>> for next messages).
>>>
>>>
>>> Agreed and again I confirm the remote processor in question just sets
>>> the flag always and correctly and doesn't use a protocol ACK.
>>>
>> As I note above, the arm_scpi.c tells a different story.
>>
>
> You are just concluding this from my stupid comment.
>
> [..]
>
>>>>> Hope this clarifies the reasons for switching to hrtimer.
>>>>>
>>>> I am not against using hrtimer, just need to make sure we don't simply
>>>> suppress the symptoms of wrong implementation.
>>>
>>>
>>> Agreed, and that's a valid concern. So far based on the testing and
>>> benchmarking done so far, we don't think this patch is suppressing
>>> anything incorrectly.
>>>
>>> If you still have concerns with this solution, please explain them here
>>> so that we can discuss and come to conclusion and the issue is fixed.
>>>
>> I just replied on the patch where you set
>>       cl->knows_txdone = false;
>> and which is not the case.
>>
>> We may use hrtimer for polling, but your platform doesn't have to rely on
>> that.
>>
>
> Again, sorry for misleading comment, we do need hrtimer as replied on
> scpi thread. Any other concern with this patch ?
>
Polling by hrtimers is OK. Not to mean this is the best solution for
your platform. Please revise the changelog completely.

Thanks.

Re: [PATCH 1/2] mailbox: switch to hrtimer for tx_complete polling

[Sudeep Holla]

On 30/07/15 19:11, Jassi Brar wrote:
> On Wed, Jul 29, 2015 at 2:18 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:

[..]

>>
>> Again, sorry for misleading comment, we do need hrtimer as replied on
>> scpi thread. Any other concern with this patch ?
>>
> Polling by hrtimers is OK. Not to mean this is the best solution for
> your platform. Please revise the changelog completely.
>

OK, how about:

"The mailbox core uses jiffy based timer to handle polling for the
transmit completion. If the client/protocol have/support notification
of the last packet transmit completion via ACK packet, then we tick the
Tx state machine immediately in the callback. However if the client
doesn't support that mechanism we might end-up waiting for atleast a
jiffy even though the remote is ready to receive the next request.

This patch switches the timer used for that polling from jiffy-based
to hrtimer-based so that we can support polling at much higher time
resolution."

Re: [PATCH 1/2] mailbox: switch to hrtimer for tx_complete polling

[Jassi Brar]

On Fri, Jul 31, 2015 at 3:22 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>>>
>>> Again, sorry for misleading comment, we do need hrtimer as replied on
>>> scpi thread. Any other concern with this patch ?
>>>
>> Polling by hrtimers is OK. Not to mean this is the best solution for
>> your platform. Please revise the changelog completely.
>>
>
> OK, how about:
>
> "The mailbox core uses jiffy based timer to handle polling for the
> transmit completion. If the client/protocol have/support notification
> of the last packet transmit completion via ACK packet, then we tick the
> Tx state machine immediately in the callback. However if the client
> doesn't support that mechanism we might end-up waiting for atleast a
> jiffy even though the remote is ready to receive the next request.
>
> This patch switches the timer used for that polling from jiffy-based
> to hrtimer-based so that we can support polling at much higher time
> resolution."

Perfect.

Re: [PATCH 1/2] mailbox: switch to hrtimer for tx_complete polling

[Sudeep Holla]

On 31/07/15 11:30, Jassi Brar wrote:
> On Fri, Jul 31, 2015 at 3:22 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
>>>>
>>>> Again, sorry for misleading comment, we do need hrtimer as replied on
>>>> scpi thread. Any other concern with this patch ?
>>>>
>>> Polling by hrtimers is OK. Not to mean this is the best solution for
>>> your platform. Please revise the changelog completely.
>>>
>>
>> OK, how about:
>>
>> "The mailbox core uses jiffy based timer to handle polling for the
>> transmit completion. If the client/protocol have/support notification
>> of the last packet transmit completion via ACK packet, then we tick the
>> Tx state machine immediately in the callback. However if the client
>> doesn't support that mechanism we might end-up waiting for atleast a
>> jiffy even though the remote is ready to receive the next request.
>>
>> This patch switches the timer used for that polling from jiffy-based
>> to hrtimer-based so that we can support polling at much higher time
>> resolution."
>
> Perfect.
>
Thanks, will respin the patches soon.

Regards,
Sudeep

[PATCH v2 1/2] mailbox: switch to hrtimer for tx_complete polling

[Sudeep Holla]

The mailbox core uses jiffy based timer to handle polling for the
transmit completion. If the client/protocol have/support notification
of the last packet transmit completion via ACK packet, then we tick the
Tx state machine immediately in the callback. However if the client
doesn't support that mechanism we might end-up waiting for atleast a
jiffy even though the remote is ready to receive the next request.

This patch switches the timer used for that polling from jiffy-based
to hrtimer-based so that we can support polling at much higher time
resolution.

Cc: Jassi Brar <jassisinghbrar@gmail.com>
Reported-and-suggested-by: Juri Lelli <Juri.Lelli@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/mailbox.c          | 27 +++++++++++++++------------
 include/linux/mailbox_controller.h |  7 ++++---
 2 files changed, 19 insertions(+), 15 deletions(-)

v2->v1:
	- Updated the changelog as suggested by Jassi

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index c7fdb57fd166..6a4811f85705 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -26,8 +26,6 @@
 static LIST_HEAD(mbox_cons);
 static DEFINE_MUTEX(con_mutex);
 
-static void poll_txdone(unsigned long data);
-
 static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
 {
 	int idx;
@@ -88,7 +86,9 @@ static void msg_submit(struct mbox_chan *chan)
 	spin_unlock_irqrestore(&chan->lock, flags);
 
 	if (!err && (chan->txdone_method & TXDONE_BY_POLL))
-		poll_txdone((unsigned long)chan->mbox);
+		/* kick start the timer immediately to avoid delays */
+		hrtimer_start(&chan->mbox->poll_hrt, ktime_set(0, 0),
+			      HRTIMER_MODE_REL);
 }
 
 static void tx_tick(struct mbox_chan *chan, int r)
@@ -112,9 +112,10 @@ static void tx_tick(struct mbox_chan *chan, int r)
 		complete(&chan->tx_complete);
 }
 
-static void poll_txdone(unsigned long data)
+static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
 {
-	struct mbox_controller *mbox = (struct mbox_controller *)data;
+	struct mbox_controller *mbox =
+		container_of(hrtimer, struct mbox_controller, poll_hrt);
 	bool txdone, resched = false;
 	int i;
 
@@ -130,9 +131,11 @@ static void poll_txdone(unsigned long data)
 		}
 	}
 
-	if (resched)
-		mod_timer(&mbox->poll, jiffies +
-				msecs_to_jiffies(mbox->txpoll_period));
+	if (resched) {
+		hrtimer_forward_now(hrtimer, ms_to_ktime(mbox->txpoll_period));
+		return HRTIMER_RESTART;
+	}
+	return HRTIMER_NORESTART;
 }
 
 /**
@@ -451,9 +454,9 @@ int mbox_controller_register(struct mbox_controller *mbox)
 		txdone = TXDONE_BY_ACK;
 
 	if (txdone == TXDONE_BY_POLL) {
-		mbox->poll.function = &poll_txdone;
-		mbox->poll.data = (unsigned long)mbox;
-		init_timer(&mbox->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++) {
@@ -495,7 +498,7 @@ void mbox_controller_unregister(struct mbox_controller *mbox)
 		mbox_free_channel(&mbox->chans[i]);
 
 	if (mbox->txdone_poll)
-		del_timer_sync(&mbox->poll);
+		hrtimer_cancel(&mbox->poll_hrt);
 
 	mutex_unlock(&con_mutex);
 }
diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
index 68c42454439b..74deadb42d76 100644
--- a/include/linux/mailbox_controller.h
+++ b/include/linux/mailbox_controller.h
@@ -9,7 +9,7 @@
 
 #include <linux/of.h>
 #include <linux/types.h>
-#include <linux/timer.h>
+#include <linux/hrtimer.h>
 #include <linux/device.h>
 #include <linux/completion.h>
 
@@ -67,7 +67,8 @@ struct mbox_chan_ops {
  * @txpoll_period:	If 'txdone_poll' is in effect, the API polls for
  *			last TX's status after these many millisecs
  * @of_xlate:		Controller driver specific mapping of channel via DT
- * @poll:		API private. Used to poll for TXDONE on all channels.
+ * @poll_hrt:		API private. hrtimer used to poll for TXDONE on all
+ *			channels.
  * @node:		API private. To hook into list of controllers.
  */
 struct mbox_controller {
@@ -81,7 +82,7 @@ struct mbox_controller {
 	struct mbox_chan *(*of_xlate)(struct mbox_controller *mbox,
 				      const struct of_phandle_args *sp);
 	/* Internal to API */
-	struct timer_list poll;
+	struct hrtimer poll_hrt;
 	struct list_head node;
 };
 
-- 
1.9.1

[PATCH v2 2/2] mailbox: arm_mhu: reduce txpoll_period from 10ms to 1 ms

[Sudeep Holla]

Since the mailbox core users hrtimers now, it can handle much higher
resolutions. We can reduce the txpoll_period to 1 ms as the transmit
usually takes just few microseconds.

Cc: Jassi Brar <jassisinghbrar@gmail.com>
Reported-and-suggested-by: Juri Lelli <Juri.Lelli@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/arm_mhu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c
index d9e99f981aa9..438c2896ef22 100644
--- a/drivers/mailbox/arm_mhu.c
+++ b/drivers/mailbox/arm_mhu.c
@@ -148,7 +148,7 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 	mhu->mbox.ops = &mhu_ops;
 	mhu->mbox.txdone_irq = false;
 	mhu->mbox.txdone_poll = true;
-	mhu->mbox.txpoll_period = 10;
+	mhu->mbox.txpoll_period = 1;
 
 	amba_set_drvdata(adev, mhu);
 
-- 
1.9.1