diff mbox series

[1/2] PCI: mediatek-gen3: Stop acquiring spinlocks in {suspend,resume}_noirq

Message ID 20230504113509.184633-2-angelogioacchino.delregno@collabora.com (mailing list archive)
State Changes Requested
Delegated to: Krzysztof Wilczyński
Headers show
Series MediaTek PCIe Gen3: Suspend fixes | expand

Commit Message

AngeloGioacchino Del Regno May 4, 2023, 11:35 a.m. UTC
In mtk_pcie_suspend_noirq() and mtk_pcie_resume_noirq() we are,
respectively, disabling and enabling generation of interrupts and
then saving and restoring the enabled interrupts register: since
we're using noirq PM callbacks, that can be safely done without
holding any spin lock.

That was noticed because of, and solves, the following issue:

<4>[   74.185982] ========================================================
<4>[   74.192629] WARNING: possible irq lock inversion dependency detected
<4>[   74.199276] 6.3.0-next-20230428+ #51 Tainted: G        W
<4>[   74.205664] --------------------------------------------------------
<4>[   74.212309] systemd-sleep/809 just changed the state of lock:
<4>[   74.218347] ffff65a5c34c65a0 (&pcie->irq_lock){+...}-{2:2}, at: mtk_pcie_resume+0x50/0xa8
<4>[   74.226870] but this lock was taken by another, HARDIRQ-safe lock in the past:
<4>[   74.234389]  (&irq_desc_lock_class){-.-.}-{2:2}
<4>[   74.234409]
<4>[   74.234409]
<4>[   74.234409] and interrupts could create inverse lock ordering between them.
<4>[   74.234409]
<4>[   74.251704]
<4>[   74.251704] other info that might help us debug this:
<4>[   74.258785]  Possible interrupt unsafe locking scenario:
<4>[   74.258785]
<4>[   74.266126]        CPU0                    CPU1
<4>[   74.270942]        ----                    ----
<4>[   74.275758]   lock(&pcie->irq_lock);
<4>[   74.279627]                                local_irq_disable();
<4>[   74.285836]                                lock(&irq_desc_lock_class);
<4>[   74.292667]                                lock(&pcie->irq_lock);
<4>[   74.299061]   <Interrupt>
<4>[   74.301960]     lock(&irq_desc_lock_class);
<4>[   74.306438]
<4>[   74.306438]  *** DEADLOCK ***

Fixes: d537dc125f07 ("PCI: mediatek-gen3: Add system PM support")
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 8 --------
 1 file changed, 8 deletions(-)

Comments

Oliver Neukum May 8, 2023, 7:44 a.m. UTC | #1
On 04.05.23 13:35, AngeloGioacchino Del Regno wrote:

Hi,

looking at your patch I am afraid there is an issue.

> In mtk_pcie_suspend_noirq() and mtk_pcie_resume_noirq() we are,
> respectively, disabling and enabling generation of interrupts and
> then saving and restoring the enabled interrupts register: since
> we're using noirq PM callbacks, that can be safely done without
> holding any spin lock.

Why? You can still race with another CPU in task context.
That is if you say that you do not need locking to touch
PCIE_INT_ENABLE_REG that is fine, but then why do you remove
it from one place only?
It is also touched in mtk_pcie_probe() at a minimum.

  
> That was noticed because of, and solves, the following issue:
> 
> <4>[   74.185982] ========================================================
> <4>[   74.192629] WARNING: possible irq lock inversion dependency detected
> <4>[   74.199276] 6.3.0-next-20230428+ #51 Tainted: G        W
> <4>[   74.205664] --------------------------------------------------------
> <4>[   74.212309] systemd-sleep/809 just changed the state of lock:
> <4>[   74.218347] ffff65a5c34c65a0 (&pcie->irq_lock){+...}-{2:2}, at: mtk_pcie_resume+0x50/0xa8
> <4>[   74.226870] but this lock was taken by another, HARDIRQ-safe lock in the past:
> <4>[   74.234389]  (&irq_desc_lock_class){-.-.}-{2:2}
> <4>[   74.234409]
> <4>[   74.234409]
> <4>[   74.234409] and interrupts could create inverse lock ordering between them.
> <4>[   74.234409]
> <4>[   74.251704]
> <4>[   74.251704] other info that might help us debug this:
> <4>[   74.258785]  Possible interrupt unsafe locking scenario:
> <4>[   74.258785]
> <4>[   74.266126]        CPU0                    CPU1
> <4>[   74.270942]        ----                    ----
> <4>[   74.275758]   lock(&pcie->irq_lock);

Lock A

> <4>[   74.279627]                                local_irq_disable();

strictly speaking irrelevant

