| Message ID | 20180807085406.3863-2-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu/rockchip: Runtime PM fixes | expand |
Hi Marc, Am Dienstag, 7. August 2018, 10:54:05 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, the Rockchip IOMMU driver tends to conflate the two things > by considering a non-zero return value as successful. > > This has the consequence of hiding other bugs, so let's handle this > case throughout the driver, with a WARN_ON_ONCE so that we can try > and work out what happened. > > Fixes: 0f181d3cf7d98 ("iommu/rockchip: Add runtime PM support") > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> I'm still not sure about the !CONFIG_PM case, as it was probably silently working in that case before. But on the other hand we're also already running over it in other places like in the iommu-shutdown and I guess if someone _really_ disabled CONFIG_PM, a lot of additional stuff would fail anyway. So should we wrap that in some #ifdef magic, just ignore it or simply select PM similar to what Tegra, Renesas and Vexpress seem to do? I guess I like the 3rd option best ;-) Heiko > drivers/iommu/rockchip-iommu.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c > index 054cd2c8e9c8..4e0f9b61cd7f 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 (WARN_ON_ONCE(err <= 0)) > + return ret; > > if (WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks))) > goto out; > @@ -620,11 +621,15 @@ static void rk_iommu_zap_iova(struct rk_iommu_domain *rk_domain, > spin_lock_irqsave(&rk_domain->iommus_lock, flags); > list_for_each(pos, &rk_domain->iommus) { > struct rk_iommu *iommu; > + int ret; > > iommu = list_entry(pos, struct rk_iommu, node); > > /* Only zap TLBs of IOMMUs that are powered on. */ > - if (pm_runtime_get_if_in_use(iommu->dev)) { > + ret = pm_runtime_get_if_in_use(iommu->dev); > + if (WARN_ON_ONCE(ret < 0)) > + continue; > + if (ret) { > WARN_ON(clk_bulk_enable(iommu->num_clocks, > iommu->clocks)); > rk_iommu_zap_lines(iommu, iova, size); > @@ -891,6 +896,7 @@ static void rk_iommu_detach_device(struct iommu_domain *domain, > struct rk_iommu *iommu; > struct rk_iommu_domain *rk_domain = to_rk_domain(domain); > unsigned long flags; > + int ret; > > /* Allow 'virtual devices' (eg drm) to detach from domain */ > iommu = rk_iommu_from_dev(dev); > @@ -909,7 +915,9 @@ static void rk_iommu_detach_device(struct iommu_domain *domain, > list_del_init(&iommu->node); > spin_unlock_irqrestore(&rk_domain->iommus_lock, flags); > > - if (pm_runtime_get_if_in_use(iommu->dev)) { > + ret = pm_runtime_get_if_in_use(iommu->dev); > + WARN_ON_ONCE(ret < 0); > + if (ret > 0) { > rk_iommu_disable(iommu); > pm_runtime_put(iommu->dev); > } > @@ -946,7 +954,8 @@ static int rk_iommu_attach_device(struct iommu_domain *domain, > list_add_tail(&iommu->node, &rk_domain->iommus); > spin_unlock_irqrestore(&rk_domain->iommus_lock, flags); > > - if (!pm_runtime_get_if_in_use(iommu->dev)) > + ret = pm_runtime_get_if_in_use(iommu->dev); > + if (!ret || WARN_ON_ONCE(ret < 0)) > return 0; > ret = rk_iommu_enable(iommu); >
On 07/08/18 13:09, Heiko Stuebner wrote: > Hi Marc, > > Am Dienstag, 7. August 2018, 10:54:05 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, the Rockchip IOMMU driver tends to conflate the two things >> by considering a non-zero return value as successful. >> >> This has the consequence of hiding other bugs, so let's handle this >> case throughout the driver, with a WARN_ON_ONCE so that we can try >> and work out what happened. >> >> Fixes: 0f181d3cf7d98 ("iommu/rockchip: Add runtime PM support") >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > > I'm still not sure about the !CONFIG_PM case, as it was probably silently > working in that case before Do we agree that this is an orthogonal problem though? > > But on the other hand we're also already running over it in other places > like in the iommu-shutdown and I guess if someone _really_ disabled > CONFIG_PM, a lot of additional stuff would fail anyway. > > So should we wrap that in some #ifdef magic, just ignore it or simply > select PM similar to what Tegra, Renesas and Vexpress seem to do? > > I guess I like the 3rd option best ;-) It probably doesn't hurt. At what level do you want it? As a dependency to the IOMMU? or to the platform? Thanks, M.
Am Dienstag, 7. August 2018, 14:31:49 CEST schrieb Marc Zyngier: > On 07/08/18 13:09, Heiko Stuebner wrote: > > Hi Marc, > > > > Am Dienstag, 7. August 2018, 10:54:05 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, the Rockchip IOMMU driver tends to conflate the two things > >> by considering a non-zero return value as successful. > >> > >> This has the consequence of hiding other bugs, so let's handle this > >> case throughout the driver, with a WARN_ON_ONCE so that we can try > >> and work out what happened. > >> > >> Fixes: 0f181d3cf7d98 ("iommu/rockchip: Add runtime PM support") > >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > > > > I'm still not sure about the !CONFIG_PM case, as it was probably silently > > working in that case before > > Do we agree that this is an orthogonal problem though? Nope ;-) .... I.e. right now the code ignores the -EINVAL from disabled PM and continues, possibly even handling the irq correctly. If it actually worked is a different matter, as I guess nobody really tried with !PM in the past. Now with error-handling we always return IRQ_NONE for !PM. > > But on the other hand we're also already running over it in other places > > like in the iommu-shutdown and I guess if someone _really_ disabled > > CONFIG_PM, a lot of additional stuff would fail anyway. > > > > So should we wrap that in some #ifdef magic, just ignore it or simply > > select PM similar to what Tegra, Renesas and Vexpress seem to do? > > > > I guess I like the 3rd option best ;-) > > It probably doesn't hurt. At what level do you want it? As a dependency > to the IOMMU? or to the platform? I guess it might be best to go the Tegra, etc way. Whoever in their right mind would want to drive a mobile platform without any form for power management ;-) . I can do these patches for arm32+arm64 myself ... I just wanted to put that thought out there - in case that was just a stupid idea of mine :-D . Heiko
On 07/08/18 14:15, Heiko Stuebner wrote: > Am Dienstag, 7. August 2018, 14:31:49 CEST schrieb Marc Zyngier: >> On 07/08/18 13:09, Heiko Stuebner wrote: >>> Hi Marc, >>> >>> Am Dienstag, 7. August 2018, 10:54:05 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, the Rockchip IOMMU driver tends to conflate the two things >>>> by considering a non-zero return value as successful. >>>> >>>> This has the consequence of hiding other bugs, so let's handle this >>>> case throughout the driver, with a WARN_ON_ONCE so that we can try >>>> and work out what happened. >>>> >>>> Fixes: 0f181d3cf7d98 ("iommu/rockchip: Add runtime PM support") >>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >>> >>> I'm still not sure about the !CONFIG_PM case, as it was probably silently >>> working in that case before >> >> Do we agree that this is an orthogonal problem though? > > Nope ;-) .... I.e. right now the code ignores the -EINVAL from disabled PM > and continues, possibly even handling the irq correctly. Ah, I now see what you mean. Yeah, this is a bit rubbish. It would have been better if the API returned something more sensible in that case, but that's a bit late... > If it actually worked is a different matter, as I guess nobody really tried > with !PM in the past. I don't think anyone noticed. !CONFIG_PM on something like rk3399 probably isn't very popular, and certainly comes for free on a multiplatform kernel. > Now with error-handling we always return IRQ_NONE for !PM. Yup. >>> But on the other hand we're also already running over it in other places >>> like in the iommu-shutdown and I guess if someone _really_ disabled >>> CONFIG_PM, a lot of additional stuff would fail anyway. >>> >>> So should we wrap that in some #ifdef magic, just ignore it or simply >>> select PM similar to what Tegra, Renesas and Vexpress seem to do? >>> >>> I guess I like the 3rd option best ;-) >> >> It probably doesn't hurt. At what level do you want it? As a dependency >> to the IOMMU? or to the platform? > > I guess it might be best to go the Tegra, etc way. Whoever in their right > mind would want to drive a mobile platform without any form for power > management ;-) . > > I can do these patches for arm32+arm64 myself ... I just wanted to put > that thought out there - in case that was just a stupid idea of mine :-D . Not stupid at all. Regarding this very patch: where do you want me to take it? M.
Am Dienstag, 7. August 2018, 16:25:53 CEST schrieb Marc Zyngier: > On 07/08/18 14:15, Heiko Stuebner wrote: > > Am Dienstag, 7. August 2018, 14:31:49 CEST schrieb Marc Zyngier: > >> On 07/08/18 13:09, Heiko Stuebner wrote: > >>> Hi Marc, > >>> > >>> Am Dienstag, 7. August 2018, 10:54:05 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, the Rockchip IOMMU driver tends to conflate the two things > >>>> by considering a non-zero return value as successful. > >>>> > >>>> This has the consequence of hiding other bugs, so let's handle this > >>>> case throughout the driver, with a WARN_ON_ONCE so that we can try > >>>> and work out what happened. > >>>> > >>>> Fixes: 0f181d3cf7d98 ("iommu/rockchip: Add runtime PM support") > >>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > >>> > >>> I'm still not sure about the !CONFIG_PM case, as it was probably silently > >>> working in that case before > >> > >> Do we agree that this is an orthogonal problem though? > > > > Nope ;-) .... I.e. right now the code ignores the -EINVAL from disabled PM > > and continues, possibly even handling the irq correctly. > > Ah, I now see what you mean. Yeah, this is a bit rubbish. It would have > been better if the API returned something more sensible in that case, > but that's a bit late... > > > If it actually worked is a different matter, as I guess nobody really tried > > with !PM in the past. > > I don't think anyone noticed. !CONFIG_PM on something like rk3399 > probably isn't very popular, and certainly comes for free on a > multiplatform kernel. > > > Now with error-handling we always return IRQ_NONE for !PM. > > Yup. > > >>> But on the other hand we're also already running over it in other places > >>> like in the iommu-shutdown and I guess if someone _really_ disabled > >>> CONFIG_PM, a lot of additional stuff would fail anyway. > >>> > >>> So should we wrap that in some #ifdef magic, just ignore it or simply > >>> select PM similar to what Tegra, Renesas and Vexpress seem to do? > >>> > >>> I guess I like the 3rd option best ;-) > >> > >> It probably doesn't hurt. At what level do you want it? As a dependency > >> to the IOMMU? or to the platform? > > > > I guess it might be best to go the Tegra, etc way. Whoever in their right > > mind would want to drive a mobile platform without any form for power > > management ;-) . > > > > I can do these patches for arm32+arm64 myself ... I just wanted to put > > that thought out there - in case that was just a stupid idea of mine :-D . > > Not stupid at all. Regarding this very patch: where do you want me to > take it? If you want to add select PM for Rockchip yourself (32+64 bit), just send them regularly and maybe include arm@kernel.org directly, so they can apply them directly, with just a reviewed-by tag from me. Heiko
Am Dienstag, 7. August 2018, 10:54:05 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, the Rockchip IOMMU driver tends to conflate the two things > by considering a non-zero return value as successful. > > This has the consequence of hiding other bugs, so let's handle this > case throughout the driver, with a WARN_ON_ONCE so that we can try > and work out what happened. > > Fixes: 0f181d3cf7d98 ("iommu/rockchip: Add runtime PM support") > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> With Rockchip platforms always selecting PM [see other longer thread in reply to the patch] Reviewed-by: Heiko Stuebner <heiko@sntech.de>
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c index 054cd2c8e9c8..4e0f9b61cd7f 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 (WARN_ON_ONCE(err <= 0)) + return ret; if (WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks))) goto out; @@ -620,11 +621,15 @@ static void rk_iommu_zap_iova(struct rk_iommu_domain *rk_domain, spin_lock_irqsave(&rk_domain->iommus_lock, flags); list_for_each(pos, &rk_domain->iommus) { struct rk_iommu *iommu; + int ret; iommu = list_entry(pos, struct rk_iommu, node); /* Only zap TLBs of IOMMUs that are powered on. */ - if (pm_runtime_get_if_in_use(iommu->dev)) { + ret = pm_runtime_get_if_in_use(iommu->dev); + if (WARN_ON_ONCE(ret < 0)) + continue; + if (ret) { WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks)); rk_iommu_zap_lines(iommu, iova, size); @@ -891,6 +896,7 @@ static void rk_iommu_detach_device(struct iommu_domain *domain, struct rk_iommu *iommu; struct rk_iommu_domain *rk_domain = to_rk_domain(domain); unsigned long flags; + int ret; /* Allow 'virtual devices' (eg drm) to detach from domain */ iommu = rk_iommu_from_dev(dev); @@ -909,7 +915,9 @@ static void rk_iommu_detach_device(struct iommu_domain *domain, list_del_init(&iommu->node); spin_unlock_irqrestore(&rk_domain->iommus_lock, flags); - if (pm_runtime_get_if_in_use(iommu->dev)) { + ret = pm_runtime_get_if_in_use(iommu->dev); + WARN_ON_ONCE(ret < 0); + if (ret > 0) { rk_iommu_disable(iommu); pm_runtime_put(iommu->dev); } @@ -946,7 +954,8 @@ static int rk_iommu_attach_device(struct iommu_domain *domain, list_add_tail(&iommu->node, &rk_domain->iommus); spin_unlock_irqrestore(&rk_domain->iommus_lock, flags); - if (!pm_runtime_get_if_in_use(iommu->dev)) + ret = pm_runtime_get_if_in_use(iommu->dev); + if (!ret || WARN_ON_ONCE(ret < 0)) return 0; ret = rk_iommu_enable(iommu);
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, the Rockchip IOMMU driver tends to conflate the two things by considering a non-zero return value as successful. This has the consequence of hiding other bugs, so let's handle this case throughout the driver, with a WARN_ON_ONCE so that we can try and work out what happened. Fixes: 0f181d3cf7d98 ("iommu/rockchip: Add runtime PM support") Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- drivers/iommu/rockchip-iommu.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)