[PATCH v3 1/2] PCI / PM: Add needs_resume flag to avoid suspend complete optimization

[PATCH v3 1/2] PCI / PM: Add needs_resume flag to avoid suspend complete optimization

[Imre Deak]

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(-)

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)
-- 
2.5.0

[PATCH v3 2/2] drm/i915: Prevent the system suspend complete optimization

[Imre Deak]

Since

commit bac2a909a096c9110525c18cbb8ce73c660d5f71
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Date:   Wed Jan 21 02:17:42 2015 +0100

    PCI / PM: Avoid resuming PCI devices during system suspend

PCI devices will default to allowing the system suspend complete
optimization where devices are not woken up during system suspend if
they were already runtime suspended. This however breaks the i915/HDA
drivers for two reasons:

- The i915 driver has system suspend specific steps that it needs to
  run, that bring the device to a different state than its runtime
  suspended state.

- The HDA driver's suspend handler requires power that it will request
  from the i915 driver's power domain handler. This in turn requires the
  i915 driver to runtime resume itself, but this won't be possible if the
  suspend complete optimization is in effect: in this case the i915
  runtime PM is disabled and trying to get an RPM reference returns
  -EACCESS.

Solve this by requiring the PCI/PM core to resume the device during
system suspend which in effect disables the suspend complete optimization.

Regardless of the above commit the optimization stayed disabled for DRM
devices until

commit d14d2a8453d650bea32a1c5271af1458cd283a0f
Author: Lukas Wunner <lukas@wunner.de>
Date:   Wed Jun 8 12:49:29 2016 +0200

    drm: Remove dev_pm_ops from drm_class

so this patch is in practice a fix for this commit. Another reason for
the bug staying hidden for so long is that the optimization for a device
is disabled if it's disabled for any of its children devices. i915 may
have a backlight device as its child which doesn't support runtime PM
and so doesn't allow the optimization either.  So if this backlight
device got registered the bug stayed hidden.

Credits to Marta, Tomi and David who enabled pstore logging,
that caught one instance of this issue across a suspend/
resume-to-ram and Ville who rememberd that the optimization was enabled
for some devices at one point.

The first WARN triggered by the problem:

[ 6250.746445] WARNING: CPU: 2 PID: 17384 at drivers/gpu/drm/i915/intel_runtime_pm.c:2846 intel_runtime_pm_get+0x6b/0xd0 [i915]
[ 6250.746448] pm_runtime_get_sync() failed: -13
[ 6250.746451] Modules linked in: snd_hda_intel i915 vgem snd_hda_codec_hdmi x86_pkg_temp_thermal intel_powerclamp coretemp crct10dif_pclmul crc32_pclmul
snd_hda_codec_realtek snd_hda_codec_generic ghash_clmulni_intel e1000e snd_hda_codec snd_hwdep snd_hda_core ptp mei_me pps_core snd_pcm lpc_ich mei prime_
numbers i2c_hid i2c_designware_platform i2c_designware_core [last unloaded: i915]
[ 6250.746512] CPU: 2 PID: 17384 Comm: kworker/u8:0 Tainted: G     U  W       4.11.0-rc5-CI-CI_DRM_334+ #1
[ 6250.746515] Hardware name:                  /NUC5i5RYB, BIOS RYBDWi35.86A.0362.2017.0118.0940 01/18/2017
[ 6250.746521] Workqueue: events_unbound async_run_entry_fn
[ 6250.746525] Call Trace:
[ 6250.746530]  dump_stack+0x67/0x92
[ 6250.746536]  __warn+0xc6/0xe0
[ 6250.746542]  ? pci_restore_standard_config+0x40/0x40
[ 6250.746546]  warn_slowpath_fmt+0x46/0x50
[ 6250.746553]  ? __pm_runtime_resume+0x56/0x80
[ 6250.746584]  intel_runtime_pm_get+0x6b/0xd0 [i915]
[ 6250.746610]  intel_display_power_get+0x1b/0x40 [i915]
[ 6250.746646]  i915_audio_component_get_power+0x15/0x20 [i915]
[ 6250.746654]  snd_hdac_display_power+0xc8/0x110 [snd_hda_core]
[ 6250.746661]  azx_runtime_resume+0x218/0x280 [snd_hda_intel]
[ 6250.746667]  pci_pm_runtime_resume+0x76/0xa0
[ 6250.746672]  __rpm_callback+0xb4/0x1f0
[ 6250.746677]  ? pci_restore_standard_config+0x40/0x40
[ 6250.746682]  rpm_callback+0x1f/0x80
[ 6250.746686]  ? pci_restore_standard_config+0x40/0x40
[ 6250.746690]  rpm_resume+0x4ba/0x740
[ 6250.746698]  __pm_runtime_resume+0x49/0x80
[ 6250.746703]  pci_pm_suspend+0x57/0x140
[ 6250.746709]  dpm_run_callback+0x6f/0x330
[ 6250.746713]  ? pci_pm_freeze+0xe0/0xe0
[ 6250.746718]  __device_suspend+0xf9/0x370
[ 6250.746724]  ? dpm_watchdog_set+0x60/0x60
[ 6250.746730]  async_suspend+0x1a/0x90
[ 6250.746735]  async_run_entry_fn+0x34/0x160
[ 6250.746741]  process_one_work+0x1f2/0x6d0
[ 6250.746749]  worker_thread+0x49/0x4a0
[ 6250.746755]  kthread+0x107/0x140
[ 6250.746759]  ? process_one_work+0x6d0/0x6d0
[ 6250.746763]  ? kthread_create_on_node+0x40/0x40
[ 6250.746768]  ret_from_fork+0x2e/0x40
[ 6250.746778] ---[ end trace 102a62fd2160f5e6 ]---

