diff mbox series

[1/2] mailbox: reset txdone_method TXDONE_BY_POLL if client knows_txdone

Message ID 1498835681-23975-1-git-send-email-sudeep.holla@arm.com
State New
Headers show
Series [1/2] mailbox: reset txdone_method TXDONE_BY_POLL if client knows_txdone | expand

Commit Message

Sudeep Holla June 30, 2017, 3:14 p.m. UTC 
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 <alexey.klimov@arm.com>
Cc: Jassi Brar <jaswinder.singh@linaro.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

---
 drivers/mailbox/mailbox.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.7.4

Comments

Jassi Brar July 1, 2017, 11:25 a.m. UTC | #1 
On Fri, Jun 30, 2017 at 8:44 PM, Sudeep Holla <sudeep.holla@arm.com> 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 <alexey.klimov@arm.com>

> Cc: Jassi Brar <jaswinder.singh@linaro.org>

> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

> ---

>  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
Sudeep Holla July 3, 2017, 8:57 a.m. UTC | #2 
On 01/07/17 12:25, Jassi Brar wrote:
> On Fri, Jun 30, 2017 at 8:44 PM, Sudeep Holla <sudeep.holla@arm.com> 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 <alexey.klimov@arm.com>

>> Cc: Jassi Brar <jaswinder.singh@linaro.org>

>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

>> ---

>>  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
Sudeep Holla July 3, 2017, 9:35 a.m. UTC | #3 
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 <sudeep.holla@arm.com> 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 <alexey.klimov@arm.com>

>>> Cc: Jassi Brar <jaswinder.singh@linaro.org>

>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

>>> ---

>>>  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
Jassi Brar July 3, 2017, 11:50 a.m. UTC | #4 
On 3 July 2017 at 15:05, Sudeep Holla <sudeep.holla@arm.com> 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 <sudeep.holla@arm.com> 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 <alexey.klimov@arm.com>

>>>> Cc: Jassi Brar <jaswinder.singh@linaro.org>

>>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

>>>> ---

>>>>  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)
Sudeep Holla July 3, 2017, 12:55 p.m. UTC | #5 
On 03/07/17 12:50, Jassi Brar wrote:
> On 3 July 2017 at 15:05, Sudeep Holla <sudeep.holla@arm.com> 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 <sudeep.holla@arm.com> 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 <alexey.klimov@arm.com>

>>>>> Cc: Jassi Brar <jaswinder.singh@linaro.org>

>>>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

>>>>> ---

>>>>>  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
diff mbox series

Patch

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);