Re: [REPORT_ISSUE]: RK3399 pd power down failed

On Wed, 20 Jan 2021 at 10:30, zhangqing@rock-chips.com
[off-list ref] 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