On 28 August 2017 at 15:40, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
> On Monday, August 28, 2017 2:54:40 PM CEST Ulf Hansson wrote:
>> On 28 August 2017 at 14:39, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
>> > On Monday, August 28, 2017 10:31:44 AM CEST Ulf Hansson wrote:
>> >> On 28 August 2017 at 03:30, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
>> >> > On Friday, August 25, 2017 3:42:35 PM CEST Rafael J. Wysocki wrote:
>> >> >> On Thursday, August 24, 2017 11:50:40 PM CEST Rafael J. Wysocki wrote:
>> >> >> > On Thursday, August 24, 2017 6:35:49 PM CEST Rafael J. Wysocki wrote:
>> >> >> > > On Thursday, August 24, 2017 11:15:26 AM CEST Ulf Hansson wrote:
>
> [cut]
>
>> >> >
>> >> > Well, not really, because if the device remains runtime suspended,
>> >> > ->runtime_suspend() will not be called by pm_runtime_force_suspend() and
>> >> > acpi_dev_suspend_late() should not be called then.
>> >> >
>> >> > So more changes in the ACPI PM domain are needed after all.
>> >>
>> >> Yes, that's what I thought as well.
>> >>
>> >> Anyway, let me cook a new version of the series - trying to address
>> >> the first bits you have pointed out. Then we can continue with
>> >> fine-tuning on top, addressing further optimizations of the ACPI PM
>> >> domain.
>> >
>> > Actually, please hold on and let me show you what I would like to do
>> > first.
>>
>> Hmm.
>>
>> I think I have almost done the work for the ACPI PM domain already.
>> It's just a matter of minor tweaks to the changes in patch 6 and 7
>> (and of course to get them into a shape that you prefer) and then
>> dropping patch 5 altogether.
>>
>> Wouldn't it be better if you build upon my changes?
>>
>> Anyway, if you have strong opinion of driving this, I am fine stepping aside.
>
> OK, so see below. :-)
>
> If what you want is to make the i2c designware driver use _force_suspend() and
> _force_resume(), then to me this is only tangentially related to direct_complete
> and can be done without messing up with that one.
>
> So the problem is not when direct_complete is set (becasue the driver's and
> PM domain's callbacks will not be invoked then even), but when it is not set.
For i2c designware driver case - then you are right! Although, that's
because the i2c designware driver has nothing else to do than to call
pm_runtime_force_suspend|resume() from the late suspend and early
resume callbacks.
However, for other drivers this isn't the case. A driver may have some
additional things to cope with during system sleep, besides making
sure to call pm_runtime_force_suspend|resume().
Then as I stated earlier, it would be of great value if we could
remain having runtime PM enabled during the entire device_suspend()
phase. I am not sure how you intend to address that, or perhaps you
did in some of those earlier patches you posted.
In my re-spinned series (not posted yet), I am still addressing both
issues above, but also not preventing the direct_complete path for
parent/suppliers when the runtime PM centric path is used.
> If direct_complete is not set, the ACPI PM domain resumes the device in
> acpi_subsys_suspend(), because it doesn't know two things: (a) why direct_complete
> is not set and (b) whether or not the drivers PM callbacks can cope with a
> runtime suspended device. These two things are separate, so they need to
> be addressed separately.
Yes.
>
> For (b) I'd like to use the SAFE_SUSPEND flag from my previous patch.
Seems like a reasonable approach.
>
> As far as (a) is concerned, there are two possible reasons for not setting
> direct_complete. First, it may be necessary to change the power state of the
> device and in that case the device *should* be resumed in acpi_subsys_suspend().
> Second, direct_complete may not be set for the device's children and in that
> case acpi_subsys_suspend() may not care as long as SAFE_SUSPEND is set.
Okay!
>
> So the ACPI PM domain needs to distinguish these two cases and for that reason
> it has to track whether or not the power state of the device needs to be updated
> which is what the (untested) patch below does, but of course it doesn't
> cover the LPSS.
>
> ---
> Documentation/driver-api/pm/devices.rst | 7 ++
> drivers/acpi/device_pm.c | 72 +++++++++++++++++++++-------
> drivers/base/dd.c | 2
> drivers/i2c/busses/i2c-designware-platdrv.c | 4 +
> include/acpi/acpi_bus.h | 1
> include/linux/pm.h | 16 ++++++
> 6 files changed, 83 insertions(+), 19 deletions(-)
>
> Index: linux-pm/include/linux/pm.h
> ===================================================================
> --- linux-pm.orig/include/linux/pm.h
> +++ linux-pm/include/linux/pm.h
> @@ -550,6 +550,21 @@ struct pm_subsys_data {
> #endif
> };
>
> +/*
> + * Driver flags to control system suspend/resume behavior.
> + *
> + * These flags can be set by device drivers at the probe time. They need not be
> + * cleared by the drivers as the driver core will take care of that.
> + *
> + * SAFE_SUSPEND: No need to runtime resume the device during system suspend.
> + *
> + * Setting SAFE_SUSPEND instructs bus types and PM domains which may want to
> + * runtime resume the device upfront during system suspend that doing so is not
> + * necessary from the driver's perspective, because the system suspend callbacks
> + * provided by it can cope with a runtime suspended device.
> + */
> +#define DPM_FLAG_SAFE_SUSPEND BIT(0)
> +
> struct dev_pm_info {
> pm_message_t power_state;
> unsigned int can_wakeup:1;
> @@ -561,6 +576,7 @@ struct dev_pm_info {
> bool is_late_suspended:1;
> bool early_init:1; /* Owned by the PM core */
> bool direct_complete:1; /* Owned by the PM core */
> + unsigned int driver_flags;
> spinlock_t lock;
> #ifdef CONFIG_PM_SLEEP
> struct list_head entry;
> Index: linux-pm/drivers/acpi/device_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/device_pm.c
> +++ linux-pm/drivers/acpi/device_pm.c
> @@ -899,6 +899,7 @@ int acpi_dev_runtime_resume(struct devic
>
> error = acpi_dev_pm_full_power(adev);
> acpi_device_wakeup_disable(adev);
> + adev->power.update_state = true;
> return error;
> }
> EXPORT_SYMBOL_GPL(acpi_dev_runtime_resume);
> @@ -989,33 +990,47 @@ int acpi_dev_resume_early(struct device
> }
> EXPORT_SYMBOL_GPL(acpi_dev_resume_early);
>
> -/**
> - * acpi_subsys_prepare - Prepare device for system transition to a sleep state.
> - * @dev: Device to prepare.
> - */
> -int acpi_subsys_prepare(struct device *dev)
> +static bool acpi_dev_state_update_needed(struct device *dev)
> {
> struct acpi_device *adev = ACPI_COMPANION(dev);
> u32 sys_target;
> int ret, state;
>
> - ret = pm_generic_prepare(dev);
> - if (ret < 0)
> - return ret;
> + if (!adev || !pm_runtime_suspended(dev))
> + return true;
>
> - if (!adev || !pm_runtime_suspended(dev)
> - || device_may_wakeup(dev) != !!adev->wakeup.prepare_count)
> - return 0;
> + if (device_may_wakeup(dev) != !!adev->wakeup.prepare_count)
> + return true;
>
> sys_target = acpi_target_system_state();
> if (sys_target == ACPI_STATE_S0)
> - return 1;
> + return false;
>
> if (adev->power.flags.dsw_present)
> - return 0;
> + return true;
>
> ret = acpi_dev_pm_get_state(dev, adev, sys_target, NULL, &state);
> - return !ret && state == adev->power.state;
> + if (ret)
> + return true;
> +
> + return state != adev->power.state;
> +}
> +
> +/**
> + * acpi_subsys_prepare - Prepare device for system transition to a sleep state.
> + * @dev: Device to prepare.
> + */
> +int acpi_subsys_prepare(struct device *dev)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(dev);
> + int ret;
> +
> + ret = pm_generic_prepare(dev);
> + if (ret < 0)
> + return ret;
> +
> + adev->power.update_state = acpi_dev_state_update_needed(dev);
> + return !adev->power.update_state;
> }
> EXPORT_SYMBOL_GPL(acpi_subsys_prepare);
>
> @@ -1024,11 +1039,20 @@ EXPORT_SYMBOL_GPL(acpi_subsys_prepare);
> * @dev: Device to handle.
> *
> * Follow PCI and resume devices suspended at run time before running their
> - * system suspend callbacks.
> + * system suspend callbacks, unless the DPM_FLAG_SAFE_SUSPEND driver flag is
> + * set for them.
> */
> int acpi_subsys_suspend(struct device *dev)
> {
> - pm_runtime_resume(dev);
> + struct acpi_device *adev = ACPI_COMPANION(dev);
> +
> + if (!adev)
> + return 0;
> +
> + if (adev->power.update_state ||
> + !(dev->power.driver_flags & DPM_FLAG_SAFE_SUSPEND))
> + pm_runtime_resume(dev);
> +
> return pm_generic_suspend(dev);
> }
> EXPORT_SYMBOL_GPL(acpi_subsys_suspend);
> @@ -1042,8 +1066,20 @@ EXPORT_SYMBOL_GPL(acpi_subsys_suspend);
> */
> int acpi_subsys_suspend_late(struct device *dev)
> {
> - int ret = pm_generic_suspend_late(dev);
> - return ret ? ret : acpi_dev_suspend_late(dev);
> + struct acpi_device *adev = ACPI_COMPANION(dev);
> + int ret;
> +
> + if (!adev)
> + return 0;
> +
> + ret = pm_generic_suspend_late(dev);
> + if (ret)
> + return ret;
> +
> + if (adev->power.update_state)
> + return acpi_dev_suspend_late(dev);
> +
> + return 0;
> }
> EXPORT_SYMBOL_GPL(acpi_subsys_suspend_late);
>
> Index: linux-pm/drivers/base/dd.c
> ===================================================================
> --- linux-pm.orig/drivers/base/dd.c
> +++ linux-pm/drivers/base/dd.c
> @@ -436,6 +436,7 @@ pinctrl_bind_failed:
> if (dev->pm_domain && dev->pm_domain->dismiss)
> dev->pm_domain->dismiss(dev);
> pm_runtime_reinit(dev);
> + dev->power.driver_flags = 0;
>
> switch (ret) {
> case -EPROBE_DEFER:
> @@ -841,6 +842,7 @@ static void __device_release_driver(stru
> if (dev->pm_domain && dev->pm_domain->dismiss)
> dev->pm_domain->dismiss(dev);
> pm_runtime_reinit(dev);
> + dev->power.driver_flags = 0;
>
> klist_remove(&dev->p->knode_driver);
> device_pm_check_callbacks(dev);
> Index: linux-pm/Documentation/driver-api/pm/devices.rst
> ===================================================================
> --- linux-pm.orig/Documentation/driver-api/pm/devices.rst
> +++ linux-pm/Documentation/driver-api/pm/devices.rst
> @@ -729,6 +729,13 @@ state temporarily, for example so that i
> disabled. This all depends on the hardware and the design of the subsystem and
> device driver in question.
>
> +Some bus types and PM domains have a policy to runtime resume all
> +devices upfront in their ``->suspend`` callbacks, but that may not be really
> +necessary if the system suspend-resume callbacks provided by the device's
> +driver can cope with a runtime-suspended device. The driver can indicate that
> +by setting ``DPM_FLAG_SAFE_SUSPEND`` in :c:member:`power.driver_flags` at the
> +probe time.
> +
> During system-wide resume from a sleep state it's easiest to put devices into
> the full-power state, as explained in :file:`Documentation/power/runtime_pm.txt`.
> Refer to that document for more information regarding this particular issue as
> Index: linux-pm/include/acpi/acpi_bus.h
> ===================================================================
> --- linux-pm.orig/include/acpi/acpi_bus.h
> +++ linux-pm/include/acpi/acpi_bus.h
> @@ -287,6 +287,7 @@ struct acpi_device_power {
> int state; /* Current state */
> struct acpi_device_power_flags flags;
> struct acpi_device_power_state states[ACPI_D_STATE_COUNT]; /* Power states (D0-D3Cold) */
> + bool update_state;
> };
>
> /* Performance Management */
> Index: linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c
> ===================================================================
> --- linux-pm.orig/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -358,6 +358,7 @@ static int dw_i2c_plat_probe(struct plat
> if (dev->pm_disabled) {
> pm_runtime_forbid(&pdev->dev);
> } else {
> + dev->power.driver_flags = DPM_FLAG_SAFE_SUSPEND;
> pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> pm_runtime_use_autosuspend(&pdev->dev);
> pm_runtime_set_active(&pdev->dev);
> @@ -455,7 +456,8 @@ static int dw_i2c_plat_resume(struct dev
> static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
> .prepare = dw_i2c_plat_prepare,
> .complete = dw_i2c_plat_complete,
> - SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
> + SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> + pm_runtime_force_resume)
> SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume, NULL)
> };
>
>
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
