* Re: [PATCH v2] PCI/PM: Avoid suspending the device with errors
2025-04-22 13:53 [PATCH v2] PCI/PM: Avoid suspending the device with errors Raag Jadav
@ 2025-04-22 18:01 ` kernel test robot
2025-04-22 19:45 ` Bjorn Helgaas
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2025-04-22 18:01 UTC (permalink / raw)
To: Raag Jadav, rafael, mahesh, oohall, bhelgaas
Cc: oe-kbuild-all, linux-pci, linux-kernel, ilpo.jarvinen, lukas,
aravind.iddamsetty, Raag Jadav
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] PCI/PM: Avoid suspending the device with errors
2025-04-22 13:53 [PATCH v2] PCI/PM: Avoid suspending the device with errors Raag Jadav
2025-04-22 18:01 ` kernel test robot
@ 2025-04-22 19:45 ` Bjorn Helgaas
2025-04-24 5:12 ` Raag Jadav
2025-04-22 23:39 ` kernel test robot
2025-04-23 12:41 ` Rafael J. Wysocki
3 siblings, 1 reply; 11+ messages in thread
From: Bjorn Helgaas @ 2025-04-22 19:45 UTC (permalink / raw)
To: Raag Jadav
Cc: rafael, mahesh, oohall, bhelgaas, linux-pci, linux-kernel,
ilpo.jarvinen, lukas, aravind.iddamsetty
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, ®16);
> + 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
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] PCI/PM: Avoid suspending the device with errors
2025-04-22 19:45 ` Bjorn Helgaas
@ 2025-04-24 5:12 ` Raag Jadav
2025-04-24 10:59 ` Rafael J. Wysocki
0 siblings, 1 reply; 11+ messages in thread
From: Raag Jadav @ 2025-04-24 5:12 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: rafael, mahesh, oohall, bhelgaas, linux-pci, linux-kernel,
ilpo.jarvinen, lukas, aravind.iddamsetty
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] PCI/PM: Avoid suspending the device with errors
2025-04-24 5:12 ` Raag Jadav
@ 2025-04-24 10:59 ` Rafael J. Wysocki
2025-04-26 19:24 ` Raag Jadav
0 siblings, 1 reply; 11+ messages in thread
From: Rafael J. Wysocki @ 2025-04-24 10:59 UTC (permalink / raw)
To: Raag Jadav
Cc: Bjorn Helgaas, rafael, mahesh, oohall, bhelgaas, linux-pci,
linux-kernel, ilpo.jarvinen, lukas, aravind.iddamsetty
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.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] PCI/PM: Avoid suspending the device with errors
2025-04-24 10:59 ` Rafael J. Wysocki
@ 2025-04-26 19:24 ` Raag Jadav
0 siblings, 0 replies; 11+ messages in thread
From: Raag Jadav @ 2025-04-26 19:24 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Bjorn Helgaas, mahesh, oohall, bhelgaas, linux-pci, linux-kernel,
ilpo.jarvinen, lukas, aravind.iddamsetty
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] PCI/PM: Avoid suspending the device with errors
2025-04-22 13:53 [PATCH v2] PCI/PM: Avoid suspending the device with errors Raag Jadav
2025-04-22 18:01 ` kernel test robot
2025-04-22 19:45 ` Bjorn Helgaas
@ 2025-04-22 23:39 ` kernel test robot
2025-04-23 12:41 ` Rafael J. Wysocki
3 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2025-04-22 23:39 UTC (permalink / raw)
To: Raag Jadav, rafael, mahesh, oohall, bhelgaas
Cc: llvm, oe-kbuild-all, linux-pci, linux-kernel, ilpo.jarvinen,
lukas, aravind.iddamsetty, Raag Jadav
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] PCI/PM: Avoid suspending the device with errors
2025-04-22 13:53 [PATCH v2] PCI/PM: Avoid suspending the device with errors Raag Jadav
` (2 preceding siblings ...)
2025-04-22 23:39 ` kernel test robot
@ 2025-04-23 12:41 ` Rafael J. Wysocki
2025-04-24 5:38 ` Raag Jadav
3 siblings, 1 reply; 11+ messages in thread
From: Rafael J. Wysocki @ 2025-04-23 12:41 UTC (permalink / raw)
To: Raag Jadav
Cc: rafael, mahesh, oohall, bhelgaas, linux-pci, linux-kernel,
ilpo.jarvinen, lukas, aravind.iddamsetty
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, ®16);
> + 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
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] PCI/PM: Avoid suspending the device with errors
2025-04-23 12:41 ` Rafael J. Wysocki
@ 2025-04-24 5:38 ` Raag Jadav
2025-04-24 11:02 ` Rafael J. Wysocki
0 siblings, 1 reply; 11+ messages in thread
From: Raag Jadav @ 2025-04-24 5:38 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: mahesh, oohall, bhelgaas, linux-pci, linux-kernel, ilpo.jarvinen,
lukas, aravind.iddamsetty
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] PCI/PM: Avoid suspending the device with errors
2025-04-24 5:38 ` Raag Jadav
@ 2025-04-24 11:02 ` Rafael J. Wysocki
2025-04-26 20:18 ` Raag Jadav
0 siblings, 1 reply; 11+ messages in thread
From: Rafael J. Wysocki @ 2025-04-24 11:02 UTC (permalink / raw)
To: Raag Jadav
Cc: Rafael J. Wysocki, mahesh, oohall, bhelgaas, linux-pci,
linux-kernel, ilpo.jarvinen, lukas, aravind.iddamsetty
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.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] PCI/PM: Avoid suspending the device with errors
2025-04-24 11:02 ` Rafael J. Wysocki
@ 2025-04-26 20:18 ` Raag Jadav
0 siblings, 0 replies; 11+ messages in thread
From: Raag Jadav @ 2025-04-26 20:18 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: mahesh, oohall, bhelgaas, linux-pci, linux-kernel, ilpo.jarvinen,
lukas, aravind.iddamsetty
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
^ permalink raw reply [flat|nested] 11+ messages in thread