[RFC] ACPI PM during kernel poweroff/reboot

[RFC] ACPI PM during kernel poweroff/reboot

[Furquan Shaikh]

On x86 Chromebooks, we have observed this issue for a long time now -
when the system is powered off or rebooted, ACPI PM is not invoked and
this results in PowerResource _OFF methods not being invoked for any
of the devices. The _OFF methods are invoked correctly in case of
suspend-to-idle (S0ix) and suspend-to-memory(S3). However, they do not
get invoked when `poweroff` or `reboot` are triggered.

One of the differences between suspend, hibernate and shutdown paths
in Linux kernel is that the shutdown path does not use the typical
device PM phases (prepare, freeze/suspend, poweroff) as used by
suspend/hibernate. Instead the shutdown path makes use of
.shutdown_pre() and .shutdown() callbacks.

If I understand correctly, .shutdown() has been around for a long time
and existed even before the PM callbacks were added. Thus,
pm->poweroff() and .shutdown() are supposed to be analogous and
consistent in the behavior. This is why runtime PM is disallowed by
device_shutdown() before it calls .shutdown() (i.e. to keep behavior
consistent for both paths). However, in practice, there are
differences in behavior for the pm->poweroff() and .shutdown() paths
since the shutdown path does not execute any PM domain operations.

Because of this difference in behavior, shutdown path never invokes
ACPI PM and thus the ACPI PowerResources are not turned off when the
system is rebooted or powered off (sleep S5). On Chromebooks, it is
critical to run the _OFF methods for poweroff/reboot in order to
ensure that the device power off sequencing requirements are met.
Currently, these requirements are violated which impact the
reliability of devices over the lifetime of the platform.

There are a few ways in which this can be addressed:

1. Similar to the case of hibernation, a new
PMSG_POWEROFF/PM_EVENT_POWEROFF can be introduced to invoke device
power management phases using `dpm_suspend_start(PMSG_POWEROFF)` and
`dpm_suspend_end(PMSG_POWEROFF)`. However, as the shutdown path uses
the class/bus/driver .shutdown() callbacks, adding dpm phases for
poweroff complicates the order of operations. If the dpm phases are
run before .shutdown() callbacks, then it will result in the callbacks
accessing devices after they are powered off. If the .shutdown()
callbacks are run before dpm phases, then the pm->poweroff() calls are
made after the device shutdown is done. Since .shutdown() and
pm->poweroff() are supposed to be analogous, having both calls in the
shutdown path is not only redundant but also results in incorrect
behavior.

2. Another option is to update device_shutdown() to make
pm_domain.poweroff calls after the class/bus/driver .shutdown() is
done. However, this suffers from the same problem as #1 above i.e. it
is redundant and creates conflicting order of operations.

3. Third possible solution is to detach the device from the PM domain
after it is shutdown. Currently, device drivers perform a detach
operation only when the device is removed. However, in case of
poweroff/reboot as the device is already shutdown, detaching PM domain
will give it the opportunity to ensure that any power resources are
correctly turned off before the system shuts down.

Out of these, I think #3 makes the most sense as it does not introduce
any conflicting operations. I verified that the following diff results
in _OFF methods getting invoked in both poweroff and reboot cases:

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 94df2ba1bbed..e55d65fbb4a9 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -23,6 +23,7 @@
 #include <linux/of_device.h>
 #include <linux/genhd.h>
 #include <linux/mutex.h>
+#include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
 #include <linux/netdevice.h>
 #include <linux/sched/signal.h>
@@ -3230,6 +3231,8 @@ void device_shutdown(void)
                        dev->driver->shutdown(dev);
                }

+               dev_pm_domain_detach(dev, true);
+
                device_unlock(dev);
                if (parent)
                        device_unlock(parent);

This was discussed on the mailing list some time back[1] in the
context of a different use case. However, the idea of detaching
devices (on any bus) from the PM domain during shutdown is important
to ensure correct power sequencing for the devices.

One of the concerns that was raised on the above thread was slowing
down the shutdown process when not needed. I think this can be handled
by adding a sysfs attribute to allow platforms to decide if they need
the ability to power off PM domains on shutdown/reboot path.

Questions that I am looking to get feedback/comments are:

1. Is my assessment of the problem and understanding of the
.shutdown() and pm.poweroff() correct?
2. Does the solution #3 i.e. detaching PM domain after shutting down
device on shutdown path makes sense?
3. Are there other possible approaches to solve this problem that can
be explored?
4. Do we still have the performance concern about the shutdown path? I
don’t think anything has changed since that thread, so this is
probably still true.
5. Does the use of sysfs attribute make sense to let platform control
if it wants to detach PM domains on shutdown path?

Sorry about the long thread and thank you so much for your time!

Thanks,
Furquan

[1] https://lore.kernel.org/linux-pm/HE1PR04MB30046668C9F4FFAB5C07E693886D0@HE1PR04MB3004.eurprd04.prod.outlook.com/T/#mbd80804857f38c66aa5e825cdd4b61ba6b12317d

Re: [RFC] ACPI PM during kernel poweroff/reboot

[Furquan Shaikh]

