Re: [PATCH v2] PM: runtime: avoid priority inversion on PREEMPT_RT

[Rafael J. Wysocki]

On Wed, Nov 17, 2021 at 7:37 PM John Keeping <john@xxxxxxxxxxxx> wrote:
>
> With PREEMPT_RT the cpu_relax() loops in rpm_suspend and rpm_resume can
> cause unbounded latency if they preempt an asynchronous suspend.  The
> main scenario where this can happen is when a realtime thread resumes a
> device while it is asynchronously suspending on a worker thread.
>
> I'm not convinced this can actually happen in the rpm_suspend case, or
> at least it's a lot less likely for a synchronous suspend to run at the
> same time as an asynchronous suspend, but both functions are updated
> here for symmetry.
>
> For devices setting power.irq_safe, it is possible that RPM functions
> will be called with a spinlock held (for example in
> pl330_issue_pending()).  This means a normal call to schedule() can't be
> used, but to avoid the priority inversion it is necessary to wait and
> schedule.  schedule_rtlock() is only available when CONFIG_PREEMPT_RT is
> defined, so even though the logic is correct without any preprocessor
> guards around schedule_rtlock(), they are necessary for compilation.
>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Signed-off-by: John Keeping <john@xxxxxxxxxxxx>
> ---
> Changes since v1:
> - Use schedule_rtlock() instead of schedule() for PREEMPT_RT & irq_safe
> - Rewritten commit description
>
>  drivers/base/power/runtime.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index f3de7bfc7f5b..fdf461bfae8c 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -596,7 +596,7 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>                         goto out;
>                 }
>
> -               if (dev->power.irq_safe) {
> +               if (dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT)) {

Please add a helper to avoid code duplication related to this (even
though there is a small amount of it).

>                         spin_unlock(&dev->power.lock);
>
>                         cpu_relax();
> @@ -614,7 +614,12 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>
>                         spin_unlock_irq(&dev->power.lock);
>
> -                       schedule();
> +#ifdef CONFIG_PREEMPT_RT
> +                       if (dev->power.irq_safe)
> +                               schedule_rtlock();
> +                       else
> +#endif

Same here, and please use the #ifdet inside the helper.

> +                               schedule();
>
>                         spin_lock_irq(&dev->power.lock);
>                 }
> @@ -779,7 +784,7 @@ static int rpm_resume(struct device *dev, int rpmflags)
>                         goto out;
>                 }
>
> -               if (dev->power.irq_safe) {
> +               if (dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT)) {
>                         spin_unlock(&dev->power.lock);
>
>                         cpu_relax();
> @@ -798,7 +803,12 @@ static int rpm_resume(struct device *dev, int rpmflags)
>
>                         spin_unlock_irq(&dev->power.lock);
>
> -                       schedule();
> +#ifdef CONFIG_PREEMPT_RT
> +                       if (dev->power.irq_safe)
> +                               schedule_rtlock();
> +                       else
> +#endif
> +                               schedule();
>
>                         spin_lock_irq(&dev->power.lock);
>                 }
> --