Re: [REPORT_ISSUE]: RK3399 pd power down failed

[Ulf Hansson <ulf.hansson@linaro.org>]

On Wed, 20 Jan 2021 at 10:30, zhangqing@rock-chips.com
<zhangqing@rock-chips.com> wrote:
>
> Hi, Heiko :
>
> In rk3399 evb board,  I found a probabilistic problem about PD. Turning off PD occasionally failed.
>
> log show：
> Open the vop
> #modetest -M rockchip -s 42@36:1536x2048 -P 31@36:1536x2048@AR24 -a
>
> close the vop
> #enter
>
>  # cat sys/kernel/debug/pm_genpd/pm_genpd_summary
> domain                          status          slaves
>     /device                                             runtime status
> ----------------------------------------------------------------------
> pd_vopl                         off
> pd_vopb                         on
>     /devices/platform/ff903f00.iommu                     suspended
>     /devices/platform/ff900000.vop                          suspended
>
> I have checked the codes and concluded that there is a window of time for PD to be closed when using the device link. Once queue_work is executed immediately,  PD power off may be failed.
> The process is as follows:
>
> VOP requests to power off PD:
> pm_runtime_put_sync(vop->dev)
>     -> rpm_idle(vop)
>         -> rpm_suspend(vop)
>             -> __update_runtime_status(dev, RPM_SUSPENDING)
>                 -> rpm_callback(vop)
>                     -> __rpm_callback(vop)
>                         -> do power off pd callback(genpd_power_off)
>                             -> list_for_each_entry(pdd, &genpd->dev_list, list_node), ff900000.vop: suspending， ff903f00.iommu : active，so not_suspended = 2 return -EBUSY; Not really power off PD。
>                                 -> Handle link device callbacks according to device link（rpm_put_suppliers）
>                                     -> pm_runtime_put(link->supplier)
>                                         -> queue_work(pm_wq, &dev->power.work), execute immediately
>                                             ->rpm_idle(iommu)
>                                                 -> rpm_suspend(iommu)
>                                                     -> rpm_callback(iommu)
>                                                         -> rk_iommu_suspend
>                                                             ->  do power off pd callback(genpd_power_off)
>                                                                 -> list_for_each_entry(pdd, &genpd->dev_list, list_node), ff900000.vop: suspending， ff903f00.iommu : suspending，so not_suspended = 2 return -EBUSY; Not really power off PD。
>                                                                     -> iommu do __update_runtime_status(dev, RPM_SUSPENDED)
>                                                                         -> vop do __update_runtime_status(dev, RPM_SUSPENDED)

So, rpm_suspend() tries to suspend the supplier device link via
rpm_put_suppliers(), before it has updated its consumer device's state
to RPM_SUSPENDED.

This looks worrying to me, both because it's seems wrong to allow a
supplier to be suspended before a consumers device's state has reached
RPM_SUSPENDED - but also because it's not consistent with the way we
treat parent/child devices. The child's state will always be set to
RPM_SUSPENDED, before we try to suspend its parent by calling
rpm_idle() for it in rpm_suspend().

Rafael, what's your take on this? Would it make sense to align the
behavior for consumer/supplier-links in rpm_suspend() according to
child/parents?

>
> So the whole process goes down, the PD is not really power off, but the state of the child devices is SUSPENDED.
> Pm_runtime_put (link->supplier) used to power off PD is asynchronous and is added to queue_work(pm_wq, &dev-bbb>wer-work);
> If the queue_work is queued, the PD shutdown will succeed
> The call is as follows:
>
> VOP requests to power off PD:
> pm_runtime_put_sync(vop->dev)
>     -> rpm_idle(vop)
>         -> rpm_suspend(vop)
>             -> __update_runtime_status(dev, RPM_SUSPENDING)
>                 -> rpm_callback(vop)
>                     -> __rpm_callback(vop)
>                         -> do power off pd callback(genpd_power_off)
>                             ->  list_for_each_entry(pdd, &genpd->dev_list, list_node), ff900000.vop: suspending， ff903f00.iommu : active，so not_suspended = 2 return -EBUSY; Not really power off PD。
>                                  -> Handle link device callbacks according to device link（rpm_put_suppliers）
>                                     -> pm_runtime_put(link->supplier)
>                                          -> queue_work(pm_wq, &dev->power.work),
>                                              -> vop do  __update_runtime_status(dev, RPM_SUSPENDED)
>                                                 -> do queue_work (pm_runtime_work)
>                                                     ->  rpm_idle(iommu)
>                                                          -> rpm_suspend(iommu)
>                                                             -> do callback( rk_iommu_suspend)
>                                                                 -> do callback (genpd_power_off)
>                                                                     -> list_for_each_entry(pdd, &genpd->dev_list, list_node), ff900000.vop: suspended， ff903f00.iommu : suspending，so not_suspended = 1; do _genpd_power_off, Really turn off PD。
>                                                                         -> iommu do  __update_runtime_status(dev, RPM_SUSPENDED)
>

[...]

Kind regards
Uffe