linux-kernel - Re: [PATCH v2] mailbox: Hi3660: Fixup mailbox state machine malfunction issue

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 for Android: free password hash cracker in your pocket

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

Message-ID: <d3ede0ab-b635-344c-faba-a9b1531b7f05@arm.com>
Date:   Mon, 10 Dec 2018 18:31:47 +0000
From:   Valentin Schneider <valentin.schneider@....com>
To:     Kevin Wangtao <kevin.wangtao@...ilicon.com>,
        jassisinghbrar@...il.com
Cc:     linux-kernel@...r.kernel.org, leo.yan@...aro.org,
        gengyanping@...ilicon.com, suzhuangluan@...ilicon.com
Subject: Re: [PATCH v2] mailbox: Hi3660: Fixup mailbox state machine
 malfunction issue

Hi,

On 03/12/2018 06:13, Kevin Wangtao wrote:
> Current mailbox driver of Hi3660 release the mailbox directly
> before sending a new message which may cause last message lost
> and next message sending doesn't take effect actually.
> 
> This patch fixes this issue by following the right process below,
> each time before sending a message, mailbox driver will check
> whether the mailbox is in ready state, if last message has been
> acknowledged, mailbox driver will clear the ack state to turn
> the mailbox to ready state again.
> 
> Signed-off-by: Kevin Wangtao <kevin.wangtao@...ilicon.com>

Validated using sysbench at each OPP ([1]). Test passes with patch and fails
without:

| CPU  |     OPP | Base | Patch |
|------+---------+------+-------|
| CPU0 |  533000 |  104 |   104 |
| CPU0 |  999000 |  104 |   201 |
| CPU0 |  140200 |  285 |   285 |
| CPU0 | 1709000 |  285 |   349 |
| CPU0 | 1844000 |  377 |   377 |
|------+---------+------+-------|
| CPU4 |  903000 |  249 |   248 |
| CPU4 | 1421000 |  249 |   394 |
| CPU4 | 1805000 |  500 |   500 |
| CPU4 | 2112000 |  499 |   583 |
| CPU4 | 2362000 |  653 |   654 |

We need this pretty badly, otherwise frequency setting is a coin toss.

Tested-by: Valentin Schneider <valentin.schneider@....com>

Thanks,
Valentin



[1]: https://github.com/ARM-software/lisa/blob/next/lisa/tests/kernel/cpufreq/sanity.py#L23

Ran the above with this script on each kernel:

----->8-----
#!/usr/bin/env python3

from lisa.env import TestEnv
from lisa.tests.kernel.cpufreq.sanity import UserspaceSanity

te = TestEnv.from_cli()

print(UserspaceSanity.from_testenv(te).test_performance_sanity())
-----8<-----


> ---
> Changes v1 -> v2:
>  - update commit message
> 
>  drivers/mailbox/hi3660-mailbox.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mailbox/hi3660-mailbox.c b/drivers/mailbox/hi3660-mailbox.c
> index 3eea6b6..035b71a 100644
> --- a/drivers/mailbox/hi3660-mailbox.c
> +++ b/drivers/mailbox/hi3660-mailbox.c
> @@ -38,6 +38,7 @@
>  #define MBOX_AUTOMATIC_ACK		1
>  
>  #define MBOX_STATE_IDLE			BIT(4)
> +#define MBOX_STATE_READY		BIT(5)
>  #define MBOX_STATE_ACK			BIT(7)
>  
>  #define MBOX_MSG_LEN			8
> @@ -91,8 +92,8 @@ static int hi3660_mbox_check_state(struct mbox_chan *chan)
>  	unsigned long val;
>  	unsigned int ret;
>  
> -	/* Mailbox is idle so directly bail out */
> -	if (readl(base + MBOX_MODE_REG) & MBOX_STATE_IDLE)
> +	/* Mailbox is ready to use */
> +	if (readl(base + MBOX_MODE_REG) & MBOX_STATE_READY)
>  		return 0;
>  
>  	/* Wait for acknowledge from remote */
> @@ -103,9 +104,9 @@ static int hi3660_mbox_check_state(struct mbox_chan *chan)
>  		return ret;
>  	}
>  
> -	/* Ensure channel is released */
> -	writel(0xffffffff, base + MBOX_IMASK_REG);
> -	writel(BIT(mchan->ack_irq), base + MBOX_SRC_REG);
> +	/* clear ack state, mailbox will get back to ready state */
> +	writel(BIT(mchan->ack_irq), base + MBOX_ICLR_REG);
> +
>  	return 0;
>  }
>  
> @@ -160,10 +161,6 @@ static int hi3660_mbox_startup(struct mbox_chan *chan)
>  {
>  	int ret;
>  
> -	ret = hi3660_mbox_check_state(chan);
> -	if (ret)
> -		return ret;
> -
>  	ret = hi3660_mbox_unlock(chan);
>  	if (ret)
>  		return ret;
> @@ -183,10 +180,11 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg)
>  	void __iomem *base = MBOX_BASE(mbox, ch);
>  	u32 *buf = msg;
>  	unsigned int i;
> +	int ret;
>  
> -	/* Ensure channel is released */
> -	writel_relaxed(0xffffffff, base + MBOX_IMASK_REG);
> -	writel_relaxed(BIT(mchan->ack_irq), base + MBOX_SRC_REG);
> +	ret = hi3660_mbox_check_state(chan);
> +	if (ret)
> +		return ret;
>  
>  	/* Clear mask for destination interrupt */
>  	writel_relaxed(~BIT(mchan->dst_irq), base + MBOX_IMASK_REG);
>