On Thu, Nov 12, 2020 at 11:19 AM Furquan Shaikh <furquan@google.com> wrote:
>
> On x86 Chromebooks, we have observed this issue for a long time now -
> when the system is powered off or rebooted, ACPI PM is not invoked and
> this results in PowerResource _OFF methods not being invoked for any
> of the devices. The _OFF methods are invoked correctly in case of
> suspend-to-idle (S0ix) and suspend-to-memory(S3). However, they do not
> get invoked when `poweroff` or `reboot` are triggered.
>
> One of the differences between suspend, hibernate and shutdown paths
> in Linux kernel is that the shutdown path does not use the typical
> device PM phases (prepare, freeze/suspend, poweroff) as used by
> suspend/hibernate. Instead the shutdown path makes use of
> .shutdown_pre() and .shutdown() callbacks.
>
> If I understand correctly, .shutdown() has been around for a long time
> and existed even before the PM callbacks were added. Thus,
> pm->poweroff() and .shutdown() are supposed to be analogous and
> consistent in the behavior. This is why runtime PM is disallowed by
> device_shutdown() before it calls .shutdown() (i.e. to keep behavior
> consistent for both paths). However, in practice, there are
> differences in behavior for the pm->poweroff() and .shutdown() paths
> since the shutdown path does not execute any PM domain operations.
>
> Because of this difference in behavior, shutdown path never invokes
> ACPI PM and thus the ACPI PowerResources are not turned off when the
> system is rebooted or powered off (sleep S5). On Chromebooks, it is
> critical to run the _OFF methods for poweroff/reboot in order to
> ensure that the device power off sequencing requirements are met.
> Currently, these requirements are violated which impact the
> reliability of devices over the lifetime of the platform.
>
> There are a few ways in which this can be addressed:
>
> 1. Similar to the case of hibernation, a new
> PMSG_POWEROFF/PM_EVENT_POWEROFF can be introduced to invoke device
> power management phases using `dpm_suspend_start(PMSG_POWEROFF)` and
> `dpm_suspend_end(PMSG_POWEROFF)`. However, as the shutdown path uses
> the class/bus/driver .shutdown() callbacks, adding dpm phases for
> poweroff complicates the order of operations. If the dpm phases are
> run before .shutdown() callbacks, then it will result in the callbacks
> accessing devices after they are powered off. If the .shutdown()
> callbacks are run before dpm phases, then the pm->poweroff() calls are
> made after the device shutdown is done. Since .shutdown() and
> pm->poweroff() are supposed to be analogous, having both calls in the
> shutdown path is not only redundant but also results in incorrect
> behavior.
>
> 2. Another option is to update device_shutdown() to make
> pm_domain.poweroff calls after the class/bus/driver .shutdown() is
> done. However, this suffers from the same problem as #1 above i.e. it
> is redundant and creates conflicting order of operations.
>
> 3. Third possible solution is to detach the device from the PM domain
> after it is shutdown. Currently, device drivers perform a detach
> operation only when the device is removed. However, in case of
> poweroff/reboot as the device is already shutdown, detaching PM domain
> will give it the opportunity to ensure that any power resources are
> correctly turned off before the system shuts down.
>
> Out of these, I think #3 makes the most sense as it does not introduce
> any conflicting operations. I verified that the following diff results
> in _OFF methods getting invoked in both poweroff and reboot cases:
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 94df2ba1bbed..e55d65fbb4a9 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -23,6 +23,7 @@
>  #include <linux/of_device.h>
>  #include <linux/genhd.h>
>  #include <linux/mutex.h>
> +#include <linux/pm_domain.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/netdevice.h>
>  #include <linux/sched/signal.h>
> @@ -3230,6 +3231,8 @@ void device_shutdown(void)
>                         dev->driver->shutdown(dev);
>                 }
>
> +               dev_pm_domain_detach(dev, true);
> +
>                 device_unlock(dev);
>                 if (parent)
>                         device_unlock(parent);
>
> This was discussed on the mailing list some time back[1] in the
> context of a different use case. However, the idea of detaching
> devices (on any bus) from the PM domain during shutdown is important
> to ensure correct power sequencing for the devices.
>
> One of the concerns that was raised on the above thread was slowing
> down the shutdown process when not needed. I think this can be handled
> by adding a sysfs attribute to allow platforms to decide if they need
> the ability to power off PM domains on shutdown/reboot path.
>
> Questions that I am looking to get feedback/comments are:
>
> 1. Is my assessment of the problem and understanding of the
> .shutdown() and pm.poweroff() correct?
> 2. Does the solution #3 i.e. detaching PM domain after shutting down
> device on shutdown path makes sense?
> 3. Are there other possible approaches to solve this problem that can
> be explored?
> 4. Do we still have the performance concern about the shutdown path? I
> don’t think anything has changed since that thread, so this is
> probably still true.
> 5. Does the use of sysfs attribute make sense to let platform control
> if it wants to detach PM domains on shutdown path?
>
> Sorry about the long thread and thank you so much for your time!
>
> Thanks,
> Furquan
>
> [1] https://lore.kernel.org/linux-pm/HE1PR04MB30046668C9F4FFAB5C07E693886D0@HE1PR04MB3004.eurprd04.prod.outlook.com/T/#mbd80804857f38c66aa5e825cdd4b61ba6b12317d

Hello,

Gentle ping. Just wanted to check if there are any
comments/suggestions on the proposal above or how this problem can be
addressed. This has been one of the long standing problems impacting
all ACPI-based Chrome OS devices.

Thanks,
Furquan

Re: [RFC] ACPI PM during kernel poweroff/reboot

[Rafael J. Wysocki]

On Thu, Nov 12, 2020 at 8:19 PM Furquan Shaikh <furquan@google.com> wrote:
>
> On x86 Chromebooks, we have observed this issue for a long time now -
> when the system is powered off or rebooted, ACPI PM is not invoked and
> this results in PowerResource _OFF methods not being invoked for any
> of the devices. The _OFF methods are invoked correctly in case of
> suspend-to-idle (S0ix) and suspend-to-memory(S3). However, they do not
> get invoked when `poweroff` or `reboot` are triggered.
>
> One of the differences between suspend, hibernate and shutdown paths
> in Linux kernel is that the shutdown path does not use the typical
> device PM phases (prepare, freeze/suspend, poweroff) as used by
> suspend/hibernate. Instead the shutdown path makes use of
> .shutdown_pre() and .shutdown() callbacks.
>
> If I understand correctly, .shutdown() has been around for a long time
> and existed even before the PM callbacks were added. Thus,
> pm->poweroff() and .shutdown() are supposed to be analogous and
> consistent in the behavior.

Well, not quite.

->shutdown() is expected to be a lightweight operation also suitable
for kexec() and similar situations where ->poweroff() may not work.

> This is why runtime PM is disallowed by
> device_shutdown() before it calls .shutdown() (i.e. to keep behavior
> consistent for both paths). However, in practice, there are
> differences in behavior for the pm->poweroff() and .shutdown() paths
> since the shutdown path does not execute any PM domain operations.

That's correct.