> <4>[   74.285836]                                lock(&irq_desc_lock_class);

lock B

> <4>[   74.292667]                                lock(&pcie->irq_lock);

lock A

> <4>[   74.299061]   <Interrupt>

You do not need that interrupt.

> <4>[   74.301960]     lock(&irq_desc_lock_class);

lock B

> <4>[   74.306438]
> <4>[   74.306438]  *** DEADLOCK ***
AngeloGioacchino Del Regno June 8, 2023, 10:17 a.m. UTC | #2
Il 08/05/23 09:44, Oliver Neukum ha scritto:
> On 04.05.23 13:35, AngeloGioacchino Del Regno wrote:
> 
> Hi,
> 
> looking at your patch I am afraid there is an issue.
> 
>> In mtk_pcie_suspend_noirq() and mtk_pcie_resume_noirq() we are,
>> respectively, disabling and enabling generation of interrupts and
>> then saving and restoring the enabled interrupts register: since
>> we're using noirq PM callbacks, that can be safely done without
>> holding any spin lock.
> 
> Why? You can still race with another CPU in task context.
> That is if you say that you do not need locking to touch
> PCIE_INT_ENABLE_REG that is fine, but then why do you remove
> it from one place only?
> It is also touched in mtk_pcie_probe() at a minimum.
> 
> 
>> That was noticed because of, and solves, the following issue:
>>
>> <4>[   74.185982] ========================================================
>> <4>[   74.192629] WARNING: possible irq lock inversion dependency detected
>> <4>[   74.199276] 6.3.0-next-20230428+ #51 Tainted: G        W
>> <4>[   74.205664] --------------------------------------------------------
>> <4>[   74.212309] systemd-sleep/809 just changed the state of lock:
>> <4>[   74.218347] ffff65a5c34c65a0 (&pcie->irq_lock){+...}-{2:2}, at: 
>> mtk_pcie_resume+0x50/0xa8
>> <4>[   74.226870] but this lock was taken by another, HARDIRQ-safe lock in the past:
>> <4>[   74.234389]  (&irq_desc_lock_class){-.-.}-{2:2}
>> <4>[   74.234409]
>> <4>[   74.234409]
>> <4>[   74.234409] and interrupts could create inverse lock ordering between them.
>> <4>[   74.234409]
>> <4>[   74.251704]
>> <4>[   74.251704] other info that might help us debug this:
>> <4>[   74.258785]  Possible interrupt unsafe locking scenario:
>> <4>[   74.258785]
>> <4>[   74.266126]        CPU0                    CPU1
>> <4>[   74.270942]        ----                    ----
>> <4>[   74.275758]   lock(&pcie->irq_lock);
> 
> Lock A
> 
>> <4>[   74.279627]                                local_irq_disable();
> 
> strictly speaking irrelevant
> 
>> <4>[   74.285836]                                lock(&irq_desc_lock_class);
> 
> lock B
> 
>> <4>[   74.292667]                                lock(&pcie->irq_lock);
> 
> lock A
> 
>> <4>[   74.299061]   <Interrupt>
> 
> You do not need that interrupt.
> 
>> <4>[   74.301960]     lock(&irq_desc_lock_class);
> 
> lock B
> 
>> <4>[   74.306438]
>> <4>[   74.306438]  *** DEADLOCK ***


Sorry for the very late reply. I just noticed this.

I'm unsure, at this point, about how to solve this warning; ideas?

Thanks,
Angelo
Bjorn Helgaas June 8, 2023, 5:15 p.m. UTC | #3
On Thu, May 04, 2023 at 01:35:08PM +0200, AngeloGioacchino Del Regno wrote:
> In mtk_pcie_suspend_noirq() and mtk_pcie_resume_noirq() we are,
> respectively, disabling and enabling generation of interrupts and
> then saving and restoring the enabled interrupts register: since
> we're using noirq PM callbacks, that can be safely done without
> holding any spin lock.

Tangent: it's annoying that pcie-mediatek.c and pcie-mediatek-gen3.c
use identical "mtk_pcie_suspend_noirq()" names.  That makes browsing
harder than it needs to be.  But I see that you refer to
mediatek-gen3.

> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -963,8 +963,6 @@ static void mtk_pcie_irq_save(struct mtk_gen3_pcie *pcie)
>  {
>  	int i;
>  
> -	raw_spin_lock(&pcie->irq_lock);
> -
>  	pcie->saved_irq_state = readl_relaxed(pcie->base + PCIE_INT_ENABLE_REG);
>  
>  	for (i = 0; i < PCIE_MSI_SET_NUM; i++) {
> @@ -973,16 +971,12 @@ static void mtk_pcie_irq_save(struct mtk_gen3_pcie *pcie)
>  		msi_set->saved_irq_state = readl_relaxed(msi_set->base +
>  					   PCIE_MSI_SET_ENABLE_OFFSET);
>  	}
> -
> -	raw_spin_unlock(&pcie->irq_lock);
>  }