v2:
- Use the new pci_dev->needs_resume flag, to avoid any overhead during
  the ->pm_prepare hook. (Rafael)

v3:
- Update commit message to reference the actual regressing commit.
  (Lukas)

Fixes: d14d2a8453d6 ("drm: Remove dev_pm_ops from drm_class")
References: https://bugs.freedesktop.org/show_bug.cgi?id=100378
References: https://bugs.freedesktop.org/show_bug.cgi?id=100770
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Marta Lofstedt <marta.lofstedt@intel.com>
Cc: David Weinehall <david.weinehall@linux.intel.com>
Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: linux-pci@vger.kernel.org
Cc: stable@vger.kernel.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v1)
Reported-and-tested-by: Marta Lofstedt <marta.lofstedt@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 2d3c4264..964248c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1238,6 +1238,15 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto out_fini;
 
 	pci_set_drvdata(pdev, &dev_priv->drm);
+	/*
+	 * Disable the system suspend direct complete optimization, which can
+	 * leave the device suspended skipping the driver's suspend handlers
+	 * if the device was already runtime suspended. This is needed due to
+	 * the difference in our runtime and system suspend sequence and
+	 * becaue the HDA driver may require us to enable the audio power
+	 * domain during system suspend.
+	 */
+	pci_resume_before_suspend(pdev, true);
 
 	ret = i915_driver_init_early(dev_priv, ent);
 	if (ret < 0)
-- 
2.5.0

[PATCH v3 2/2] drm/i915: Prevent the system suspend complete optimization

[Imre Deak]

Since

commit bac2a909a096c9110525c18cbb8ce73c660d5f71
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Date:   Wed Jan 21 02:17:42 2015 +0100

    PCI / PM: Avoid resuming PCI devices during system suspend

PCI devices will default to allowing the system suspend complete
optimization where devices are not woken up during system suspend if
they were already runtime suspended. This however breaks the i915/HDA
drivers for two reasons:

- The i915 driver has system suspend specific steps that it needs to
  run, that bring the device to a different state than its runtime
  suspended state.

- The HDA driver's suspend handler requires power that it will request
  from the i915 driver's power domain handler. This in turn requires the
  i915 driver to runtime resume itself, but this won't be possible if the
  suspend complete optimization is in effect: in this case the i915
  runtime PM is disabled and trying to get an RPM reference returns
  -EACCESS.

