Currently the mailbox framework sets txdone_method to TXDONE_BY_POLL if
the controller sets txdone_by_poll. However some clients can have a
mechanism to do TXDONE_BY_ACK which they can specify by knows_txdone.
However, we endup setting both TXDONE_BY_POLL and TXDONE_BY_ACK in that
case. In such scenario, we may end up with below warnings as the tx
ticker is run both by mailbox framework and the client.
WARNING: CPU: 1 PID: 0 at kernel/time/hrtimer.c:805 hrtimer_forward+0x88/0xd8
CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.12.0-rc5 #242
Hardware name: ARM LTD ARM Juno Development Platform
task: ffff8009768ca700 task.stack: ffff8009768f8000
PC is at hrtimer_forward+0x88/0xd8
LR is at txdone_hrtimer+0xd4/0xf8
Call trace:
hrtimer_forward+0x88/0xd8
__hrtimer_run_queues+0xe4/0x158
hrtimer_interrupt+0xa4/0x220
arch_timer_handler_phys+0x30/0x40
handle_percpu_devid_irq+0x78/0x130
generic_handle_irq+0x24/0x38
__handle_domain_irq+0x5c/0xb8
gic_handle_irq+0x54/0xa8
This patch fixes the issue by resetting TXDONE_BY_POLL if client has set
knows_txdone.
Cc: Alexey Klimov <[email protected]>
Cc: Jassi Brar <[email protected]>
Signed-off-by: Sudeep Holla <[email protected]>
---
drivers/mailbox/mailbox.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 537f4f6d009b..8da30f833262 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -351,7 +351,7 @@ struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index)
init_completion(&chan->tx_complete);
if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
- chan->txdone_method |= TXDONE_BY_ACK;
+ chan->txdone_method = TXDONE_BY_ACK;
spin_unlock_irqrestore(&chan->lock, flags);
--
2.7.4
Currently we rely on the first byte of the Rx buffer to check if there's
any data available to be read. If the first byte of the received buffer
is zero (i.e. null character), then we fail to signal that data is
available even when it's available.
Instead introduce a boolean variable to track the data availability and
update it in the channel receive callback as ready and clear it when the
data is read.
Cc: Jassi Brar <[email protected]>
Fixes: baef9a35d246 ("mailbox: mailbox-test: add support for fasync/poll")
Signed-off-by: Sudeep Holla <[email protected]>
---
drivers/mailbox/mailbox-test.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/mailbox/mailbox-test.c b/drivers/mailbox/mailbox-test.c
index 97fb956bb6e0..93f3d4d61fa7 100644
--- a/drivers/mailbox/mailbox-test.c
+++ b/drivers/mailbox/mailbox-test.c
@@ -30,6 +30,7 @@
#define MBOX_HEXDUMP_MAX_LEN (MBOX_HEXDUMP_LINE_LEN * \
(MBOX_MAX_MSG_LEN / MBOX_BYTES_PER_LINE))
+static bool mbox_data_ready;
static struct dentry *root_debugfs_dir;
struct mbox_test_device {
@@ -152,16 +153,14 @@ static ssize_t mbox_test_message_write(struct file *filp,
static bool mbox_test_message_data_ready(struct mbox_test_device *tdev)
{
- unsigned char data;
+ bool data_ready;
unsigned long flags;
spin_lock_irqsave(&tdev->lock, flags);
- data = tdev->rx_buffer[0];
+ data_ready = mbox_data_ready;
spin_unlock_irqrestore(&tdev->lock, flags);
- if (data != '\0')
- return true;
- return false;
+ return data_ready;
}
static ssize_t mbox_test_message_read(struct file *filp, char __user *userbuf,
@@ -223,6 +222,7 @@ static ssize_t mbox_test_message_read(struct file *filp, char __user *userbuf,
*(touser + l) = '\0';
memset(tdev->rx_buffer, 0, MBOX_MAX_MSG_LEN);
+ mbox_data_ready = false;
spin_unlock_irqrestore(&tdev->lock, flags);
@@ -292,6 +292,7 @@ static void mbox_test_receive_message(struct mbox_client *client, void *message)
message, MBOX_MAX_MSG_LEN);
memcpy(tdev->rx_buffer, message, MBOX_MAX_MSG_LEN);
}
+ mbox_data_ready = true;
spin_unlock_irqrestore(&tdev->lock, flags);
wake_up_interruptible(&tdev->waitq);
--
2.7.4
On Fri, Jun 30, 2017 at 8:44 PM, Sudeep Holla <[email protected]> wrote:
> Currently the mailbox framework sets txdone_method to TXDONE_BY_POLL if
> the controller sets txdone_by_poll. However some clients can have a
> mechanism to do TXDONE_BY_ACK which they can specify by knows_txdone.
> However, we endup setting both TXDONE_BY_POLL and TXDONE_BY_ACK in that
> case. In such scenario, we may end up with below warnings as the tx
> ticker is run both by mailbox framework and the client.
>
> WARNING: CPU: 1 PID: 0 at kernel/time/hrtimer.c:805 hrtimer_forward+0x88/0xd8
> CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.12.0-rc5 #242
> Hardware name: ARM LTD ARM Juno Development Platform
> task: ffff8009768ca700 task.stack: ffff8009768f8000
> PC is at hrtimer_forward+0x88/0xd8
> LR is at txdone_hrtimer+0xd4/0xf8
> Call trace:
> hrtimer_forward+0x88/0xd8
> __hrtimer_run_queues+0xe4/0x158
> hrtimer_interrupt+0xa4/0x220
> arch_timer_handler_phys+0x30/0x40
> handle_percpu_devid_irq+0x78/0x130
> generic_handle_irq+0x24/0x38
> __handle_domain_irq+0x5c/0xb8
> gic_handle_irq+0x54/0xa8
>
> This patch fixes the issue by resetting TXDONE_BY_POLL if client has set
> knows_txdone.
>
> Cc: Alexey Klimov <[email protected]>
> Cc: Jassi Brar <[email protected]>
> Signed-off-by: Sudeep Holla <[email protected]>
> ---
> drivers/mailbox/mailbox.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> index 537f4f6d009b..8da30f833262 100644
> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -351,7 +351,7 @@ struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index)
> init_completion(&chan->tx_complete);
>
> if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
> - chan->txdone_method |= TXDONE_BY_ACK;
> + chan->txdone_method = TXDONE_BY_ACK;
>
It has to be restored back in mbox_free_channel()
thanks
On 01/07/17 12:25, Jassi Brar wrote:
> On Fri, Jun 30, 2017 at 8:44 PM, Sudeep Holla <[email protected]> wrote:
>> Currently the mailbox framework sets txdone_method to TXDONE_BY_POLL if
>> the controller sets txdone_by_poll. However some clients can have a
>> mechanism to do TXDONE_BY_ACK which they can specify by knows_txdone.
>> However, we endup setting both TXDONE_BY_POLL and TXDONE_BY_ACK in that
>> case. In such scenario, we may end up with below warnings as the tx
>> ticker is run both by mailbox framework and the client.
>>
>> WARNING: CPU: 1 PID: 0 at kernel/time/hrtimer.c:805 hrtimer_forward+0x88/0xd8
>> CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.12.0-rc5 #242
>> Hardware name: ARM LTD ARM Juno Development Platform
>> task: ffff8009768ca700 task.stack: ffff8009768f8000
>> PC is at hrtimer_forward+0x88/0xd8
>> LR is at txdone_hrtimer+0xd4/0xf8
>> Call trace:
>> hrtimer_forward+0x88/0xd8
>> __hrtimer_run_queues+0xe4/0x158
>> hrtimer_interrupt+0xa4/0x220
>> arch_timer_handler_phys+0x30/0x40
>> handle_percpu_devid_irq+0x78/0x130
>> generic_handle_irq+0x24/0x38
>> __handle_domain_irq+0x5c/0xb8
>> gic_handle_irq+0x54/0xa8
>>
>> This patch fixes the issue by resetting TXDONE_BY_POLL if client has set
>> knows_txdone.
>>
>> Cc: Alexey Klimov <[email protected]>
>> Cc: Jassi Brar <[email protected]>
>> Signed-off-by: Sudeep Holla <[email protected]>
>> ---
>> drivers/mailbox/mailbox.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>> index 537f4f6d009b..8da30f833262 100644
>> --- a/drivers/mailbox/mailbox.c
>> +++ b/drivers/mailbox/mailbox.c
>> @@ -351,7 +351,7 @@ struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index)
>> init_completion(&chan->tx_complete);
>>
>> if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
>> - chan->txdone_method |= TXDONE_BY_ACK;
>> + chan->txdone_method = TXDONE_BY_ACK;
>>
> It has to be restored back in mbox_free_channel()
Ah right, will fix and repost.
--
Regards,
Sudeep
On 03/07/17 09:57, Sudeep Holla wrote:
>
>
> On 01/07/17 12:25, Jassi Brar wrote:
>> On Fri, Jun 30, 2017 at 8:44 PM, Sudeep Holla <[email protected]> wrote:
>>> Currently the mailbox framework sets txdone_method to TXDONE_BY_POLL if
>>> the controller sets txdone_by_poll. However some clients can have a
>>> mechanism to do TXDONE_BY_ACK which they can specify by knows_txdone.
>>> However, we endup setting both TXDONE_BY_POLL and TXDONE_BY_ACK in that
>>> case. In such scenario, we may end up with below warnings as the tx
>>> ticker is run both by mailbox framework and the client.
>>>
>>> WARNING: CPU: 1 PID: 0 at kernel/time/hrtimer.c:805 hrtimer_forward+0x88/0xd8
>>> CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.12.0-rc5 #242
>>> Hardware name: ARM LTD ARM Juno Development Platform
>>> task: ffff8009768ca700 task.stack: ffff8009768f8000
>>> PC is at hrtimer_forward+0x88/0xd8
>>> LR is at txdone_hrtimer+0xd4/0xf8
>>> Call trace:
>>> hrtimer_forward+0x88/0xd8
>>> __hrtimer_run_queues+0xe4/0x158
>>> hrtimer_interrupt+0xa4/0x220
>>> arch_timer_handler_phys+0x30/0x40
>>> handle_percpu_devid_irq+0x78/0x130
>>> generic_handle_irq+0x24/0x38
>>> __handle_domain_irq+0x5c/0xb8
>>> gic_handle_irq+0x54/0xa8
>>>
>>> This patch fixes the issue by resetting TXDONE_BY_POLL if client has set
>>> knows_txdone.
>>>
>>> Cc: Alexey Klimov <[email protected]>
>>> Cc: Jassi Brar <[email protected]>
>>> Signed-off-by: Sudeep Holla <[email protected]>
>>> ---
>>> drivers/mailbox/mailbox.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>>> index 537f4f6d009b..8da30f833262 100644
>>> --- a/drivers/mailbox/mailbox.c
>>> +++ b/drivers/mailbox/mailbox.c
>>> @@ -351,7 +351,7 @@ struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index)
>>> init_completion(&chan->tx_complete);
>>>
>>> if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
>>> - chan->txdone_method |= TXDONE_BY_ACK;
>>> + chan->txdone_method = TXDONE_BY_ACK;
>>>
>> It has to be restored back in mbox_free_channel()
>
> Ah right, will fix and repost.
>
I was too fast to response, I see we already take care of that in free
channel. So I think it should be fine as is.
if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
chan->txdone_method = TXDONE_BY_POLL;
--
Regards,
Sudeep
On 3 July 2017 at 15:05, Sudeep Holla <[email protected]> wrote:
>
>
> On 03/07/17 09:57, Sudeep Holla wrote:
>>
>>
>> On 01/07/17 12:25, Jassi Brar wrote:
>>> On Fri, Jun 30, 2017 at 8:44 PM, Sudeep Holla <[email protected]> wrote:
>>>> Currently the mailbox framework sets txdone_method to TXDONE_BY_POLL if
>>>> the controller sets txdone_by_poll. However some clients can have a
>>>> mechanism to do TXDONE_BY_ACK which they can specify by knows_txdone.
>>>> However, we endup setting both TXDONE_BY_POLL and TXDONE_BY_ACK in that
>>>> case. In such scenario, we may end up with below warnings as the tx
>>>> ticker is run both by mailbox framework and the client.
>>>>
>>>> WARNING: CPU: 1 PID: 0 at kernel/time/hrtimer.c:805 hrtimer_forward+0x88/0xd8
>>>> CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.12.0-rc5 #242
>>>> Hardware name: ARM LTD ARM Juno Development Platform
>>>> task: ffff8009768ca700 task.stack: ffff8009768f8000
>>>> PC is at hrtimer_forward+0x88/0xd8
>>>> LR is at txdone_hrtimer+0xd4/0xf8
>>>> Call trace:
>>>> hrtimer_forward+0x88/0xd8
>>>> __hrtimer_run_queues+0xe4/0x158
>>>> hrtimer_interrupt+0xa4/0x220
>>>> arch_timer_handler_phys+0x30/0x40
>>>> handle_percpu_devid_irq+0x78/0x130
>>>> generic_handle_irq+0x24/0x38
>>>> __handle_domain_irq+0x5c/0xb8
>>>> gic_handle_irq+0x54/0xa8
>>>>
>>>> This patch fixes the issue by resetting TXDONE_BY_POLL if client has set
>>>> knows_txdone.
>>>>
>>>> Cc: Alexey Klimov <[email protected]>
>>>> Cc: Jassi Brar <[email protected]>
>>>> Signed-off-by: Sudeep Holla <[email protected]>
>>>> ---
>>>> drivers/mailbox/mailbox.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>>>> index 537f4f6d009b..8da30f833262 100644
>>>> --- a/drivers/mailbox/mailbox.c
>>>> +++ b/drivers/mailbox/mailbox.c
>>>> @@ -351,7 +351,7 @@ struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index)
>>>> init_completion(&chan->tx_complete);
>>>>
>>>> if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
>>>> - chan->txdone_method |= TXDONE_BY_ACK;
>>>> + chan->txdone_method = TXDONE_BY_ACK;
>>>>
>>> It has to be restored back in mbox_free_channel()
>>
>> Ah right, will fix and repost.
>>
>
> I was too fast to response, I see we already take care of that in free
> channel. So I think it should be fine as is.
>
> if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
> chan->txdone_method = TXDONE_BY_POLL;
>
You were too fast this time :)
chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK)
is not same as
(chan->txdone_method == TXDONE_BY_POLL || chan->txdone_method == TXDONE_BY_ACK)
On 03/07/17 12:50, Jassi Brar wrote:
> On 3 July 2017 at 15:05, Sudeep Holla <[email protected]> wrote:
>>
>>
>> On 03/07/17 09:57, Sudeep Holla wrote:
>>>
>>>
>>> On 01/07/17 12:25, Jassi Brar wrote:
>>>> On Fri, Jun 30, 2017 at 8:44 PM, Sudeep Holla <[email protected]> wrote:
>>>>> Currently the mailbox framework sets txdone_method to TXDONE_BY_POLL if
>>>>> the controller sets txdone_by_poll. However some clients can have a
>>>>> mechanism to do TXDONE_BY_ACK which they can specify by knows_txdone.
>>>>> However, we endup setting both TXDONE_BY_POLL and TXDONE_BY_ACK in that
>>>>> case. In such scenario, we may end up with below warnings as the tx
>>>>> ticker is run both by mailbox framework and the client.
>>>>>
>>>>> WARNING: CPU: 1 PID: 0 at kernel/time/hrtimer.c:805 hrtimer_forward+0x88/0xd8
>>>>> CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.12.0-rc5 #242
>>>>> Hardware name: ARM LTD ARM Juno Development Platform
>>>>> task: ffff8009768ca700 task.stack: ffff8009768f8000
>>>>> PC is at hrtimer_forward+0x88/0xd8
>>>>> LR is at txdone_hrtimer+0xd4/0xf8
>>>>> Call trace:
>>>>> hrtimer_forward+0x88/0xd8
>>>>> __hrtimer_run_queues+0xe4/0x158
>>>>> hrtimer_interrupt+0xa4/0x220
>>>>> arch_timer_handler_phys+0x30/0x40
>>>>> handle_percpu_devid_irq+0x78/0x130
>>>>> generic_handle_irq+0x24/0x38
>>>>> __handle_domain_irq+0x5c/0xb8
>>>>> gic_handle_irq+0x54/0xa8
>>>>>
>>>>> This patch fixes the issue by resetting TXDONE_BY_POLL if client has set
>>>>> knows_txdone.
>>>>>
>>>>> Cc: Alexey Klimov <[email protected]>
>>>>> Cc: Jassi Brar <[email protected]>
>>>>> Signed-off-by: Sudeep Holla <[email protected]>
>>>>> ---
>>>>> drivers/mailbox/mailbox.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>>>>> index 537f4f6d009b..8da30f833262 100644
>>>>> --- a/drivers/mailbox/mailbox.c
>>>>> +++ b/drivers/mailbox/mailbox.c
>>>>> @@ -351,7 +351,7 @@ struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index)
>>>>> init_completion(&chan->tx_complete);
>>>>>
>>>>> if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
>>>>> - chan->txdone_method |= TXDONE_BY_ACK;
>>>>> + chan->txdone_method = TXDONE_BY_ACK;
>>>>>
>>>> It has to be restored back in mbox_free_channel()
>>>
>>> Ah right, will fix and repost.
>>>
>>
>> I was too fast to response, I see we already take care of that in free
>> channel. So I think it should be fine as is.
>>
>> if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
>> chan->txdone_method = TXDONE_BY_POLL;
>>
> You were too fast this time :)
>
Indeed, my understanding was correct before.
> chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK)
>
> is not same as
>
> (chan->txdone_method == TXDONE_BY_POLL || chan->txdone_method == TXDONE_BY_ACK)
>
Ah crap, yes and I have no idea why I thought so when I was about to
change it :(
--
Regards,
Sudeep
Currently the mailbox framework sets txdone_method to TXDONE_BY_POLL if
the controller sets txdone_by_poll. However some clients can have a
mechanism to do TXDONE_BY_ACK which they can specify by knows_txdone.
However, we endup setting both TXDONE_BY_POLL and TXDONE_BY_ACK in that
case. In such scenario, we may end up with below warnings as the tx
ticker is run both by mailbox framework and the client.
WARNING: CPU: 1 PID: 0 at kernel/time/hrtimer.c:805 hrtimer_forward+0x88/0xd8
CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.12.0-rc5 #242
Hardware name: ARM LTD ARM Juno Development Platform
task: ffff8009768ca700 task.stack: ffff8009768f8000
PC is at hrtimer_forward+0x88/0xd8
LR is at txdone_hrtimer+0xd4/0xf8
Call trace:
hrtimer_forward+0x88/0xd8
__hrtimer_run_queues+0xe4/0x158
hrtimer_interrupt+0xa4/0x220
arch_timer_handler_phys+0x30/0x40
handle_percpu_devid_irq+0x78/0x130
generic_handle_irq+0x24/0x38
__handle_domain_irq+0x5c/0xb8
gic_handle_irq+0x54/0xa8
This patch fixes the issue by resetting TXDONE_BY_POLL if client has set
knows_txdone.
Cc: Alexey Klimov <[email protected]>
Cc: Jassi Brar <[email protected]>
Signed-off-by: Sudeep Holla <[email protected]>
---
drivers/mailbox/mailbox.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 537f4f6d009b..674b35f402f5 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -351,7 +351,7 @@ struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index)
init_completion(&chan->tx_complete);
if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
- chan->txdone_method |= TXDONE_BY_ACK;
+ chan->txdone_method = TXDONE_BY_ACK;
spin_unlock_irqrestore(&chan->lock, flags);
@@ -418,7 +418,7 @@ 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 | TXDONE_BY_ACK))
+ if (chan->txdone_method == TXDONE_BY_ACK)
chan->txdone_method = TXDONE_BY_POLL;
module_put(chan->mbox->dev->driver->owner);
--
2.7.4