Rafeal,
On Wed, May 18, 2022 at 11:01 AM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>
> On Wed, May 18, 2022 at 7:50 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> >
> > On Wed, May 18, 2022 at 01:38:49PM +0200, Rafael J. Wysocki wrote:
> > > On Wed, May 18, 2022 at 12:08 AM Tanjore Suresh <tansuresh@xxxxxxxxxx> wrote:
> > > >
> > > > This changes the bus driver interface with additional entry points
> > > > to enable devices to implement asynchronous shutdown. The existing
> > > > synchronous interface to shutdown is unmodified and retained for
> > > > backward compatibility.
> > > >
> > > > This changes the common device shutdown code to enable devices to
> > > > participate in asynchronous shutdown implementation.
> > > >
> > > > Signed-off-by: Tanjore Suresh <tansuresh@xxxxxxxxxx>
> > > > ---
> > > > drivers/base/core.c | 38 +++++++++++++++++++++++++++++++++++++-
> > > > include/linux/device/bus.h | 12 ++++++++++++
> > > > 2 files changed, 49 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > index 3d6430eb0c6a..ba267ae70a22 100644
> > > > --- a/drivers/base/core.c
> > > > +++ b/drivers/base/core.c
> > > > @@ -4479,6 +4479,7 @@ EXPORT_SYMBOL_GPL(device_change_owner);
> > > > void device_shutdown(void)
> > > > {
> > > > struct device *dev, *parent;
> > > > + LIST_HEAD(async_shutdown_list);
> > > >
> > > > wait_for_device_probe();
> > > > device_block_probing();
> > > > @@ -4523,7 +4524,13 @@ void device_shutdown(void)
> > > > dev_info(dev, "shutdown_pre\n");
> > > > dev->class->shutdown_pre(dev);
> > > > }
> > > > - if (dev->bus && dev->bus->shutdown) {
> > > > + if (dev->bus && dev->bus->async_shutdown_start) {
> > > > + if (initcall_debug)
> > > > + dev_info(dev, "async_shutdown_start\n");
> > > > + dev->bus->async_shutdown_start(dev);
> > > > + list_add_tail(&dev->kobj.entry,
> > > > + &async_shutdown_list);
> > > > + } else if (dev->bus && dev->bus->shutdown) {
> > > > if (initcall_debug)
> > > > dev_info(dev, "shutdown\n");
> > > > dev->bus->shutdown(dev);
> > > > @@ -4543,6 +4550,35 @@ void device_shutdown(void)
> > > > spin_lock(&devices_kset->list_lock);
> > > > }
> > > > spin_unlock(&devices_kset->list_lock);
> > > > +
> > > > + /*
> > > > + * Second pass spin for only devices, that have configured
> > > > + * Asynchronous shutdown.
> > > > + */
> > > > + while (!list_empty(&async_shutdown_list)) {
> > > > + dev = list_entry(async_shutdown_list.next, struct device,
> > > > + kobj.entry);
> > > > + parent = get_device(dev->parent);
> > > > + get_device(dev);
> > > > + /*
> > > > + * Make sure the device is off the list
> > > > + */
> > > > + list_del_init(&dev->kobj.entry);
> > > > + if (parent)
> > > > + device_lock(parent);
> > > > + device_lock(dev);
> > > > + if (dev->bus && dev->bus->async_shutdown_end) {
> > > > + if (initcall_debug)
> > > > + dev_info(dev,
> > > > + "async_shutdown_end called\n");
> > > > + dev->bus->async_shutdown_end(dev);
> > > > + }
> > > > + device_unlock(dev);
> > > > + if (parent)
> > > > + device_unlock(parent);
> > > > + put_device(dev);
> > > > + put_device(parent);
> > > > + }
> > > > }
> > > >
> > > > /*
> > > > diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h
> > > > index a039ab809753..f582c9d21515 100644
> > > > --- a/include/linux/device/bus.h
> > > > +++ b/include/linux/device/bus.h
> > > > @@ -49,6 +49,16 @@ struct fwnode_handle;
> > > > * will never get called until they do.
> > > > * @remove: Called when a device removed from this bus.
> > > > * @shutdown: Called at shut-down time to quiesce the device.
> > > > + * @async_shutdown_start: Called at the shutdown-time to start
> > > > + * the shutdown process on the device.
> > > > + * This entry point will be called only
> > > > + * when the bus driver has indicated it would
> > > > + * like to participate in asynchronous shutdown
> > > > + * completion.
> > > > + * @async_shutdown_end: Called at shutdown-time to complete the shutdown
> > > > + * process of the device. This entry point will be called
> > > > + * only when the bus drive has indicated it would like to
> > > > + * participate in the asynchronous shutdown completion.
> > >
> > > I'm going to repeat my point here, but only once.
> > >
> > > I see no reason to do async shutdown this way, instead of adding a
> > > flag for drivers to opt in for calling their existing shutdown
> > > callbacks asynchronously, in analogy with the async suspend and resume
> > > implementation.
> >
> > There's a lot of code here that mere mortals like myself don't
> > understand very well, so here's my meager understanding of how
> > async suspend works and what you're suggesting to make this a
> > little more concrete.
> >
> > 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.
> >
> > 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
>
> Yes, that's the idea IIUC.
Thanks for the clarification, I misunderstood your earlier comment,
thanks for explaining and clarification. Let me evaluate and get back
to you as soon as possible.
Thanks
sureshtk
