Re: [PATCH v2] PM: runtime: Unify error handling during suspend and resume

[Ulf Hansson]

On Tue, 25 Feb 2025 at 18:06, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>
> There is a confusing difference in error handling between rpm_suspend()
> and rpm_resume() related to the special way in which -EAGAIN and -EBUSY
> error values are treated by the former.  Also, converting -EACCES coming
> from the callback to I/O error, which it quite likely is not, may
> confuse runtime PM users.
>
> To address the above, modify rpm_callback() to convert -EACCES coming
> from the driver to -EAGAIN and to set power.runtime_error only if the
> return value is not -EAGAIN or -EBUSY.
>
> This will cause the error handling in rpm_resume() and rpm_suspend() to
> work consistently, so drop the no longer needed -EAGAIN or -EBUSY
> special case from the latter and make it retry autosuspend if
> power.runtime_error is unset.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>

Seems reasonable to me!

Reviewed-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>

Kind regards
Uffe


> ---
>
> v1 -> v2: Add comment explaining the -EACCES error code conversion (Raag)
>
> ---
>  drivers/base/power/runtime.c |   40 ++++++++++++++++++++++++----------------
>  1 file changed, 24 insertions(+), 16 deletions(-)
>
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -448,8 +448,19 @@
>                 retval = __rpm_callback(cb, dev);
>         }
>
> -       dev->power.runtime_error = retval;
> -       return retval != -EACCES ? retval : -EIO;
> +       /*
> +        * Since -EACCES means that runtime PM is disabled for the given device,
> +        * it should not be returned by runtime PM callbacks.  If it is returned
> +        * nevertheless, assume it to be a transient error and convert it to
> +        * -EAGAIN.
> +        */
> +       if (retval == -EACCES)
> +               retval = -EAGAIN;
> +
> +       if (retval != -EAGAIN && retval != -EBUSY)
> +               dev->power.runtime_error = retval;
> +
> +       return retval;
>  }
>
>  /**
> @@ -725,21 +736,18 @@
>         dev->power.deferred_resume = false;
>         wake_up_all(&dev->power.wait_queue);
>
> -       if (retval == -EAGAIN || retval == -EBUSY) {
> -               dev->power.runtime_error = 0;
> +       /*
> +        * On transient errors, if the callback routine failed an autosuspend,
> +        * and if the last_busy time has been updated so that there is a new
> +        * autosuspend expiration time, automatically reschedule another
> +        * autosuspend.
> +        */
> +       if (!dev->power.runtime_error && (rpmflags & RPM_AUTO) &&
> +           pm_runtime_autosuspend_expiration(dev) != 0)
> +               goto repeat;
> +
> +       pm_runtime_cancel_pending(dev);
>
> -               /*
> -                * If the callback routine failed an autosuspend, and
> -                * if the last_busy time has been updated so that there
> -                * is a new autosuspend expiration time, automatically
> -                * reschedule another autosuspend.
> -                */
> -               if ((rpmflags & RPM_AUTO) &&
> -                   pm_runtime_autosuspend_expiration(dev) != 0)
> -                       goto repeat;
> -       } else {
> -               pm_runtime_cancel_pending(dev);
> -       }
>         goto out;
>  }
>
>
>
>