[PATCH 0/2] MediaTek PCIe Gen3: Suspend fixes

[PATCH 0/2] MediaTek PCIe Gen3: Suspend fixes

[AngeloGioacchino Del Regno]

This series fixes PM suspend/resume for MediaTek PCIe Gen3, seen on
the MT8195 Tomato Chromebook, which would deadlock at resume.

Tested on a MT8195 Tomato Chromebook.

AngeloGioacchino Del Regno (2):
  PCI: mediatek-gen3: Stop acquiring spinlocks in {suspend,resume}_noirq
  PCI: mediatek-gen3: Assert MAC reset only if PHY reset also present

 drivers/pci/controller/pcie-mediatek-gen3.c | 33 +++++++++++----------
 1 file changed, 17 insertions(+), 16 deletions(-)

-- 
2.40.1

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

[AngeloGioacchino Del Regno]

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

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

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

[Oliver Neukum]

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 ***

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

[AngeloGioacchino Del Regno]

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

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

[Bjorn Helgaas]

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

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

[AngeloGioacchino Del Regno]

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

[PATCH 2/2] PCI: mediatek-gen3: Assert MAC reset only if PHY reset also present

[AngeloGioacchino Del Regno]

Some SoCs have two PCI-Express controllers: in the case of MT8195,
one of them is using a dedicated PHY, but the other uses a combo PHY
that is shared with USB and in that case the PHY cannot be reset
from the PCIe driver, or USB functionality will be unable to resume.

Resetting the PCIe MAC without also resetting the PHY will result in
a full system lockup at PCIe resume time and the only option to
resume operation is to hard reboot the system (with a PMIC cut-off).

To resolve this issue, check if we've got both a PHY and a MAC reset
and, if not, never assert resets at PM suspend time: in that case,
the link is still getting powered down as both the clocks and the
power domains will go down anyway.

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 | 25 ++++++++++++++-------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index 52f52ca5db71..480621ca1450 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -859,17 +859,26 @@ static int mtk_pcie_power_up(struct mtk_gen3_pcie *pcie)
 	return err;
 }
 
-static void mtk_pcie_power_down(struct mtk_gen3_pcie *pcie)
+static void mtk_pcie_power_down(struct mtk_gen3_pcie *pcie, bool is_suspend)
 {
+	bool suspend_reset_supported = pcie->mac_reset && pcie->phy_reset;
+
 	clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
 
 	pm_runtime_put_sync(pcie->dev);
 	pm_runtime_disable(pcie->dev);
-	reset_control_assert(pcie->mac_reset);
+
+	/*
+	 * Assert MAC reset only if we also got a PHY reset, otherwise
+	 * the system will lockup at PM resume time.
+	 */
+	if (is_suspend && suspend_reset_supported)
+		reset_control_assert(pcie->mac_reset);
 
 	phy_power_off(pcie->phy);
 	phy_exit(pcie->phy);
-	reset_control_assert(pcie->phy_reset);
+	if (is_suspend && suspend_reset_supported)
+		reset_control_assert(pcie->phy_reset);
 }
 
 static int mtk_pcie_setup(struct mtk_gen3_pcie *pcie)
@@ -905,7 +914,7 @@ static int mtk_pcie_setup(struct mtk_gen3_pcie *pcie)
 	return 0;
 
 err_setup:
-	mtk_pcie_power_down(pcie);
+	mtk_pcie_power_down(pcie, false);
 
 	return err;
 }
@@ -936,7 +945,7 @@ static int mtk_pcie_probe(struct platform_device *pdev)
 	err = pci_host_probe(host);
 	if (err) {
 		mtk_pcie_irq_teardown(pcie);
-		mtk_pcie_power_down(pcie);
+		mtk_pcie_power_down(pcie, false);
 		return err;
 	}
 
@@ -954,7 +963,7 @@ static int mtk_pcie_remove(struct platform_device *pdev)
 	pci_unlock_rescan_remove();
 
 	mtk_pcie_irq_teardown(pcie);
-	mtk_pcie_power_down(pcie);
+	mtk_pcie_power_down(pcie, false);
 
 	return 0;
 }
@@ -1023,7 +1032,7 @@ static int mtk_pcie_suspend_noirq(struct device *dev)
 	dev_dbg(pcie->dev, "entered L2 states successfully");
 
 	mtk_pcie_irq_save(pcie);
-	mtk_pcie_power_down(pcie);
+	mtk_pcie_power_down(pcie, true);
 
 	return 0;
 }
@@ -1039,7 +1048,7 @@ static int mtk_pcie_resume_noirq(struct device *dev)
 
 	err = mtk_pcie_startup_port(pcie);
 	if (err) {
-		mtk_pcie_power_down(pcie);
+		mtk_pcie_power_down(pcie, false);
 		return err;
 	}
 
-- 
2.40.1

Re: [PATCH 2/2] PCI: mediatek-gen3: Assert MAC reset only if PHY reset also present

[Nícolas F. R. A. Prado]

On Thu, May 04, 2023 at 01:35:09PM +0200, AngeloGioacchino Del Regno wrote:
> Some SoCs have two PCI-Express controllers: in the case of MT8195,
> one of them is using a dedicated PHY, but the other uses a combo PHY
> that is shared with USB and in that case the PHY cannot be reset
> from the PCIe driver, or USB functionality will be unable to resume.
> 
> Resetting the PCIe MAC without also resetting the PHY will result in
> a full system lockup at PCIe resume time and the only option to
> resume operation is to hard reboot the system (with a PMIC cut-off).
> 
> To resolve this issue, check if we've got both a PHY and a MAC reset
> and, if not, never assert resets at PM suspend time: in that case,
> the link is still getting powered down as both the clocks and the
> power domains will go down anyway.
> 
> Fixes: d537dc125f07 ("PCI: mediatek-gen3: Add system PM support")
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Hi Angelo,

It seems this patch was forgotten but it's still very much needed. As you
describe above, the Tomato Chromebook (MT8195-based) is currently unable to
resume from suspend due to this issue. Upon resume, the following error is
printed, and the system hangs:

[   67.018281] mtk-pcie-gen3 112f8000.pcie: PCIe link down, current LTSSM state: detect.quiet (0x0)
[   67.027162] mtk-pcie-gen3 112f8000.pcie: PM: dpm_run_callback(): genpd_resume_noirq+0x0/0x24 returns -110
[   67.036791] mtk-pcie-gen3 112f8000.pcie: PM: failed to resume noirq: error -110

And further investigation showed that all PCIe registers return 0x0 when read in
this situation.

Commenting out the MAC reset in the PCIe DT node fixes the issue: the PCIe
registers can be read correctly upon resume and resume proceeds succesfully.
Your patch here essentially does the same as not providing the MAC reset, with
the benefit of us still being able to describe the reset in DT and thus having a
more complete HW description.

But this patch no longer applies, so please rebase it so we can get working
suspend/resume on MT8195-Tomato :).

Thanks,
Nícolas