> Because of this difference in behavior, shutdown path never invokes
> ACPI PM and thus the ACPI PowerResources are not turned off when the
> system is rebooted or powered off (sleep S5). On Chromebooks, it is
> critical to run the _OFF methods for poweroff/reboot in order to
> ensure that the device power off sequencing requirements are met.
> Currently, these requirements are violated which impact the
> reliability of devices over the lifetime of the platform.
>
> There are a few ways in which this can be addressed:
>
> 1. Similar to the case of hibernation, a new
> PMSG_POWEROFF/PM_EVENT_POWEROFF can be introduced to invoke device
> power management phases using `dpm_suspend_start(PMSG_POWEROFF)` and
> `dpm_suspend_end(PMSG_POWEROFF)`. However, as the shutdown path uses
> the class/bus/driver .shutdown() callbacks, adding dpm phases for
> poweroff complicates the order of operations. If the dpm phases are
> run before .shutdown() callbacks, then it will result in the callbacks
> accessing devices after they are powered off. If the .shutdown()
> callbacks are run before dpm phases, then the pm->poweroff() calls are
> made after the device shutdown is done. Since .shutdown() and
> pm->poweroff() are supposed to be analogous, having both calls in the
> shutdown path is not only redundant but also results in incorrect
> behavior.
>
> 2. Another option is to update device_shutdown() to make
> pm_domain.poweroff calls after the class/bus/driver .shutdown() is
> done. However, this suffers from the same problem as #1 above i.e. it
> is redundant and creates conflicting order of operations.
>
> 3. Third possible solution is to detach the device from the PM domain
> after it is shutdown. Currently, device drivers perform a detach
> operation only when the device is removed. However, in case of
> poweroff/reboot as the device is already shutdown, detaching PM domain
> will give it the opportunity to ensure that any power resources are
> correctly turned off before the system shuts down.

4. Make Chromebooks call something like hibernation_platform_enter()
on S5 entries (including reboot).

> Out of these, I think #3 makes the most sense as it does not introduce
> any conflicting operations. I verified that the following diff results
> in _OFF methods getting invoked in both poweroff and reboot cases:

This won't work for PCI devices though, only for devices in the ACPI
PM domain, so it is not sufficient in general.

> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 94df2ba1bbed..e55d65fbb4a9 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -23,6 +23,7 @@
>  #include <linux/of_device.h>
>  #include <linux/genhd.h>
>  #include <linux/mutex.h>
> +#include <linux/pm_domain.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/netdevice.h>
>  #include <linux/sched/signal.h>
> @@ -3230,6 +3231,8 @@ void device_shutdown(void)
>                         dev->driver->shutdown(dev);
>                 }
>
> +               dev_pm_domain_detach(dev, true);
> +

It generally makes sense to do this, because ->shutdown() is sort of
analogous to ->remove() from the driver model perspective, so if it is
sufficient for you, please feel free to send a formal patch with that
change.

>                 device_unlock(dev);
>                 if (parent)
>                         device_unlock(parent);
>
> This was discussed on the mailing list some time back[1] in the
> context of a different use case. However, the idea of detaching
> devices (on any bus) from the PM domain during shutdown is important
> to ensure correct power sequencing for the devices.
>
> One of the concerns that was raised on the above thread was slowing
> down the shutdown process when not needed. I think this can be handled
> by adding a sysfs attribute to allow platforms to decide if they need
> the ability to power off PM domains on shutdown/reboot path.

If you need to do that on a per-platform basis, I would go for option
4 above instead.

> Questions that I am looking to get feedback/comments are:
>
> 1. Is my assessment of the problem and understanding of the
> .shutdown() and pm.poweroff() correct?

Not exactly.

> 2. Does the solution #3 i.e. detaching PM domain after shutting down
> device on shutdown path makes sense?

Yes, it does (to me), but no, it is not sufficient to address the
problem at hand.

> 3. Are there other possible approaches to solve this problem that can
> be explored?

Yes, there are.  See option 4 above.

> 4. Do we still have the performance concern about the shutdown path? I
> don’t think anything has changed since that thread, so this is
> probably still true.

I think that it is the case.  Whoever had had any performance concerns
regarding this before is still going to have them.

> 5. Does the use of sysfs attribute make sense to let platform control
> if it wants to detach PM domains on shutdown path?

That would be clunky IMV.

> Sorry about the long thread and thank you so much for your time!

No worries.

Thanks!

Re: [RFC] ACPI PM during kernel poweroff/reboot

[Furquan Shaikh]

On Wed, Nov 25, 2020 at 8:39 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Nov 12, 2020 at 8:19 PM Furquan Shaikh <furquan@google.com> wrote:
> >
> > On x86 Chromebooks, we have observed this issue for a long time now -
> > when the system is powered off or rebooted, ACPI PM is not invoked and
> > this results in PowerResource _OFF methods not being invoked for any
> > of the devices. The _OFF methods are invoked correctly in case of
> > suspend-to-idle (S0ix) and suspend-to-memory(S3). However, they do not
> > get invoked when `poweroff` or `reboot` are triggered.
> >
> > One of the differences between suspend, hibernate and shutdown paths
> > in Linux kernel is that the shutdown path does not use the typical
> > device PM phases (prepare, freeze/suspend, poweroff) as used by
> > suspend/hibernate. Instead the shutdown path makes use of
> > .shutdown_pre() and .shutdown() callbacks.
> >
> > If I understand correctly, .shutdown() has been around for a long time
> > and existed even before the PM callbacks were added. Thus,
> > pm->poweroff() and .shutdown() are supposed to be analogous and
> > consistent in the behavior.
>
> Well, not quite.
>
> ->shutdown() is expected to be a lightweight operation also suitable
> for kexec() and similar situations where ->poweroff() may not work.
>
> > This is why runtime PM is disallowed by
> > device_shutdown() before it calls .shutdown() (i.e. to keep behavior
> > consistent for both paths). However, in practice, there are
> > differences in behavior for the pm->poweroff() and .shutdown() paths
> > since the shutdown path does not execute any PM domain operations.
>
> That's correct.
>
> > Because of this difference in behavior, shutdown path never invokes
> > ACPI PM and thus the ACPI PowerResources are not turned off when the
> > system is rebooted or powered off (sleep S5). On Chromebooks, it is
> > critical to run the _OFF methods for poweroff/reboot in order to
> > ensure that the device power off sequencing requirements are met.
> > Currently, these requirements are violated which impact the
> > reliability of devices over the lifetime of the platform.
> >
> > There are a few ways in which this can be addressed:
> >
> > 1. Similar to the case of hibernation, a new
> > PMSG_POWEROFF/PM_EVENT_POWEROFF can be introduced to invoke device
> > power management phases using `dpm_suspend_start(PMSG_POWEROFF)` and
> > `dpm_suspend_end(PMSG_POWEROFF)`. However, as the shutdown path uses
> > the class/bus/driver .shutdown() callbacks, adding dpm phases for
> > poweroff complicates the order of operations. If the dpm phases are
> > run before .shutdown() callbacks, then it will result in the callbacks
> > accessing devices after they are powered off. If the .shutdown()
> > callbacks are run before dpm phases, then the pm->poweroff() calls are
> > made after the device shutdown is done. Since .shutdown() and
> > pm->poweroff() are supposed to be analogous, having both calls in the
> > shutdown path is not only redundant but also results in incorrect
> > behavior.
> >
> > 2. Another option is to update device_shutdown() to make
> > pm_domain.poweroff calls after the class/bus/driver .shutdown() is
> > done. However, this suffers from the same problem as #1 above i.e. it
> > is redundant and creates conflicting order of operations.
> >
> > 3. Third possible solution is to detach the device from the PM domain
> > after it is shutdown. Currently, device drivers perform a detach
> > operation only when the device is removed. However, in case of
> > poweroff/reboot as the device is already shutdown, detaching PM domain
> > will give it the opportunity to ensure that any power resources are
> > correctly turned off before the system shuts down.
>
> 4. Make Chromebooks call something like hibernation_platform_enter()
> on S5 entries (including reboot).

