[PATCH v2] PCI/PM: Avoid suspending the device with errors

[PATCH v2] PCI/PM: Avoid suspending the device with errors

[Raag Jadav]

If an error is triggered before or during system suspend, any bus level
power state transition will result in unpredictable behaviour by the
device with failed recovery. Avoid suspending such a device and leave
it in its existing power state.

This only covers the devices that rely on PCI core PM for their power
state transition.

Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---

v2: Synchronize AER handling with PCI PM (Rafael)

More discussion on [1].
[1] https://lore.kernel.org/all/CAJZ5v0g-aJXfVH+Uc=9eRPuW08t-6PwzdyMXsC6FZRKYJtY03Q@mail.gmail.com/

 drivers/pci/pci-driver.c |  3 ++-
 drivers/pci/pcie/aer.c   | 11 +++++++++++
 include/linux/aer.h      |  2 ++
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index f57ea36d125d..289a1fa7cb2d 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -884,7 +884,8 @@ static int pci_pm_suspend_noirq(struct device *dev)
 		}
 	}
 
-	if (!pci_dev->state_saved) {
+	/* Avoid suspending the device with errors */
+	if (!pci_aer_in_progress(pci_dev) && !pci_dev->state_saved) {
 		pci_save_state(pci_dev);
 
 		/*
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 508474e17183..b70f5011d4f5 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -233,6 +233,17 @@ int pcie_aer_is_native(struct pci_dev *dev)
 }
 EXPORT_SYMBOL_NS_GPL(pcie_aer_is_native, "CXL");
 
+bool pci_aer_in_progress(struct pci_dev *dev)
+{
+	u16 reg16;
+
+	if (!pcie_aer_is_native(dev))
+		return false;
+
+	pcie_capability_read_word(dev, PCI_EXP_DEVSTA, &reg16);
+	return !!(reg16 & PCI_EXP_AER_FLAGS);
+}
+
 static int pci_enable_pcie_error_reporting(struct pci_dev *dev)
 {
 	int rc;
diff --git a/include/linux/aer.h b/include/linux/aer.h
index 947b63091902..68ae5378256e 100644
--- a/include/linux/aer.h
+++ b/include/linux/aer.h
@@ -48,12 +48,14 @@ struct aer_capability_regs {
 #if defined(CONFIG_PCIEAER)
 int pci_aer_clear_nonfatal_status(struct pci_dev *dev);
 int pcie_aer_is_native(struct pci_dev *dev);
+bool pci_aer_in_progress(struct pci_dev *dev);
 #else
 static inline int pci_aer_clear_nonfatal_status(struct pci_dev *dev)
 {
 	return -EINVAL;
 }
 static inline int pcie_aer_is_native(struct pci_dev *dev) { return 0; }
+static inline bool pci_aer_in_progress(struct pci_dev *dev) { return false; }
 #endif
 
 void pci_print_aer(struct pci_dev *dev, int aer_severity,
-- 
2.34.1

Re: [PATCH v2] PCI/PM: Avoid suspending the device with errors

[kernel test robot]

Hi Raag,

kernel test robot noticed the following build errors:

[auto build test ERROR on pci/next]
[also build test ERROR on pci/for-linus linus/master v6.15-rc3 next-20250422]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Raag-Jadav/PCI-PM-Avoid-suspending-the-device-with-errors/20250422-215734
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20250422135341.2780925-1-raag.jadav%40intel.com
patch subject: [PATCH v2] PCI/PM: Avoid suspending the device with errors
config: loongarch-randconfig-002-20250422 (https://download.01.org/0day-ci/archive/20250423/202504230101.o7uTJFn5-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250423/202504230101.o7uTJFn5-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504230101.o7uTJFn5-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/linux/array_size.h:5,
                    from include/linux/string.h:6,
                    from include/linux/uuid.h:11,
                    from include/linux/mod_devicetable.h:14,
                    from include/linux/pci.h:27,
                    from drivers/pci/pci-driver.c:7:
   drivers/pci/pci-driver.c: In function 'pci_pm_suspend_noirq':
>> drivers/pci/pci-driver.c:887:14: error: implicit declaration of function 'pci_aer_in_progress' [-Wimplicit-function-declaration]
     887 |         if (!pci_aer_in_progress(pci_dev) && !pci_dev->state_saved) {
         |              ^~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:57:52: note: in definition of macro '__trace_if_var'
      57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                    ^~~~
   drivers/pci/pci-driver.c:887:9: note: in expansion of macro 'if'
     887 |         if (!pci_aer_in_progress(pci_dev) && !pci_dev->state_saved) {
         |         ^~


vim +/pci_aer_in_progress +887 drivers/pci/pci-driver.c

   851	
   852	static int pci_pm_suspend_noirq(struct device *dev)
   853	{
   854		struct pci_dev *pci_dev = to_pci_dev(dev);
   855		const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
   856	
   857		if (dev_pm_skip_suspend(dev))
   858			return 0;
   859	
   860		if (pci_has_legacy_pm_support(pci_dev))
   861			return pci_legacy_suspend_late(dev);
   862	
   863		if (!pm) {
   864			pci_save_state(pci_dev);
   865			goto Fixup;
   866		}
   867	
   868		if (pm->suspend_noirq) {
   869			pci_power_t prev = pci_dev->current_state;
   870			int error;
   871	
   872			error = pm->suspend_noirq(dev);
   873			suspend_report_result(dev, pm->suspend_noirq, error);
   874			if (error)
   875				return error;
   876	
   877			if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0
   878			    && pci_dev->current_state != PCI_UNKNOWN) {
   879				pci_WARN_ONCE(pci_dev, pci_dev->current_state != prev,
   880					      "PCI PM: State of device not saved by %pS\n",
   881					      pm->suspend_noirq);
   882				goto Fixup;
   883			}
   884		}
   885	
   886		/* Avoid suspending the device with errors */
 > 887		if (!pci_aer_in_progress(pci_dev) && !pci_dev->state_saved) {
   888			pci_save_state(pci_dev);
   889	
   890			/*
   891			 * If the device is a bridge with a child in D0 below it,
   892			 * it needs to stay in D0, so check skip_bus_pm to avoid
   893			 * putting it into a low-power state in that case.
   894			 */
   895			if (!pci_dev->skip_bus_pm && pci_power_manageable(pci_dev))
   896				pci_prepare_to_sleep(pci_dev);
   897		}
   898	
   899		pci_dbg(pci_dev, "PCI PM: Suspend power state: %s\n",
   900			pci_power_name(pci_dev->current_state));
   901	
   902		if (pci_dev->current_state == PCI_D0) {
   903			pci_dev->skip_bus_pm = true;
   904			/*
   905			 * Per PCI PM r1.2, table 6-1, a bridge must be in D0 if any
   906			 * downstream device is in D0, so avoid changing the power state
   907			 * of the parent bridge by setting the skip_bus_pm flag for it.
   908			 */
   909			if (pci_dev->bus->self)
   910				pci_dev->bus->self->skip_bus_pm = true;
   911		}
   912	
   913		if (pci_dev->skip_bus_pm && pm_suspend_no_platform()) {
   914			pci_dbg(pci_dev, "PCI PM: Skipped\n");
   915			goto Fixup;
   916		}
   917	
   918		pci_pm_set_unknown_state(pci_dev);
   919	
   920		/*
   921		 * Some BIOSes from ASUS have a bug: If a USB EHCI host controller's
   922		 * PCI COMMAND register isn't 0, the BIOS assumes that the controller
   923		 * hasn't been quiesced and tries to turn it off.  If the controller
   924		 * is already in D3, this can hang or cause memory corruption.
   925		 *
   926		 * Since the value of the COMMAND register doesn't matter once the
   927		 * device has been suspended, we can safely set it to 0 here.
   928		 */
   929		if (pci_dev->class == PCI_CLASS_SERIAL_USB_EHCI)
   930			pci_write_config_word(pci_dev, PCI_COMMAND, 0);
   931	
   932	Fixup:
   933		pci_fixup_device(pci_fixup_suspend_late, pci_dev);
   934	
   935		/*
   936		 * If the target system sleep state is suspend-to-idle, it is sufficient
   937		 * to check whether or not the device's wakeup settings are good for
   938		 * runtime PM.  Otherwise, the pm_resume_via_firmware() check will cause
   939		 * pci_pm_complete() to take care of fixing up the device's state
   940		 * anyway, if need be.
   941		 */
   942		if (device_can_wakeup(dev) && !device_may_wakeup(dev))
   943			dev->power.may_skip_resume = false;
   944	
   945		return 0;
   946	}
   947	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2] PCI/PM: Avoid suspending the device with errors

