[Beginning of thread: https://lore.kernel.org/r/20220517220816.1635044-2-tansuresh@xxxxxxxxxx] On Wed, May 18, 2022 at 12:50:02PM -0500, Bjorn Helgaas wrote: > Devices have this async_suspend bit: > > struct device { > struct dev_pm_info { > unsigned int async_suspend:1; > > Drivers call device_enable_async_suspend() to set async_suspend if > they want it. The system suspend path is something like this: > > suspend_enter > dpm_suspend_noirq(PMSG_SUSPEND) > dpm_noirq_suspend_devices(PMSG_SUSPEND) > pm_transition = PMSG_SUSPEND > while (!list_empty(&dpm_late_early_list)) > device_suspend_noirq(dev) > dpm_async_fn(dev, async_suspend_noirq) > if (is_async(dev)) > async_schedule_dev(async_suspend_noirq) # async path > > async_suspend_noirq # called asynchronously > __device_suspend_noirq(dev, PMSG_SUSPEND, true) > callback = pm_noirq_op(PMSG_SUSPEND) # .suspend_noirq() > dpm_run_callback(callback) # async call > > __device_suspend_noirq(dev, pm_transition, false) # sync path > callback = pm_noirq_op(PMSG_SUSPEND) # .suspend_noirq() > dpm_run_callback(callback) # sync call > > async_synchronize_full # wait > > If a driver has called device_enable_async_suspend(), we'll use the > async_schedule_dev() path to schedule the appropriate .suspend_noirq() > method. After scheduling it via the async path or directly calling it > via the sync path, the async_synchronize_full() waits for completion > of all the async methods. Correct me if I'm wrong: in the suspend scenario, there are several phases, and async_synchronize_full() ensures that one phase finishes before the next phase starts. For example: dpm_suspend_end dpm_suspend_late # phase 1 while (!list_empty(&dpm_suspended_list)) device_suspend_late async_synchronize_full # finish phase 1 dpm_suspend_noirq # phase 2 dpm_noirq_suspend_devices while (!list_empty(&dpm_late_early_list)) device_suspend_noirq async_synchronize_full The device .suspend_late() and .suspend_noirq() methods may all be started asynchronously. So far there's nothing to order them within the phase, but async_synchronize_full() ensures that all the .suspend_late() methods finish before the .suspend_noirq() methods start. Obviously we do want a child's method to complete before we run the parent's method. If I understand correctly, that parent/child synchronization is done by a different method: __device_suspend_late() and __device_suspend_noirq() call dpm_wait_for_subordinate(), which waits for &dev->power.completion for all children: __device_suspend_late dpm_wait_for_subordinate dpm_wait_for_children # wait for children .suspend_late() device_for_each_child(dev, &async, dpm_wait_fn) dpm_wait_fn dpm_wait wait_for_completion(&dev->power.completion) dpm_run_callback # run parent method, e.g., ops->suspend_late complete_all(&dev->power.completion) # note completion of parent > I assume your suggestion is to do something like this: > > struct device { > struct dev_pm_info { > unsigned int async_suspend:1; > + unsigned int async_shutdown:1; > > + void device_enable_async_shutdown(struct device *dev) > + dev->power.async_shutdown = true; > > device_shutdown > while (!list_empty(&devices_kset->list)) > - dev->...->shutdown() > + if (is_async_shutdown(dev)) > + async_schedule_dev(async_shutdown) # async path > + > + async_shutdown # called asynchronously > + dev->...->shutdown() > + > + else > + dev->...->shutdown() # sync path > + > + async_synchronize_full # wait In the shutdown case, I think we still probably need the async_synchronize_full() to ensure that all the .shutdown() methods complete before we turn the power off, reboot, or kexec. But I think we also need a mechanism like dev->power.completion to make sure all the child .shutdown() methods complete before we run a parent's .shutdown(). There's not much overlap between the suspend path and the shutdown path (probably none at all), so it's tempting to use the existing dev->power.completion for shutdown as well. But I don't think that's feasible because dev->power.completion is tied up with dev->power.async_suspend, which is set by device_enable_async_suspend(). That's a different concept than async shutdown, and drivers will want one without the other. Does this make sense? Bjorn