Solve this by requiring the PCI/PM core to resume the device during
system suspend which in effect disables the suspend complete optimization.

Regardless of the above commit the optimization stayed disabled for DRM
devices until

commit d14d2a8453d650bea32a1c5271af1458cd283a0f
Author: Lukas Wunner <lukas@wunner.de>
Date:   Wed Jun 8 12:49:29 2016 +0200

    drm: Remove dev_pm_ops from drm_class

so this patch is in practice a fix for this commit. Another reason for
the bug staying hidden for so long is that the optimization for a device
is disabled if it's disabled for any of its children devices. i915 may
have a backlight device as its child which doesn't support runtime PM
and so doesn't allow the optimization either.  So if this backlight
device got registered the bug stayed hidden.

Credits to Marta, Tomi and David who enabled pstore logging,
that caught one instance of this issue across a suspend/
resume-to-ram and Ville who rememberd that the optimization was enabled
for some devices at one point.

The first WARN triggered by the problem:

[ 6250.746445] WARNING: CPU: 2 PID: 17384 at drivers/gpu/drm/i915/intel_runtime_pm.c:2846 intel_runtime_pm_get+0x6b/0xd0 [i915]
[ 6250.746448] pm_runtime_get_sync() failed: -13
[ 6250.746451] Modules linked in: snd_hda_intel i915 vgem snd_hda_codec_hdmi x86_pkg_temp_thermal intel_powerclamp coretemp crct10dif_pclmul crc32_pclmul
snd_hda_codec_realtek snd_hda_codec_generic ghash_clmulni_intel e1000e snd_hda_codec snd_hwdep snd_hda_core ptp mei_me pps_core snd_pcm lpc_ich mei prime_
numbers i2c_hid i2c_designware_platform i2c_designware_core [last unloaded: i915]
[ 6250.746512] CPU: 2 PID: 17384 Comm: kworker/u8:0 Tainted: G     U  W       4.11.0-rc5-CI-CI_DRM_334+ #1
[ 6250.746515] Hardware name:                  /NUC5i5RYB, BIOS RYBDWi35.86A.0362.2017.0118.0940 01/18/2017
[ 6250.746521] Workqueue: events_unbound async_run_entry_fn
[ 6250.746525] Call Trace:
[ 6250.746530]  dump_stack+0x67/0x92
[ 6250.746536]  __warn+0xc6/0xe0
[ 6250.746542]  ? pci_restore_standard_config+0x40/0x40
[ 6250.746546]  warn_slowpath_fmt+0x46/0x50
[ 6250.746553]  ? __pm_runtime_resume+0x56/0x80
[ 6250.746584]  intel_runtime_pm_get+0x6b/0xd0 [i915]
[ 6250.746610]  intel_display_power_get+0x1b/0x40 [i915]
[ 6250.746646]  i915_audio_component_get_power+0x15/0x20 [i915]
[ 6250.746654]  snd_hdac_display_power+0xc8/0x110 [snd_hda_core]
[ 6250.746661]  azx_runtime_resume+0x218/0x280 [snd_hda_intel]
[ 6250.746667]  pci_pm_runtime_resume+0x76/0xa0
[ 6250.746672]  __rpm_callback+0xb4/0x1f0
[ 6250.746677]  ? pci_restore_standard_config+0x40/0x40
[ 6250.746682]  rpm_callback+0x1f/0x80
[ 6250.746686]  ? pci_restore_standard_config+0x40/0x40
[ 6250.746690]  rpm_resume+0x4ba/0x740
[ 6250.746698]  __pm_runtime_resume+0x49/0x80
[ 6250.746703]  pci_pm_suspend+0x57/0x140
[ 6250.746709]  dpm_run_callback+0x6f/0x330
[ 6250.746713]  ? pci_pm_freeze+0xe0/0xe0
[ 6250.746718]  __device_suspend+0xf9/0x370
[ 6250.746724]  ? dpm_watchdog_set+0x60/0x60
[ 6250.746730]  async_suspend+0x1a/0x90
[ 6250.746735]  async_run_entry_fn+0x34/0x160
[ 6250.746741]  process_one_work+0x1f2/0x6d0
[ 6250.746749]  worker_thread+0x49/0x4a0
[ 6250.746755]  kthread+0x107/0x140
[ 6250.746759]  ? process_one_work+0x6d0/0x6d0
[ 6250.746763]  ? kthread_create_on_node+0x40/0x40
[ 6250.746768]  ret_from_fork+0x2e/0x40
[ 6250.746778] ---[ end trace 102a62fd2160f5e6 ]---

