[PATCH] PM / Domains: Propagate start and restore errors during runtime resume

[PATCH] PM / Domains: Propagate start and restore errors during runtime resume

[Laurent Pinchart]

During runtime resume the return values of the start and restore steps
are ignored. As a result drivers are not notified of runtime resume
failures and can't propagate them up. Fix it by returning an error if
either the start or restore step fails, and clean up properly in the
error path.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/base/power/domain.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

This fixes an issue I've noticed with my driver's .runtime_resume() handler
returning an error that was never propagated out of pm_runtime_get_sync().

A second issue then appeared. The device .runtime_error field is set to the
error code returned by my .runtime_resume() handler, but it never reset. Any
subsequent try to resume the device fails with -EINVAL. I'm not sure what the
right way to solve that is, advices are welcome.

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 301b785f9f56..8cfcb8d6179b 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -485,8 +485,13 @@ static int pm_genpd_runtime_resume(struct device *dev)
 	if (timed && runtime_pm)
 		time_start = ktime_get();
 
-	genpd_start_dev(genpd, dev);
-	genpd_restore_dev(genpd, dev);
+	ret = genpd_start_dev(genpd, dev);
+	if (ret)
+		goto err_poweroff;
+
+	ret = genpd_restore_dev(genpd, dev);
+	if (ret)
+		goto err_stop;
 
 	/* Update resume latency value if the measured time exceeds it. */
 	if (timed && runtime_pm) {
@@ -501,6 +506,17 @@ static int pm_genpd_runtime_resume(struct device *dev)
 	}
 
 	return 0;
+
+err_stop:
+	genpd_stop_dev(genpd, dev);
+err_poweroff:
+	if (!dev->power.irq_safe) {
+		mutex_lock(&genpd->lock);
+		genpd_poweroff(genpd, 0);
+		mutex_unlock(&genpd->lock);
+	}
+
+	return ret;
 }
 
 static bool pd_ignore_unused;
-- 
Regards,

Laurent Pinchart

Re: [PATCH] PM / Domains: Propagate start and restore errors during runtime resume

[Kevin Hilman]

Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> writes:

> During runtime resume the return values of the start and restore steps
> are ignored. As a result drivers are not notified of runtime resume
> failures and can't propagate them up. Fix it by returning an error if
> either the start or restore step fails, and clean up properly in the
> error path.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/base/power/domain.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
>
> This fixes an issue I've noticed with my driver's .runtime_resume() handler
> returning an error that was never propagated out of pm_runtime_get_sync().

Acked-by: Kevin Hilman <khilman@baylibre.com>

> A second issue then appeared. The device .runtime_error field is set to the
> error code returned by my .runtime_resume() handler, but it never reset. Any
> subsequent try to resume the device fails with -EINVAL. I'm not sure what the
> right way to solve that is, advices are welcome.

Probably setting it (back) to zero after each successful runtime_suspend
or runtime_resume is the right way.  Rafael?

Kevin

Re: [PATCH] PM / Domains: Propagate start and restore errors during runtime resume

[Rafael J. Wysocki]

On Thu, Mar 3, 2016 at 9:24 PM, Kevin Hilman <khilman@baylibre.com> wrote:
> Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> writes:
>
>> During runtime resume the return values of the start and restore steps
>> are ignored. As a result drivers are not notified of runtime resume
>> failures and can't propagate them up. Fix it by returning an error if
>> either the start or restore step fails, and clean up properly in the
>> error path.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>> ---
>>  drivers/base/power/domain.c | 20 ++++++++++++++++++--
>>  1 file changed, 18 insertions(+), 2 deletions(-)
>>
>> This fixes an issue I've noticed with my driver's .runtime_resume() handler
>> returning an error that was never propagated out of pm_runtime_get_sync().
>
> Acked-by: Kevin Hilman <khilman@baylibre.com>
>
>> A second issue then appeared. The device .runtime_error field is set to the
>> error code returned by my .runtime_resume() handler, but it never reset. Any
>> subsequent try to resume the device fails with -EINVAL. I'm not sure what the
>> right way to solve that is, advices are welcome.
>
> Probably setting it (back) to zero after each successful runtime_suspend
> or runtime_resume is the right way.  Rafael?

That follows the assumption that runtime PM usually won't be reliable
after an error, so runtime_error has to be cleared explicitly via
pm_runtime_set_status().

Thanks,
Rafael