[Bjorn Helgaas]

On Tue, Apr 22, 2025 at 07:23:41PM +0530, Raag Jadav wrote:
> If an error is triggered before or during system suspend, any bus level
> power state transition will result in unpredictable behaviour by the
> device with failed recovery. Avoid suspending such a device and leave
> it in its existing power state.
> 
> This only covers the devices that rely on PCI core PM for their power
> state transition.
> 
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> ---
> 
> v2: Synchronize AER handling with PCI PM (Rafael)
> 
> More discussion on [1].
> [1] https://lore.kernel.org/all/CAJZ5v0g-aJXfVH+Uc=9eRPuW08t-6PwzdyMXsC6FZRKYJtY03Q@mail.gmail.com/

Thanks for the pointer, but the commit log for this patch needs to be
complete in itself.  "Unpredictable behavior" is kind of hand-wavy and
doesn't really help understand the problem.

>  drivers/pci/pci-driver.c |  3 ++-
>  drivers/pci/pcie/aer.c   | 11 +++++++++++
>  include/linux/aer.h      |  2 ++
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index f57ea36d125d..289a1fa7cb2d 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -884,7 +884,8 @@ static int pci_pm_suspend_noirq(struct device *dev)
>  		}
>  	}
>  
> -	if (!pci_dev->state_saved) {
> +	/* Avoid suspending the device with errors */
> +	if (!pci_aer_in_progress(pci_dev) && !pci_dev->state_saved) {

This looks potentially racy, since hardware can set bits in
PCI_EXP_DEVSTA at any time.

The comment restates the C code, but doesn't say *why*.  The "why" is
important for ongoing maintenance.

>  		pci_save_state(pci_dev);
>  
>  		/*
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index 508474e17183..b70f5011d4f5 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -233,6 +233,17 @@ int pcie_aer_is_native(struct pci_dev *dev)
>  }
>  EXPORT_SYMBOL_NS_GPL(pcie_aer_is_native, "CXL");
>  
> +bool pci_aer_in_progress(struct pci_dev *dev)
> +{
> +	u16 reg16;
> +
> +	if (!pcie_aer_is_native(dev))
> +		return false;
> +
> +	pcie_capability_read_word(dev, PCI_EXP_DEVSTA, &reg16);
> +	return !!(reg16 & PCI_EXP_AER_FLAGS);
> +}
> +
>  static int pci_enable_pcie_error_reporting(struct pci_dev *dev)
>  {
>  	int rc;
> diff --git a/include/linux/aer.h b/include/linux/aer.h
> index 947b63091902..68ae5378256e 100644
> --- a/include/linux/aer.h
> +++ b/include/linux/aer.h
> @@ -48,12 +48,14 @@ struct aer_capability_regs {
>  #if defined(CONFIG_PCIEAER)
>  int pci_aer_clear_nonfatal_status(struct pci_dev *dev);
>  int pcie_aer_is_native(struct pci_dev *dev);
> +bool pci_aer_in_progress(struct pci_dev *dev);
>  #else
>  static inline int pci_aer_clear_nonfatal_status(struct pci_dev *dev)
>  {
>  	return -EINVAL;
>  }
>  static inline int pcie_aer_is_native(struct pci_dev *dev) { return 0; }
> +static inline bool pci_aer_in_progress(struct pci_dev *dev) { return false; }
>  #endif
>  
>  void pci_print_aer(struct pci_dev *dev, int aer_severity,
> -- 
> 2.34.1
> 

Re: [PATCH v2] PCI/PM: Avoid suspending the device with errors

[kernel test robot]

Hi Raag,

kernel test robot noticed the following build errors:

[auto build test ERROR on pci/next]
[also build test ERROR on pci/for-linus linus/master v6.15-rc3 next-20250422]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Raag-Jadav/PCI-PM-Avoid-suspending-the-device-with-errors/20250422-215734
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20250422135341.2780925-1-raag.jadav%40intel.com
patch subject: [PATCH v2] PCI/PM: Avoid suspending the device with errors
config: i386-defconfig (https://download.01.org/0day-ci/archive/20250423/202504230757.wEJUX3v6-lkp@intel.com/config)
compiler: clang version 20.1.2 (https://github.com/llvm/llvm-project 58df0ef89dd64126512e4ee27b4ac3fd8ddf6247)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250423/202504230757.wEJUX3v6-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504230757.wEJUX3v6-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/pci/pci-driver.c:887:7: error: call to undeclared function 'pci_aer_in_progress'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     887 |         if (!pci_aer_in_progress(pci_dev) && !pci_dev->state_saved) {
         |              ^
   1 error generated.


vim +/pci_aer_in_progress +887 drivers/pci/pci-driver.c

   851	
   852	static int pci_pm_suspend_noirq(struct device *dev)
   853	{
   854		struct pci_dev *pci_dev = to_pci_dev(dev);
   855		const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
   856	
   857		if (dev_pm_skip_suspend(dev))
   858			return 0;
   859	
   860		if (pci_has_legacy_pm_support(pci_dev))
   861			return pci_legacy_suspend_late(dev);
   862	
   863		if (!pm) {
   864			pci_save_state(pci_dev);
   865			goto Fixup;
   866		}
   867	
   868		if (pm->suspend_noirq) {
   869			pci_power_t prev = pci_dev->current_state;
   870			int error;
   871	
   872			error = pm->suspend_noirq(dev);
   873			suspend_report_result(dev, pm->suspend_noirq, error);
   874			if (error)
   875				return error;
   876	
   877			if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0
   878			    && pci_dev->current_state != PCI_UNKNOWN) {
   879				pci_WARN_ONCE(pci_dev, pci_dev->current_state != prev,
   880					      "PCI PM: State of device not saved by %pS\n",
   881					      pm->suspend_noirq);
   882				goto Fixup;
   883			}
   884		}
   885	
   886		/* Avoid suspending the device with errors */
 > 887		if (!pci_aer_in_progress(pci_dev) && !pci_dev->state_saved) {
   888			pci_save_state(pci_dev);
   889	
   890			/*
   891			 * If the device is a bridge with a child in D0 below it,
   892			 * it needs to stay in D0, so check skip_bus_pm to avoid
   893			 * putting it into a low-power state in that case.
   894			 */
   895			if (!pci_dev->skip_bus_pm && pci_power_manageable(pci_dev))
   896				pci_prepare_to_sleep(pci_dev);
   897		}
   898	
   899		pci_dbg(pci_dev, "PCI PM: Suspend power state: %s\n",
   900			pci_power_name(pci_dev->current_state));
   901	
   902		if (pci_dev->current_state == PCI_D0) {
   903			pci_dev->skip_bus_pm = true;
   904			/*
   905			 * Per PCI PM r1.2, table 6-1, a bridge must be in D0 if any
   906			 * downstream device is in D0, so avoid changing the power state
   907			 * of the parent bridge by setting the skip_bus_pm flag for it.
   908			 */
   909			if (pci_dev->bus->self)
   910				pci_dev->bus->self->skip_bus_pm = true;
   911		}
   912	
   913		if (pci_dev->skip_bus_pm && pm_suspend_no_platform()) {
   914			pci_dbg(pci_dev, "PCI PM: Skipped\n");
   915			goto Fixup;
   916		}
   917	
   918		pci_pm_set_unknown_state(pci_dev);
   919	
   920		/*
   921		 * Some BIOSes from ASUS have a bug: If a USB EHCI host controller's
   922		 * PCI COMMAND register isn't 0, the BIOS assumes that the controller
   923		 * hasn't been quiesced and tries to turn it off.  If the controller
   924		 * is already in D3, this can hang or cause memory corruption.
   925		 *
   926		 * Since the value of the COMMAND register doesn't matter once the
   927		 * device has been suspended, we can safely set it to 0 here.
   928		 */
   929		if (pci_dev->class == PCI_CLASS_SERIAL_USB_EHCI)
   930			pci_write_config_word(pci_dev, PCI_COMMAND, 0);
   931	
   932	Fixup:
   933		pci_fixup_device(pci_fixup_suspend_late, pci_dev);
   934	
   935		/*
   936		 * If the target system sleep state is suspend-to-idle, it is sufficient
   937		 * to check whether or not the device's wakeup settings are good for
   938		 * runtime PM.  Otherwise, the pm_resume_via_firmware() check will cause
   939		 * pci_pm_complete() to take care of fixing up the device's state
   940		 * anyway, if need be.
   941		 */
   942		if (device_can_wakeup(dev) && !device_may_wakeup(dev))
   943			dev->power.may_skip_resume = false;
   944	
   945		return 0;
   946	}
   947	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2] PCI/PM: Avoid suspending the device with errors

