| Message ID | 2333939.YcVPemMPxH@aspire.rjw.lan |
|---|---|
| State | Not Applicable |
| Headers | show |
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] > > > > > [BTW, it is not entirely clear to me why it ever is necessary to runtime resume > > > a device with direct_complete set after __device_suspend(), because it can only > > > have direct_complete set at that point if all of the hierarchy below it has > > > this flag set too and so runtime PM has to be disabled for all of those > > > devices as well.] > > > > Which makes me realize that we should take a step back and look at what > > problems there are. > > > > First, there are devices (I know about two examples so far and both are PCI) > > that may need to be runtime resumed during system suspend for reasons other > > than the ones checked by the ACPI PM domain (or the PCI bus type). There needs > > to be a way to indicate that from the driver side. > > > > However, it still may be valuable to check the power-related conditions for > > leaving the device in runtime suspend over system suspend/resume in case > > it actually doesn't need to be runtime resumed during system suspend after > > all. That's what the majority of my patch was about. > > > > The second problem is that the ACPI PM domain (and the PCI bus type) > > runtime resumes all devices unconditionally in its ->suspend callback, > > even though that may not be necessary for some devices. Therefore there > > needs to be a way to indicate that too. That still would be good to > > have *regardless* of the direct_complete mechanism, because the direct_complete > > flag may not be set very often due to dependencies and then the > > resume-during-suspend will take place unnecessarily. > > > > Accordingly, it looks like we need a "no need to resume me" flag in the first > > place. That would indicate to interested pieces of code that, from the > > driver perspective, the device doesn't need to be runtime resumed before > > invoking its system suspend callbacks. This should be clear enough to everyone > > IMO. > > > > [Note that if that flag is set for all devices, we may drop it along with > > direct_complete, but before that happens both are needed.] > > I think we are in agreement that direct_complete will not be necessary any > more when all drivers/bus types/PM domains and so on can do the "safe > suspend", but we're not there yet. :-) > > > To address the first issue I would add something like the flag in the patches > > I sent (but without the ACPI PM domain part which should be covered by the > > "no need to resume me" flag above), because that allows the device's ->suspend > > callback to run in principle and the driver may use that callback even to > > runtime resume the device if that's what it wants to do. So something like > > "run my ->suspend callback even though I might stay in runtime suspend". > > > > I would probably add driver_flags to dev_pm_info for that to set at the probe > > time (and I would make the core clear that on driver removal). > > > > The complexity concern is there, but honestly I don't see a better way at > > this point. > > So below is a prototype patch. It still is missing a documentation update, but > other than that it should be complete unless I missed something. > > The way it works is that the SAFE_SUSPEND flag is not looked at by the core > at all. The ACPI PM domain looks at it and the PCI bus type can be modified > to take it into account in the future. That is what causes the "runtime resume > during system suspend" to be skipped. > > In turn, the ALWAYS_SUSPEND flag is only looked at by the core and it causes > the decision on whether or not to use direct_complete to be deferred to the > __device_suspend_late() time. If you set it for a PCI device, the effect is > equivalent to "no direct_complete". If you set it for a device in the ACPI > PM domain, that depends on whether or not SAFE_SUSPEND is set. If it isn't > set, the effect is equivalent to "no direct_complete" too, but if it is set, > the core may still try to use direct_complete for the device, but it will > make the decision on it in __device_suspend_late() and then it will not invoke > the ->suspend_late callback for the device if it is still runtime suspended. > [Note that you cannot runtime resume and runtime suspend again a device during > system suspend, so if it is runtime suspended in __device_suspend_late(), it > has been runtime suspend all the way since device_prepare().] > > So say you point the ->suspend_late and ->resume_early callbacks of > the designware i2c driver to pm_runtime_force_suspend() and > pm_runtime_force_resume(), respectively, and set both the SAFE_SUSPEND > and ALWAYS_SUSPEND flags for the device. > > If system suspend is started and the device is not runtime suspended, > direct_complete is not set for it and everything works as usual, so say > the device is runtime suspended in device_prepare(). Then, the ACPI PM > domain checks the other conditions for leaving it in runtime suspend and > returns either 0 or a positive number from acpi_subsys_prepare(). > > If 0 is returned, direct_complete is not set by the core and > acpi_subsys_suspend() is called. It checks the SAFE_SUSPEND flag and sees > that the device need not be runtime resumed, so it invokes the driver's > ->suspend callback (which is not present, so it doesn't do anything). > Next, in __device_suspend_late(), acpi_subsys_suspend_late() is invoked > and it calls pm_runtime_force_suspend(), which executes the driver's > ->runtime_suspend() callback, and then (if successful) calls > acpi_dev_suspend_late() to put the device into a low-power state. The > resume path is a reverse of the above in this case. So far, so good. 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. Thanks, Rafael
On 28 August 2017 at 03:30, Rafael J. Wysocki <rjw@rjwysocki.net> 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] >> > >> > > [BTW, it is not entirely clear to me why it ever is necessary to runtime resume >> > > a device with direct_complete set after __device_suspend(), because it can only >> > > have direct_complete set at that point if all of the hierarchy below it has >> > > this flag set too and so runtime PM has to be disabled for all of those >> > > devices as well.] >> > >> > Which makes me realize that we should take a step back and look at what >> > problems there are. >> > >> > First, there are devices (I know about two examples so far and both are PCI) >> > that may need to be runtime resumed during system suspend for reasons other >> > than the ones checked by the ACPI PM domain (or the PCI bus type). There needs >> > to be a way to indicate that from the driver side. >> > >> > However, it still may be valuable to check the power-related conditions for >> > leaving the device in runtime suspend over system suspend/resume in case >> > it actually doesn't need to be runtime resumed during system suspend after >> > all. That's what the majority of my patch was about. >> > >> > The second problem is that the ACPI PM domain (and the PCI bus type) >> > runtime resumes all devices unconditionally in its ->suspend callback, >> > even though that may not be necessary for some devices. Therefore there >> > needs to be a way to indicate that too. That still would be good to >> > have *regardless* of the direct_complete mechanism, because the direct_complete >> > flag may not be set very often due to dependencies and then the >> > resume-during-suspend will take place unnecessarily. >> > >> > Accordingly, it looks like we need a "no need to resume me" flag in the first >> > place. That would indicate to interested pieces of code that, from the >> > driver perspective, the device doesn't need to be runtime resumed before >> > invoking its system suspend callbacks. This should be clear enough to everyone >> > IMO. >> > >> > [Note that if that flag is set for all devices, we may drop it along with >> > direct_complete, but before that happens both are needed.] >> >> I think we are in agreement that direct_complete will not be necessary any >> more when all drivers/bus types/PM domains and so on can do the "safe >> suspend", but we're not there yet. :-) >> >> > To address the first issue I would add something like the flag in the patches >> > I sent (but without the ACPI PM domain part which should be covered by the >> > "no need to resume me" flag above), because that allows the device's ->suspend >> > callback to run in principle and the driver may use that callback even to >> > runtime resume the device if that's what it wants to do. So something like >> > "run my ->suspend callback even though I might stay in runtime suspend". >> > >> > I would probably add driver_flags to dev_pm_info for that to set at the probe >> > time (and I would make the core clear that on driver removal). >> > >> > The complexity concern is there, but honestly I don't see a better way at >> > this point. >> >> So below is a prototype patch. It still is missing a documentation update, but >> other than that it should be complete unless I missed something. >> >> The way it works is that the SAFE_SUSPEND flag is not looked at by the core >> at all. The ACPI PM domain looks at it and the PCI bus type can be modified >> to take it into account in the future. That is what causes the "runtime resume >> during system suspend" to be skipped. >> >> In turn, the ALWAYS_SUSPEND flag is only looked at by the core and it causes >> the decision on whether or not to use direct_complete to be deferred to the >> __device_suspend_late() time. If you set it for a PCI device, the effect is >> equivalent to "no direct_complete". If you set it for a device in the ACPI >> PM domain, that depends on whether or not SAFE_SUSPEND is set. If it isn't >> set, the effect is equivalent to "no direct_complete" too, but if it is set, >> the core may still try to use direct_complete for the device, but it will >> make the decision on it in __device_suspend_late() and then it will not invoke >> the ->suspend_late callback for the device if it is still runtime suspended. >> [Note that you cannot runtime resume and runtime suspend again a device during >> system suspend, so if it is runtime suspended in __device_suspend_late(), it >> has been runtime suspend all the way since device_prepare().] >> >> So say you point the ->suspend_late and ->resume_early callbacks of >> the designware i2c driver to pm_runtime_force_suspend() and >> pm_runtime_force_resume(), respectively, and set both the SAFE_SUSPEND >> and ALWAYS_SUSPEND flags for the device. >> >> If system suspend is started and the device is not runtime suspended, >> direct_complete is not set for it and everything works as usual, so say >> the device is runtime suspended in device_prepare(). Then, the ACPI PM >> domain checks the other conditions for leaving it in runtime suspend and >> returns either 0 or a positive number from acpi_subsys_prepare(). >> >> If 0 is returned, direct_complete is not set by the core and >> acpi_subsys_suspend() is called. It checks the SAFE_SUSPEND flag and sees >> that the device need not be runtime resumed, so it invokes the driver's >> ->suspend callback (which is not present, so it doesn't do anything). >> Next, in __device_suspend_late(), acpi_subsys_suspend_late() is invoked >> and it calls pm_runtime_force_suspend(), which executes the driver's >> ->runtime_suspend() callback, and then (if successful) calls >> acpi_dev_suspend_late() to put the device into a low-power state. The >> resume path is a reverse of the above in this case. So far, so good. > > 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. Kind regards Uffe
On Monday, August 28, 2017 10:31:44 AM CEST Ulf Hansson wrote: > On 28 August 2017 at 03:30, Rafael J. Wysocki <rjw@rjwysocki.net> 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] > >> > > >> > > [BTW, it is not entirely clear to me why it ever is necessary to runtime resume > >> > > a device with direct_complete set after __device_suspend(), because it can only > >> > > have direct_complete set at that point if all of the hierarchy below it has > >> > > this flag set too and so runtime PM has to be disabled for all of those > >> > > devices as well.] > >> > > >> > Which makes me realize that we should take a step back and look at what > >> > problems there are. > >> > > >> > First, there are devices (I know about two examples so far and both are PCI) > >> > that may need to be runtime resumed during system suspend for reasons other > >> > than the ones checked by the ACPI PM domain (or the PCI bus type). There needs > >> > to be a way to indicate that from the driver side. > >> > > >> > However, it still may be valuable to check the power-related conditions for > >> > leaving the device in runtime suspend over system suspend/resume in case > >> > it actually doesn't need to be runtime resumed during system suspend after > >> > all. That's what the majority of my patch was about. > >> > > >> > The second problem is that the ACPI PM domain (and the PCI bus type) > >> > runtime resumes all devices unconditionally in its ->suspend callback, > >> > even though that may not be necessary for some devices. Therefore there > >> > needs to be a way to indicate that too. That still would be good to > >> > have *regardless* of the direct_complete mechanism, because the direct_complete > >> > flag may not be set very often due to dependencies and then the > >> > resume-during-suspend will take place unnecessarily. > >> > > >> > Accordingly, it looks like we need a "no need to resume me" flag in the first > >> > place. That would indicate to interested pieces of code that, from the > >> > driver perspective, the device doesn't need to be runtime resumed before > >> > invoking its system suspend callbacks. This should be clear enough to everyone > >> > IMO. > >> > > >> > [Note that if that flag is set for all devices, we may drop it along with > >> > direct_complete, but before that happens both are needed.] > >> > >> I think we are in agreement that direct_complete will not be necessary any > >> more when all drivers/bus types/PM domains and so on can do the "safe > >> suspend", but we're not there yet. :-) > >> > >> > To address the first issue I would add something like the flag in the patches > >> > I sent (but without the ACPI PM domain part which should be covered by the > >> > "no need to resume me" flag above), because that allows the device's ->suspend > >> > callback to run in principle and the driver may use that callback even to > >> > runtime resume the device if that's what it wants to do. So something like > >> > "run my ->suspend callback even though I might stay in runtime suspend". > >> > > >> > I would probably add driver_flags to dev_pm_info for that to set at the probe > >> > time (and I would make the core clear that on driver removal). > >> > > >> > The complexity concern is there, but honestly I don't see a better way at > >> > this point. > >> > >> So below is a prototype patch. It still is missing a documentation update, but > >> other than that it should be complete unless I missed something. > >> > >> The way it works is that the SAFE_SUSPEND flag is not looked at by the core > >> at all. The ACPI PM domain looks at it and the PCI bus type can be modified > >> to take it into account in the future. That is what causes the "runtime resume > >> during system suspend" to be skipped. > >> > >> In turn, the ALWAYS_SUSPEND flag is only looked at by the core and it causes > >> the decision on whether or not to use direct_complete to be deferred to the > >> __device_suspend_late() time. If you set it for a PCI device, the effect is > >> equivalent to "no direct_complete". If you set it for a device in the ACPI > >> PM domain, that depends on whether or not SAFE_SUSPEND is set. If it isn't > >> set, the effect is equivalent to "no direct_complete" too, but if it is set, > >> the core may still try to use direct_complete for the device, but it will > >> make the decision on it in __device_suspend_late() and then it will not invoke > >> the ->suspend_late callback for the device if it is still runtime suspended. > >> [Note that you cannot runtime resume and runtime suspend again a device during > >> system suspend, so if it is runtime suspended in __device_suspend_late(), it > >> has been runtime suspend all the way since device_prepare().] > >> > >> So say you point the ->suspend_late and ->resume_early callbacks of > >> the designware i2c driver to pm_runtime_force_suspend() and > >> pm_runtime_force_resume(), respectively, and set both the SAFE_SUSPEND > >> and ALWAYS_SUSPEND flags for the device. > >> > >> If system suspend is started and the device is not runtime suspended, > >> direct_complete is not set for it and everything works as usual, so say > >> the device is runtime suspended in device_prepare(). Then, the ACPI PM > >> domain checks the other conditions for leaving it in runtime suspend and > >> returns either 0 or a positive number from acpi_subsys_prepare(). > >> > >> If 0 is returned, direct_complete is not set by the core and > >> acpi_subsys_suspend() is called. It checks the SAFE_SUSPEND flag and sees > >> that the device need not be runtime resumed, so it invokes the driver's > >> ->suspend callback (which is not present, so it doesn't do anything). > >> Next, in __device_suspend_late(), acpi_subsys_suspend_late() is invoked > >> and it calls pm_runtime_force_suspend(), which executes the driver's > >> ->runtime_suspend() callback, and then (if successful) calls > >> acpi_dev_suspend_late() to put the device into a low-power state. The > >> resume path is a reverse of the above in this case. So far, so good. > > > > 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. Thanks, Rafael
On 28 August 2017 at 14:39, Rafael J. Wysocki <rjw@rjwysocki.net> 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@rjwysocki.net> 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] >> >> > >> >> > > [BTW, it is not entirely clear to me why it ever is necessary to runtime resume >> >> > > a device with direct_complete set after __device_suspend(), because it can only >> >> > > have direct_complete set at that point if all of the hierarchy below it has >> >> > > this flag set too and so runtime PM has to be disabled for all of those >> >> > > devices as well.] >> >> > >> >> > Which makes me realize that we should take a step back and look at what >> >> > problems there are. >> >> > >> >> > First, there are devices (I know about two examples so far and both are PCI) >> >> > that may need to be runtime resumed during system suspend for reasons other >> >> > than the ones checked by the ACPI PM domain (or the PCI bus type). There needs >> >> > to be a way to indicate that from the driver side. >> >> > >> >> > However, it still may be valuable to check the power-related conditions for >> >> > leaving the device in runtime suspend over system suspend/resume in case >> >> > it actually doesn't need to be runtime resumed during system suspend after >> >> > all. That's what the majority of my patch was about. >> >> > >> >> > The second problem is that the ACPI PM domain (and the PCI bus type) >> >> > runtime resumes all devices unconditionally in its ->suspend callback, >> >> > even though that may not be necessary for some devices. Therefore there >> >> > needs to be a way to indicate that too. That still would be good to >> >> > have *regardless* of the direct_complete mechanism, because the direct_complete >> >> > flag may not be set very often due to dependencies and then the >> >> > resume-during-suspend will take place unnecessarily. >> >> > >> >> > Accordingly, it looks like we need a "no need to resume me" flag in the first >> >> > place. That would indicate to interested pieces of code that, from the >> >> > driver perspective, the device doesn't need to be runtime resumed before >> >> > invoking its system suspend callbacks. This should be clear enough to everyone >> >> > IMO. >> >> > >> >> > [Note that if that flag is set for all devices, we may drop it along with >> >> > direct_complete, but before that happens both are needed.] >> >> >> >> I think we are in agreement that direct_complete will not be necessary any >> >> more when all drivers/bus types/PM domains and so on can do the "safe >> >> suspend", but we're not there yet. :-) >> >> >> >> > To address the first issue I would add something like the flag in the patches >> >> > I sent (but without the ACPI PM domain part which should be covered by the >> >> > "no need to resume me" flag above), because that allows the device's ->suspend >> >> > callback to run in principle and the driver may use that callback even to >> >> > runtime resume the device if that's what it wants to do. So something like >> >> > "run my ->suspend callback even though I might stay in runtime suspend". >> >> > >> >> > I would probably add driver_flags to dev_pm_info for that to set at the probe >> >> > time (and I would make the core clear that on driver removal). >> >> > >> >> > The complexity concern is there, but honestly I don't see a better way at >> >> > this point. >> >> >> >> So below is a prototype patch. It still is missing a documentation update, but >> >> other than that it should be complete unless I missed something. >> >> >> >> The way it works is that the SAFE_SUSPEND flag is not looked at by the core >> >> at all. The ACPI PM domain looks at it and the PCI bus type can be modified >> >> to take it into account in the future. That is what causes the "runtime resume >> >> during system suspend" to be skipped. >> >> >> >> In turn, the ALWAYS_SUSPEND flag is only looked at by the core and it causes >> >> the decision on whether or not to use direct_complete to be deferred to the >> >> __device_suspend_late() time. If you set it for a PCI device, the effect is >> >> equivalent to "no direct_complete". If you set it for a device in the ACPI >> >> PM domain, that depends on whether or not SAFE_SUSPEND is set. If it isn't >> >> set, the effect is equivalent to "no direct_complete" too, but if it is set, >> >> the core may still try to use direct_complete for the device, but it will >> >> make the decision on it in __device_suspend_late() and then it will not invoke >> >> the ->suspend_late callback for the device if it is still runtime suspended. >> >> [Note that you cannot runtime resume and runtime suspend again a device during >> >> system suspend, so if it is runtime suspended in __device_suspend_late(), it >> >> has been runtime suspend all the way since device_prepare().] >> >> >> >> So say you point the ->suspend_late and ->resume_early callbacks of >> >> the designware i2c driver to pm_runtime_force_suspend() and >> >> pm_runtime_force_resume(), respectively, and set both the SAFE_SUSPEND >> >> and ALWAYS_SUSPEND flags for the device. >> >> >> >> If system suspend is started and the device is not runtime suspended, >> >> direct_complete is not set for it and everything works as usual, so say >> >> the device is runtime suspended in device_prepare(). Then, the ACPI PM >> >> domain checks the other conditions for leaving it in runtime suspend and >> >> returns either 0 or a positive number from acpi_subsys_prepare(). >> >> >> >> If 0 is returned, direct_complete is not set by the core and >> >> acpi_subsys_suspend() is called. It checks the SAFE_SUSPEND flag and sees >> >> that the device need not be runtime resumed, so it invokes the driver's >> >> ->suspend callback (which is not present, so it doesn't do anything). >> >> Next, in __device_suspend_late(), acpi_subsys_suspend_late() is invoked >> >> and it calls pm_runtime_force_suspend(), which executes the driver's >> >> ->runtime_suspend() callback, and then (if successful) calls >> >> acpi_dev_suspend_late() to put the device into a low-power state. The >> >> resume path is a reverse of the above in this case. So far, so good. >> > >> > 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. Kind regards Uffe
On Monday, August 28, 2017 2:54:40 PM CEST Ulf Hansson wrote: > On 28 August 2017 at 14:39, Rafael J. Wysocki <rjw@rjwysocki.net> 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@rjwysocki.net> 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. 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. For (b) I'd like to use the SAFE_SUSPEND flag from my previous patch. 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. 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) };
On 28 August 2017 at 15:40, Rafael J. Wysocki <rjw@rjwysocki.net> 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@rjwysocki.net> 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@rjwysocki.net> 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
On Monday, August 28, 2017 4:24:51 PM CEST Ulf Hansson wrote: > On 28 August 2017 at 15:40, Rafael J. Wysocki <rjw@rjwysocki.net> 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@rjwysocki.net> 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@rjwysocki.net> 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(). Sure enough, but I'm seeing that as a separate issue. > 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. I did, but I'd prefer to get back to that later. If the issues occurring when direct_complete is not set are addressed first, disabling direct_complete for the devices that cannot use it should be straightforward. > 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! Good. :-) Let me split the patch into a more manageable series, then, clean it up a bit and add the LPSS coverage to the ACPI part. Thanks, Rafael
Index: linux-pm/drivers/base/power/main.c =================================================================== --- linux-pm.orig/drivers/base/power/main.c +++ linux-pm/drivers/base/power/main.c @@ -1271,9 +1271,16 @@ static int __device_suspend_late(struct goto Complete; } - if (dev->power.syscore || dev->power.direct_complete) + if (dev->power.syscore) goto Complete; + if (dev->power.direct_complete) { + if (pm_runtime_status_suspended(dev)) + goto Complete; + + dev->power.direct_complete = false; + } + if (dev->pm_domain) { info = "late power domain "; callback = pm_late_early_op(&dev->pm_domain->ops, state); @@ -1437,7 +1444,10 @@ static void dpm_clear_suppliers_direct_c list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) { spin_lock_irq(&link->supplier->power.lock); - link->supplier->power.direct_complete = false; + + if (!(link->supplier->power.driver_flags & DPM_FLAG_ALWAYS_SUSPEND)) + link->supplier->power.direct_complete = false; + spin_unlock_irq(&link->supplier->power.lock); } @@ -1482,7 +1492,8 @@ static int __device_suspend(struct devic if (dev->power.syscore) goto Complete; - if (dev->power.direct_complete) { + if (dev->power.direct_complete && + !(dev->power.driver_flags & DPM_FLAG_ALWAYS_SUSPEND)) { if (pm_runtime_status_suspended(dev)) { pm_runtime_disable(dev); if (pm_runtime_status_suspended(dev)) @@ -1549,7 +1560,9 @@ static int __device_suspend(struct devic if (parent) { spin_lock_irq(&parent->power.lock); - dev->parent->power.direct_complete = false; + if (!(dev->parent->power.driver_flags & DPM_FLAG_ALWAYS_SUSPEND)) + dev->parent->power.direct_complete = false; + if (dev->power.wakeup_path && !dev->parent->power.ignore_children) dev->parent->power.wakeup_path = true; Index: linux-pm/include/linux/pm.h =================================================================== --- linux-pm.orig/include/linux/pm.h +++ linux-pm/include/linux/pm.h @@ -550,6 +550,37 @@ struct pm_subsys_data { #endif }; +/* + * Driver flags to control system suspend/resume behavior. + * + * SAFE_SUSPEND: No need to runtime resume the device during system suspend. + * ALWAYS_SUSPEND: Invoke ->suspend callback regardless of runtime PM status. + * + * 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. + * + * Setting SAFE_SUSPEND instructs bus types and PM domains that may want to + * runtime resume the device upfront during system suspend that doing so is not + * necessary from the driver's perspective, because its ->suspend callback can + * cope with a runtime suspended device (or is not present at all). + * + * Setting ALWAYS_SUSPEND instructs the PM core to always invoke the top-level + * ->suspend callback for the device during system suspend even though it may + * be runtime suspended at that point and might be left alone in principle. + * That is required for some devices that need to be prepared for system + * suspend in a special way which only is known to their drivers. However, + * setting ALWAYS_SUSPEND may also affect the devices parent and its parent and + * so on, as it may cause those devices to be runtime resumed upfront during + * system suspend unless SAFE_SUSPEND is set for them. + * + * Moreover, some bus types and PM domains have a policy to runtime resume all + * devices upfront in their ->suspend callbacks, so if ALWAYS_SUSPEND is set and + * SAFE_SUSPEND is not set for a device belonging to one of them, the device + * will be runtime resumed upfront every time during system suspend. + */ +#define DPM_FLAG_SAFE_SUSPEND BIT(0) +#define DPM_FLAG_ALWAYS_SUSPEND BIT(1) + struct dev_pm_info { pm_message_t power_state; unsigned int can_wakeup:1; @@ -561,6 +592,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 @@ -1024,11 +1024,14 @@ 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); + if (!(dev->power.driver_flags & DPM_FLAG_SAFE_SUSPEND)) + pm_runtime_resume(dev); + return pm_generic_suspend(dev); } EXPORT_SYMBOL_GPL(acpi_subsys_suspend); 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);