Re: [PATCH] PM / Domains: Propagate start and restore errors during runtime resume

[Laurent Pinchart]

Hi Kevin,

Thank you for the review.

On Thursday 03 March 2016 12:24:23 Kevin Hilman wrote:
> Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> writes:
> > During runtime resume the return values of the start and restore steps
> > are ignored. As a result drivers are not notified of runtime resume
> > failures and can't propagate them up. Fix it by returning an error if
> > either the start or restore step fails, and clean up properly in the
> > error path.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  drivers/base/power/domain.c | 20 ++++++++++++++++++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> > 
> > This fixes an issue I've noticed with my driver's .runtime_resume()
> > handler returning an error that was never propagated out of
> > pm_runtime_get_sync().
> 
> Acked-by: Kevin Hilman <khilman@baylibre.com>
> 
> > A second issue then appeared. The device .runtime_error field is set to
> > the error code returned by my .runtime_resume() handler, but it never
> > reset. Any subsequent try to resume the device fails with -EINVAL. I'm not
> > sure what the right way to solve that is, advices are welcome.
> 
> Probably setting it (back) to zero after each successful runtime_suspend
> or runtime_resume is the right way.  Rafael?

It would if you could try resuming again after a failed attempt, but you'll 
receive an error immediately if you try with .runtime_error set.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] PM / Domains: Propagate start and restore errors during runtime resume

[Rafael J. Wysocki]

On Thu, Mar 3, 2016 at 9:32 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, Mar 3, 2016 at 9:24 PM, Kevin Hilman <khilman@baylibre.com> wrote:
>> Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> writes:
>>
>>> During runtime resume the return values of the start and restore steps
>>> are ignored. As a result drivers are not notified of runtime resume
>>> failures and can't propagate them up. Fix it by returning an error if
>>> either the start or restore step fails, and clean up properly in the
>>> error path.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>> ---
>>>  drivers/base/power/domain.c | 20 ++++++++++++++++++--
>>>  1 file changed, 18 insertions(+), 2 deletions(-)
>>>
>>> This fixes an issue I've noticed with my driver's .runtime_resume() handler
>>> returning an error that was never propagated out of pm_runtime_get_sync().
>>
>> Acked-by: Kevin Hilman <khilman@baylibre.com>
>>
>>> A second issue then appeared. The device .runtime_error field is set to the
>>> error code returned by my .runtime_resume() handler, but it never reset. Any
>>> subsequent try to resume the device fails with -EINVAL. I'm not sure what the
>>> right way to solve that is, advices are welcome.
>>
>> Probably setting it (back) to zero after each successful runtime_suspend
>> or runtime_resume is the right way.  Rafael?
>
> That follows the assumption that runtime PM usually won't be reliable
> after an error, so runtime_error has to be cleared explicitly via
> pm_runtime_set_status().

Sorry, that won't work.

Anyway, the idea is that the error has to be cleared manually after a failure.

Re: [PATCH] PM / Domains: Propagate start and restore errors during runtime resume

[Laurent Pinchart]

Hi Rafael,

On Thursday 03 March 2016 21:34:04 Rafael J. Wysocki wrote:
> On Thu, Mar 3, 2016 at 9:32 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Thu, Mar 3, 2016 at 9:24 PM, Kevin Hilman <khilman@baylibre.com> wrote:
> >> Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> writes:
> >>> During runtime resume the return values of the start and restore steps
> >>> are ignored. As a result drivers are not notified of runtime resume
> >>> failures and can't propagate them up. Fix it by returning an error if
> >>> either the start or restore step fails, and clean up properly in the
> >>> error path.
> >>> 
> >>> Signed-off-by: Laurent Pinchart
> >>> <laurent.pinchart+renesas@ideasonboard.com>
> >>> ---
> >>> 
> >>>  drivers/base/power/domain.c | 20 ++++++++++++++++++--
> >>>  1 file changed, 18 insertions(+), 2 deletions(-)
> >>> 
> >>> This fixes an issue I've noticed with my driver's .runtime_resume()
> >>> handler returning an error that was never propagated out of
> >>> pm_runtime_get_sync().
> >> 
> >> Acked-by: Kevin Hilman <khilman@baylibre.com>
> >> 
> >>> A second issue then appeared. The device .runtime_error field is set to
> >>> the error code returned by my .runtime_resume() handler, but it never
> >>> reset. Any subsequent try to resume the device fails with -EINVAL. I'm
> >>> not sure what the right way to solve that is, advices are welcome.
> >> 
> >> Probably setting it (back) to zero after each successful runtime_suspend
> >> or runtime_resume is the right way.  Rafael?
> > 
> > That follows the assumption that runtime PM usually won't be reliable
> > after an error, so runtime_error has to be cleared explicitly via
> > pm_runtime_set_status().
> 
> Sorry, that won't work.
> 
> Anyway, the idea is that the error has to be cleared manually after a
> failure.

