linux-kernel - Re: [PATCH] PM: runtime: Fix unpaired parent child_count for force

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8ee97450-a568-765f-77b8-d48a494f1da1@ideasonboard.com>
Date:   Fri, 7 May 2021 16:23:49 +0300
From:   Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To:     Tony Lindgren <tony@...mide.com>,
        "Rafael J . Wysocki" <rafael.j.wysocki@...el.com>
Cc:     Ulf Hansson <ulf.hansson@...aro.org>, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Sebastian Reichel <sebastian.reichel@...labora.com>
Subject: Re: [PATCH] PM: runtime: Fix unpaired parent child_count for
 force_resume

On 05/05/2021 14:09, Tony Lindgren wrote:
> As pm_runtime_need_not_resume() relies also on usage_count, it can return
> a different value in pm_runtime_force_suspend() compared to when called in
> pm_runtime_force_resume(). Different return values can happen if anything
> calls PM runtime functions in between, and causes the parent child_count
> to increase on every resume.
> 
> So far I've seen the issue only for omapdrm that does complicated things
> with PM runtime calls during system suspend for legacy reasons:
> 
> omap_atomic_commit_tail() for omapdrm.0
>   dispc_runtime_get()
>    wakes up 58000000.dss as it's the dispc parent
>     dispc_runtime_resume()
>      rpm_resume() increases parent child_count
>   dispc_runtime_put() won't idle, PM runtime suspend blocked
> pm_runtime_force_suspend() for 58000000.dss, !pm_runtime_need_not_resume()
>   __update_runtime_status()
> system suspended
> pm_runtime_force_resume() for 58000000.dss, pm_runtime_need_not_resume()
>   pm_runtime_enable() only called because of pm_runtime_need_not_resume()
> omap_atomic_commit_tail() for omapdrm.0
>   dispc_runtime_get()
>    wakes up 58000000.dss as it's the dispc parent
>     dispc_runtime_resume()
>      rpm_resume() increases parent child_count
>   dispc_runtime_put() won't idle, PM runtime suspend blocked
> ...
> rpm_suspend for 58000000.dss but parent child_count is now unbalanced
> 
> Let's fix the issue by adding a flag for needs_force_resume and use it in
> pm_runtime_force_resume() instead of pm_runtime_need_not_resume().
> 
> Additionally omapdrm system suspend could be simplified later on to avoid
> lots of unnecessary PM runtime calls and the complexity it adds. The
> driver can just use internal functions that are shared between the PM
> runtime and system suspend related functions.
> 
> Fixes: 4918e1f87c5f ("PM / runtime: Rework pm_runtime_force_suspend/resume()")
> Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> Cc: Sebastian Reichel <sebastian.reichel@...labora.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
> Signed-off-by: Tony Lindgren <tony@...mide.com>
> ---
>   drivers/base/power/runtime.c | 10 +++++++---
>   include/linux/pm.h           |  1 +
>   2 files changed, 8 insertions(+), 3 deletions(-)

Tested on DRA76 EVM, with and without HDMI plugged in. I can see DSS 
shutting down properly by looking at the CM_DSS_DSS_CLKCTRL register.

Tested-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>

  Tomi