| Message ID | 1503499329-28834-6-git-send-email-ulf.hansson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Aug 23, 2017 at 4:42 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > In some cases a driver for an ACPI device needs to be able to prevent the > ACPI PM domain from using the direct_complete path during system sleep. > > One typical case is when the driver for the device needs its device to stay > runtime enabled, during the __device_suspend phase. This isn't the case > when the direct_complete path is being executed by the PM core, as it then > disables runtime PM for the device in __device_suspend(). Any following > attempts to runtime resume the device after that point, just fails. OK, that is a problem. > A workaround to this problem is to let the driver runtime resume its device > from its ->prepare() callback, as that would prevent the direct_complete > path from being executed. However, that may often be a waste, especially if > it turned out that no one really needed the device. > > For this reason, invent acpi_dev_disable|enable_direct_complete(), to allow > drivers to inform the ACPI PM domain to change its default behaviour during > system sleep, and thus control whether it may use the direct_complete path > or not. But I'm not sure this is the right place to address it as it very well may affect a PCI device too. Also, this is about the device and not about its ACPI companion object, so putting the flag in there is somewhat unclean, so to speak. It looks like we need a flag in dev_pm_info saying something along the lines of "my system suspend callback can deal with runtime PM" that will cause the direct_complete update to occur in __device_suspend_late() instead of __device_suspend(). Thanks, Rafael
On 24 August 2017 at 01:39, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Wed, Aug 23, 2017 at 4:42 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> In some cases a driver for an ACPI device needs to be able to prevent the >> ACPI PM domain from using the direct_complete path during system sleep. >> >> One typical case is when the driver for the device needs its device to stay >> runtime enabled, during the __device_suspend phase. This isn't the case >> when the direct_complete path is being executed by the PM core, as it then >> disables runtime PM for the device in __device_suspend(). Any following >> attempts to runtime resume the device after that point, just fails. > > OK, that is a problem. > >> A workaround to this problem is to let the driver runtime resume its device >> from its ->prepare() callback, as that would prevent the direct_complete >> path from being executed. However, that may often be a waste, especially if >> it turned out that no one really needed the device. >> >> For this reason, invent acpi_dev_disable|enable_direct_complete(), to allow >> drivers to inform the ACPI PM domain to change its default behaviour during >> system sleep, and thus control whether it may use the direct_complete path >> or not. > > But I'm not sure this is the right place to address it as it very well > may affect a PCI device too. > > Also, this is about the device and not about its ACPI companion > object, so putting the flag in there is somewhat unclean, so to speak. > > It looks like we need a flag in dev_pm_info saying something along the > lines of "my system suspend callback can deal with runtime PM" that > will cause the direct_complete update to occur in > __device_suspend_late() instead of __device_suspend(). I realize that in the end this turns out to be a comparison between the runtime PM centric path and the direct_complete path while implementing system sleep. In patch 9, there is some more explanation around this, however if you like I can elaborate even more about this!? Regarding making changes to the PM core and adding more flags to the dev_pm_info etc, I am not sure we really want that. Isn't it already complex enough? My point is, that I am trying to improve the behavior of the ACPI PM domain by enabling the runtime PM centric path for it, and even if there is something similar that could be done for PCI, I don't think we should need involvement by the PM core. Kind regards Uffe
On Thursday, August 24, 2017 10:19:43 AM CEST Ulf Hansson wrote: > On 24 August 2017 at 01:39, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Wed, Aug 23, 2017 at 4:42 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > >> In some cases a driver for an ACPI device needs to be able to prevent the > >> ACPI PM domain from using the direct_complete path during system sleep. > >> > >> One typical case is when the driver for the device needs its device to stay > >> runtime enabled, during the __device_suspend phase. This isn't the case > >> when the direct_complete path is being executed by the PM core, as it then > >> disables runtime PM for the device in __device_suspend(). Any following > >> attempts to runtime resume the device after that point, just fails. > > > > OK, that is a problem. > > > >> A workaround to this problem is to let the driver runtime resume its device > >> from its ->prepare() callback, as that would prevent the direct_complete > >> path from being executed. However, that may often be a waste, especially if > >> it turned out that no one really needed the device. > >> > >> For this reason, invent acpi_dev_disable|enable_direct_complete(), to allow > >> drivers to inform the ACPI PM domain to change its default behaviour during > >> system sleep, and thus control whether it may use the direct_complete path > >> or not. > > > > But I'm not sure this is the right place to address it as it very well > > may affect a PCI device too. > > > > Also, this is about the device and not about its ACPI companion > > object, so putting the flag in there is somewhat unclean, so to speak. > > > > It looks like we need a flag in dev_pm_info saying something along the > > lines of "my system suspend callback can deal with runtime PM" that > > will cause the direct_complete update to occur in > > __device_suspend_late() instead of __device_suspend(). > > I realize that in the end this turns out to be a comparison between > the runtime PM centric path and the direct_complete path while > implementing system sleep. In patch 9, there is some more explanation > around this, however if you like I can elaborate even more about > this!? > > Regarding making changes to the PM core and adding more flags to the > dev_pm_info etc, I am not sure we really want that. Isn't it already > complex enough? Maybe it is. > My point is, that I am trying to improve the behavior of the ACPI PM > domain by enabling the runtime PM centric path for it, and even if > there is something similar that could be done for PCI, I don't think > we should need involvement by the PM core. Well, this generally simply doesn't work. The whole "runtime PM centric approach" idea is generally fine by me, but the fact today is that there are drivers not ready for it. Which is why there is the direct_complete thing (it may be regarded as a sort-of workaround for the unreadiness of drivers if you will). Now, buy adding the no_direct_complete flag just to the ACPI PM domain you basically overlook the fact that this potentially affects the parents of the devices in question by preventing direct_complete from being set for them. And those parents may not be in the ACPI PM domain in principle, so the problem needs to be addressed in the core. Thanks, Rafael
On 24 August 2017 at 16:57, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Thursday, August 24, 2017 10:19:43 AM CEST Ulf Hansson wrote: >> On 24 August 2017 at 01:39, Rafael J. Wysocki <rafael@kernel.org> wrote: >> > On Wed, Aug 23, 2017 at 4:42 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> >> In some cases a driver for an ACPI device needs to be able to prevent the >> >> ACPI PM domain from using the direct_complete path during system sleep. >> >> >> >> One typical case is when the driver for the device needs its device to stay >> >> runtime enabled, during the __device_suspend phase. This isn't the case >> >> when the direct_complete path is being executed by the PM core, as it then >> >> disables runtime PM for the device in __device_suspend(). Any following >> >> attempts to runtime resume the device after that point, just fails. >> > >> > OK, that is a problem. >> > >> >> A workaround to this problem is to let the driver runtime resume its device >> >> from its ->prepare() callback, as that would prevent the direct_complete >> >> path from being executed. However, that may often be a waste, especially if >> >> it turned out that no one really needed the device. >> >> >> >> For this reason, invent acpi_dev_disable|enable_direct_complete(), to allow >> >> drivers to inform the ACPI PM domain to change its default behaviour during >> >> system sleep, and thus control whether it may use the direct_complete path >> >> or not. >> > >> > But I'm not sure this is the right place to address it as it very well >> > may affect a PCI device too. >> > >> > Also, this is about the device and not about its ACPI companion >> > object, so putting the flag in there is somewhat unclean, so to speak. >> > >> > It looks like we need a flag in dev_pm_info saying something along the >> > lines of "my system suspend callback can deal with runtime PM" that >> > will cause the direct_complete update to occur in >> > __device_suspend_late() instead of __device_suspend(). >> >> I realize that in the end this turns out to be a comparison between >> the runtime PM centric path and the direct_complete path while >> implementing system sleep. In patch 9, there is some more explanation >> around this, however if you like I can elaborate even more about >> this!? >> >> Regarding making changes to the PM core and adding more flags to the >> dev_pm_info etc, I am not sure we really want that. Isn't it already >> complex enough? > > Maybe it is. > >> My point is, that I am trying to improve the behavior of the ACPI PM >> domain by enabling the runtime PM centric path for it, and even if >> there is something similar that could be done for PCI, I don't think >> we should need involvement by the PM core. > > Well, this generally simply doesn't work. > > The whole "runtime PM centric approach" idea is generally fine by me, > but the fact today is that there are drivers not ready for it. Which > is why there is the direct_complete thing (it may be regarded as a > sort-of workaround for the unreadiness of drivers if you will). This is how I see it: The runtime PM centric path is being widely deployed, however it takes time to convert drivers. The direct_complete path offers a great intermediate step for the ACPI PM domain as it affects all its devices - while we wait for further optimizations being deployed using the runtime PM centric path. > > Now, buy adding the no_direct_complete flag just to the ACPI PM domain > you basically overlook the fact that this potentially affects the parents > of the devices in question by preventing direct_complete from being set > for them. And those parents may not be in the ACPI PM domain in principle, > so the problem needs to be addressed in the core. Okay, let's move the flag to the dev_pm* structures, to not limit this to the ACPI PM domain. However in the current approach taken in this series, as it's coded as opt-in to use for drivers, I am questioning how big of a problem parent devices not being able to use the direct_complete path could be!? Couldn't it be good enough to just adopt the behavior of the ACPI PM domain and more or less leave the core out of it - at least for now!? I am also thinking, that for those parent devices that potentially may suffer from not be able to use the direct_complete path, those can be fixed by deploying the runtime PM centric path in their subsystems/drivers. That would even mean that the parent devices get the additional benefits that the runtime PM centric path offers. So, in the end we would end up having a better optimized solution than we had before. What do you think? Kind regards Uffe
diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c index 5181057..f7bf596 100644 --- a/drivers/acpi/device_pm.c +++ b/drivers/acpi/device_pm.c @@ -933,6 +933,41 @@ EXPORT_SYMBOL_GPL(acpi_subsys_runtime_resume); #ifdef CONFIG_PM_SLEEP /** + * acpi_dev_disable_direct_complete - Disable the direct_complete path for ACPI. + * @dev: Device to disable the path for. + * + * Per default the ACPI PM domain tries to use the direct_complete path for its + * devices during system sleep. This function allows a user, typically a driver + * during probe, to disable the direct_complete path from being used by ACPI. + */ +void acpi_dev_disable_direct_complete(struct device *dev) +{ + struct acpi_device *adev = ACPI_COMPANION(dev); + + if (adev) + adev->power.no_direct_complete = true; +} +EXPORT_SYMBOL_GPL(acpi_dev_disable_direct_complete); + +/** + * acpi_dev_enable_direct_complete - Enable the direct_complete path for ACPI. + * @dev: Device to enable the path for. + * + * Enable the direct_complete path to be used during system suspend for the ACPI + * PM domain, which is the default option. Typically a driver that disabled the + * path during ->probe(), must call this function during ->remove() to re-enable + * the direct_complete path to be used by ACPI. + */ +void acpi_dev_enable_direct_complete(struct device *dev) +{ + struct acpi_device *adev = ACPI_COMPANION(dev); + + if (adev) + adev->power.no_direct_complete = false; +} +EXPORT_SYMBOL_GPL(acpi_dev_enable_direct_complete); + +/** * acpi_dev_suspend_late - Put device into a low-power state using ACPI. * @dev: Device to put into a low-power state. * @@ -1023,7 +1058,8 @@ int acpi_subsys_prepare(struct device *dev) if (ret < 0) return ret; - if (!adev || !pm_runtime_suspended(dev)) + if (!adev || adev->power.no_direct_complete || + !pm_runtime_suspended(dev)) return 0; return !acpi_dev_needs_resume(dev, adev); diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index dedf9d7..bdec5f2 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -285,6 +285,7 @@ struct acpi_device_power_state { struct acpi_device_power { int state; /* Current state */ + bool no_direct_complete; struct acpi_device_power_flags flags; struct acpi_device_power_state states[ACPI_D_STATE_COUNT]; /* Power states (D0-D3Cold) */ }; diff --git a/include/linux/acpi.h b/include/linux/acpi.h index c1a5213..ec33c41 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -867,6 +867,8 @@ static inline int acpi_dev_pm_attach(struct device *dev, bool power_on) #endif #if defined(CONFIG_ACPI) && defined(CONFIG_PM_SLEEP) +void acpi_dev_disable_direct_complete(struct device *dev); +void acpi_dev_enable_direct_complete(struct device *dev); int acpi_dev_suspend_late(struct device *dev); int acpi_dev_resume_early(struct device *dev); int acpi_subsys_prepare(struct device *dev); @@ -876,6 +878,8 @@ int acpi_subsys_resume_early(struct device *dev); int acpi_subsys_suspend(struct device *dev); int acpi_subsys_freeze(struct device *dev); #else +static inline void acpi_dev_disable_direct_complete(struct device *dev) {} +static inline void acpi_dev_enable_direct_complete(struct device *dev) {} static inline int acpi_dev_suspend_late(struct device *dev) { return 0; } static inline int acpi_dev_resume_early(struct device *dev) { return 0; } static inline int acpi_subsys_prepare(struct device *dev) { return 0; }
In some cases a driver for an ACPI device needs to be able to prevent the ACPI PM domain from using the direct_complete path during system sleep. One typical case is when the driver for the device needs its device to stay runtime enabled, during the __device_suspend phase. This isn't the case when the direct_complete path is being executed by the PM core, as it then disables runtime PM for the device in __device_suspend(). Any following attempts to runtime resume the device after that point, just fails. A workaround to this problem is to let the driver runtime resume its device from its ->prepare() callback, as that would prevent the direct_complete path from being executed. However, that may often be a waste, especially if it turned out that no one really needed the device. For this reason, invent acpi_dev_disable|enable_direct_complete(), to allow drivers to inform the ACPI PM domain to change its default behaviour during system sleep, and thus control whether it may use the direct_complete path or not. Typically a driver should call acpi_dev_disable_direct_comlete() during ->probe() and acpi_dev_enable_direct_complete() in ->remove(). Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- Changes in v2: - Moved the no_direct_complete flag to the struct acpi_device_power. --- drivers/acpi/device_pm.c | 38 +++++++++++++++++++++++++++++++++++++- include/acpi/acpi_bus.h | 1 + include/linux/acpi.h | 4 ++++ 3 files changed, 42 insertions(+), 1 deletion(-)