Thanks for the advice, I'll try doing so in my driver.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] PM / Domains: Propagate start and restore errors during runtime resume

[Ulf Hansson]

On 2 March 2016 at 00:20, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
> During runtime resume the return values of the start and restore steps
> are ignored. As a result drivers are not notified of runtime resume
> failures and can't propagate them up. Fix it by returning an error if
> either the start or restore step fails, and clean up properly in the
> error path.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/base/power/domain.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
>
> This fixes an issue I've noticed with my driver's .runtime_resume() handler
> returning an error that was never propagated out of pm_runtime_get_sync().
>
> A second issue then appeared. The device .runtime_error field is set to the
> error code returned by my .runtime_resume() handler, but it never reset. Any
> subsequent try to resume the device fails with -EINVAL. I'm not sure what the
> right way to solve that is, advices are welcome.
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 301b785f9f56..8cfcb8d6179b 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -485,8 +485,13 @@ static int pm_genpd_runtime_resume(struct device *dev)
>         if (timed && runtime_pm)
>                 time_start = ktime_get();
>
> -       genpd_start_dev(genpd, dev);
> -       genpd_restore_dev(genpd, dev);
> +       ret = genpd_start_dev(genpd, dev);
> +       if (ret)
> +               goto err_poweroff;
> +
> +       ret = genpd_restore_dev(genpd, dev);
> +       if (ret)
> +               goto err_stop;
>
>         /* Update resume latency value if the measured time exceeds it. */
>         if (timed && runtime_pm) {
> @@ -501,6 +506,17 @@ static int pm_genpd_runtime_resume(struct device *dev)
>         }
>
>         return 0;
> +
> +err_stop:
> +       genpd_stop_dev(genpd, dev);
> +err_poweroff:
> +       if (!dev->power.irq_safe) {

There's a helper function for this:
pm_runtime_is_irq_safe()

Perhaps, we can leave this as is here and then make a separate patch
converting all occurrences of the above into using the helper function
instead.

> +               mutex_lock(&genpd->lock);
> +               genpd_poweroff(genpd, 0);
> +               mutex_unlock(&genpd->lock);
> +       }
> +
> +       return ret;
>  }
>
>  static bool pd_ignore_unused;

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

Re: [PATCH] PM / Domains: Propagate start and restore errors during runtime resume

[Laurent Pinchart]

Hi Ulf,

Thank you for the review.

On Friday 04 March 2016 11:22:49 Ulf Hansson wrote:
> On 2 March 2016 at 00:20, Laurent Pinchart wrote:
> > During runtime resume the return values of the start and restore steps
> > are ignored. As a result drivers are not notified of runtime resume
> > failures and can't propagate them up. Fix it by returning an error if
> > either the start or restore step fails, and clean up properly in the
> > error path.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  drivers/base/power/domain.c | 20 ++++++++++++++++++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> > 
> > This fixes an issue I've noticed with my driver's .runtime_resume()
> > handler returning an error that was never propagated out of
> > pm_runtime_get_sync().
> > 
> > A second issue then appeared. The device .runtime_error field is set to
> > the error code returned by my .runtime_resume() handler, but it never
> > reset. Any subsequent try to resume the device fails with -EINVAL. I'm not
> > sure what the right way to solve that is, advices are welcome.
> > 
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index 301b785f9f56..8cfcb8d6179b 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -485,8 +485,13 @@ static int pm_genpd_runtime_resume(struct device
> > *dev)
> >         if (timed && runtime_pm)
> >                 time_start = ktime_get();
> > 
> > -       genpd_start_dev(genpd, dev);
> > -       genpd_restore_dev(genpd, dev);
> > +       ret = genpd_start_dev(genpd, dev);
> > +       if (ret)
> > +               goto err_poweroff;
> > +
> > +       ret = genpd_restore_dev(genpd, dev);
> > +       if (ret)
> > +               goto err_stop;
> > 
> >         /* Update resume latency value if the measured time exceeds it. */
> >         if (timed && runtime_pm) {
> > @@ -501,6 +506,17 @@ static int pm_genpd_runtime_resume(struct device
> > *dev)
> >         }
> >         
> >         return 0;
> > +
> > +err_stop:
> > +       genpd_stop_dev(genpd, dev);
> > +err_poweroff:
> > +       if (!dev->power.irq_safe) {
> 
> There's a helper function for this:
> pm_runtime_is_irq_safe()
> 
> Perhaps, we can leave this as is here and then make a separate patch
> converting all occurrences of the above into using the helper function
> instead.

If there are other occurrences a separate patch would make sense, I agree.

> > +               mutex_lock(&genpd->lock);
> > +               genpd_poweroff(genpd, 0);
> > +               mutex_unlock(&genpd->lock);
> > +       }
> > +
> > +       return ret;
> > 
> >  }
> >  
> >  static bool pd_ignore_unused;
> 
> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Thank you. Do you plan to take the patch in your tree for v4.6 ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] PM / Domains: Propagate start and restore errors during runtime resume

