iommu/rockchip: Handle errors returned from PM framework

PATCH New Marc Zyngier 2 participants 3 messages Aug 5, 2018 – Aug 6, 2018

Subsystems (2)

Arm Rockchip IOMMU

lore

Share & tools

lore

Marc Zyngier

author

2018-08-05 12:46 UTC #0 lore raw

PATCH 1/1 iommu/rockchip: Handle errors returned from PM framework

pm_runtime_get_if_in_use can fail: either PM has been disabled altogether (-EINVAL), or the device hasn't been enabled yet (0). Sadly, we ignore the first case at the moment, and end up treating it as if we had received a valid interrupt.

The first case could happen because the interrupt line is shared (with the VOP for example), and although that device hasn't been programmed yet, an interrupt may be pending (think kexec/kdump).

The second case means that we've enabled the IOMMU, but it is not powered-on just yet. This could be another instance of the above, but as it deserves investigation, let's output a single warning (instead of flodding the console).

In both cases, bail with an IRQ_NONE.

Fixes 0f181d3cf7d98 ("iommu/rockchip: Add runtime PM support")

Signed-off-by Marc Zyngier <[email protected]>

---
 drivers/iommu/rockchip-iommu.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/iommu/rockchip-iommu.c +4 -3

index 054cd2c8e9c8..9d21495a8433 100644
@@ -521,10 +521,11 @@ static irqreturn_t rk_iommu_irq(int irq, void *dev_id)
 	u32 int_status;
 	dma_addr_t iova;
 	irqreturn_t ret = IRQ_NONE;
-	int i;
+	int i, err;
 
-	if (WARN_ON(!pm_runtime_get_if_in_use(iommu->dev)))
-		return 0;
+	err = pm_runtime_get_if_in_use(iommu->dev);
+	if ((err < 0) || WARN_ON_ONCE(!err))
+		return ret;
 
 	if (WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks)))
 		goto out;

Signature · 1 line

2.14.2

Heiko Stuebner

maintainer

2018-08-06 12:09 UTC #1 lore raw

PATCH 1/1 iommu/rockchip: Handle errors returned from PM framework

Hi Marc,

Am Sonntag, 5. August 2018, 14:46:16 CEST schrieb Marc Zyngier:

Quoted 14 lines

pm_runtime_get_if_in_use can fail: either PM has been disabled altogether (-EINVAL), or the device hasn't been enabled yet (0). Sadly, we ignore the first case at the moment, and end up treating it as if we had received a valid interrupt.
The first case could happen because the interrupt line is shared (with the VOP for example), and although that device hasn't been programmed yet, an interrupt may be pending (think kexec/kdump).
The second case means that we've enabled the IOMMU, but it is not powered-on just yet. This could be another instance of the above, but as it deserves investigation, let's output a single warning (instead of flodding the console).

Quoted 4 lines

In both cases, bail with an IRQ_NONE.
Fixes 0f181d3cf7d98 ("iommu/rockchip: Add runtime PM support")
Signed-off-by Marc Zyngier <[email protected]>

Hmm, maybe my thinking is flawed, but to me it looks a bit different.

I.e. the iommu, as well as the vop have the capability to check if the irq is for them via their status registers (INT_STATUS and MMU_STATUS).

For this to happen the power-domain must be active and the device clocked. Clock handling is done on both the vop and iommu and in the !CONFIG_PM case all power-domains are left on.

Right now a !CONFIG_PM just passes through the driver unnoticed but with this change, we would actually make it depend on PM?

What am I missing?

Also there are quite a bit more instances of pm_runtime_get_if_in_use present in the iommu driver.

Heiko

Quoted 23 lines

 drivers/iommu/rockchip-iommu.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/iommu/rockchip-iommu.c +4 -3

index 054cd2c8e9c8..9d21495a8433 100644
@@ -521,10 +521,11 @@ static irqreturn_t rk_iommu_irq(int irq, void *dev_id)
 	u32 int_status;
 	dma_addr_t iova;
 	irqreturn_t ret = IRQ_NONE;
-	int i;
+	int i, err;
 