[Rafael J. Wysocki]

On Tue, Apr 22, 2025 at 3:55 PM Raag Jadav <raag.jadav@intel.com> wrote:
>
> If an error is triggered before or during system suspend, any bus level
> power state transition will result in unpredictable behaviour by the
> device with failed recovery. Avoid suspending such a device and leave
> it in its existing power state.
>
> This only covers the devices that rely on PCI core PM for their power
> state transition.
>
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> ---
>
> v2: Synchronize AER handling with PCI PM (Rafael)
>
> More discussion on [1].
> [1] https://lore.kernel.org/all/CAJZ5v0g-aJXfVH+Uc=9eRPuW08t-6PwzdyMXsC6FZRKYJtY03Q@mail.gmail.com/
>
>  drivers/pci/pci-driver.c |  3 ++-
>  drivers/pci/pcie/aer.c   | 11 +++++++++++
>  include/linux/aer.h      |  2 ++
>  3 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index f57ea36d125d..289a1fa7cb2d 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -884,7 +884,8 @@ static int pci_pm_suspend_noirq(struct device *dev)
>                 }
>         }
>
> -       if (!pci_dev->state_saved) {
> +       /* Avoid suspending the device with errors */
> +       if (!pci_aer_in_progress(pci_dev) && !pci_dev->state_saved) {

Apart from the potential raciness mentioned by Bjorn, doing it just
here is questionable because this is not the only place where the PCI
device power state can change.

It would be better to catch this in pci_set_low_power_state() IMO.


>                 pci_save_state(pci_dev);
>
>                 /*
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index 508474e17183..b70f5011d4f5 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -233,6 +233,17 @@ int pcie_aer_is_native(struct pci_dev *dev)
>  }
>  EXPORT_SYMBOL_NS_GPL(pcie_aer_is_native, "CXL");
>
> +bool pci_aer_in_progress(struct pci_dev *dev)
> +{
> +       u16 reg16;
> +
> +       if (!pcie_aer_is_native(dev))
> +               return false;
> +
> +       pcie_capability_read_word(dev, PCI_EXP_DEVSTA, &reg16);
> +       return !!(reg16 & PCI_EXP_AER_FLAGS);
> +}
> +
>  static int pci_enable_pcie_error_reporting(struct pci_dev *dev)
>  {
>         int rc;
> diff --git a/include/linux/aer.h b/include/linux/aer.h
> index 947b63091902..68ae5378256e 100644
> --- a/include/linux/aer.h
> +++ b/include/linux/aer.h
> @@ -48,12 +48,14 @@ struct aer_capability_regs {
>  #if defined(CONFIG_PCIEAER)
>  int pci_aer_clear_nonfatal_status(struct pci_dev *dev);
>  int pcie_aer_is_native(struct pci_dev *dev);
> +bool pci_aer_in_progress(struct pci_dev *dev);
>  #else
>  static inline int pci_aer_clear_nonfatal_status(struct pci_dev *dev)
>  {
>         return -EINVAL;
>  }
>  static inline int pcie_aer_is_native(struct pci_dev *dev) { return 0; }
> +static inline bool pci_aer_in_progress(struct pci_dev *dev) { return false; }
>  #endif
>
>  void pci_print_aer(struct pci_dev *dev, int aer_severity,
> --
> 2.34.1
>

Re: [PATCH v2] PCI/PM: Avoid suspending the device with errors

[Raag Jadav]

On Tue, Apr 22, 2025 at 02:45:37PM -0500, Bjorn Helgaas wrote:
> On Tue, Apr 22, 2025 at 07:23:41PM +0530, Raag Jadav wrote:
> > If an error is triggered before or during system suspend, any bus level
> > power state transition will result in unpredictable behaviour by the
> > device with failed recovery. Avoid suspending such a device and leave
> > it in its existing power state.
> > 
> > This only covers the devices that rely on PCI core PM for their power
> > state transition.
> > 
> > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > ---
> > 
> > v2: Synchronize AER handling with PCI PM (Rafael)
> > 
> > More discussion on [1].
> > [1] https://lore.kernel.org/all/CAJZ5v0g-aJXfVH+Uc=9eRPuW08t-6PwzdyMXsC6FZRKYJtY03Q@mail.gmail.com/
> 
> Thanks for the pointer, but the commit log for this patch needs to be
> complete in itself.  "Unpredictable behavior" is kind of hand-wavy and
> doesn't really help understand the problem.
> 
> >  drivers/pci/pci-driver.c |  3 ++-
> >  drivers/pci/pcie/aer.c   | 11 +++++++++++
> >  include/linux/aer.h      |  2 ++
> >  3 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > index f57ea36d125d..289a1fa7cb2d 100644
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -884,7 +884,8 @@ static int pci_pm_suspend_noirq(struct device *dev)
> >  		}
> >  	}
> >  
> > -	if (!pci_dev->state_saved) {
> > +	/* Avoid suspending the device with errors */
> > +	if (!pci_aer_in_progress(pci_dev) && !pci_dev->state_saved) {
> 
> This looks potentially racy, since hardware can set bits in
> PCI_EXP_DEVSTA at any time.

Which is why it's placed in ->suspend_noirq() callback. Can it still race?

Raag

Re: [PATCH v2] PCI/PM: Avoid suspending the device with errors

[Raag Jadav]

On Wed, Apr 23, 2025 at 02:41:52PM +0200, Rafael J. Wysocki wrote:
> On Tue, Apr 22, 2025 at 3:55 PM Raag Jadav <raag.jadav@intel.com> wrote:
> >
> > If an error is triggered before or during system suspend, any bus level
> > power state transition will result in unpredictable behaviour by the
> > device with failed recovery. Avoid suspending such a device and leave
> > it in its existing power state.
> >
> > This only covers the devices that rely on PCI core PM for their power
> > state transition.
> >
> > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > ---
> >
> > v2: Synchronize AER handling with PCI PM (Rafael)
> >
> > More discussion on [1].
> > [1] https://lore.kernel.org/all/CAJZ5v0g-aJXfVH+Uc=9eRPuW08t-6PwzdyMXsC6FZRKYJtY03Q@mail.gmail.com/
> >
> >  drivers/pci/pci-driver.c |  3 ++-
> >  drivers/pci/pcie/aer.c   | 11 +++++++++++
> >  include/linux/aer.h      |  2 ++
> >  3 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > index f57ea36d125d..289a1fa7cb2d 100644
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -884,7 +884,8 @@ static int pci_pm_suspend_noirq(struct device *dev)
> >                 }
> >         }
> >
> > -       if (!pci_dev->state_saved) {
> > +       /* Avoid suspending the device with errors */
> > +       if (!pci_aer_in_progress(pci_dev) && !pci_dev->state_saved) {
> 
> Apart from the potential raciness mentioned by Bjorn, doing it just
> here is questionable because this is not the only place where the PCI
> device power state can change.
> 
> It would be better to catch this in pci_set_low_power_state() IMO.

I'm not sure if we should prevent power state transition for the users
that explicitly want to transition.

Also, the device state can potentially be corrupted because of the errors,
so we'd probably want to avoid pci_save_state() as well, which is what
I attempted here.

Raag

Re: [PATCH v2] PCI/PM: Avoid suspending the device with errors

[Rafael J. Wysocki]

On Thu, Apr 24, 2025 at 7:12 AM Raag Jadav <raag.jadav@intel.com> wrote:
>
> On Tue, Apr 22, 2025 at 02:45:37PM -0500, Bjorn Helgaas wrote:
> > On Tue, Apr 22, 2025 at 07:23:41PM +0530, Raag Jadav wrote:
> > > If an error is triggered before or during system suspend, any bus level
> > > power state transition will result in unpredictable behaviour by the
> > > device with failed recovery. Avoid suspending such a device and leave
> > > it in its existing power state.
> > >
> > > This only covers the devices that rely on PCI core PM for their power
> > > state transition.
> > >
> > > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > > ---
> > >
> > > v2: Synchronize AER handling with PCI PM (Rafael)
> > >
> > > More discussion on [1].
> > > [1] https://lore.kernel.org/all/CAJZ5v0g-aJXfVH+Uc=9eRPuW08t-6PwzdyMXsC6FZRKYJtY03Q@mail.gmail.com/
> >
> > Thanks for the pointer, but the commit log for this patch needs to be
> > complete in itself.  "Unpredictable behavior" is kind of hand-wavy and
> > doesn't really help understand the problem.
> >
> > >  drivers/pci/pci-driver.c |  3 ++-
> > >  drivers/pci/pcie/aer.c   | 11 +++++++++++
> > >  include/linux/aer.h      |  2 ++
> > >  3 files changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > > index f57ea36d125d..289a1fa7cb2d 100644
> > > --- a/drivers/pci/pci-driver.c
> > > +++ b/drivers/pci/pci-driver.c
> > > @@ -884,7 +884,8 @@ static int pci_pm_suspend_noirq(struct device *dev)
> > >             }
> > >     }
> > >
> > > -   if (!pci_dev->state_saved) {
> > > +   /* Avoid suspending the device with errors */
> > > +   if (!pci_aer_in_progress(pci_dev) && !pci_dev->state_saved) {
> >
> > This looks potentially racy, since hardware can set bits in
> > PCI_EXP_DEVSTA at any time.
>
> Which is why it's placed in ->suspend_noirq() callback. Can it still race?

With the hardware?  Yes.

Re: [PATCH v2] PCI/PM: Avoid suspending the device with errors

[Rafael J. Wysocki]

On Thu, Apr 24, 2025 at 7:38 AM Raag Jadav <raag.jadav@intel.com> wrote:
>
> On Wed, Apr 23, 2025 at 02:41:52PM +0200, Rafael J. Wysocki wrote:
> > On Tue, Apr 22, 2025 at 3:55 PM Raag Jadav <raag.jadav@intel.com> wrote:
> > >
> > > If an error is triggered before or during system suspend, any bus level
> > > power state transition will result in unpredictable behaviour by the
> > > device with failed recovery. Avoid suspending such a device and leave
> > > it in its existing power state.
> > >
> > > This only covers the devices that rely on PCI core PM for their power
> > > state transition.
> > >
> > > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > > ---
> > >
> > > v2: Synchronize AER handling with PCI PM (Rafael)
> > >
> > > More discussion on [1].
> > > [1] https://lore.kernel.org/all/CAJZ5v0g-aJXfVH+Uc=9eRPuW08t-6PwzdyMXsC6FZRKYJtY03Q@mail.gmail.com/
> > >
> > >  drivers/pci/pci-driver.c |  3 ++-
> > >  drivers/pci/pcie/aer.c   | 11 +++++++++++
> > >  include/linux/aer.h      |  2 ++
> > >  3 files changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > > index f57ea36d125d..289a1fa7cb2d 100644
> > > --- a/drivers/pci/pci-driver.c
> > > +++ b/drivers/pci/pci-driver.c
> > > @@ -884,7 +884,8 @@ static int pci_pm_suspend_noirq(struct device *dev)
> > >                 }
> > >         }
> > >
> > > -       if (!pci_dev->state_saved) {
> > > +       /* Avoid suspending the device with errors */
> > > +       if (!pci_aer_in_progress(pci_dev) && !pci_dev->state_saved) {
> >
> > Apart from the potential raciness mentioned by Bjorn, doing it just
> > here is questionable because this is not the only place where the PCI
> > device power state can change.
> >
> > It would be better to catch this in pci_set_low_power_state() IMO.
>
> I'm not sure if we should prevent power state transition for the users
> that explicitly want to transition.
>
> Also, the device state can potentially be corrupted because of the errors,
> so we'd probably want to avoid pci_save_state() as well, which is what
> I attempted here.

But it's not what the changelog is saying.

If you want to avoid pci_save_state(), there are also other places
when it is called and then you also may want to avoid the transition
because if the state is not saved, it won't be possible to restore it.

Re: [PATCH v2] PCI/PM: Avoid suspending the device with errors

[Raag Jadav]

On Thu, Apr 24, 2025 at 12:59:55PM +0200, Rafael J. Wysocki wrote:
> On Thu, Apr 24, 2025 at 7:12 AM Raag Jadav <raag.jadav@intel.com> wrote:
> >
> > On Tue, Apr 22, 2025 at 02:45:37PM -0500, Bjorn Helgaas wrote:
> > > On Tue, Apr 22, 2025 at 07:23:41PM +0530, Raag Jadav wrote:
> > > > If an error is triggered before or during system suspend, any bus level
> > > > power state transition will result in unpredictable behaviour by the
> > > > device with failed recovery. Avoid suspending such a device and leave
> > > > it in its existing power state.
> > > >
> > > > This only covers the devices that rely on PCI core PM for their power
> > > > state transition.
> > > >
> > > > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > > > ---
> > > >
> > > > v2: Synchronize AER handling with PCI PM (Rafael)
> > > >
> > > > More discussion on [1].
> > > > [1] https://lore.kernel.org/all/CAJZ5v0g-aJXfVH+Uc=9eRPuW08t-6PwzdyMXsC6FZRKYJtY03Q@mail.gmail.com/
> > >
> > > Thanks for the pointer, but the commit log for this patch needs to be
> > > complete in itself.  "Unpredictable behavior" is kind of hand-wavy and
> > > doesn't really help understand the problem.
> > >
> > > >  drivers/pci/pci-driver.c |  3 ++-
> > > >  drivers/pci/pcie/aer.c   | 11 +++++++++++
> > > >  include/linux/aer.h      |  2 ++
> > > >  3 files changed, 15 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > > > index f57ea36d125d..289a1fa7cb2d 100644
> > > > --- a/drivers/pci/pci-driver.c
> > > > +++ b/drivers/pci/pci-driver.c
> > > > @@ -884,7 +884,8 @@ static int pci_pm_suspend_noirq(struct device *dev)
> > > >             }
> > > >     }
> > > >
> > > > -   if (!pci_dev->state_saved) {
> > > > +   /* Avoid suspending the device with errors */
> > > > +   if (!pci_aer_in_progress(pci_dev) && !pci_dev->state_saved) {
> > >
> > > This looks potentially racy, since hardware can set bits in
> > > PCI_EXP_DEVSTA at any time.
> >
> > Which is why it's placed in ->suspend_noirq() callback. Can it still race?
> 
> With the hardware?  Yes.

Any thoughts on potential side effects and how to address them?

Raag

Re: [PATCH v2] PCI/PM: Avoid suspending the device with errors

[Raag Jadav]

On Thu, Apr 24, 2025 at 01:02:58PM +0200, Rafael J. Wysocki wrote:
> On Thu, Apr 24, 2025 at 7:38 AM Raag Jadav <raag.jadav@intel.com> wrote:
> >
> > On Wed, Apr 23, 2025 at 02:41:52PM +0200, Rafael J. Wysocki wrote:
> > > On Tue, Apr 22, 2025 at 3:55 PM Raag Jadav <raag.jadav@intel.com> wrote:
> > > >
> > > > If an error is triggered before or during system suspend, any bus level
> > > > power state transition will result in unpredictable behaviour by the
> > > > device with failed recovery. Avoid suspending such a device and leave
> > > > it in its existing power state.
> > > >
> > > > This only covers the devices that rely on PCI core PM for their power
> > > > state transition.
> > > >
> > > > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > > > ---
> > > >
> > > > v2: Synchronize AER handling with PCI PM (Rafael)
> > > >
> > > > More discussion on [1].
> > > > [1] https://lore.kernel.org/all/CAJZ5v0g-aJXfVH+Uc=9eRPuW08t-6PwzdyMXsC6FZRKYJtY03Q@mail.gmail.com/
> > > >
> > > >  drivers/pci/pci-driver.c |  3 ++-
> > > >  drivers/pci/pcie/aer.c   | 11 +++++++++++
> > > >  include/linux/aer.h      |  2 ++
> > > >  3 files changed, 15 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > > > index f57ea36d125d..289a1fa7cb2d 100644
> > > > --- a/drivers/pci/pci-driver.c
> > > > +++ b/drivers/pci/pci-driver.c
> > > > @@ -884,7 +884,8 @@ static int pci_pm_suspend_noirq(struct device *dev)
> > > >                 }
> > > >         }
> > > >
> > > > -       if (!pci_dev->state_saved) {
> > > > +       /* Avoid suspending the device with errors */
> > > > +       if (!pci_aer_in_progress(pci_dev) && !pci_dev->state_saved) {
> > >
> > > Apart from the potential raciness mentioned by Bjorn, doing it just
> > > here is questionable because this is not the only place where the PCI
> > > device power state can change.
> > >
> > > It would be better to catch this in pci_set_low_power_state() IMO.
> >
> > I'm not sure if we should prevent power state transition for the users
> > that explicitly want to transition.
> >
> > Also, the device state can potentially be corrupted because of the errors,
> > so we'd probably want to avoid pci_save_state() as well, which is what
> > I attempted here.
> 
> But it's not what the changelog is saying.
> 
> If you want to avoid pci_save_state(), there are also other places
> when it is called and then you also may want to avoid the transition
> because if the state is not saved, it won't be possible to restore it.

Yes, above logic will skip both pci_save_state() and pci_prepare_to_sleep()
and in turn set skip_bus_pm flag to prevent power state transition.

Considering we're targeting only system PM cases, I could find pci_save_state()
being used in pci_pm_suspend_noirq() and pci_pm_freeze_noirq() (excluding
legacy suspend). pci_pm_freeze_noirq() doesn't seem to attempt any power state
transition by itself, so any other cases that you think are worth being covered
here?

Raag