[Rafael J. Wysocki]

On Fri, Mar 4, 2016 at 9:38 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Ulf,
>
> Thank you for the review.
>
> On Friday 04 March 2016 11:22:49 Ulf Hansson wrote:
>> On 2 March 2016 at 00:20, Laurent Pinchart wrote:
>> > During runtime resume the return values of the start and restore steps
>> > are ignored. As a result drivers are not notified of runtime resume
>> > failures and can't propagate them up. Fix it by returning an error if
>> > either the start or restore step fails, and clean up properly in the
>> > error path.
>> >
>> > Signed-off-by: Laurent Pinchart
>> > <laurent.pinchart+renesas@ideasonboard.com>
>> > ---
>> >
>> >  drivers/base/power/domain.c | 20 ++++++++++++++++++--
>> >  1 file changed, 18 insertions(+), 2 deletions(-)
>> >
>> > This fixes an issue I've noticed with my driver's .runtime_resume()
>> > handler returning an error that was never propagated out of
>> > pm_runtime_get_sync().
>> >
>> > A second issue then appeared. The device .runtime_error field is set to
>> > the error code returned by my .runtime_resume() handler, but it never
>> > reset. Any subsequent try to resume the device fails with -EINVAL. I'm not
>> > sure what the right way to solve that is, advices are welcome.
>> >
>> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> > index 301b785f9f56..8cfcb8d6179b 100644
>> > --- a/drivers/base/power/domain.c
>> > +++ b/drivers/base/power/domain.c
>> > @@ -485,8 +485,13 @@ static int pm_genpd_runtime_resume(struct device
>> > *dev)
>> >         if (timed && runtime_pm)
>> >                 time_start = ktime_get();
>> >
>> > -       genpd_start_dev(genpd, dev);
>> > -       genpd_restore_dev(genpd, dev);
>> > +       ret = genpd_start_dev(genpd, dev);
>> > +       if (ret)
>> > +               goto err_poweroff;
>> > +
>> > +       ret = genpd_restore_dev(genpd, dev);
>> > +       if (ret)
>> > +               goto err_stop;
>> >
>> >         /* Update resume latency value if the measured time exceeds it. */
>> >         if (timed && runtime_pm) {
>> > @@ -501,6 +506,17 @@ static int pm_genpd_runtime_resume(struct device
>> > *dev)
>> >         }
>> >
>> >         return 0;
>> > +
>> > +err_stop:
>> > +       genpd_stop_dev(genpd, dev);
>> > +err_poweroff:
>> > +       if (!dev->power.irq_safe) {
>>
>> There's a helper function for this:
>> pm_runtime_is_irq_safe()
>>
>> Perhaps, we can leave this as is here and then make a separate patch
>> converting all occurrences of the above into using the helper function
>> instead.
>
> If there are other occurrences a separate patch would make sense, I agree.
>
>> > +               mutex_lock(&genpd->lock);
>> > +               genpd_poweroff(genpd, 0);
>> > +               mutex_unlock(&genpd->lock);
>> > +       }
>> > +
>> > +       return ret;
>> >
>> >  }
>> >
>> >  static bool pd_ignore_unused;
>>
>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> Thank you. Do you plan to take the patch in your tree for v4.6 ?

I'll do that.

Thanks,
Rafael