| Message ID | 20220412224348.1038613-2-tansuresh@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Asynchronous shutdown interface and example implementation | expand |
On Wed, Apr 13, 2022 at 12:44 AM Tanjore Suresh <tansuresh@google.com> 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@google.com> Is there any specific reason why you didn't follow the design of, say, dpm_suspend(), where the "async" devices only need to have a flag set and the driver is not required to implement any new callbacks? IMO having different driver interfaces for asynchronous suspend and shutdown would be quite confusing for driver developers, wouldn't it? > --- > 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. > * > * @online: Called to put the device back online (after offlining it). > * @offline: Called to put the device offline for hot-removal. May fail. > @@ -93,6 +103,8 @@ struct bus_type { > void (*sync_state)(struct device *dev); > void (*remove)(struct device *dev); > void (*shutdown)(struct device *dev); > + void (*async_shutdown_start)(struct device *dev); > + void (*async_shutdown_end)(struct device *dev); > > int (*online)(struct device *dev); > int (*offline)(struct device *dev); > -- > 2.36.0.rc0.470.gd361397f0d-goog >
Rafael, That is a good observation, however, many of the use cases in data centers (deployment of devices in data centers) do not exploit device power management. Therefore, I'm not sure that is the right way to design this. Also if you look into device_shutdown (drivers/base/core.c) generic kernel routine does not exploit the shutdown method suggested by you instead use the generic shutdown methods defined in bus, class, drivers structures. Therefore, adopting to expand on those generic shutdown interfaces is the incremental choice to not confuse driver developers. Hope this clarifies why this extension is proposed when compared to overloading a generic shutdown entry point with device power management requirements. Thanks sureshtk On Fri, Apr 15, 2022 at 7:42 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Wed, Apr 13, 2022 at 12:44 AM Tanjore Suresh <tansuresh@google.com> 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@google.com> > > Is there any specific reason why you didn't follow the design of, say, > dpm_suspend(), where the "async" devices only need to have a flag set > and the driver is not required to implement any new callbacks? > > IMO having different driver interfaces for asynchronous suspend and > shutdown would be quite confusing for driver developers, wouldn't it? > > > --- > > 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. > > * > > * @online: Called to put the device back online (after offlining it). > > * @offline: Called to put the device offline for hot-removal. May fail. > > @@ -93,6 +103,8 @@ struct bus_type { > > void (*sync_state)(struct device *dev); > > void (*remove)(struct device *dev); > > void (*shutdown)(struct device *dev); > > + void (*async_shutdown_start)(struct device *dev); > > + void (*async_shutdown_end)(struct device *dev); > > > > int (*online)(struct device *dev); > > int (*offline)(struct device *dev); > > -- > > 2.36.0.rc0.470.gd361397f0d-goog > >
A: http://en.wikipedia.org/wiki/Top_post Q: Were do I find info about this thing called top-posting? A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in e-mail? A: No. Q: Should I include quotations after my reply? http://daringfireball.net/2007/07/on_top On Fri, Apr 29, 2022 at 11:03:07AM -0700, Tanjore Suresh wrote: > Rafael, > > That is a good observation, however, many of the use cases in data > centers (deployment of devices in data centers) do not exploit device > power management. Therefore, I'm not sure that is the right way to > design this. Yes it is, enable device power management and use that interface please. Devices in data centers should of course be doing the same thing as everyone else, as it actually saves real money in power costs. To not do so is very odd. thanks, greg k-h
On Sat, Apr 30, 2022 at 12:52 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > A: http://en.wikipedia.org/wiki/Top_post > Q: Were do I find info about this thing called top-posting? > A: Because it messes up the order in which people normally read text. > Q: Why is top-posting such a bad thing? > A: Top-posting. > Q: What is the most annoying thing in e-mail? > > A: No. > Q: Should I include quotations after my reply? > > http://daringfireball.net/2007/07/on_top > > On Fri, Apr 29, 2022 at 11:03:07AM -0700, Tanjore Suresh wrote: > > Rafael, > > > > That is a good observation, however, many of the use cases in data > > centers (deployment of devices in data centers) do not exploit device > > power management. Therefore, I'm not sure that is the right way to > > design this. > > Yes it is, enable device power management and use that interface please. > Devices in data centers should of course be doing the same thing as > everyone else, as it actually saves real money in power costs. To not > do so is very odd. > I guess we are intermixing the terminology of device power management with shutdown. My second, third reasoning in my previous e-mail, thought it brings out that difference. Maybe not. I will try one more time, my thought process on this one. This patch is only for shutdown. The shutdown can be done in a system in various flavors, (this may include a power being pulled from the system components when all the devices are quiescent and it can also be soft shutdown, where power is not removed from the system, but system could be attempting a reboot) The device power management allows the device to bring down any devices that may be idle to various power states that device may support in a selective manner & based on the transition allowed by the device. Such a transition initiated by the system can be achieved using the 'dpm' interface for runtime power management (more for extending laptop battery life). It can also be exploited for system sleep models (suspend and resume - where state is preserved and restarted from where it left off --> More applicable for laptops/desktops). That does not mean data center devices cannot exploit, but they worry about slight latency variation in any I/O initiated to any device. Such power management could introduce more latency when it transitions from one state to another. Therefore, the use case is more apt for Laptops, in certain cases desktops in my opinion or understanding. The shutdown entry point has been traditionally different and the semantics is that the whole system is going down to a quiescent state and power may be pulled or may not be, IMO, i am seeing both are independent requirements, in my view. Let me know if I am mistaken. I am not sure why we should break the shutdown semantics as understood by driver developers and overload it with dpm requirements? Thanks sureshtk > thanks, > > greg k-h
Greg On Mon, May 2, 2022 at 12:13 PM Tanjore Suresh <tansuresh@google.com> wrote: > > On Sat, Apr 30, 2022 at 12:52 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > A: http://en.wikipedia.org/wiki/Top_post > > Q: Were do I find info about this thing called top-posting? > > A: Because it messes up the order in which people normally read text. > > Q: Why is top-posting such a bad thing? > > A: Top-posting. > > Q: What is the most annoying thing in e-mail? > > > > A: No. > > Q: Should I include quotations after my reply? > > > > http://daringfireball.net/2007/07/on_top > > > > On Fri, Apr 29, 2022 at 11:03:07AM -0700, Tanjore Suresh wrote: > > > Rafael, > > > > > > That is a good observation, however, many of the use cases in data > > > centers (deployment of devices in data centers) do not exploit device > > > power management. Therefore, I'm not sure that is the right way to > > > design this. > > > > Yes it is, enable device power management and use that interface please. > > Devices in data centers should of course be doing the same thing as > > everyone else, as it actually saves real money in power costs. To not > > do so is very odd. > > > > I guess we are intermixing the terminology of device power management > with shutdown. > My second, third reasoning in my previous e-mail, thought it brings > out that difference. Maybe not. > I will try one more time, my thought process on this one. > > This patch is only for shutdown. The shutdown can be done in a system > in various flavors, > (this may include a power being pulled from the system components when > all the devices > are quiescent and it can also be soft shutdown, where power is not > removed from the system, but system > could be attempting a reboot) > > The device power management allows the device to bring down any > devices that may be idle to various power states that > device may support in a selective manner & based on the transition > allowed by the device. Such a transition initiated by > the system can be achieved using the 'dpm' interface for runtime power > management (more for extending laptop battery life). > It can also be exploited for system sleep models (suspend and resume - > where state is preserved and restarted from where it left off > --> More applicable for laptops/desktops). That does not mean data > center devices cannot exploit, but they worry about slight latency > variation in any > I/O initiated to any device. Such power management could introduce > more latency when it transitions from one state to another. > Therefore, the use case is more apt for Laptops, in certain cases > desktops in my opinion or understanding. > > The shutdown entry point has been traditionally different and the > semantics is that the whole system is going down to a > quiescent state and power may be pulled or may not be, IMO, i am > seeing both are independent requirements, in my view. > Let me know if I am mistaken. I am not sure why we should break the > shutdown semantics as understood by driver developers and > overload it with dpm requirements? > I have not seen additional comments, I request your help in moving this further, please. If i have missed something, Please let me know. Thanks sureshtk > Thanks > sureshtk > > > > thanks, > > > > greg k-h
On Wed, May 11, 2022 at 02:02:08PM -0700, Tanjore Suresh wrote: > Greg > > On Mon, May 2, 2022 at 12:13 PM Tanjore Suresh <tansuresh@google.com> wrote: > > > > On Sat, Apr 30, 2022 at 12:52 AM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > A: http://en.wikipedia.org/wiki/Top_post > > > Q: Were do I find info about this thing called top-posting? > > > A: Because it messes up the order in which people normally read text. > > > Q: Why is top-posting such a bad thing? > > > A: Top-posting. > > > Q: What is the most annoying thing in e-mail? > > > > > > A: No. > > > Q: Should I include quotations after my reply? > > > > > > http://daringfireball.net/2007/07/on_top > > > > > > On Fri, Apr 29, 2022 at 11:03:07AM -0700, Tanjore Suresh wrote: > > > > Rafael, > > > > > > > > That is a good observation, however, many of the use cases in data > > > > centers (deployment of devices in data centers) do not exploit device > > > > power management. Therefore, I'm not sure that is the right way to > > > > design this. > > > > > > Yes it is, enable device power management and use that interface please. > > > Devices in data centers should of course be doing the same thing as > > > everyone else, as it actually saves real money in power costs. To not > > > do so is very odd. > > > > > > > I guess we are intermixing the terminology of device power management > > with shutdown. > > My second, third reasoning in my previous e-mail, thought it brings > > out that difference. Maybe not. > > I will try one more time, my thought process on this one. > > > > This patch is only for shutdown. The shutdown can be done in a system > > in various flavors, > > (this may include a power being pulled from the system components when > > all the devices > > are quiescent and it can also be soft shutdown, where power is not > > removed from the system, but system > > could be attempting a reboot) > > > > The device power management allows the device to bring down any > > devices that may be idle to various power states that > > device may support in a selective manner & based on the transition > > allowed by the device. Such a transition initiated by > > the system can be achieved using the 'dpm' interface for runtime power > > management (more for extending laptop battery life). > > It can also be exploited for system sleep models (suspend and resume - > > where state is preserved and restarted from where it left off > > --> More applicable for laptops/desktops). That does not mean data > > center devices cannot exploit, but they worry about slight latency > > variation in any > > I/O initiated to any device. Such power management could introduce > > more latency when it transitions from one state to another. > > Therefore, the use case is more apt for Laptops, in certain cases > > desktops in my opinion or understanding. > > > > The shutdown entry point has been traditionally different and the > > semantics is that the whole system is going down to a > > quiescent state and power may be pulled or may not be, IMO, i am > > seeing both are independent requirements, in my view. > > Let me know if I am mistaken. I am not sure why we should break the > > shutdown semantics as understood by driver developers and > > overload it with dpm requirements? > > > > I have not seen additional comments, I request your help > in moving this further, please. If i have missed something, > Please let me know. Please rebase and resubmit your series with the extra information you have provided here in the changelog text so we can review it again. The patch series is long gone from my queue, sorry. thanks, greg k-h
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. * * @online: Called to put the device back online (after offlining it). * @offline: Called to put the device offline for hot-removal. May fail. @@ -93,6 +103,8 @@ struct bus_type { void (*sync_state)(struct device *dev); void (*remove)(struct device *dev); void (*shutdown)(struct device *dev); + void (*async_shutdown_start)(struct device *dev); + void (*async_shutdown_end)(struct device *dev); int (*online)(struct device *dev); int (*offline)(struct device *dev);
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@google.com> --- drivers/base/core.c | 38 +++++++++++++++++++++++++++++++++++++- include/linux/device/bus.h | 12 ++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-)