| Message ID | 20180805124616.18020-1-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu/rockchip: Handle errors returned from PM framework | expand |
Hi Marc, Am Sonntag, 5. August 2018, 14:46:16 CEST schrieb Marc Zyngier: > 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 <marc.zyngier@arm.com> 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 > drivers/iommu/rockchip-iommu.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c > index 054cd2c8e9c8..9d21495a8433 100644 > --- a/drivers/iommu/rockchip-iommu.c > +++ b/drivers/iommu/rockchip-iommu.c > @@ -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; >
Hi Heiko, On 06/08/18 13:09, Heiko Stuebner wrote: > Hi Marc, > > Am Sonntag, 5. August 2018, 14:46:16 CEST schrieb Marc Zyngier: >> 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 <marc.zyngier@arm.com> > > 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? 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. > 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. diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c index 054cd2c8e9c8..94942712959d 100644 --- a/drivers/iommu/rockchip-iommu.c +++ b/drivers/iommu/rockchip-iommu.c @@ -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);
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c index 054cd2c8e9c8..9d21495a8433 100644 --- a/drivers/iommu/rockchip-iommu.c +++ b/drivers/iommu/rockchip-iommu.c @@ -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;
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 <marc.zyngier@arm.com> --- drivers/iommu/rockchip-iommu.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)