Re: [PATCH v2] mailbox: PCC: Fix lockdep warning when request PCC channel
Hi Prashanth,
On Fri, Oct 21, 2016 at 9:13 AM, Prakash, Prashanth
wrote:
> Hi Hoan,
>
> On 10/18/2016 1:00 AM, Hoan Tran wrote:
>> This patch fixes the lockdep warning below
>>
>> [7.229767] DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags))
>> [7.229776] [ cut here ]
>> [7.229787] WARNING: CPU: 1 PID: 1 at
>> linux-next/kernel/locking/lockdep.c:2876 loc
>> kdep_trace_alloc+0xe0/0xf0
>> [7.229790] Modules linked in:
>> [7.229793]
>> [7.229798] CPU: 1 PID: 1 Comm: swapper/0 Not tainted
>> 4.8.0-11756-g86c5152 #46
>> ...
>> [7.229900] Call trace:
>> [7.229903] Exception stack(0x8007da837890 to 0x8007da8379c0)
>> [7.229906] 7880: 8007da834000
>> 0001
>> [7.229909] 78a0: 8007da837a70 08a0 60c5
>> 003d
>> [7.229912] 78c0: 9374bc6a7f3c7832 00381878 09db7ab8
>> 002f
>> [7.229915] 78e0: 0811aabc 08be2548 8007da837990
>> 0811adf8
>> [7.229918] 7900: 8007da834000 024080c0 00c0
>> 09021000
>> [7.229921] 7920: 08c8f7c8
>> 8007da579810
>> [7.229923] 7940: 002f 8007da858000
>> 0001
>> [7.229926] 7960: 0001 0811a468
>> 0002
>> [7.229929] 7980: 656c62617369645f 00038187 00ee
>> 8007da837850
>> [7.229932] 79a0: 09db50c0 09db569d 0006
>> 89db568f
>> [7.229936] [] lockdep_trace_alloc+0xe0/0xf0
>> [7.229940] [] __kmalloc_track_caller+0x50/0x250
>> [7.229945] [] devres_alloc_node+0x28/0x60
>> [7.229949] [] devm_request_threaded_irq+0x50/0xe0
>> [7.229955] [] pcc_mbox_request_channel+0x110/0x170
>> [7.229960] [] acpi_cppc_processor_probe+0x264/0x414
>> [7.229963] [] __acpi_processor_start+0x28/0xa0
>> [7.229966] [] acpi_processor_start+0x44/0x54
>> [7.229970] [] driver_probe_device+0x1fc/0x2b0
>> [7.229974] [] __driver_attach+0xb4/0xc0
>> [7.229977] [] bus_for_each_dev+0x5c/0xa0
>> [7.229980] [] driver_attach+0x20/0x30
>> [7.229983] [] bus_add_driver+0x110/0x230
>> [7.229987] [] driver_register+0x60/0x100
>> [7.229991] [] acpi_processor_driver_init+0x2c/0xb0
>> [7.229996] [] do_one_initcall+0x38/0x130
>> [7.23] [] kernel_init_freeable+0x210/0x2b4
>> [7.230004] [] kernel_init+0x10/0x110
>> [7.230007] [] ret_from_fork+0x10/0x50
>>
>> It's because the spinlock inside pcc_mbox_request_channel() is
>> kept too long. This patch releases spinlock before request_irq()
>> and free_irq() to fix this issue as spinlock is only needed to
>> protect the channel data.
>>
>> Signed-off-by: Hoan Tran
>> ---
>> v2
>> * Release spinlock before request_irq() and free_irq() instead of
>> using mutex
>>
>> drivers/mailbox/pcc.c | 8
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index 08c87fa..b5275a8 100644
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
>> @@ -267,6 +267,8 @@ struct mbox_chan *pcc_mbox_request_channel(struct
>> mbox_client *cl,
>> if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
>> chan->txdone_method |= TXDONE_BY_ACK;
>>
>> + spin_unlock_irqrestore(&chan->lock, flags);
>> +
>> if (pcc_doorbell_irq[subspace_id] > 0) {
>> int rc;
>>
>> @@ -279,8 +281,6 @@ struct mbox_chan *pcc_mbox_request_channel(struct
>> mbox_client *cl,
>> }
>> }
>>
>> - spin_unlock_irqrestore(&chan->lock, flags);
>> -
>> return chan;
>> }
>> EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);
>> @@ -310,10 +310,10 @@ void pcc_mbox_free_channel(struct mbox_chan *chan)
>> if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
>> chan->txdone_method = TXDONE_BY_POLL;
>>
>> + spin_unlock_irqrestore(&chan->lock, flags);
>> +
>> if (pcc_doorbell_irq[id] > 0)
>> devm_free_irq(chan->mbox->dev, pcc_doorbell_irq[id], chan);
> Shouldn't we free the irq first and then reset the channel state?
> To avoid the scenario where an interrupt might get triggered after
> we reset the channel state but before we release the interrupt
Yes, and another change for next version that if it fails to
request_irq(), call free_channel() function.
Thanks
Hoan
>
> --
> Thanks,
> Prashanth
Re: [PATCH v2] mailbox: PCC: Fix lockdep warning when request PCC channel
Hi Hoan,
On 10/18/2016 1:00 AM, Hoan Tran wrote:
> This patch fixes the lockdep warning below
>
> [7.229767] DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags))
> [7.229776] [ cut here ]
> [7.229787] WARNING: CPU: 1 PID: 1 at
> linux-next/kernel/locking/lockdep.c:2876 loc
> kdep_trace_alloc+0xe0/0xf0
> [7.229790] Modules linked in:
> [7.229793]
> [7.229798] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.8.0-11756-g86c5152
> #46
> ...
> [7.229900] Call trace:
> [7.229903] Exception stack(0x8007da837890 to 0x8007da8379c0)
> [7.229906] 7880: 8007da834000
> 0001
> [7.229909] 78a0: 8007da837a70 08a0 60c5
> 003d
> [7.229912] 78c0: 9374bc6a7f3c7832 00381878 09db7ab8
> 002f
> [7.229915] 78e0: 0811aabc 08be2548 8007da837990
> 0811adf8
> [7.229918] 7900: 8007da834000 024080c0 00c0
> 09021000
> [7.229921] 7920: 08c8f7c8
> 8007da579810
> [7.229923] 7940: 002f 8007da858000
> 0001
> [7.229926] 7960: 0001 0811a468
> 0002
> [7.229929] 7980: 656c62617369645f 00038187 00ee
> 8007da837850
> [7.229932] 79a0: 09db50c0 09db569d 0006
> 89db568f
> [7.229936] [] lockdep_trace_alloc+0xe0/0xf0
> [7.229940] [] __kmalloc_track_caller+0x50/0x250
> [7.229945] [] devres_alloc_node+0x28/0x60
> [7.229949] [] devm_request_threaded_irq+0x50/0xe0
> [7.229955] [] pcc_mbox_request_channel+0x110/0x170
> [7.229960] [] acpi_cppc_processor_probe+0x264/0x414
> [7.229963] [] __acpi_processor_start+0x28/0xa0
> [7.229966] [] acpi_processor_start+0x44/0x54
> [7.229970] [] driver_probe_device+0x1fc/0x2b0
> [7.229974] [] __driver_attach+0xb4/0xc0
> [7.229977] [] bus_for_each_dev+0x5c/0xa0
> [7.229980] [] driver_attach+0x20/0x30
> [7.229983] [] bus_add_driver+0x110/0x230
> [7.229987] [] driver_register+0x60/0x100
> [7.229991] [] acpi_processor_driver_init+0x2c/0xb0
> [7.229996] [] do_one_initcall+0x38/0x130
> [7.23] [] kernel_init_freeable+0x210/0x2b4
> [7.230004] [] kernel_init+0x10/0x110
> [7.230007] [] ret_from_fork+0x10/0x50
>
> It's because the spinlock inside pcc_mbox_request_channel() is
> kept too long. This patch releases spinlock before request_irq()
> and free_irq() to fix this issue as spinlock is only needed to
> protect the channel data.
>
> Signed-off-by: Hoan Tran
> ---
> v2
> * Release spinlock before request_irq() and free_irq() instead of
> using mutex
>
> drivers/mailbox/pcc.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 08c87fa..b5275a8 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -267,6 +267,8 @@ struct mbox_chan *pcc_mbox_request_channel(struct
> mbox_client *cl,
> if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
> chan->txdone_method |= TXDONE_BY_ACK;
>
> + spin_unlock_irqrestore(&chan->lock, flags);
> +
> if (pcc_doorbell_irq[subspace_id] > 0) {
> int rc;
>
> @@ -279,8 +281,6 @@ struct mbox_chan *pcc_mbox_request_channel(struct
> mbox_client *cl,
> }
> }
>
> - spin_unlock_irqrestore(&chan->lock, flags);
> -
> return chan;
> }
> EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);
> @@ -310,10 +310,10 @@ void pcc_mbox_free_channel(struct mbox_chan *chan)
> if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
> chan->txdone_method = TXDONE_BY_POLL;
>
> + spin_unlock_irqrestore(&chan->lock, flags);
> +
> if (pcc_doorbell_irq[id] > 0)
> devm_free_irq(chan->mbox->dev, pcc_doorbell_irq[id], chan);
Shouldn't we free the irq first and then reset the channel state?
To avoid the scenario where an interrupt might get triggered after
we reset the channel state but before we release the interrupt
--
Thanks,
Prashanth
[PATCH v2] mailbox: PCC: Fix lockdep warning when request PCC channel
This patch fixes the lockdep warning below
[7.229767] DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags))
[7.229776] [ cut here ]
[7.229787] WARNING: CPU: 1 PID: 1 at
linux-next/kernel/locking/lockdep.c:2876 loc
kdep_trace_alloc+0xe0/0xf0
[7.229790] Modules linked in:
[7.229793]
[7.229798] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.8.0-11756-g86c5152
#46
...
[7.229900] Call trace:
[7.229903] Exception stack(0x8007da837890 to 0x8007da8379c0)
[7.229906] 7880: 8007da834000
0001
[7.229909] 78a0: 8007da837a70 08a0 60c5
003d
[7.229912] 78c0: 9374bc6a7f3c7832 00381878 09db7ab8
002f
[7.229915] 78e0: 0811aabc 08be2548 8007da837990
0811adf8
[7.229918] 7900: 8007da834000 024080c0 00c0
09021000
[7.229921] 7920: 08c8f7c8
8007da579810
[7.229923] 7940: 002f 8007da858000
0001
[7.229926] 7960: 0001 0811a468
0002
[7.229929] 7980: 656c62617369645f 00038187 00ee
8007da837850
[7.229932] 79a0: 09db50c0 09db569d 0006
89db568f
[7.229936] [] lockdep_trace_alloc+0xe0/0xf0
[7.229940] [] __kmalloc_track_caller+0x50/0x250
[7.229945] [] devres_alloc_node+0x28/0x60
[7.229949] [] devm_request_threaded_irq+0x50/0xe0
[7.229955] [] pcc_mbox_request_channel+0x110/0x170
[7.229960] [] acpi_cppc_processor_probe+0x264/0x414
[7.229963] [] __acpi_processor_start+0x28/0xa0
[7.229966] [] acpi_processor_start+0x44/0x54
[7.229970] [] driver_probe_device+0x1fc/0x2b0
[7.229974] [] __driver_attach+0xb4/0xc0
[7.229977] [] bus_for_each_dev+0x5c/0xa0
[7.229980] [] driver_attach+0x20/0x30
[7.229983] [] bus_add_driver+0x110/0x230
[7.229987] [] driver_register+0x60/0x100
[7.229991] [] acpi_processor_driver_init+0x2c/0xb0
[7.229996] [] do_one_initcall+0x38/0x130
[7.23] [] kernel_init_freeable+0x210/0x2b4
[7.230004] [] kernel_init+0x10/0x110
[7.230007] [] ret_from_fork+0x10/0x50
It's because the spinlock inside pcc_mbox_request_channel() is
kept too long. This patch releases spinlock before request_irq()
and free_irq() to fix this issue as spinlock is only needed to
protect the channel data.
Signed-off-by: Hoan Tran
---
v2
* Release spinlock before request_irq() and free_irq() instead of
using mutex
drivers/mailbox/pcc.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 08c87fa..b5275a8 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -267,6 +267,8 @@ struct mbox_chan *pcc_mbox_request_channel(struct
mbox_client *cl,
if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
chan->txdone_method |= TXDONE_BY_ACK;
+ spin_unlock_irqrestore(&chan->lock, flags);
+
if (pcc_doorbell_irq[subspace_id] > 0) {
int rc;
@@ -279,8 +281,6 @@ struct mbox_chan *pcc_mbox_request_channel(struct
mbox_client *cl,
}
}
- spin_unlock_irqrestore(&chan->lock, flags);
-
return chan;
}
EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);
@@ -310,10 +310,10 @@ void pcc_mbox_free_channel(struct mbox_chan *chan)
if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
chan->txdone_method = TXDONE_BY_POLL;
+ spin_unlock_irqrestore(&chan->lock, flags);
+
if (pcc_doorbell_irq[id] > 0)
devm_free_irq(chan->mbox->dev, pcc_doorbell_irq[id], chan);
-
- spin_unlock_irqrestore(&chan->lock, flags);
}
EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
--
1.9.1
