Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1347094 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] PM / Domains: Propagate start and restore errors during runtime resume Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> - 2016-03-02 00:30 +0100
    Re: [PATCH] PM / Domains: Propagate start and restore errors during runtime resume Kevin Hilman <khilman@baylibre.com> - 2016-03-03 21:30 +0100
      Re: [PATCH] PM / Domains: Propagate start and restore errors during  runtime resume "Rafael J. Wysocki" <rafael@kernel.org> - 2016-03-03 21:40 +0100
        Re: [PATCH] PM / Domains: Propagate start and restore errors during runtime resume Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-03-03 21:50 +0100
      Re: [PATCH] PM / Domains: Propagate start and restore errors during  runtime resume "Rafael J. Wysocki" <rafael@kernel.org> - 2016-03-03 21:40 +0100
      Re: [PATCH] PM / Domains: Propagate start and restore errors during runtime resume Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-03-03 21:40 +0100
    Re: [PATCH] PM / Domains: Propagate start and restore errors during  runtime resume Ulf Hansson <ulf.hansson@linaro.org> - 2016-03-04 11:30 +0100
      Re: [PATCH] PM / Domains: Propagate start and restore errors during runtime resume Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-03-04 21:40 +0100
        Re: [PATCH] PM / Domains: Propagate start and restore errors during  runtime resume "Rafael J. Wysocki" <rafael@kernel.org> - 2016-03-04 22:30 +0100

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

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

[toc] | [next] | [standalone]

#1349557

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

#1349566

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1349560

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1350567

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web