Actually, Chromebooks do not support S4 and hence CONFIG_HIBERNATION.
This is done for a number of reasons including security. Hence, I
don't think using hibernation_platform_enter() would be an option.

>
> > Out of these, I think #3 makes the most sense as it does not introduce
> > any conflicting operations. I verified that the following diff results
> > in _OFF methods getting invoked in both poweroff and reboot cases:
>
> This won't work for PCI devices though, only for devices in the ACPI
> PM domain, so it is not sufficient in general.

That is true. The proposed solution only handles detaching of PM
domains. I understand your point about this not working for any
devices not part of the PM domain. The issues that we have observed in
shutdown/reboot paths have been specific to ACPI power resources
controlling the sequencing to external devices.

>
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 94df2ba1bbed..e55d65fbb4a9 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -23,6 +23,7 @@
> >  #include <linux/of_device.h>
> >  #include <linux/genhd.h>
> >  #include <linux/mutex.h>
> > +#include <linux/pm_domain.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/netdevice.h>
> >  #include <linux/sched/signal.h>
> > @@ -3230,6 +3231,8 @@ void device_shutdown(void)
> >                         dev->driver->shutdown(dev);
> >                 }
> >
> > +               dev_pm_domain_detach(dev, true);
> > +
>
> It generally makes sense to do this, because ->shutdown() is sort of
> analogous to ->remove() from the driver model perspective, so if it is
> sufficient for you, please feel free to send a formal patch with that
> change.

I will work on creating a formal patch and send it for review.

>
> >                 device_unlock(dev);
> >                 if (parent)
> >                         device_unlock(parent);
> >
> > This was discussed on the mailing list some time back[1] in the
> > context of a different use case. However, the idea of detaching
> > devices (on any bus) from the PM domain during shutdown is important
> > to ensure correct power sequencing for the devices.
> >
> > One of the concerns that was raised on the above thread was slowing
> > down the shutdown process when not needed. I think this can be handled
> > by adding a sysfs attribute to allow platforms to decide if they need
> > the ability to power off PM domains on shutdown/reboot path.
>
> If you need to do that on a per-platform basis, I would go for option
> 4 above instead.

For Chromebooks, I don't see a reason to do this on a per-platform
basis. But, I wanted to make sure that the proposed change does not
have any side-effect on any other user upstream.

>
> > Questions that I am looking to get feedback/comments are:
> >
> > 1. Is my assessment of the problem and understanding of the
> > .shutdown() and pm.poweroff() correct?
>
> Not exactly.
>
> > 2. Does the solution #3 i.e. detaching PM domain after shutting down
> > device on shutdown path makes sense?
>
> Yes, it does (to me), but no, it is not sufficient to address the
> problem at hand.
>
> > 3. Are there other possible approaches to solve this problem that can
> > be explored?
>
> Yes, there are.  See option 4 above.
>
> > 4. Do we still have the performance concern about the shutdown path? I
> > don’t think anything has changed since that thread, so this is
> > probably still true.
>
> I think that it is the case.  Whoever had had any performance concerns
> regarding this before is still going to have them.
>
> > 5. Does the use of sysfs attribute make sense to let platform control
> > if it wants to detach PM domains on shutdown path?
>
> That would be clunky IMV.

I can start with the diff I pasted above as a formal patch to see if
it makes sense. If more changes are required to it, I can work on
them.

>
> > Sorry about the long thread and thank you so much for your time!
>
> No worries.

Thanks again for your time. I really appreciate the insight and comments.

- Furquan

>
> Thanks!

Re: [RFC] ACPI PM during kernel poweroff/reboot

[Rafael J. Wysocki]