v2:
- Use the new pci_dev->needs_resume flag, to avoid any overhead during
  the ->pm_prepare hook. (Rafael)

v3:
- Update commit message to reference the actual regressing commit.
  (Lukas)

Fixes: d14d2a8453d6 ("drm: Remove dev_pm_ops from drm_class")
References: https://bugs.freedesktop.org/show_bug.cgi?id=100378
References: https://bugs.freedesktop.org/show_bug.cgi?id=100770
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Marta Lofstedt <marta.lofstedt@intel.com>
Cc: David Weinehall <david.weinehall@linux.intel.com>
Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: linux-pci@vger.kernel.org
Cc: stable@vger.kernel.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v1)
Reported-and-tested-by: Marta Lofstedt <marta.lofstedt@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 2d3c4264..964248c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1238,6 +1238,15 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto out_fini;
 
 	pci_set_drvdata(pdev, &dev_priv->drm);
+	/*
+	 * Disable the system suspend direct complete optimization, which can
+	 * leave the device suspended skipping the driver's suspend handlers
+	 * if the device was already runtime suspended. This is needed due to
+	 * the difference in our runtime and system suspend sequence and
+	 * becaue the HDA driver may require us to enable the audio power
+	 * domain during system suspend.
+	 */
+	pci_resume_before_suspend(pdev, true);
 
 	ret = i915_driver_init_early(dev_priv, ent);
 	if (ret < 0)
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [v3,1/2] PCI / PM: Add needs_resume flag to avoid suspend complete optimization

[Patchwork]

== Series Details ==

Series: series starting with [v3,1/2] PCI / PM: Add needs_resume flag to avoid suspend complete optimization
URL   : https://patchwork.freedesktop.org/series/23707/
State : success

== Summary ==

Series 23707v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/23707/revisions/1/mbox/

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:434s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:425s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:578s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:512s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time:562s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:487s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:478s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:413s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:409s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:417s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:495s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:467s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:454s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:570s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:455s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time:580s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:455s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:491s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:427s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:524s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:399s

1d490e4b6d5324cfbf8dc800cf4a99471252802c drm-tip: 2017y-04m-28d-14h-14m-47s UTC integration manifest
afe0d7e drm/i915: Prevent the system suspend complete optimization
926c9e5 PCI / PM: Add needs_resume flag to avoid suspend complete optimization

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4581/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/2] PCI / PM: Add needs_resume flag to avoid suspend complete optimization

[Rafael J. Wysocki]

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)
> 

Re: [PATCH v3 1/2] PCI / PM: Add needs_resume flag to avoid suspend complete optimization

[Rafael J. Wysocki]

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

Re: [PATCH v3 1/2] PCI / PM: Add needs_resume flag to avoid suspend complete optimization

[Imre Deak]

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

Re: [PATCH v3 1/2] PCI / PM: Add needs_resume flag to avoid suspend complete optimization

[Imre Deak]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/2] PCI / PM: Add needs_resume flag to avoid suspend complete optimization

[Rafael J. Wysocki]

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

Re: [PATCH v3 1/2] PCI / PM: Add needs_resume flag to avoid suspend complete optimization

[Imre Deak]

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

Re: [PATCH v3 1/2] PCI / PM: Add needs_resume flag to avoid suspend complete optimization

[Imre Deak]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/2] PCI / PM: Add needs_resume flag to avoid suspend complete optimization

[Rafael J. Wysocki]

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