[PATCH 0/2] iommu/rockchip: Runtime PM fixes

[PATCH 0/2] iommu/rockchip: Runtime PM fixes

[Marc Zyngier]

This small series addresses a couple of runtime PM issues I've spotted
while running 4.18 on a Chromebook Plus (kevin, rk3399) platform, and
specifically doing kexec.

Note that even with these two patches, kexec is still fairly broken on
rk3399, as the VOP is never turned off (see [1] for a fix).

[1] https://www.spinics.net/lists/arm-kernel/msg670229.html

Marc Zyngier (2):
  iommu/rockchip: Handle errors returned from PM framework
  iommu/rockchip: Move irq request past pm_runtime_enable

 drivers/iommu/rockchip-iommu.c | 45 +++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 17 deletions(-)

-- 
2.18.0

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

[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>
---
 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);
-- 
2.18.0

[PATCH 2/2] iommu/rockchip: Move irq request past pm_runtime_enable

[Marc Zyngier]

Enabling the interrupt early, before power has been applied to the
device, can result in an interrupt being delivered too early if:

- the IOMMU shares an interrupt with a VOP
- the VOP has a pending interrupt (after a kexec, for example)

In these conditions, we end-up taking the interrupt without
the IOMMU being ready to handle the interrupt (not powered on).

Moving the interrupt request past the pm_runtime_enable() call
makes sure we can at least access the IOMMU registers. Note that
this is only a partial fix, and that the VOP interrupt will still
be screaming until the VOP driver kicks in, which advocates for
a more synchronized interrupt enabling/disabling approach.

Fixes: 0f181d3cf7d98 ("iommu/rockchip: Add runtime PM support")
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/iommu/rockchip-iommu.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 4e0f9b61cd7f..2b1724e8d307 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1161,17 +1161,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");
 
@@ -1228,6 +1217,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);
-- 
2.18.0

Re: [PATCH 2/2] iommu/rockchip: Move irq request past pm_runtime_enable

[Heiko Stuebner]

Am Dienstag, 7. August 2018, 10:54:06 CEST schrieb Marc Zyngier:
> Enabling the interrupt early, before power has been applied to the
> device, can result in an interrupt being delivered too early if:
> 
> - the IOMMU shares an interrupt with a VOP
> - the VOP has a pending interrupt (after a kexec, for example)
> 
> In these conditions, we end-up taking the interrupt without
> the IOMMU being ready to handle the interrupt (not powered on).
> 
> Moving the interrupt request past the pm_runtime_enable() call
> makes sure we can at least access the IOMMU registers. Note that
> this is only a partial fix, and that the VOP interrupt will still
> be screaming until the VOP driver kicks in, which advocates for
> a more synchronized interrupt enabling/disabling approach.
> 
> Fixes: 0f181d3cf7d98 ("iommu/rockchip: Add runtime PM support")
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

Re: [PATCH 1/2] iommu/rockchip: Handle errors returned from PM framework

[Heiko Stuebner]

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

Re: [PATCH 1/2] iommu/rockchip: Handle errors returned from PM framework

[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?

> 
> 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.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/2] iommu/rockchip: Handle errors returned from PM framework

[Heiko Stuebner]

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

Re: [PATCH 1/2] iommu/rockchip: Handle errors returned from PM framework

[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?

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

Re: [PATCH 1/2] iommu/rockchip: Handle errors returned from PM framework

[Heiko Stuebner]

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

Re: [PATCH 1/2] iommu/rockchip: Handle errors returned from PM framework

[Heiko Stuebner]

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>