On Wed, Nov 25, 2020 at 6:43 PM Furquan Shaikh <furquan@google.com> wrote:
>
> On Wed, Nov 25, 2020 at 8:39 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Thu, Nov 12, 2020 at 8:19 PM Furquan Shaikh <furquan@google.com> wrote:
> > >
> > > On x86 Chromebooks, we have observed this issue for a long time now -
> > > when the system is powered off or rebooted, ACPI PM is not invoked and
> > > this results in PowerResource _OFF methods not being invoked for any
> > > of the devices. The _OFF methods are invoked correctly in case of
> > > suspend-to-idle (S0ix) and suspend-to-memory(S3). However, they do not
> > > get invoked when `poweroff` or `reboot` are triggered.
> > >
> > > One of the differences between suspend, hibernate and shutdown paths
> > > in Linux kernel is that the shutdown path does not use the typical
> > > device PM phases (prepare, freeze/suspend, poweroff) as used by
> > > suspend/hibernate. Instead the shutdown path makes use of
> > > .shutdown_pre() and .shutdown() callbacks.
> > >
> > > If I understand correctly, .shutdown() has been around for a long time
> > > and existed even before the PM callbacks were added. Thus,
> > > pm->poweroff() and .shutdown() are supposed to be analogous and
> > > consistent in the behavior.
> >
> > Well, not quite.
> >
> > ->shutdown() is expected to be a lightweight operation also suitable
> > for kexec() and similar situations where ->poweroff() may not work.
> >
> > > This is why runtime PM is disallowed by
> > > device_shutdown() before it calls .shutdown() (i.e. to keep behavior
> > > consistent for both paths). However, in practice, there are
> > > differences in behavior for the pm->poweroff() and .shutdown() paths
> > > since the shutdown path does not execute any PM domain operations.
> >
> > That's correct.
> >
> > > Because of this difference in behavior, shutdown path never invokes
> > > ACPI PM and thus the ACPI PowerResources are not turned off when the
> > > system is rebooted or powered off (sleep S5). On Chromebooks, it is
> > > critical to run the _OFF methods for poweroff/reboot in order to
> > > ensure that the device power off sequencing requirements are met.
> > > Currently, these requirements are violated which impact the
> > > reliability of devices over the lifetime of the platform.
> > >
> > > There are a few ways in which this can be addressed:
> > >
> > > 1. Similar to the case of hibernation, a new
> > > PMSG_POWEROFF/PM_EVENT_POWEROFF can be introduced to invoke device
> > > power management phases using `dpm_suspend_start(PMSG_POWEROFF)` and
> > > `dpm_suspend_end(PMSG_POWEROFF)`. However, as the shutdown path uses
> > > the class/bus/driver .shutdown() callbacks, adding dpm phases for
> > > poweroff complicates the order of operations. If the dpm phases are
> > > run before .shutdown() callbacks, then it will result in the callbacks
> > > accessing devices after they are powered off. If the .shutdown()
> > > callbacks are run before dpm phases, then the pm->poweroff() calls are
> > > made after the device shutdown is done. Since .shutdown() and
> > > pm->poweroff() are supposed to be analogous, having both calls in the
> > > shutdown path is not only redundant but also results in incorrect
> > > behavior.
> > >
> > > 2. Another option is to update device_shutdown() to make
> > > pm_domain.poweroff calls after the class/bus/driver .shutdown() is
> > > done. However, this suffers from the same problem as #1 above i.e. it
> > > is redundant and creates conflicting order of operations.
> > >
> > > 3. Third possible solution is to detach the device from the PM domain
> > > after it is shutdown. Currently, device drivers perform a detach
> > > operation only when the device is removed. However, in case of
> > > poweroff/reboot as the device is already shutdown, detaching PM domain
> > > will give it the opportunity to ensure that any power resources are
> > > correctly turned off before the system shuts down.
> >
> > 4. Make Chromebooks call something like hibernation_platform_enter()
> > on S5 entries (including reboot).
>
> Actually, Chromebooks do not support S4 and hence CONFIG_HIBERNATION.

This doesn't matter.  The ->poweroff callbacks can still be used by
them (of course, that part of the current hibernation support code
needs to be put under a more general Kconfig option for that, but this
is a technical detail).

> This is done for a number of reasons including security. Hence, I
> don't think using hibernation_platform_enter() would be an option.

Yes, it is an option.

Having "hibernation" in the name need not mean that the given piece of
code is really hibernation-specific ...

> >
> > > Out of these, I think #3 makes the most sense as it does not introduce
> > > any conflicting operations. I verified that the following diff results
> > > in _OFF methods getting invoked in both poweroff and reboot cases:
> >
> > This won't work for PCI devices though, only for devices in the ACPI
> > PM domain, so it is not sufficient in general.
>
> That is true. The proposed solution only handles detaching of PM
> domains. I understand your point about this not working for any
> devices not part of the PM domain. The issues that we have observed in
> shutdown/reboot paths have been specific to ACPI power resources
> controlling the sequencing to external devices.

PCI devices PM can use power resources too.  For instance, this has
been quite common for discrete GPUs in laptops IIRC.

Re: [RFC] ACPI PM during kernel poweroff/reboot

[Furquan Shaikh]

