| Message ID | 1493388963-5278-1-git-send-email-imre.deak@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
On Friday, April 28, 2017 05:16:02 PM Imre Deak wrote: > Some drivers - like i915 - may not support the system suspend direct > complete optimization due to differences in their runtime and system > suspend sequence. Add a flag that when set resumes the device before > calling the driver's system suspend handlers which effectively disables > the optimization. > > Needed by the next patch fixing suspend/resume on i915. > > Suggested by Rafael. > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: linux-pci@vger.kernel.org > Cc: stable@vger.kernel.org > Signed-off-by: Imre Deak <imre.deak@intel.com> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> The reason why the opt-out flag was not added on day one was because we were not sure whether or not it would be necessary at all. Quite evidently, it is needed. > --- > > [ After discussing with Jani, I'm going to apply this and the next patch > for now to the intel-gfx specific CI branch to unblock our testing. ] > > drivers/pci/pci.c | 3 ++- > include/linux/pci.h | 7 +++++++ > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 7904d02..020f02d 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -2141,7 +2141,8 @@ bool pci_dev_keep_suspended(struct pci_dev *pci_dev) > > if (!pm_runtime_suspended(dev) > || pci_target_state(pci_dev) != pci_dev->current_state > - || platform_pci_need_resume(pci_dev)) > + || platform_pci_need_resume(pci_dev) > + || pci_dev->needs_resume) > return false; > > /* > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 5948cfd..2f012f8 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -316,6 +316,9 @@ struct pci_dev { > unsigned int hotplug_user_indicators:1; /* SlotCtl indicators > controlled exclusively by > user sysfs */ > + unsigned int needs_resume:1; /* Resume before calling the driver's > + system suspend hooks, disabling the > + direct_complete optimization. */ > unsigned int d3_delay; /* D3->D0 transition time in ms */ > unsigned int d3cold_delay; /* D3cold->D0 transition time in ms */ > > @@ -1110,6 +1113,10 @@ bool pci_check_pme_status(struct pci_dev *dev); > void pci_pme_wakeup_bus(struct pci_bus *bus); > void pci_d3cold_enable(struct pci_dev *dev); > void pci_d3cold_disable(struct pci_dev *dev); > +static inline void pci_resume_before_suspend(struct pci_dev *dev, bool enable) > +{ > + dev->needs_resume = enable; > +} > > static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state, > bool enable) >
On Friday, April 28, 2017 11:33:02 PM Rafael J. Wysocki wrote: > On Friday, April 28, 2017 05:16:02 PM Imre Deak wrote: > > Some drivers - like i915 - may not support the system suspend direct > > complete optimization due to differences in their runtime and system > > suspend sequence. Add a flag that when set resumes the device before > > calling the driver's system suspend handlers which effectively disables > > the optimization. > > > > Needed by the next patch fixing suspend/resume on i915. > > > > Suggested by Rafael. > > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > Cc: linux-pci@vger.kernel.org > > Cc: stable@vger.kernel.org > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > The reason why the opt-out flag was not added on day one was because we were > not sure whether or not it would be necessary at all. > > Quite evidently, it is needed. But that said, it actually can be implemented as a flag in dev_flags too, say PCI_DEV_FLAGS_NEEDS_RESUME, in analogy with PCI_DEV_FLAGS_NO_D3 that's already there. The struct size would not need to grow then which I guess would be better? Thanks, Rafael
On Sat, Apr 29, 2017 at 12:21:57PM +0200, Rafael J. Wysocki wrote: > On Friday, April 28, 2017 11:33:02 PM Rafael J. Wysocki wrote: > > On Friday, April 28, 2017 05:16:02 PM Imre Deak wrote: > > > Some drivers - like i915 - may not support the system suspend direct > > > complete optimization due to differences in their runtime and system > > > suspend sequence. Add a flag that when set resumes the device before > > > calling the driver's system suspend handlers which effectively disables > > > the optimization. > > > > > > Needed by the next patch fixing suspend/resume on i915. > > > > > > Suggested by Rafael. > > > > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > > Cc: linux-pci@vger.kernel.org > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > The reason why the opt-out flag was not added on day one was because we were > > not sure whether or not it would be necessary at all. > > > > Quite evidently, it is needed. > > But that said, it actually can be implemented as a flag in dev_flags too, say > PCI_DEV_FLAGS_NEEDS_RESUME, in analogy with PCI_DEV_FLAGS_NO_D3 that's > already there. > > The struct size would not need to grow then which I guess would be better? Hm, both the bit field and the flag would need to increase if running out of bits, so what's the difference? (Atm, the struct size wouldn't change either way.) --Imre
On Sunday, April 30, 2017 03:57:13 PM Imre Deak wrote: > On Sat, Apr 29, 2017 at 12:21:57PM +0200, Rafael J. Wysocki wrote: > > On Friday, April 28, 2017 11:33:02 PM Rafael J. Wysocki wrote: > > > On Friday, April 28, 2017 05:16:02 PM Imre Deak wrote: > > > > Some drivers - like i915 - may not support the system suspend direct > > > > complete optimization due to differences in their runtime and system > > > > suspend sequence. Add a flag that when set resumes the device before > > > > calling the driver's system suspend handlers which effectively disables > > > > the optimization. > > > > > > > > Needed by the next patch fixing suspend/resume on i915. > > > > > > > > Suggested by Rafael. > > > > > > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > > > Cc: linux-pci@vger.kernel.org > > > > Cc: stable@vger.kernel.org > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > The reason why the opt-out flag was not added on day one was because we were > > > not sure whether or not it would be necessary at all. > > > > > > Quite evidently, it is needed. > > > > But that said, it actually can be implemented as a flag in dev_flags too, say > > PCI_DEV_FLAGS_NEEDS_RESUME, in analogy with PCI_DEV_FLAGS_NO_D3 that's > > already there. > > > > The struct size would not need to grow then which I guess would be better? > > Hm, both the bit field and the flag would need to increase if running > out of bits, so what's the difference? (Atm, the struct size wouldn't > change either way.) In the bit field case this depends on what the compiler thinks is better to be entirely precise, so they are not 100% equivalent. Plus, since there already are things related to PM in dev_flags, why to depart from that pattern? Thanks, Rafael
On Mon, May 01, 2017 at 10:36:13PM +0200, Rafael J. Wysocki wrote: > On Sunday, April 30, 2017 03:57:13 PM Imre Deak wrote: > > On Sat, Apr 29, 2017 at 12:21:57PM +0200, Rafael J. Wysocki wrote: > > > On Friday, April 28, 2017 11:33:02 PM Rafael J. Wysocki wrote: > > > > On Friday, April 28, 2017 05:16:02 PM Imre Deak wrote: > > > > > Some drivers - like i915 - may not support the system suspend direct > > > > > complete optimization due to differences in their runtime and system > > > > > suspend sequence. Add a flag that when set resumes the device before > > > > > calling the driver's system suspend handlers which effectively disables > > > > > the optimization. > > > > > > > > > > Needed by the next patch fixing suspend/resume on i915. > > > > > > > > > > Suggested by Rafael. > > > > > > > > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > > > > Cc: linux-pci@vger.kernel.org > > > > > Cc: stable@vger.kernel.org > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > > > > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > The reason why the opt-out flag was not added on day one was because we were > > > > not sure whether or not it would be necessary at all. > > > > > > > > Quite evidently, it is needed. > > > > > > But that said, it actually can be implemented as a flag in dev_flags too, say > > > PCI_DEV_FLAGS_NEEDS_RESUME, in analogy with PCI_DEV_FLAGS_NO_D3 that's > > > already there. > > > > > > The struct size would not need to grow then which I guess would be better? > > > > Hm, both the bit field and the flag would need to increase if running > > out of bits, so what's the difference? (Atm, the struct size wouldn't > > change either way.) > > In the bit field case this depends on what the compiler thinks is better to be > entirely precise, so they are not 100% equivalent. > > Plus, since there already are things related to PM in dev_flags, why to depart > from that pattern? There are a few PM related flags in the bit field too. The need for moving it is still not clear to me, but I don't see any problem with it either, so will move it there. --Imre
On Tuesday, May 02, 2017 12:05:38 PM Imre Deak wrote: > On Mon, May 01, 2017 at 10:36:13PM +0200, Rafael J. Wysocki wrote: > > On Sunday, April 30, 2017 03:57:13 PM Imre Deak wrote: > > > On Sat, Apr 29, 2017 at 12:21:57PM +0200, Rafael J. Wysocki wrote: > > > > On Friday, April 28, 2017 11:33:02 PM Rafael J. Wysocki wrote: > > > > > On Friday, April 28, 2017 05:16:02 PM Imre Deak wrote: > > > > > > Some drivers - like i915 - may not support the system suspend direct > > > > > > complete optimization due to differences in their runtime and system > > > > > > suspend sequence. Add a flag that when set resumes the device before > > > > > > calling the driver's system suspend handlers which effectively disables > > > > > > the optimization. > > > > > > > > > > > > Needed by the next patch fixing suspend/resume on i915. > > > > > > > > > > > > Suggested by Rafael. > > > > > > > > > > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > > > > > Cc: linux-pci@vger.kernel.org > > > > > > Cc: stable@vger.kernel.org > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > > > > > > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > > > The reason why the opt-out flag was not added on day one was because we were > > > > > not sure whether or not it would be necessary at all. > > > > > > > > > > Quite evidently, it is needed. > > > > > > > > But that said, it actually can be implemented as a flag in dev_flags too, say > > > > PCI_DEV_FLAGS_NEEDS_RESUME, in analogy with PCI_DEV_FLAGS_NO_D3 that's > > > > already there. > > > > > > > > The struct size would not need to grow then which I guess would be better? > > > > > > Hm, both the bit field and the flag would need to increase if running > > > out of bits, so what's the difference? (Atm, the struct size wouldn't > > > change either way.) > > > > In the bit field case this depends on what the compiler thinks is better to be > > entirely precise, so they are not 100% equivalent. > > > > Plus, since there already are things related to PM in dev_flags, why to depart > > from that pattern? > > There are a few PM related flags in the bit field too. > > The need for moving it is still not clear to me, but I don't see any > problem with it either, so will move it there. Well, we are not too consistent in that respect overall. Either way works, so I guess it's Bjorn's call at this point. :-) Thanks, Rafael
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 7904d02..020f02d 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -2141,7 +2141,8 @@ bool pci_dev_keep_suspended(struct pci_dev *pci_dev) if (!pm_runtime_suspended(dev) || pci_target_state(pci_dev) != pci_dev->current_state - || platform_pci_need_resume(pci_dev)) + || platform_pci_need_resume(pci_dev) + || pci_dev->needs_resume) return false; /* diff --git a/include/linux/pci.h b/include/linux/pci.h index 5948cfd..2f012f8 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -316,6 +316,9 @@ struct pci_dev { unsigned int hotplug_user_indicators:1; /* SlotCtl indicators controlled exclusively by user sysfs */ + unsigned int needs_resume:1; /* Resume before calling the driver's + system suspend hooks, disabling the + direct_complete optimization. */ unsigned int d3_delay; /* D3->D0 transition time in ms */ unsigned int d3cold_delay; /* D3cold->D0 transition time in ms */ @@ -1110,6 +1113,10 @@ bool pci_check_pme_status(struct pci_dev *dev); void pci_pme_wakeup_bus(struct pci_bus *bus); void pci_d3cold_enable(struct pci_dev *dev); void pci_d3cold_disable(struct pci_dev *dev); +static inline void pci_resume_before_suspend(struct pci_dev *dev, bool enable) +{ + dev->needs_resume = enable; +} static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state, bool enable)
Some drivers - like i915 - may not support the system suspend direct complete optimization due to differences in their runtime and system suspend sequence. Add a flag that when set resumes the device before calling the driver's system suspend handlers which effectively disables the optimization. Needed by the next patch fixing suspend/resume on i915. Suggested by Rafael. Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: linux-pci@vger.kernel.org Cc: stable@vger.kernel.org Signed-off-by: Imre Deak <imre.deak@intel.com> --- [ After discussing with Jani, I'm going to apply this and the next patch for now to the intel-gfx specific CI branch to unblock our testing. ] drivers/pci/pci.c | 3 ++- include/linux/pci.h | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletion(-)