Message ID | 1498072888-14782-7-git-send-email-ulf.hansson@linaro.org |
---|---|
State | Superseded |
Headers | show |
Series | PM / ACPI / i2c: Fix system suspend and deploy runtime PM centric path for ACPI | expand |
On Wed, Jun 21, 2017 at 9:21 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. > > 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> > --- > drivers/acpi/device_pm.c | 37 ++++++++++++++++++++++++++++++++++++- > include/acpi/acpi_bus.h | 1 + > include/linux/acpi.h | 4 ++++ > 3 files changed, 41 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c > index ee51e75..2393a1a 100644 > --- a/drivers/acpi/device_pm.c > +++ b/drivers/acpi/device_pm.c > @@ -879,6 +879,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->no_direct_complete = true; We have an analogous flag in PCI now and it is called needs_resume, so it would be good to be consistent with that. > +} > +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->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. > * > @@ -967,7 +1002,7 @@ int acpi_subsys_prepare(struct device *dev) > if (ret < 0) > return ret; > > - if (!adev || !pm_runtime_suspended(dev)) > + if (!adev || adev->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 63a90a6..2293d24 100644 > --- a/include/acpi/acpi_bus.h > +++ b/include/acpi/acpi_bus.h > @@ -380,6 +380,7 @@ struct acpi_device { > struct list_head physical_node_list; > struct mutex physical_node_lock; > void (*remove)(struct acpi_device *); > + bool no_direct_complete; Also what about adding this to struct acpi_device_power instead? Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 21 June 2017 at 23:42, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Wed, Jun 21, 2017 at 9:21 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. >> >> 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> >> --- >> drivers/acpi/device_pm.c | 37 ++++++++++++++++++++++++++++++++++++- >> include/acpi/acpi_bus.h | 1 + >> include/linux/acpi.h | 4 ++++ >> 3 files changed, 41 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c >> index ee51e75..2393a1a 100644 >> --- a/drivers/acpi/device_pm.c >> +++ b/drivers/acpi/device_pm.c >> @@ -879,6 +879,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->no_direct_complete = true; > > We have an analogous flag in PCI now and it is called needs_resume, so > it would be good to be consistent with that. I was trying to come up with a nice name, however I couldn't find anything better than no_direct_complete. However as the next patch somewhat, extends the flag to also be use for the runtime PM centric path, I should perhaps choose something more related to that? I think make "needs_resume" is going to e bit confusing, especially while extending the usage for the flag it in the next patch, no? > >> +} >> +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->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. >> * >> @@ -967,7 +1002,7 @@ int acpi_subsys_prepare(struct device *dev) >> if (ret < 0) >> return ret; >> >> - if (!adev || !pm_runtime_suspended(dev)) >> + if (!adev || adev->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 63a90a6..2293d24 100644 >> --- a/include/acpi/acpi_bus.h >> +++ b/include/acpi/acpi_bus.h >> @@ -380,6 +380,7 @@ struct acpi_device { >> struct list_head physical_node_list; >> struct mutex physical_node_lock; >> void (*remove)(struct acpi_device *); >> + bool no_direct_complete; > > Also what about adding this to struct acpi_device_power instead? Yes, thanks for the suggestion! > > Thanks, > Rafael Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, June 22, 2017 11:35:35 AM Ulf Hansson wrote: > On 21 June 2017 at 23:42, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Wed, Jun 21, 2017 at 9:21 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. > >> > >> 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> > >> --- > >> drivers/acpi/device_pm.c | 37 ++++++++++++++++++++++++++++++++++++- > >> include/acpi/acpi_bus.h | 1 + > >> include/linux/acpi.h | 4 ++++ > >> 3 files changed, 41 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c > >> index ee51e75..2393a1a 100644 > >> --- a/drivers/acpi/device_pm.c > >> +++ b/drivers/acpi/device_pm.c > >> @@ -879,6 +879,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->no_direct_complete = true; > > > > We have an analogous flag in PCI now and it is called needs_resume, so > > it would be good to be consistent with that. > > I was trying to come up with a nice name, however I couldn't find > anything better than no_direct_complete. > > However as the next patch somewhat, extends the flag to also be use > for the runtime PM centric path, I should perhaps choose something > more related to that? > > I think make "needs_resume" is going to e bit confusing, especially > while extending the usage for the flag it in the next patch, no? OK, fair enough, but I still think the name isn't really nice. :-) In fact, I'm not sure if the new flag is necessary at all. If the driver is expected to use pm_runtime_force_suspend|resume() along with it every time, why not to make the ACPI PM domain code check if the driver's callback pointers point to pm_runtime_force_suspend|resume() and work accordingly? Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 22, 2017 at 11:29 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 22 June 2017 at 16:38, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> On Thursday, June 22, 2017 11:35:35 AM Ulf Hansson wrote: >>> On 21 June 2017 at 23:42, Rafael J. Wysocki <rafael@kernel.org> wrote: >>> > On Wed, Jun 21, 2017 at 9:21 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. >>> >> >>> >> 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> >>> >> --- >>> >> drivers/acpi/device_pm.c | 37 ++++++++++++++++++++++++++++++++++++- >>> >> include/acpi/acpi_bus.h | 1 + >>> >> include/linux/acpi.h | 4 ++++ >>> >> 3 files changed, 41 insertions(+), 1 deletion(-) >>> >> >>> >> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c >>> >> index ee51e75..2393a1a 100644 >>> >> --- a/drivers/acpi/device_pm.c >>> >> +++ b/drivers/acpi/device_pm.c >>> >> @@ -879,6 +879,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->no_direct_complete = true; >>> > >>> > We have an analogous flag in PCI now and it is called needs_resume, so >>> > it would be good to be consistent with that. >>> >>> I was trying to come up with a nice name, however I couldn't find >>> anything better than no_direct_complete. >>> >>> However as the next patch somewhat, extends the flag to also be use >>> for the runtime PM centric path, I should perhaps choose something >>> more related to that? >>> >>> I think make "needs_resume" is going to e bit confusing, especially >>> while extending the usage for the flag it in the next patch, no? >> >> OK, fair enough, but I still think the name isn't really nice. :-) > > How about use_rpm_centric_sleep or use_rpm_ops_for_sleep? > > :-) no_direct_complete is better than that IMO. >> >> In fact, I'm not sure if the new flag is necessary at all. >> >> If the driver is expected to use pm_runtime_force_suspend|resume() along with >> it every time, why not to make the ACPI PM domain code check if the driver's >> callback pointers point to pm_runtime_force_suspend|resume() and work >> accordingly? > > I get the idea, however it would limit the driver from doing other > specific things from its sleep callbacks. > > It may for example deal with wakeups during system suspend, and when > that is done, it can call pm_runtime_force_suspend() from that > callback. I see. > Potentially that could be done from the runtime PM callback itself, > (via checking pm_runtime_enabled)... Anyway, it would be good to have a way to verify that the driver is doing the right thing or we'll have bugs in that area and people wondering what's up. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c index ee51e75..2393a1a 100644 --- a/drivers/acpi/device_pm.c +++ b/drivers/acpi/device_pm.c @@ -879,6 +879,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->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->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. * @@ -967,7 +1002,7 @@ int acpi_subsys_prepare(struct device *dev) if (ret < 0) return ret; - if (!adev || !pm_runtime_suspended(dev)) + if (!adev || adev->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 63a90a6..2293d24 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -380,6 +380,7 @@ struct acpi_device { struct list_head physical_node_list; struct mutex physical_node_lock; void (*remove)(struct acpi_device *); + bool no_direct_complete; }; /* Non-device subnode */ diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 137e4a3..a41cca5 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -859,6 +859,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); @@ -868,6 +870,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> --- drivers/acpi/device_pm.c | 37 ++++++++++++++++++++++++++++++++++++- include/acpi/acpi_bus.h | 1 + include/linux/acpi.h | 4 ++++ 3 files changed, 41 insertions(+), 1 deletion(-) -- 2.7.4