On Wed, Nov 25, 2020 at 9:51 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Nov 25, 2020 at 6:43 PM Furquan Shaikh <furquan@google.com> wrote:
> >
> > On Wed, Nov 25, 2020 at 8:39 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Thu, Nov 12, 2020 at 8:19 PM Furquan Shaikh <furquan@google.com> wrote:
> > > >
> > > > On x86 Chromebooks, we have observed this issue for a long time now -
> > > > when the system is powered off or rebooted, ACPI PM is not invoked and
> > > > this results in PowerResource _OFF methods not being invoked for any
> > > > of the devices. The _OFF methods are invoked correctly in case of
> > > > suspend-to-idle (S0ix) and suspend-to-memory(S3). However, they do not
> > > > get invoked when `poweroff` or `reboot` are triggered.
> > > >
> > > > One of the differences between suspend, hibernate and shutdown paths
> > > > in Linux kernel is that the shutdown path does not use the typical
> > > > device PM phases (prepare, freeze/suspend, poweroff) as used by
> > > > suspend/hibernate. Instead the shutdown path makes use of
> > > > .shutdown_pre() and .shutdown() callbacks.
> > > >
> > > > If I understand correctly, .shutdown() has been around for a long time
> > > > and existed even before the PM callbacks were added. Thus,
> > > > pm->poweroff() and .shutdown() are supposed to be analogous and
> > > > consistent in the behavior.
> > >
> > > Well, not quite.
> > >
> > > ->shutdown() is expected to be a lightweight operation also suitable
> > > for kexec() and similar situations where ->poweroff() may not work.
> > >
> > > > This is why runtime PM is disallowed by
> > > > device_shutdown() before it calls .shutdown() (i.e. to keep behavior
> > > > consistent for both paths). However, in practice, there are
> > > > differences in behavior for the pm->poweroff() and .shutdown() paths
> > > > since the shutdown path does not execute any PM domain operations.
> > >
> > > That's correct.
> > >
> > > > Because of this difference in behavior, shutdown path never invokes
> > > > ACPI PM and thus the ACPI PowerResources are not turned off when the
> > > > system is rebooted or powered off (sleep S5). On Chromebooks, it is
> > > > critical to run the _OFF methods for poweroff/reboot in order to
> > > > ensure that the device power off sequencing requirements are met.
> > > > Currently, these requirements are violated which impact the
> > > > reliability of devices over the lifetime of the platform.
> > > >
> > > > There are a few ways in which this can be addressed:
> > > >
> > > > 1. Similar to the case of hibernation, a new
> > > > PMSG_POWEROFF/PM_EVENT_POWEROFF can be introduced to invoke device
> > > > power management phases using `dpm_suspend_start(PMSG_POWEROFF)` and
> > > > `dpm_suspend_end(PMSG_POWEROFF)`. However, as the shutdown path uses
> > > > the class/bus/driver .shutdown() callbacks, adding dpm phases for
> > > > poweroff complicates the order of operations. If the dpm phases are
> > > > run before .shutdown() callbacks, then it will result in the callbacks
> > > > accessing devices after they are powered off. If the .shutdown()
> > > > callbacks are run before dpm phases, then the pm->poweroff() calls are
> > > > made after the device shutdown is done. Since .shutdown() and
> > > > pm->poweroff() are supposed to be analogous, having both calls in the
> > > > shutdown path is not only redundant but also results in incorrect
> > > > behavior.
> > > >
> > > > 2. Another option is to update device_shutdown() to make
> > > > pm_domain.poweroff calls after the class/bus/driver .shutdown() is
> > > > done. However, this suffers from the same problem as #1 above i.e. it
> > > > is redundant and creates conflicting order of operations.
> > > >
> > > > 3. Third possible solution is to detach the device from the PM domain
> > > > after it is shutdown. Currently, device drivers perform a detach
> > > > operation only when the device is removed. However, in case of
> > > > poweroff/reboot as the device is already shutdown, detaching PM domain
> > > > will give it the opportunity to ensure that any power resources are
> > > > correctly turned off before the system shuts down.
> > >
> > > 4. Make Chromebooks call something like hibernation_platform_enter()
> > > on S5 entries (including reboot).
> >
> > Actually, Chromebooks do not support S4 and hence CONFIG_HIBERNATION.
>
> This doesn't matter.  The ->poweroff callbacks can still be used by
> them (of course, that part of the current hibernation support code
> needs to be put under a more general Kconfig option for that, but this
> is a technical detail).

Ah I see what you are saying. Just to be sure I understand this
correctly. Is this what you are thinking:
1. Extract hibernation_platform_enter() and any other helpers required
to trigger the PM phases for shutdown into a separate unit controlled
by a more general Kconfig.
2. Add a new Kconfig that enables support for performing PM phases
during the poweroff/reboot phases.
3. Based on this new Kconfig selection, LINUX_REBOOT_CMD_RESTART,
LINUX_REBOOT_CMD_HALT, LINUX_REBOOT_CMD_POWER_OFF will be updated to
use the new paths instead of the current lightweight calls.

>
> > This is done for a number of reasons including security. Hence, I
> > don't think using hibernation_platform_enter() would be an option.
>
> Yes, it is an option.
>
> Having "hibernation" in the name need not mean that the given piece of
> code is really hibernation-specific ...

Sorry, I had misunderstood the suggestion before. I have attempted to
outline your proposal with some more details above.

>
> > >
> > > > Out of these, I think #3 makes the most sense as it does not introduce
> > > > any conflicting operations. I verified that the following diff results
> > > > in _OFF methods getting invoked in both poweroff and reboot cases:
> > >
> > > This won't work for PCI devices though, only for devices in the ACPI
> > > PM domain, so it is not sufficient in general.
> >
> > That is true. The proposed solution only handles detaching of PM
> > domains. I understand your point about this not working for any
> > devices not part of the PM domain. The issues that we have observed in
> > shutdown/reboot paths have been specific to ACPI power resources
> > controlling the sequencing to external devices.
>
> PCI devices PM can use power resources too.  For instance, this has
> been quite common for discrete GPUs in laptops IIRC.

Sorry about my naive question: Is the power resource not described
using ACPI in this case? (I haven't run into a situation with PCI
devices using non-ACPI power resources, so curious to understand the
scenario).

Re: [RFC] ACPI PM during kernel poweroff/reboot

[Furquan Shaikh]