-	if (WARN_ON(!pm_runtime_get_if_in_use(iommu->dev)))
-		return 0;
+	err = pm_runtime_get_if_in_use(iommu->dev);
+	if ((err < 0) || WARN_ON_ONCE(!err))
+		return ret;
 
 	if (WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks)))
 		goto out;

Marc Zyngier

author

2018-08-06 12:32 UTC #2 lore raw

PATCH 1/1 iommu/rockchip: Handle errors returned from PM framework

Hi Heiko,

Quoted 27 lines · Heiko Stuebner

Hi Marc,
Am Sonntag, 5. August 2018, 14:46:16 CEST schrieb Marc Zyngier:
Quoted 14 lines
pm_runtime_get_if_in_use can fail: either PM has been disabled altogether (-EINVAL), or the device hasn't been enabled yet (0). Sadly, we ignore the first case at the moment, and end up treating it as if we had received a valid interrupt.
The first case could happen because the interrupt line is shared (with the VOP for example), and although that device hasn't been programmed yet, an interrupt may be pending (think kexec/kdump).
The second case means that we've enabled the IOMMU, but it is not powered-on just yet. This could be another instance of the above, but as it deserves investigation, let's output a single warning (instead of flodding the console).
Quoted 4 lines
In both cases, bail with an IRQ_NONE.
Fixes 0f181d3cf7d98 ("iommu/rockchip: Add runtime PM support")
Signed-off-by Marc Zyngier <[email protected]>
Hmm, maybe my thinking is flawed, but to me it looks a bit different.
I.e. the iommu, as well as the vop have the capability to check if the irq is for them via their status registers (INT_STATUS and MMU_STATUS).

Quoted 8 lines

For this to happen the power-domain must be active and the device clocked. Clock handling is done on both the vop and iommu and in the !CONFIG_PM case all power-domains are left on.
Right now a !CONFIG_PM just passes through the driver unnoticed but with this change, we would actually make it depend on PM?
What am I missing?

I'm not sure you're missing anything, except that what I'm reporting here has nothing to do with !CONFIG_PM, but with the state of runtime PM when an interrupt gets delivered.

Quoted 2 lines

Also there are quite a bit more instances of pm_runtime_get_if_in_use present in the iommu driver.

Indeed. Most of the are mishandling the return value by ignoring the error cases. And the more I look at it, the more I think I'm just fixing an effect, and not the root cause. See when the IOMMU interrupts get requested, long before pm_runtime_enable() is called.

If you get an interrupt at that point (which is perfectly likely given that the VOP shares the IRQ line), you handle it in an unexpected context. I should have spotted that yesterday...

I've quickly hacked the (untested) following patch, which should do the trick (I'll test it tonight).

Thanks,

M.

drivers/iommu/rockchip-iommu.c +13 ~~-11~~

index 054cd2c8e9c8..94942712959d 100644
@@ -1152,17 +1152,6 @@ static int rk_iommu_probe(struct platform_device *pdev)
 	if (iommu->num_mmu == 0)
 		return PTR_ERR(iommu->bases[0]);
 
-	i = 0;
-	while ((irq = platform_get_irq(pdev, i++)) != -ENXIO) {
-		if (irq < 0)
-			return irq;
-
-		err = devm_request_irq(iommu->dev, irq, rk_iommu_irq,
-				       IRQF_SHARED, dev_name(dev), iommu);
-		if (err)
-			return err;
-	}
-
 	iommu->reset_disabled = device_property_read_bool(dev,
 					"rockchip,disable-mmu-reset");

@@ -1219,6 +1208,19 @@ static int rk_iommu_probe(struct platform_device *pdev)
 
 	pm_runtime_enable(dev);
 
+	i = 0;
+	while ((irq = platform_get_irq(pdev, i++)) != -ENXIO) {
+		if (irq < 0)
+			return irq;
+
+		err = devm_request_irq(iommu->dev, irq, rk_iommu_irq,
+				       IRQF_SHARED, dev_name(dev), iommu);
+		if (err) {
+			pm_runtime_disable(dev);
+			goto err_remove_sysfs;
+		}
+	}
+
 	return 0;
 err_remove_sysfs:
 	iommu_device_sysfs_remove(&iommu->iommu);

Signature · 1 line

Jazz is not dead. It just smells funny...