Jianjun added mtk_pcie_irq_save() and mtk_pcie_irq_restore() with
d537dc125f07 ("PCI: mediatek-gen3: Add system PM support").  

I suggest looking at other drivers and structuring mediatek-gen3
similarly, including using similar function names.  No other drivers
have a .*_pcie_irq_save() function.  Several have .*_pcie_host_init(),
and some of those do include some IRQ setup.

Bjorn
AngeloGioacchino Del Regno June 9, 2023, 7:29 a.m. UTC | #4
Il 08/06/23 19:15, Bjorn Helgaas ha scritto:
> On Thu, May 04, 2023 at 01:35:08PM +0200, AngeloGioacchino Del Regno wrote:
>> In mtk_pcie_suspend_noirq() and mtk_pcie_resume_noirq() we are,
>> respectively, disabling and enabling generation of interrupts and
>> then saving and restoring the enabled interrupts register: since
>> we're using noirq PM callbacks, that can be safely done without
>> holding any spin lock.
> 
> Tangent: it's annoying that pcie-mediatek.c and pcie-mediatek-gen3.c
> use identical "mtk_pcie_suspend_noirq()" names.  That makes browsing
> harder than it needs to be.  But I see that you refer to
> mediatek-gen3.
> 
>> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
>> @@ -963,8 +963,6 @@ static void mtk_pcie_irq_save(struct mtk_gen3_pcie *pcie)
>>   {
>>   	int i;
>>   
>> -	raw_spin_lock(&pcie->irq_lock);
>> -
>>   	pcie->saved_irq_state = readl_relaxed(pcie->base + PCIE_INT_ENABLE_REG);
>>   
>>   	for (i = 0; i < PCIE_MSI_SET_NUM; i++) {
>> @@ -973,16 +971,12 @@ static void mtk_pcie_irq_save(struct mtk_gen3_pcie *pcie)
>>   		msi_set->saved_irq_state = readl_relaxed(msi_set->base +
>>   					   PCIE_MSI_SET_ENABLE_OFFSET);
>>   	}
>> -
>> -	raw_spin_unlock(&pcie->irq_lock);
>>   }
> 
> Jianjun added mtk_pcie_irq_save() and mtk_pcie_irq_restore() with
> d537dc125f07 ("PCI: mediatek-gen3: Add system PM support").
> 
> I suggest looking at other drivers and structuring mediatek-gen3
> similarly, including using similar function names.  No other drivers
> have a .*_pcie_irq_save() function.  Several have .*_pcie_host_init(),
> and some of those do include some IRQ setup.
> 
> Bjorn

Hello Bjorn,
thanks for the feedback!

Yes, I of course refer to the Gen3 driver... I'll check other drivers and
will try to improve the consistency of this one with the others as soon as
I have some bandwidth to perform the job.

Thanks again,
Angelo
diff mbox series

Patch

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index b8612ce5f4d0..52f52ca5db71 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -963,8 +963,6 @@  static void mtk_pcie_irq_save(struct mtk_gen3_pcie *pcie)
 {
 	int i;
 
-	raw_spin_lock(&pcie->irq_lock);
-
 	pcie->saved_irq_state = readl_relaxed(pcie->base + PCIE_INT_ENABLE_REG);
 
 	for (i = 0; i < PCIE_MSI_SET_NUM; i++) {
@@ -973,16 +971,12 @@  static void mtk_pcie_irq_save(struct mtk_gen3_pcie *pcie)
 		msi_set->saved_irq_state = readl_relaxed(msi_set->base +
 					   PCIE_MSI_SET_ENABLE_OFFSET);
 	}
-
-	raw_spin_unlock(&pcie->irq_lock);
 }
 
 static void mtk_pcie_irq_restore(struct mtk_gen3_pcie *pcie)
 {
 	int i;
 
-	raw_spin_lock(&pcie->irq_lock);
-
 	writel_relaxed(pcie->saved_irq_state, pcie->base + PCIE_INT_ENABLE_REG);
 
 	for (i = 0; i < PCIE_MSI_SET_NUM; i++) {
@@ -991,8 +985,6 @@  static void mtk_pcie_irq_restore(struct mtk_gen3_pcie *pcie)
 		writel_relaxed(msi_set->saved_irq_state,
 			       msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
 	}
-
-	raw_spin_unlock(&pcie->irq_lock);
 }
 
 static int mtk_pcie_turn_off_link(struct mtk_gen3_pcie *pcie)