On Wed, Nov 25, 2020 at 10:29 AM Furquan Shaikh <furquan@google.com> wrote:
>
> On Wed, Nov 25, 2020 at 9:51 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Wed, Nov 25, 2020 at 6:43 PM Furquan Shaikh <furquan@google.com> wrote:
> > >
> > > On Wed, Nov 25, 2020 at 8:39 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > >
> > > > On Thu, Nov 12, 2020 at 8:19 PM Furquan Shaikh <furquan@google.com> wrote:
> > > > >
> > > > > On x86 Chromebooks, we have observed this issue for a long time now -
> > > > > when the system is powered off or rebooted, ACPI PM is not invoked and
> > > > > this results in PowerResource _OFF methods not being invoked for any
> > > > > of the devices. The _OFF methods are invoked correctly in case of
> > > > > suspend-to-idle (S0ix) and suspend-to-memory(S3). However, they do not
> > > > > get invoked when `poweroff` or `reboot` are triggered.
> > > > >
> > > > > One of the differences between suspend, hibernate and shutdown paths
> > > > > in Linux kernel is that the shutdown path does not use the typical
> > > > > device PM phases (prepare, freeze/suspend, poweroff) as used by
> > > > > suspend/hibernate. Instead the shutdown path makes use of
> > > > > .shutdown_pre() and .shutdown() callbacks.
> > > > >
> > > > > If I understand correctly, .shutdown() has been around for a long time
> > > > > and existed even before the PM callbacks were added. Thus,
> > > > > pm->poweroff() and .shutdown() are supposed to be analogous and
> > > > > consistent in the behavior.
> > > >
> > > > Well, not quite.
> > > >
> > > > ->shutdown() is expected to be a lightweight operation also suitable
> > > > for kexec() and similar situations where ->poweroff() may not work.
> > > >
> > > > > This is why runtime PM is disallowed by
> > > > > device_shutdown() before it calls .shutdown() (i.e. to keep behavior
> > > > > consistent for both paths). However, in practice, there are
> > > > > differences in behavior for the pm->poweroff() and .shutdown() paths
> > > > > since the shutdown path does not execute any PM domain operations.
> > > >
> > > > That's correct.
> > > >
> > > > > Because of this difference in behavior, shutdown path never invokes
> > > > > ACPI PM and thus the ACPI PowerResources are not turned off when the
> > > > > system is rebooted or powered off (sleep S5). On Chromebooks, it is
> > > > > critical to run the _OFF methods for poweroff/reboot in order to
> > > > > ensure that the device power off sequencing requirements are met.
> > > > > Currently, these requirements are violated which impact the
> > > > > reliability of devices over the lifetime of the platform.
> > > > >
> > > > > There are a few ways in which this can be addressed:
> > > > >
> > > > > 1. Similar to the case of hibernation, a new
> > > > > PMSG_POWEROFF/PM_EVENT_POWEROFF can be introduced to invoke device
> > > > > power management phases using `dpm_suspend_start(PMSG_POWEROFF)` and
> > > > > `dpm_suspend_end(PMSG_POWEROFF)`. However, as the shutdown path uses
> > > > > the class/bus/driver .shutdown() callbacks, adding dpm phases for
> > > > > poweroff complicates the order of operations. If the dpm phases are
> > > > > run before .shutdown() callbacks, then it will result in the callbacks
> > > > > accessing devices after they are powered off. If the .shutdown()
> > > > > callbacks are run before dpm phases, then the pm->poweroff() calls are
> > > > > made after the device shutdown is done. Since .shutdown() and
> > > > > pm->poweroff() are supposed to be analogous, having both calls in the
> > > > > shutdown path is not only redundant but also results in incorrect
> > > > > behavior.
> > > > >
> > > > > 2. Another option is to update device_shutdown() to make
> > > > > pm_domain.poweroff calls after the class/bus/driver .shutdown() is
> > > > > done. However, this suffers from the same problem as #1 above i.e. it
> > > > > is redundant and creates conflicting order of operations.
> > > > >
> > > > > 3. Third possible solution is to detach the device from the PM domain
> > > > > after it is shutdown. Currently, device drivers perform a detach
> > > > > operation only when the device is removed. However, in case of
> > > > > poweroff/reboot as the device is already shutdown, detaching PM domain
> > > > > will give it the opportunity to ensure that any power resources are
> > > > > correctly turned off before the system shuts down.
> > > >
> > > > 4. Make Chromebooks call something like hibernation_platform_enter()
> > > > on S5 entries (including reboot).
> > >
> > > Actually, Chromebooks do not support S4 and hence CONFIG_HIBERNATION.
> >
> > This doesn't matter.  The ->poweroff callbacks can still be used by
> > them (of course, that part of the current hibernation support code
> > needs to be put under a more general Kconfig option for that, but this
> > is a technical detail).
>
> Ah I see what you are saying. Just to be sure I understand this
> correctly. Is this what you are thinking:
> 1. Extract hibernation_platform_enter() and any other helpers required
> to trigger the PM phases for shutdown into a separate unit controlled
> by a more general Kconfig.
> 2. Add a new Kconfig that enables support for performing PM phases
> during the poweroff/reboot phases.
> 3. Based on this new Kconfig selection, LINUX_REBOOT_CMD_RESTART,
> LINUX_REBOOT_CMD_HALT, LINUX_REBOOT_CMD_POWER_OFF will be updated to
> use the new paths instead of the current lightweight calls.

I am currently exploring this approach to see how the components need
to be organized to make use of hibernation_platform_enter by more than
just the hibernation path. Please let me know if the above summary
doesn't align with your suggestion.

Meanwhile, I have also sent out a formal patch for detaching the PM
domain: https://lore.kernel.org/linux-acpi/20201201213019.1558738-1-furquan@google.com/T/#u
to ensure that this addresses the issue with ACPI PM domain.

I will continue working on the above suggestion as well, but it might
take some time for me to get a good understanding of the current paths
and to cleanly implement the support for PM phases during
poweroff/reboot cases.

Thanks,
Furquan

>
> >
> > > This is done for a number of reasons including security. Hence, I
> > > don't think using hibernation_platform_enter() would be an option.
> >
> > Yes, it is an option.
> >
> > Having "hibernation" in the name need not mean that the given piece of
> > code is really hibernation-specific ...
>
> Sorry, I had misunderstood the suggestion before. I have attempted to
> outline your proposal with some more details above.
>
> >
> > > >
> > > > > Out of these, I think #3 makes the most sense as it does not introduce
> > > > > any conflicting operations. I verified that the following diff results
> > > > > in _OFF methods getting invoked in both poweroff and reboot cases:
> > > >
> > > > This won't work for PCI devices though, only for devices in the ACPI
> > > > PM domain, so it is not sufficient in general.
> > >
> > > That is true. The proposed solution only handles detaching of PM
> > > domains. I understand your point about this not working for any
> > > devices not part of the PM domain. The issues that we have observed in
> > > shutdown/reboot paths have been specific to ACPI power resources
> > > controlling the sequencing to external devices.
> >
> > PCI devices PM can use power resources too.  For instance, this has
> > been quite common for discrete GPUs in laptops IIRC.
>
> Sorry about my naive question: Is the power resource not described
> using ACPI in this case? (I haven't run into a situation with PCI
> devices using non-ACPI power resources, so curious to understand the
> scenario).

Re: [RFC] ACPI PM during kernel poweroff/reboot

[Rafael J. Wysocki]

On Tue, Dec 1, 2020 at 10:38 PM Furquan Shaikh <furquan@google.com> wrote:
>
> On Wed, Nov 25, 2020 at 10:29 AM Furquan Shaikh <furquan@google.com> wrote:
> >
> > On Wed, Nov 25, 2020 at 9:51 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Wed, Nov 25, 2020 at 6:43 PM Furquan Shaikh <furquan@google.com> wrote:
> > > >
> > > > On Wed, Nov 25, 2020 at 8:39 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > >
> > > > > On Thu, Nov 12, 2020 at 8:19 PM Furquan Shaikh <furquan@google.com> wrote:
> > > > > >
> > > > > > On x86 Chromebooks, we have observed this issue for a long time now -
> > > > > > when the system is powered off or rebooted, ACPI PM is not invoked and
> > > > > > this results in PowerResource _OFF methods not being invoked for any
> > > > > > of the devices. The _OFF methods are invoked correctly in case of
> > > > > > suspend-to-idle (S0ix) and suspend-to-memory(S3). However, they do not
> > > > > > get invoked when `poweroff` or `reboot` are triggered.
> > > > > >
> > > > > > One of the differences between suspend, hibernate and shutdown paths
> > > > > > in Linux kernel is that the shutdown path does not use the typical
> > > > > > device PM phases (prepare, freeze/suspend, poweroff) as used by
> > > > > > suspend/hibernate. Instead the shutdown path makes use of
> > > > > > .shutdown_pre() and .shutdown() callbacks.
> > > > > >
> > > > > > If I understand correctly, .shutdown() has been around for a long time
> > > > > > and existed even before the PM callbacks were added. Thus,
> > > > > > pm->poweroff() and .shutdown() are supposed to be analogous and
> > > > > > consistent in the behavior.
> > > > >
> > > > > Well, not quite.
> > > > >
> > > > > ->shutdown() is expected to be a lightweight operation also suitable
> > > > > for kexec() and similar situations where ->poweroff() may not work.
> > > > >
> > > > > > This is why runtime PM is disallowed by
> > > > > > device_shutdown() before it calls .shutdown() (i.e. to keep behavior
> > > > > > consistent for both paths). However, in practice, there are
> > > > > > differences in behavior for the pm->poweroff() and .shutdown() paths
> > > > > > since the shutdown path does not execute any PM domain operations.
> > > > >
> > > > > That's correct.
> > > > >
> > > > > > Because of this difference in behavior, shutdown path never invokes
> > > > > > ACPI PM and thus the ACPI PowerResources are not turned off when the
> > > > > > system is rebooted or powered off (sleep S5). On Chromebooks, it is
> > > > > > critical to run the _OFF methods for poweroff/reboot in order to
> > > > > > ensure that the device power off sequencing requirements are met.
> > > > > > Currently, these requirements are violated which impact the
> > > > > > reliability of devices over the lifetime of the platform.
> > > > > >
> > > > > > There are a few ways in which this can be addressed:
> > > > > >
> > > > > > 1. Similar to the case of hibernation, a new
> > > > > > PMSG_POWEROFF/PM_EVENT_POWEROFF can be introduced to invoke device
> > > > > > power management phases using `dpm_suspend_start(PMSG_POWEROFF)` and
> > > > > > `dpm_suspend_end(PMSG_POWEROFF)`. However, as the shutdown path uses
> > > > > > the class/bus/driver .shutdown() callbacks, adding dpm phases for
> > > > > > poweroff complicates the order of operations. If the dpm phases are
> > > > > > run before .shutdown() callbacks, then it will result in the callbacks
> > > > > > accessing devices after they are powered off. If the .shutdown()
> > > > > > callbacks are run before dpm phases, then the pm->poweroff() calls are
> > > > > > made after the device shutdown is done. Since .shutdown() and
> > > > > > pm->poweroff() are supposed to be analogous, having both calls in the
> > > > > > shutdown path is not only redundant but also results in incorrect
> > > > > > behavior.
> > > > > >
> > > > > > 2. Another option is to update device_shutdown() to make
> > > > > > pm_domain.poweroff calls after the class/bus/driver .shutdown() is
> > > > > > done. However, this suffers from the same problem as #1 above i.e. it
> > > > > > is redundant and creates conflicting order of operations.
> > > > > >
> > > > > > 3. Third possible solution is to detach the device from the PM domain
> > > > > > after it is shutdown. Currently, device drivers perform a detach
> > > > > > operation only when the device is removed. However, in case of
> > > > > > poweroff/reboot as the device is already shutdown, detaching PM domain
> > > > > > will give it the opportunity to ensure that any power resources are
> > > > > > correctly turned off before the system shuts down.
> > > > >
> > > > > 4. Make Chromebooks call something like hibernation_platform_enter()
> > > > > on S5 entries (including reboot).
> > > >
> > > > Actually, Chromebooks do not support S4 and hence CONFIG_HIBERNATION.
> > >
> > > This doesn't matter.  The ->poweroff callbacks can still be used by
> > > them (of course, that part of the current hibernation support code
> > > needs to be put under a more general Kconfig option for that, but this
> > > is a technical detail).
> >
> > Ah I see what you are saying. Just to be sure I understand this
> > correctly. Is this what you are thinking:
> > 1. Extract hibernation_platform_enter() and any other helpers required
> > to trigger the PM phases for shutdown into a separate unit controlled
> > by a more general Kconfig.

Yes in general, but maybe not hibernation_platform_enter() as a whole,
because it contains hibernation-specific code.

> > 2. Add a new Kconfig that enables support for performing PM phases
> > during the poweroff/reboot phases.

Yes.

> > 3. Based on this new Kconfig selection, LINUX_REBOOT_CMD_RESTART,
> > LINUX_REBOOT_CMD_HALT, LINUX_REBOOT_CMD_POWER_OFF will be updated to
> > use the new paths instead of the current lightweight calls.

Maybe not always, but depending on the platform or similar.

> I am currently exploring this approach to see how the components need
> to be organized to make use of hibernation_platform_enter by more than
> just the hibernation path. Please let me know if the above summary
> doesn't align with your suggestion.
>
> Meanwhile, I have also sent out a formal patch for detaching the PM
> domain: https://lore.kernel.org/linux-acpi/20201201213019.1558738-1-furquan@google.com/T/#u
> to ensure that this addresses the issue with ACPI PM domain.

OK, so let's see what the response to it will be.

> I will continue working on the above suggestion as well, but it might
> take some time for me to get a good understanding of the current paths
> and to cleanly implement the support for PM phases during
> poweroff/reboot cases.

Sure, please take your time!

Thanks a lot for working on this!