[PATCH] cxl/mem: Disable suspend

[PATCH] cxl/mem: Disable suspend

[Dan Williams]

The CXL specification claims S3 support at a hardware level, but at a
system software level there are some missing pieces. Section 9.4 rightly
claims that "CXL mem adapters may need aux power to retain memory
context across S3", but there is no enumeration mechanism for the OS to
determine if a given adapter has that support. Moreover the save state
and resume image for the system may inadvertantly end up in a CXL device
that needs to be restored before the save state is recoverable. I.e. a
circular dependency that is not resolvable without a third party
save-area.

Arrange for the cxl_mem driver to fail S3 attempts. This still nominaly
allows for suspend, but requires unbinding all CXL memory devices before
the suspend to ensure the typical DRAM flow is taken. The cxl_mem unbind
flow is intended to also tear down all CXL memory regions associated
with a given cxl_memdev.

It is reasonable to assume that any device participating in a System RAM
range published in the EFI memory map is covered by aux power and
save-area outside the device itself. So this restriction can be
minimized in the future once pre-existing region enumeration support
arrives, and perhaps a spec update to clarify if the EFI memory is
sufficent for determining the range of devices managed by
platform-firmware for S3 support.

Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/memdev.c |    1 -
 drivers/cxl/mem.c         |   26 ++++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 1f76b28f9826..efe4d2e9bfef 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -251,7 +251,6 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
 	dev->bus = &cxl_bus_type;
 	dev->devt = MKDEV(cxl_mem_major, cxlmd->id);
 	dev->type = &cxl_memdev_type;
-	device_set_pm_not_required(dev);
 	INIT_WORK(&cxlmd->detach_work, detach_memdev);
 
 	cdev = &cxlmd->cdev;
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 49a4b1c47299..0660bb1488cb 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -3,6 +3,7 @@
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/pm.h>
 
 #include "cxlmem.h"
 #include "cxlpci.h"
@@ -210,10 +211,35 @@ static int cxl_mem_probe(struct device *dev)
 	return rc;
 }
 
+static int cxl_mem_suspend(struct device *dev)
+{
+	/*
+	 * The kernel may be operating out of CXL memory on this device,
+	 * there is no spec defined way to determine whether this device
+	 * preserves contents over suspend, and there is no simple way
+	 * to arrange for the suspend image to avoid CXL memory which
+	 * would setup a circular dependency between PCI resume and save
+	 * state restoration.
+	 */
+	dev_err(dev, "CXL memory suspend not supported\n");
+	return -EBUSY;
+}
+
+static int cxl_mem_resume(struct device *dev)
+{
+	/* nothing to do since suspend is prevented */
+	return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(cxl_pm_ops, cxl_mem_suspend, cxl_mem_resume);
+
 static struct cxl_driver cxl_mem_driver = {
 	.name = "cxl_mem",
 	.probe = cxl_mem_probe,
 	.id = CXL_DEVICE_MEMORY_EXPANDER,
+	.drv = {
+		.pm = &cxl_pm_ops,
+	},
 };
 
 module_cxl_driver(cxl_mem_driver);

Re: [PATCH] cxl/mem: Disable suspend

[Rafael J. Wysocki]

On Sun, Apr 3, 2022 at 2:58 AM Dan Williams <dan.j.williams@intel.com> wrote:
>
> The CXL specification claims S3 support at a hardware level, but at a
> system software level there are some missing pieces. Section 9.4 rightly
> claims that "CXL mem adapters may need aux power to retain memory
> context across S3", but there is no enumeration mechanism for the OS to
> determine if a given adapter has that support. Moreover the save state
> and resume image for the system may inadvertantly end up in a CXL device
> that needs to be restored before the save state is recoverable. I.e. a
> circular dependency that is not resolvable without a third party
> save-area.
>
> Arrange for the cxl_mem driver to fail S3 attempts. This still nominaly
> allows for suspend, but requires unbinding all CXL memory devices before
> the suspend to ensure the typical DRAM flow is taken. The cxl_mem unbind
> flow is intended to also tear down all CXL memory regions associated
> with a given cxl_memdev.
>
> It is reasonable to assume that any device participating in a System RAM
> range published in the EFI memory map is covered by aux power and
> save-area outside the device itself. So this restriction can be
> minimized in the future once pre-existing region enumeration support
> arrives, and perhaps a spec update to clarify if the EFI memory is
> sufficent for determining the range of devices managed by
> platform-firmware for S3 support.
>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

A few thoughts:

1. I don't think it is necessary to fail suspend-to-idle too (which
the driver will do after the patch AFAICS).
2. Should hibernation fail too?  From the description above it looks
like that should be the case.
3. If "deep"suspend is going to fail every time, it may be better to
prevent "deep" from being written to /sys/power/mem_sleep instead of
failing suspend in progress, especially after freezing user space.

> ---
>  drivers/cxl/core/memdev.c |    1 -
>  drivers/cxl/mem.c         |   26 ++++++++++++++++++++++++++
>  2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 1f76b28f9826..efe4d2e9bfef 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -251,7 +251,6 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
>         dev->bus = &cxl_bus_type;
>         dev->devt = MKDEV(cxl_mem_major, cxlmd->id);
>         dev->type = &cxl_memdev_type;
> -       device_set_pm_not_required(dev);
>         INIT_WORK(&cxlmd->detach_work, detach_memdev);
>
>         cdev = &cxlmd->cdev;
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 49a4b1c47299..0660bb1488cb 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -3,6 +3,7 @@
>  #include <linux/device.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> +#include <linux/pm.h>
>
>  #include "cxlmem.h"
>  #include "cxlpci.h"
> @@ -210,10 +211,35 @@ static int cxl_mem_probe(struct device *dev)
>         return rc;
>  }
>
> +static int cxl_mem_suspend(struct device *dev)
> +{
> +       /*
> +        * The kernel may be operating out of CXL memory on this device,
> +        * there is no spec defined way to determine whether this device
> +        * preserves contents over suspend, and there is no simple way
> +        * to arrange for the suspend image to avoid CXL memory which
> +        * would setup a circular dependency between PCI resume and save
> +        * state restoration.
> +        */
> +       dev_err(dev, "CXL memory suspend not supported\n");
> +       return -EBUSY;
> +}
> +
> +static int cxl_mem_resume(struct device *dev)
> +{
> +       /* nothing to do since suspend is prevented */
> +       return 0;
> +}

This is not needed AFAICS.

> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(cxl_pm_ops, cxl_mem_suspend, cxl_mem_resume);
> +
>  static struct cxl_driver cxl_mem_driver = {
>         .name = "cxl_mem",
>         .probe = cxl_mem_probe,
>         .id = CXL_DEVICE_MEMORY_EXPANDER,
> +       .drv = {
> +               .pm = &cxl_pm_ops,
> +       },
>  };
>
>  module_cxl_driver(cxl_mem_driver);
>

Re: [PATCH] cxl/mem: Disable suspend

[Dan Williams]

On Mon, Apr 4, 2022 at 9:00 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Sun, Apr 3, 2022 at 2:58 AM Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > The CXL specification claims S3 support at a hardware level, but at a
> > system software level there are some missing pieces. Section 9.4 rightly
> > claims that "CXL mem adapters may need aux power to retain memory
> > context across S3", but there is no enumeration mechanism for the OS to
> > determine if a given adapter has that support. Moreover the save state
> > and resume image for the system may inadvertantly end up in a CXL device
> > that needs to be restored before the save state is recoverable. I.e. a
> > circular dependency that is not resolvable without a third party
> > save-area.
> >
> > Arrange for the cxl_mem driver to fail S3 attempts. This still nominaly
> > allows for suspend, but requires unbinding all CXL memory devices before
> > the suspend to ensure the typical DRAM flow is taken. The cxl_mem unbind
> > flow is intended to also tear down all CXL memory regions associated
> > with a given cxl_memdev.
> >
> > It is reasonable to assume that any device participating in a System RAM
> > range published in the EFI memory map is covered by aux power and
> > save-area outside the device itself. So this restriction can be
> > minimized in the future once pre-existing region enumeration support
> > arrives, and perhaps a spec update to clarify if the EFI memory is
> > sufficent for determining the range of devices managed by
> > platform-firmware for S3 support.
> >
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> A few thoughts:
>
> 1. I don't think it is necessary to fail suspend-to-idle too (which
> the driver will do after the patch AFAICS).

Ah true, I missed that this would also disable suspend to idle.

> 2. Should hibernation fail too?  From the description above it looks
> like that should be the case.

Yes, any CXL address range that was provisioned by the OS would need
some off-device save area for the device-state which seems difficult
to support in the general case.

> 3. If "deep"suspend is going to fail every time, it may be better to
> prevent "deep" from being written to /sys/power/mem_sleep instead of
> failing suspend in progress, especially after freezing user space.

Yeah, that sounds much better, let me explore that option.

>
> > ---
> >  drivers/cxl/core/memdev.c |    1 -
> >  drivers/cxl/mem.c         |   26 ++++++++++++++++++++++++++
> >  2 files changed, 26 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> > index 1f76b28f9826..efe4d2e9bfef 100644
> > --- a/drivers/cxl/core/memdev.c
> > +++ b/drivers/cxl/core/memdev.c
> > @@ -251,7 +251,6 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
> >         dev->bus = &cxl_bus_type;
> >         dev->devt = MKDEV(cxl_mem_major, cxlmd->id);
> >         dev->type = &cxl_memdev_type;
> > -       device_set_pm_not_required(dev);
> >         INIT_WORK(&cxlmd->detach_work, detach_memdev);
> >
> >         cdev = &cxlmd->cdev;
> > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> > index 49a4b1c47299..0660bb1488cb 100644
> > --- a/drivers/cxl/mem.c
> > +++ b/drivers/cxl/mem.c
> > @@ -3,6 +3,7 @@
> >  #include <linux/device.h>
> >  #include <linux/module.h>
> >  #include <linux/pci.h>
> > +#include <linux/pm.h>
> >
> >  #include "cxlmem.h"
> >  #include "cxlpci.h"
> > @@ -210,10 +211,35 @@ static int cxl_mem_probe(struct device *dev)
> >         return rc;
> >  }
> >
> > +static int cxl_mem_suspend(struct device *dev)
> > +{
> > +       /*
> > +        * The kernel may be operating out of CXL memory on this device,
> > +        * there is no spec defined way to determine whether this device
> > +        * preserves contents over suspend, and there is no simple way
> > +        * to arrange for the suspend image to avoid CXL memory which
> > +        * would setup a circular dependency between PCI resume and save
> > +        * state restoration.
> > +        */
> > +       dev_err(dev, "CXL memory suspend not supported\n");
> > +       return -EBUSY;
> > +}
> > +
> > +static int cxl_mem_resume(struct device *dev)
> > +{
> > +       /* nothing to do since suspend is prevented */
> > +       return 0;
> > +}
>
> This is not needed AFAICS.

Ok, I should have checked, but I'll circle back with sleep state
disabling rather than failing suspend.

Thanks, Rafael.

Re: [PATCH] cxl/mem: Disable suspend

[Rafael J. Wysocki]

On Mon, Apr 4, 2022 at 8:16 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Mon, Apr 4, 2022 at 9:00 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Sun, Apr 3, 2022 at 2:58 AM Dan Williams <dan.j.williams@intel.com> wrote:
> > >
> > > The CXL specification claims S3 support at a hardware level, but at a
> > > system software level there are some missing pieces. Section 9.4 rightly
> > > claims that "CXL mem adapters may need aux power to retain memory
> > > context across S3", but there is no enumeration mechanism for the OS to
> > > determine if a given adapter has that support. Moreover the save state
> > > and resume image for the system may inadvertantly end up in a CXL device
> > > that needs to be restored before the save state is recoverable. I.e. a
> > > circular dependency that is not resolvable without a third party
> > > save-area.
> > >
> > > Arrange for the cxl_mem driver to fail S3 attempts. This still nominaly
> > > allows for suspend, but requires unbinding all CXL memory devices before
> > > the suspend to ensure the typical DRAM flow is taken. The cxl_mem unbind
> > > flow is intended to also tear down all CXL memory regions associated
> > > with a given cxl_memdev.
> > >
> > > It is reasonable to assume that any device participating in a System RAM
> > > range published in the EFI memory map is covered by aux power and
> > > save-area outside the device itself. So this restriction can be
> > > minimized in the future once pre-existing region enumeration support
> > > arrives, and perhaps a spec update to clarify if the EFI memory is
> > > sufficent for determining the range of devices managed by
> > > platform-firmware for S3 support.
> > >
> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> >
> > A few thoughts:
> >
> > 1. I don't think it is necessary to fail suspend-to-idle too (which
> > the driver will do after the patch AFAICS).
>
> Ah true, I missed that this would also disable suspend to idle.
>
> > 2. Should hibernation fail too?  From the description above it looks
> > like that should be the case.
>
> Yes, any CXL address range that was provisioned by the OS would need
> some off-device save area for the device-state which seems difficult
> to support in the general case.

So it should cause errors to be returned from the hibernation path
too, ideally before the freezing of tasks.

> > 3. If "deep"suspend is going to fail every time, it may be better to
> > prevent "deep" from being written to /sys/power/mem_sleep instead of
> > failing suspend in progress, especially after freezing user space.
>
> Yeah, that sounds much better, let me explore that option.

[PATCH v2] PM: CXL: Disable suspend

[Dan Williams]

The CXL specification claims S3 support at a hardware level, but at a
system software level there are some missing pieces. Section 9.4 (CXL
2.0) rightly claims that "CXL mem adapters may need aux power to retain
memory context across S3", but there is no enumeration mechanism for the
OS to determine if a given adapter has that support. Moreover the save
state and resume image for the system may inadvertantly end up in a CXL
device that needs to be restored before the save state is recoverable.
I.e. a circular dependency that is not resolvable without a third party
save-area.

Arrange for the cxl_mem driver to fail S3 attempts. This still nominaly
allows for suspend, but requires unbinding all CXL memory devices before
the suspend to ensure the typical DRAM flow is taken. The cxl_mem unbind
flow is intended to also tear down all CXL memory regions associated
with a given cxl_memdev.

It is reasonable to assume that any device participating in a System RAM
range published in the EFI memory map is covered by aux power and
save-area outside the device itself. So this restriction can be
minimized in the future once pre-existing region enumeration support
arrives, and perhaps a spec update to clarify if the EFI memory map is
sufficent for determining the range of devices managed by
platform-firmware for S3 support.

Per Rafael, if the CXL configuration prevents suspend then it should
fail early before tasks are frozen, and mem_sleep should stop showing
'mem' as an option [1]. Effectively CXL augments the platform suspend
->valid() op since, for example, the ACPI ops are not aware of the CXL /
PCI dependencies. Given the split role of platform firmware vs OS
provisioned CXL memory it is up to the cxl_mem driver to determine if
the CXL configuration has elements that platform firmware may not be
prepared to restore.

Link: https://lore.kernel.org/r/CAJZ5v0hGVN_=3iU8OLpHY3Ak35T5+JcBM-qs8SbojKrpd0VXsA@mail.gmail.com [1]
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Len Brown <len.brown@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/Makefile           |    2 +-
 drivers/cxl/Kconfig        |    4 ++++
 drivers/cxl/Makefile       |    2 +-
 drivers/cxl/core/Makefile  |    1 +
 drivers/cxl/core/memdev.c  |    1 -
 drivers/cxl/core/suspend.c |   23 +++++++++++++++++++++++
 drivers/cxl/cxlmem.h       |   11 +++++++++++
 drivers/cxl/mem.c          |   22 +++++++++++++++++++++-
 include/linux/pm.h         |    9 +++++++++
 kernel/power/hibernate.c   |    2 +-
 kernel/power/main.c        |    5 ++++-
 kernel/power/suspend.c     |    3 ++-
 12 files changed, 78 insertions(+), 7 deletions(-)
 create mode 100644 drivers/cxl/core/suspend.c

diff --git a/drivers/Makefile b/drivers/Makefile
index 020780b6b4d2..f735c4955143 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -72,9 +72,9 @@ obj-$(CONFIG_PARPORT)		+= parport/
 obj-y				+= base/ block/ misc/ mfd/ nfc/
 obj-$(CONFIG_LIBNVDIMM)		+= nvdimm/
 obj-$(CONFIG_DAX)		+= dax/
-obj-$(CONFIG_CXL_BUS)		+= cxl/
 obj-$(CONFIG_DMA_SHARED_BUFFER) += dma-buf/
 obj-$(CONFIG_NUBUS)		+= nubus/
+obj-y				+= cxl/
 obj-y				+= macintosh/
 obj-y				+= scsi/
 obj-y				+= nvme/
diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
index b88ab956bb7c..f64e3984689f 100644
--- a/drivers/cxl/Kconfig
+++ b/drivers/cxl/Kconfig
@@ -98,4 +98,8 @@ config CXL_PORT
 	default CXL_BUS
 	tristate
 
+config CXL_SUSPEND
+	def_bool y
+	depends on SUSPEND && CXL_MEM
+
 endif
diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
index ce267ef11d93..a78270794150 100644
--- a/drivers/cxl/Makefile
+++ b/drivers/cxl/Makefile
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_CXL_BUS) += core/
+obj-y += core/
 obj-$(CONFIG_CXL_PCI) += cxl_pci.o
 obj-$(CONFIG_CXL_MEM) += cxl_mem.o
 obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
index 6d37cd78b151..9d35085d25af 100644
--- a/drivers/cxl/core/Makefile
+++ b/drivers/cxl/core/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_CXL_BUS) += cxl_core.o
+obj-$(CONFIG_CXL_SUSPEND) += suspend.o
 
 ccflags-y += -I$(srctree)/drivers/cxl
 cxl_core-y := port.o
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 1f76b28f9826..efe4d2e9bfef 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -251,7 +251,6 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
 	dev->bus = &cxl_bus_type;
 	dev->devt = MKDEV(cxl_mem_major, cxlmd->id);
 	dev->type = &cxl_memdev_type;
-	device_set_pm_not_required(dev);
 	INIT_WORK(&cxlmd->detach_work, detach_memdev);
 
 	cdev = &cxlmd->cdev;
diff --git a/drivers/cxl/core/suspend.c b/drivers/cxl/core/suspend.c
new file mode 100644
index 000000000000..88bdbe30a1df
--- /dev/null
+++ b/drivers/cxl/core/suspend.c
@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright(c) 2022 Intel Corporation. All rights reserved. */
+#include <linux/atomic.h>
+#include <linux/export.h>
+
+static atomic_t mem_active;
+
+bool cxl_mem_active(void)
+{
+	return atomic_read(&mem_active) != 0;
+}
+
+void cxl_mem_active_inc(void)
+{
+	atomic_inc(&mem_active);
+}
+EXPORT_SYMBOL_NS_GPL(cxl_mem_active_inc, CXL);
+
+void cxl_mem_active_dec(void)
+{
+	atomic_dec(&mem_active);
+}
+EXPORT_SYMBOL_NS_GPL(cxl_mem_active_dec, CXL);
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 243dd86a8b46..7235d2f976e5 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -353,6 +353,17 @@ int cxl_mem_create_range_info(struct cxl_dev_state *cxlds);
 struct cxl_dev_state *cxl_dev_state_create(struct device *dev);
 void set_exclusive_cxl_commands(struct cxl_dev_state *cxlds, unsigned long *cmds);
 void clear_exclusive_cxl_commands(struct cxl_dev_state *cxlds, unsigned long *cmds);
+#ifdef CONFIG_CXL_SUSPEND
+void cxl_mem_active_inc(void);
+void cxl_mem_active_dec(void);
+#else
+static inline void cxl_mem_active_inc(void)
+{
+}
+static inline void cxl_mem_active_dec(void)
+{
+}
+#endif
 
 struct cxl_hdm {
 	struct cxl_component_regs regs;
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 49a4b1c47299..0576d2d3df07 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -129,6 +129,11 @@ __mock bool cxl_dvsec_decode_init(struct cxl_dev_state *cxlds)
 	return do_hdm_init;
 }
 
+static void enable_suspend(void *data)
+{
+	cxl_mem_active_dec();
+}
+
 static int cxl_mem_probe(struct device *dev)
 {
 	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
@@ -207,7 +212,22 @@ static int cxl_mem_probe(struct device *dev)
 out:
 	cxl_device_unlock(&parent_port->dev);
 	put_device(&parent_port->dev);
-	return rc;
+
+	/*
+	 * The kernel may be operating out of CXL memory on this device,
+	 * there is no spec defined way to determine whether this device
+	 * preserves contents over suspend, and there is no simple way
+	 * to arrange for the suspend image to avoid CXL memory which
+	 * would setup a circular dependency between PCI resume and save
+	 * state restoration.
+	 *
+	 * TODO: support suspend when all the regions this device is
+	 * hosting are locked and covered by the system address map,
+	 * i.e. platform firmware owns restoring the HDM configuration
+	 * that it locked.
+	 */
+	cxl_mem_active_inc();
+	return devm_add_action_or_reset(dev, enable_suspend, NULL);
 }
 
 static struct cxl_driver cxl_mem_driver = {
diff --git a/include/linux/pm.h b/include/linux/pm.h
index e65b3ab28377..7911c4c9a7be 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -36,6 +36,15 @@ static inline void pm_vt_switch_unregister(struct device *dev)
 }
 #endif /* CONFIG_VT_CONSOLE_SLEEP */
 
+#ifdef CONFIG_CXL_SUSPEND
+bool cxl_mem_active(void);
+#else
+static inline bool cxl_mem_active(void)
+{
+	return false;
+}
+#endif
+
 /*
  * Device power management
  */
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 938d5c78b421..20a66bf9f465 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -83,7 +83,7 @@ bool hibernation_available(void)
 {
 	return nohibernate == 0 &&
 		!security_locked_down(LOCKDOWN_HIBERNATION) &&
-		!secretmem_active();
+		!secretmem_active() && !cxl_mem_active();
 }
 
 /**
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 7e646079fbeb..3e6be1c33e0b 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -127,7 +127,9 @@ static ssize_t mem_sleep_show(struct kobject *kobj, struct kobj_attribute *attr,
 	char *s = buf;
 	suspend_state_t i;
 
-	for (i = PM_SUSPEND_MIN; i < PM_SUSPEND_MAX; i++)
+	for (i = PM_SUSPEND_MIN; i < PM_SUSPEND_MAX; i++) {
+		if (i >= PM_SUSPEND_MEM && cxl_mem_active())
+			continue;
 		if (mem_sleep_states[i]) {
 			const char *label = mem_sleep_states[i];
 
@@ -136,6 +138,7 @@ static ssize_t mem_sleep_show(struct kobject *kobj, struct kobj_attribute *attr,
 			else
 				s += sprintf(s, "%s ", label);
 		}
+	}
 
 	/* Convert the last space to a newline if needed. */
 	if (s != buf)
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 6fcdee7e87a5..827075944d28 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -236,7 +236,8 @@ EXPORT_SYMBOL_GPL(suspend_valid_only_mem);
 
 static bool sleep_state_supported(suspend_state_t state)
 {
-	return state == PM_SUSPEND_TO_IDLE || valid_state(state);
+	return state == PM_SUSPEND_TO_IDLE ||
+	       (valid_state(state) && !cxl_mem_active());
 }
 
 static int platform_suspend_prepare(suspend_state_t state)

RE: [PATCH v2] PM: CXL: Disable suspend

[Brown, Len]

Is it possible for ACPI to be aware that CXL on that system doesn't support S3, and for ACPI to thus simply not export S3?

-Len

Re: [PATCH v2] PM: CXL: Disable suspend

[Dan Williams]

On Tue, Apr 5, 2022 at 1:05 PM Brown, Len <len.brown@intel.com> wrote:
>
> Is it possible for ACPI to be aware that CXL on that system doesn't support S3, and for ACPI to thus simply not export S3?

The ACPI domain has a chance to be aware of CXL devices attached at
boot and were configured into the System Memory Map.

However, there may be caveats that limit device visibility like
platform firmware only scans CXL devices directly attached to root
ports, not behind switches.

It would be welcome if there was an ACPI data structure to indicate
"device-X is in scope for S3" which would mean that it has aux power
and all HDM decoders (the registers that enable access to CXL memory)
are restored by the BIOS before OS resume begins.

Absent that the OS can only assume that the BIOS has no knowledge of
any HDM configurations that were defined at run time by the OS, like
after hotplug, or configurations that the OS rewrote by triggering a
secondary bus reset to unlock what the BIOS had locked and put into
the EFI memory map.

Re: [PATCH v2] PM: CXL: Disable suspend

[Rafael J. Wysocki]

On Tue, Apr 5, 2022 at 10:00 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> The CXL specification claims S3 support at a hardware level, but at a
> system software level there are some missing pieces. Section 9.4 (CXL
> 2.0) rightly claims that "CXL mem adapters may need aux power to retain
> memory context across S3", but there is no enumeration mechanism for the
> OS to determine if a given adapter has that support. Moreover the save
> state and resume image for the system may inadvertantly end up in a CXL
> device that needs to be restored before the save state is recoverable.
> I.e. a circular dependency that is not resolvable without a third party
> save-area.
>
> Arrange for the cxl_mem driver to fail S3 attempts. This still nominaly
> allows for suspend, but requires unbinding all CXL memory devices before
> the suspend to ensure the typical DRAM flow is taken. The cxl_mem unbind
> flow is intended to also tear down all CXL memory regions associated
> with a given cxl_memdev.
>
> It is reasonable to assume that any device participating in a System RAM
> range published in the EFI memory map is covered by aux power and
> save-area outside the device itself. So this restriction can be
> minimized in the future once pre-existing region enumeration support
> arrives, and perhaps a spec update to clarify if the EFI memory map is
> sufficent for determining the range of devices managed by
> platform-firmware for S3 support.
>
> Per Rafael, if the CXL configuration prevents suspend then it should
> fail early before tasks are frozen, and mem_sleep should stop showing
> 'mem' as an option [1]. Effectively CXL augments the platform suspend
> ->valid() op since, for example, the ACPI ops are not aware of the CXL /
> PCI dependencies. Given the split role of platform firmware vs OS
> provisioned CXL memory it is up to the cxl_mem driver to determine if
> the CXL configuration has elements that platform firmware may not be
> prepared to restore.
>
> Link: https://lore.kernel.org/r/CAJZ5v0hGVN_=3iU8OLpHY3Ak35T5+JcBM-qs8SbojKrpd0VXsA@mail.gmail.com [1]
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Len Brown <len.brown@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

The majority of the changes are in the CXL code, so

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

or please let me know if you want me to take this.

Thanks!

> ---
>  drivers/Makefile           |    2 +-
>  drivers/cxl/Kconfig        |    4 ++++
>  drivers/cxl/Makefile       |    2 +-
>  drivers/cxl/core/Makefile  |    1 +
>  drivers/cxl/core/memdev.c  |    1 -
>  drivers/cxl/core/suspend.c |   23 +++++++++++++++++++++++
>  drivers/cxl/cxlmem.h       |   11 +++++++++++
>  drivers/cxl/mem.c          |   22 +++++++++++++++++++++-
>  include/linux/pm.h         |    9 +++++++++
>  kernel/power/hibernate.c   |    2 +-
>  kernel/power/main.c        |    5 ++++-
>  kernel/power/suspend.c     |    3 ++-
>  12 files changed, 78 insertions(+), 7 deletions(-)
>  create mode 100644 drivers/cxl/core/suspend.c
>
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 020780b6b4d2..f735c4955143 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -72,9 +72,9 @@ obj-$(CONFIG_PARPORT)         += parport/
>  obj-y                          += base/ block/ misc/ mfd/ nfc/
>  obj-$(CONFIG_LIBNVDIMM)                += nvdimm/
>  obj-$(CONFIG_DAX)              += dax/
> -obj-$(CONFIG_CXL_BUS)          += cxl/
>  obj-$(CONFIG_DMA_SHARED_BUFFER) += dma-buf/
>  obj-$(CONFIG_NUBUS)            += nubus/
> +obj-y                          += cxl/
>  obj-y                          += macintosh/
>  obj-y                          += scsi/
>  obj-y                          += nvme/
> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> index b88ab956bb7c..f64e3984689f 100644
> --- a/drivers/cxl/Kconfig
> +++ b/drivers/cxl/Kconfig
> @@ -98,4 +98,8 @@ config CXL_PORT
>         default CXL_BUS
>         tristate
>
> +config CXL_SUSPEND
> +       def_bool y
> +       depends on SUSPEND && CXL_MEM
> +
>  endif
> diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
> index ce267ef11d93..a78270794150 100644
> --- a/drivers/cxl/Makefile
> +++ b/drivers/cxl/Makefile
> @@ -1,5 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0
> -obj-$(CONFIG_CXL_BUS) += core/
> +obj-y += core/
>  obj-$(CONFIG_CXL_PCI) += cxl_pci.o
>  obj-$(CONFIG_CXL_MEM) += cxl_mem.o
>  obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
> diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
> index 6d37cd78b151..9d35085d25af 100644
> --- a/drivers/cxl/core/Makefile
> +++ b/drivers/cxl/core/Makefile
> @@ -1,5 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_CXL_BUS) += cxl_core.o
> +obj-$(CONFIG_CXL_SUSPEND) += suspend.o
>
>  ccflags-y += -I$(srctree)/drivers/cxl
>  cxl_core-y := port.o
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 1f76b28f9826..efe4d2e9bfef 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -251,7 +251,6 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
>         dev->bus = &cxl_bus_type;
>         dev->devt = MKDEV(cxl_mem_major, cxlmd->id);
>         dev->type = &cxl_memdev_type;
> -       device_set_pm_not_required(dev);
>         INIT_WORK(&cxlmd->detach_work, detach_memdev);
>
>         cdev = &cxlmd->cdev;
> diff --git a/drivers/cxl/core/suspend.c b/drivers/cxl/core/suspend.c
> new file mode 100644
> index 000000000000..88bdbe30a1df
> --- /dev/null
> +++ b/drivers/cxl/core/suspend.c
> @@ -0,0 +1,23 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2022 Intel Corporation. All rights reserved. */
> +#include <linux/atomic.h>
> +#include <linux/export.h>
> +
> +static atomic_t mem_active;
> +
> +bool cxl_mem_active(void)
> +{
> +       return atomic_read(&mem_active) != 0;
> +}
> +
> +void cxl_mem_active_inc(void)
> +{
> +       atomic_inc(&mem_active);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_mem_active_inc, CXL);
> +
> +void cxl_mem_active_dec(void)
> +{
> +       atomic_dec(&mem_active);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_mem_active_dec, CXL);
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 243dd86a8b46..7235d2f976e5 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -353,6 +353,17 @@ int cxl_mem_create_range_info(struct cxl_dev_state *cxlds);
>  struct cxl_dev_state *cxl_dev_state_create(struct device *dev);
>  void set_exclusive_cxl_commands(struct cxl_dev_state *cxlds, unsigned long *cmds);
>  void clear_exclusive_cxl_commands(struct cxl_dev_state *cxlds, unsigned long *cmds);
> +#ifdef CONFIG_CXL_SUSPEND
> +void cxl_mem_active_inc(void);
> +void cxl_mem_active_dec(void);
> +#else
> +static inline void cxl_mem_active_inc(void)
> +{
> +}
> +static inline void cxl_mem_active_dec(void)
> +{
> +}
> +#endif
>
>  struct cxl_hdm {
>         struct cxl_component_regs regs;
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 49a4b1c47299..0576d2d3df07 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -129,6 +129,11 @@ __mock bool cxl_dvsec_decode_init(struct cxl_dev_state *cxlds)
>         return do_hdm_init;
>  }
>
> +static void enable_suspend(void *data)
> +{
> +       cxl_mem_active_dec();
> +}
> +
>  static int cxl_mem_probe(struct device *dev)
>  {
>         struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> @@ -207,7 +212,22 @@ static int cxl_mem_probe(struct device *dev)
>  out:
>         cxl_device_unlock(&parent_port->dev);
>         put_device(&parent_port->dev);
> -       return rc;
> +
> +       /*
> +        * The kernel may be operating out of CXL memory on this device,
> +        * there is no spec defined way to determine whether this device
> +        * preserves contents over suspend, and there is no simple way
> +        * to arrange for the suspend image to avoid CXL memory which
> +        * would setup a circular dependency between PCI resume and save
> +        * state restoration.
> +        *
> +        * TODO: support suspend when all the regions this device is
> +        * hosting are locked and covered by the system address map,
> +        * i.e. platform firmware owns restoring the HDM configuration
> +        * that it locked.
> +        */
> +       cxl_mem_active_inc();
> +       return devm_add_action_or_reset(dev, enable_suspend, NULL);
>  }
>
>  static struct cxl_driver cxl_mem_driver = {
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index e65b3ab28377..7911c4c9a7be 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -36,6 +36,15 @@ static inline void pm_vt_switch_unregister(struct device *dev)
>  }
>  #endif /* CONFIG_VT_CONSOLE_SLEEP */
>
> +#ifdef CONFIG_CXL_SUSPEND
> +bool cxl_mem_active(void);
> +#else
> +static inline bool cxl_mem_active(void)
> +{
> +       return false;
> +}
> +#endif
> +
>  /*
>   * Device power management
>   */
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 938d5c78b421..20a66bf9f465 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -83,7 +83,7 @@ bool hibernation_available(void)
>  {
>         return nohibernate == 0 &&
>                 !security_locked_down(LOCKDOWN_HIBERNATION) &&
> -               !secretmem_active();
> +               !secretmem_active() && !cxl_mem_active();
>  }
>
>  /**
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 7e646079fbeb..3e6be1c33e0b 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -127,7 +127,9 @@ static ssize_t mem_sleep_show(struct kobject *kobj, struct kobj_attribute *attr,
>         char *s = buf;
>         suspend_state_t i;
>
> -       for (i = PM_SUSPEND_MIN; i < PM_SUSPEND_MAX; i++)
> +       for (i = PM_SUSPEND_MIN; i < PM_SUSPEND_MAX; i++) {
> +               if (i >= PM_SUSPEND_MEM && cxl_mem_active())
> +                       continue;
>                 if (mem_sleep_states[i]) {
>                         const char *label = mem_sleep_states[i];
>
> @@ -136,6 +138,7 @@ static ssize_t mem_sleep_show(struct kobject *kobj, struct kobj_attribute *attr,
>                         else
>                                 s += sprintf(s, "%s ", label);
>                 }
> +       }
>
>         /* Convert the last space to a newline if needed. */
>         if (s != buf)
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 6fcdee7e87a5..827075944d28 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -236,7 +236,8 @@ EXPORT_SYMBOL_GPL(suspend_valid_only_mem);
>
>  static bool sleep_state_supported(suspend_state_t state)
>  {
> -       return state == PM_SUSPEND_TO_IDLE || valid_state(state);
> +       return state == PM_SUSPEND_TO_IDLE ||
> +              (valid_state(state) && !cxl_mem_active());
>  }
>
>  static int platform_suspend_prepare(suspend_state_t state)
>

Re: [PATCH v2] PM: CXL: Disable suspend

[Dan Williams]

On Tue, Apr 12, 2022 at 10:02 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Apr 5, 2022 at 10:00 PM Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > The CXL specification claims S3 support at a hardware level, but at a
> > system software level there are some missing pieces. Section 9.4 (CXL
> > 2.0) rightly claims that "CXL mem adapters may need aux power to retain
> > memory context across S3", but there is no enumeration mechanism for the
> > OS to determine if a given adapter has that support. Moreover the save
> > state and resume image for the system may inadvertantly end up in a CXL
> > device that needs to be restored before the save state is recoverable.
> > I.e. a circular dependency that is not resolvable without a third party
> > save-area.
> >
> > Arrange for the cxl_mem driver to fail S3 attempts. This still nominaly
> > allows for suspend, but requires unbinding all CXL memory devices before
> > the suspend to ensure the typical DRAM flow is taken. The cxl_mem unbind
> > flow is intended to also tear down all CXL memory regions associated
> > with a given cxl_memdev.
> >
> > It is reasonable to assume that any device participating in a System RAM
> > range published in the EFI memory map is covered by aux power and
> > save-area outside the device itself. So this restriction can be
> > minimized in the future once pre-existing region enumeration support
> > arrives, and perhaps a spec update to clarify if the EFI memory map is
> > sufficent for determining the range of devices managed by
> > platform-firmware for S3 support.
> >
> > Per Rafael, if the CXL configuration prevents suspend then it should
> > fail early before tasks are frozen, and mem_sleep should stop showing
> > 'mem' as an option [1]. Effectively CXL augments the platform suspend
> > ->valid() op since, for example, the ACPI ops are not aware of the CXL /
> > PCI dependencies. Given the split role of platform firmware vs OS
> > provisioned CXL memory it is up to the cxl_mem driver to determine if
> > the CXL configuration has elements that platform firmware may not be
> > prepared to restore.
> >
> > Link: https://lore.kernel.org/r/CAJZ5v0hGVN_=3iU8OLpHY3Ak35T5+JcBM-qs8SbojKrpd0VXsA@mail.gmail.com [1]
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Pavel Machek <pavel@ucw.cz>
> > Cc: Len Brown <len.brown@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> The majority of the changes are in the CXL code, so
>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> or please let me know if you want me to take this.

Thanks Rafael, I'll do one more update to return the call to
device_set_pm_not_required() since with this new scheme the CXL
dev_pm_ops are not used. I'll take this through cxl.git.

>
> Thanks!
>
> > ---
> >  drivers/Makefile           |    2 +-
> >  drivers/cxl/Kconfig        |    4 ++++
> >  drivers/cxl/Makefile       |    2 +-
> >  drivers/cxl/core/Makefile  |    1 +
> >  drivers/cxl/core/memdev.c  |    1 -
> >  drivers/cxl/core/suspend.c |   23 +++++++++++++++++++++++
> >  drivers/cxl/cxlmem.h       |   11 +++++++++++
> >  drivers/cxl/mem.c          |   22 +++++++++++++++++++++-
> >  include/linux/pm.h         |    9 +++++++++
> >  kernel/power/hibernate.c   |    2 +-
> >  kernel/power/main.c        |    5 ++++-
> >  kernel/power/suspend.c     |    3 ++-
> >  12 files changed, 78 insertions(+), 7 deletions(-)
> >  create mode 100644 drivers/cxl/core/suspend.c
> >
> > diff --git a/drivers/Makefile b/drivers/Makefile
> > index 020780b6b4d2..f735c4955143 100644
> > --- a/drivers/Makefile
> > +++ b/drivers/Makefile
> > @@ -72,9 +72,9 @@ obj-$(CONFIG_PARPORT)         += parport/
> >  obj-y                          += base/ block/ misc/ mfd/ nfc/
> >  obj-$(CONFIG_LIBNVDIMM)                += nvdimm/
> >  obj-$(CONFIG_DAX)              += dax/
> > -obj-$(CONFIG_CXL_BUS)          += cxl/
> >  obj-$(CONFIG_DMA_SHARED_BUFFER) += dma-buf/
> >  obj-$(CONFIG_NUBUS)            += nubus/
> > +obj-y                          += cxl/
> >  obj-y                          += macintosh/
> >  obj-y                          += scsi/
> >  obj-y                          += nvme/
> > diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> > index b88ab956bb7c..f64e3984689f 100644
> > --- a/drivers/cxl/Kconfig
> > +++ b/drivers/cxl/Kconfig
> > @@ -98,4 +98,8 @@ config CXL_PORT
> >         default CXL_BUS
> >         tristate
> >
> > +config CXL_SUSPEND
> > +       def_bool y
> > +       depends on SUSPEND && CXL_MEM
> > +
> >  endif
> > diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
> > index ce267ef11d93..a78270794150 100644
> > --- a/drivers/cxl/Makefile
> > +++ b/drivers/cxl/Makefile
> > @@ -1,5 +1,5 @@
> >  # SPDX-License-Identifier: GPL-2.0
> > -obj-$(CONFIG_CXL_BUS) += core/
> > +obj-y += core/
> >  obj-$(CONFIG_CXL_PCI) += cxl_pci.o
> >  obj-$(CONFIG_CXL_MEM) += cxl_mem.o
> >  obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
> > diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
> > index 6d37cd78b151..9d35085d25af 100644
> > --- a/drivers/cxl/core/Makefile
> > +++ b/drivers/cxl/core/Makefile
> > @@ -1,5 +1,6 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  obj-$(CONFIG_CXL_BUS) += cxl_core.o
> > +obj-$(CONFIG_CXL_SUSPEND) += suspend.o
> >
> >  ccflags-y += -I$(srctree)/drivers/cxl
> >  cxl_core-y := port.o
> > diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> > index 1f76b28f9826..efe4d2e9bfef 100644
> > --- a/drivers/cxl/core/memdev.c
> > +++ b/drivers/cxl/core/memdev.c
> > @@ -251,7 +251,6 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
> >         dev->bus = &cxl_bus_type;
> >         dev->devt = MKDEV(cxl_mem_major, cxlmd->id);
> >         dev->type = &cxl_memdev_type;
> > -       device_set_pm_not_required(dev);
> >         INIT_WORK(&cxlmd->detach_work, detach_memdev);
> >
> >         cdev = &cxlmd->cdev;
> > diff --git a/drivers/cxl/core/suspend.c b/drivers/cxl/core/suspend.c
> > new file mode 100644
> > index 000000000000..88bdbe30a1df
> > --- /dev/null
> > +++ b/drivers/cxl/core/suspend.c
> > @@ -0,0 +1,23 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright(c) 2022 Intel Corporation. All rights reserved. */
> > +#include <linux/atomic.h>
> > +#include <linux/export.h>
> > +
> > +static atomic_t mem_active;
> > +
> > +bool cxl_mem_active(void)
> > +{
> > +       return atomic_read(&mem_active) != 0;
> > +}
> > +
> > +void cxl_mem_active_inc(void)
> > +{
> > +       atomic_inc(&mem_active);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(cxl_mem_active_inc, CXL);
> > +
> > +void cxl_mem_active_dec(void)
> > +{
> > +       atomic_dec(&mem_active);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(cxl_mem_active_dec, CXL);
> > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > index 243dd86a8b46..7235d2f976e5 100644
> > --- a/drivers/cxl/cxlmem.h
> > +++ b/drivers/cxl/cxlmem.h
> > @@ -353,6 +353,17 @@ int cxl_mem_create_range_info(struct cxl_dev_state *cxlds);
> >  struct cxl_dev_state *cxl_dev_state_create(struct device *dev);
> >  void set_exclusive_cxl_commands(struct cxl_dev_state *cxlds, unsigned long *cmds);
> >  void clear_exclusive_cxl_commands(struct cxl_dev_state *cxlds, unsigned long *cmds);
> > +#ifdef CONFIG_CXL_SUSPEND
> > +void cxl_mem_active_inc(void);
> > +void cxl_mem_active_dec(void);
> > +#else
> > +static inline void cxl_mem_active_inc(void)
> > +{
> > +}
> > +static inline void cxl_mem_active_dec(void)
> > +{
> > +}
> > +#endif
> >
> >  struct cxl_hdm {
> >         struct cxl_component_regs regs;
> > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> > index 49a4b1c47299..0576d2d3df07 100644
> > --- a/drivers/cxl/mem.c
> > +++ b/drivers/cxl/mem.c
> > @@ -129,6 +129,11 @@ __mock bool cxl_dvsec_decode_init(struct cxl_dev_state *cxlds)
> >         return do_hdm_init;
> >  }
> >
> > +static void enable_suspend(void *data)
> > +{
> > +       cxl_mem_active_dec();
> > +}
> > +
> >  static int cxl_mem_probe(struct device *dev)
> >  {
> >         struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> > @@ -207,7 +212,22 @@ static int cxl_mem_probe(struct device *dev)
> >  out:
> >         cxl_device_unlock(&parent_port->dev);
> >         put_device(&parent_port->dev);
> > -       return rc;
> > +
> > +       /*
> > +        * The kernel may be operating out of CXL memory on this device,
> > +        * there is no spec defined way to determine whether this device
> > +        * preserves contents over suspend, and there is no simple way
> > +        * to arrange for the suspend image to avoid CXL memory which
> > +        * would setup a circular dependency between PCI resume and save
> > +        * state restoration.
> > +        *
> > +        * TODO: support suspend when all the regions this device is
> > +        * hosting are locked and covered by the system address map,
> > +        * i.e. platform firmware owns restoring the HDM configuration
> > +        * that it locked.
> > +        */
> > +       cxl_mem_active_inc();
> > +       return devm_add_action_or_reset(dev, enable_suspend, NULL);
> >  }
> >
> >  static struct cxl_driver cxl_mem_driver = {
> > diff --git a/include/linux/pm.h b/include/linux/pm.h
> > index e65b3ab28377..7911c4c9a7be 100644
> > --- a/include/linux/pm.h
> > +++ b/include/linux/pm.h
> > @@ -36,6 +36,15 @@ static inline void pm_vt_switch_unregister(struct device *dev)
> >  }
> >  #endif /* CONFIG_VT_CONSOLE_SLEEP */
> >
> > +#ifdef CONFIG_CXL_SUSPEND
> > +bool cxl_mem_active(void);
> > +#else
> > +static inline bool cxl_mem_active(void)
> > +{
> > +       return false;
> > +}
> > +#endif
> > +
> >  /*
> >   * Device power management
> >   */
> > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> > index 938d5c78b421..20a66bf9f465 100644
> > --- a/kernel/power/hibernate.c
> > +++ b/kernel/power/hibernate.c
> > @@ -83,7 +83,7 @@ bool hibernation_available(void)
> >  {
> >         return nohibernate == 0 &&
> >                 !security_locked_down(LOCKDOWN_HIBERNATION) &&
> > -               !secretmem_active();
> > +               !secretmem_active() && !cxl_mem_active();
> >  }
> >
> >  /**
> > diff --git a/kernel/power/main.c b/kernel/power/main.c
> > index 7e646079fbeb..3e6be1c33e0b 100644
> > --- a/kernel/power/main.c
> > +++ b/kernel/power/main.c
> > @@ -127,7 +127,9 @@ static ssize_t mem_sleep_show(struct kobject *kobj, struct kobj_attribute *attr,
> >         char *s = buf;
> >         suspend_state_t i;
> >
> > -       for (i = PM_SUSPEND_MIN; i < PM_SUSPEND_MAX; i++)
> > +       for (i = PM_SUSPEND_MIN; i < PM_SUSPEND_MAX; i++) {
> > +               if (i >= PM_SUSPEND_MEM && cxl_mem_active())
> > +                       continue;
> >                 if (mem_sleep_states[i]) {
> >                         const char *label = mem_sleep_states[i];
> >
> > @@ -136,6 +138,7 @@ static ssize_t mem_sleep_show(struct kobject *kobj, struct kobj_attribute *attr,
> >                         else
> >                                 s += sprintf(s, "%s ", label);
> >                 }
> > +       }
> >
> >         /* Convert the last space to a newline if needed. */
> >         if (s != buf)
> > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > index 6fcdee7e87a5..827075944d28 100644
> > --- a/kernel/power/suspend.c
> > +++ b/kernel/power/suspend.c
> > @@ -236,7 +236,8 @@ EXPORT_SYMBOL_GPL(suspend_valid_only_mem);
> >
> >  static bool sleep_state_supported(suspend_state_t state)
> >  {
> > -       return state == PM_SUSPEND_TO_IDLE || valid_state(state);
> > +       return state == PM_SUSPEND_TO_IDLE ||
> > +              (valid_state(state) && !cxl_mem_active());
> >  }
> >
> >  static int platform_suspend_prepare(suspend_state_t state)
> >

[PATCH v3] PM: CXL: Disable suspend

[Dan Williams]

The CXL specification claims S3 support at a hardware level, but at a
system software level there are some missing pieces. Section 9.4 (CXL
2.0) rightly claims that "CXL mem adapters may need aux power to retain
memory context across S3", but there is no enumeration mechanism for the
OS to determine if a given adapter has that support. Moreover the save
state and resume image for the system may inadvertantly end up in a CXL
device that needs to be restored before the save state is recoverable.
I.e. a circular dependency that is not resolvable without a third party
save-area.

Arrange for the cxl_mem driver to fail S3 attempts. This still nominaly
allows for suspend, but requires unbinding all CXL memory devices before
the suspend to ensure the typical DRAM flow is taken. The cxl_mem unbind
flow is intended to also tear down all CXL memory regions associated
with a given cxl_memdev.

It is reasonable to assume that any device participating in a System RAM
range published in the EFI memory map is covered by aux power and
save-area outside the device itself. So this restriction can be
minimized in the future once pre-existing region enumeration support
arrives, and perhaps a spec update to clarify if the EFI memory map is
sufficent for determining the range of devices managed by
platform-firmware for S3 support.

Per Rafael, if the CXL configuration prevents suspend then it should
fail early before tasks are frozen, and mem_sleep should stop showing
'mem' as an option [1]. Effectively CXL augments the platform suspend
->valid() op since, for example, the ACPI ops are not aware of the CXL /
PCI dependencies. Given the split role of platform firmware vs OS
provisioned CXL memory it is up to the cxl_mem driver to determine if
the CXL configuration has elements that platform firmware may not be
prepared to restore.

Link: https://lore.kernel.org/r/CAJZ5v0hGVN_=3iU8OLpHY3Ak35T5+JcBM-qs8SbojKrpd0VXsA@mail.gmail.com [1]
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Len Brown <len.brown@intel.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Changes since v2:
- revert the dropping of device_set_pm_not_required() since this
  solution no longer relies on dev_pm_ops.
- collect Rafael's reviewed-by

 drivers/Makefile           |    2 +-
 drivers/cxl/Kconfig        |    4 ++++
 drivers/cxl/Makefile       |    2 +-
 drivers/cxl/core/Makefile  |    1 +
 drivers/cxl/core/suspend.c |   23 +++++++++++++++++++++++
 drivers/cxl/cxlmem.h       |   11 +++++++++++
 drivers/cxl/mem.c          |   22 +++++++++++++++++++++-
 include/linux/pm.h         |    9 +++++++++
 kernel/power/hibernate.c   |    2 +-
 kernel/power/main.c        |    5 ++++-
 kernel/power/suspend.c     |    3 ++-
 11 files changed, 78 insertions(+), 6 deletions(-)
 create mode 100644 drivers/cxl/core/suspend.c

diff --git a/drivers/Makefile b/drivers/Makefile
index 020780b6b4d2..f735c4955143 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -72,9 +72,9 @@ obj-$(CONFIG_PARPORT)		+= parport/
 obj-y				+= base/ block/ misc/ mfd/ nfc/
 obj-$(CONFIG_LIBNVDIMM)		+= nvdimm/
 obj-$(CONFIG_DAX)		+= dax/
-obj-$(CONFIG_CXL_BUS)		+= cxl/
 obj-$(CONFIG_DMA_SHARED_BUFFER) += dma-buf/
 obj-$(CONFIG_NUBUS)		+= nubus/
+obj-y				+= cxl/
 obj-y				+= macintosh/
 obj-y				+= scsi/
 obj-y				+= nvme/
diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
index b88ab956bb7c..f64e3984689f 100644
--- a/drivers/cxl/Kconfig
+++ b/drivers/cxl/Kconfig
@@ -98,4 +98,8 @@ config CXL_PORT
 	default CXL_BUS
 	tristate
 
+config CXL_SUSPEND
+	def_bool y
+	depends on SUSPEND && CXL_MEM
+
 endif
diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
index ce267ef11d93..a78270794150 100644
--- a/drivers/cxl/Makefile
+++ b/drivers/cxl/Makefile
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_CXL_BUS) += core/
+obj-y += core/
 obj-$(CONFIG_CXL_PCI) += cxl_pci.o
 obj-$(CONFIG_CXL_MEM) += cxl_mem.o
 obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
index 6d37cd78b151..9d35085d25af 100644
--- a/drivers/cxl/core/Makefile
+++ b/drivers/cxl/core/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_CXL_BUS) += cxl_core.o
+obj-$(CONFIG_CXL_SUSPEND) += suspend.o
 
 ccflags-y += -I$(srctree)/drivers/cxl
 cxl_core-y := port.o
diff --git a/drivers/cxl/core/suspend.c b/drivers/cxl/core/suspend.c
new file mode 100644
index 000000000000..88bdbe30a1df
--- /dev/null
+++ b/drivers/cxl/core/suspend.c
@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright(c) 2022 Intel Corporation. All rights reserved. */
+#include <linux/atomic.h>
+#include <linux/export.h>
+
+static atomic_t mem_active;
+
+bool cxl_mem_active(void)
+{
+	return atomic_read(&mem_active) != 0;
+}
+
+void cxl_mem_active_inc(void)
+{
+	atomic_inc(&mem_active);
+}
+EXPORT_SYMBOL_NS_GPL(cxl_mem_active_inc, CXL);
+
+void cxl_mem_active_dec(void)
+{
+	atomic_dec(&mem_active);
+}
+EXPORT_SYMBOL_NS_GPL(cxl_mem_active_dec, CXL);
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 243dd86a8b46..7235d2f976e5 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -353,6 +353,17 @@ int cxl_mem_create_range_info(struct cxl_dev_state *cxlds);
 struct cxl_dev_state *cxl_dev_state_create(struct device *dev);
 void set_exclusive_cxl_commands(struct cxl_dev_state *cxlds, unsigned long *cmds);
 void clear_exclusive_cxl_commands(struct cxl_dev_state *cxlds, unsigned long *cmds);
+#ifdef CONFIG_CXL_SUSPEND
+void cxl_mem_active_inc(void);
+void cxl_mem_active_dec(void);
+#else
+static inline void cxl_mem_active_inc(void)
+{
+}
+static inline void cxl_mem_active_dec(void)
+{
+}
+#endif
 
 struct cxl_hdm {
 	struct cxl_component_regs regs;
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 43e73d259207..1bd2e0b67f59 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -138,6 +138,11 @@ __mock bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
 	return retval;
 }
 
+static void enable_suspend(void *data)
+{
+	cxl_mem_active_dec();
+}
+
 static int cxl_mem_probe(struct device *dev)
 {
 	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
@@ -194,7 +199,22 @@ static int cxl_mem_probe(struct device *dev)
 out:
 	cxl_device_unlock(&parent_port->dev);
 	put_device(&parent_port->dev);
-	return rc;
+
+	/*
+	 * The kernel may be operating out of CXL memory on this device,
+	 * there is no spec defined way to determine whether this device
+	 * preserves contents over suspend, and there is no simple way
+	 * to arrange for the suspend image to avoid CXL memory which
+	 * would setup a circular dependency between PCI resume and save
+	 * state restoration.
+	 *
+	 * TODO: support suspend when all the regions this device is
+	 * hosting are locked and covered by the system address map,
+	 * i.e. platform firmware owns restoring the HDM configuration
+	 * that it locked.
+	 */
+	cxl_mem_active_inc();
+	return devm_add_action_or_reset(dev, enable_suspend, NULL);
 }
 
 static struct cxl_driver cxl_mem_driver = {
diff --git a/include/linux/pm.h b/include/linux/pm.h
index e65b3ab28377..7911c4c9a7be 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -36,6 +36,15 @@ static inline void pm_vt_switch_unregister(struct device *dev)
 }
 #endif /* CONFIG_VT_CONSOLE_SLEEP */
 
+#ifdef CONFIG_CXL_SUSPEND
+bool cxl_mem_active(void);
+#else
+static inline bool cxl_mem_active(void)
+{
+	return false;
+}
+#endif
+
 /*
  * Device power management
  */
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 938d5c78b421..20a66bf9f465 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -83,7 +83,7 @@ bool hibernation_available(void)
 {
 	return nohibernate == 0 &&
 		!security_locked_down(LOCKDOWN_HIBERNATION) &&
-		!secretmem_active();
+		!secretmem_active() && !cxl_mem_active();
 }
 
 /**
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 7e646079fbeb..3e6be1c33e0b 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -127,7 +127,9 @@ static ssize_t mem_sleep_show(struct kobject *kobj, struct kobj_attribute *attr,
 	char *s = buf;
 	suspend_state_t i;
 
-	for (i = PM_SUSPEND_MIN; i < PM_SUSPEND_MAX; i++)
+	for (i = PM_SUSPEND_MIN; i < PM_SUSPEND_MAX; i++) {
+		if (i >= PM_SUSPEND_MEM && cxl_mem_active())
+			continue;
 		if (mem_sleep_states[i]) {
 			const char *label = mem_sleep_states[i];
 
@@ -136,6 +138,7 @@ static ssize_t mem_sleep_show(struct kobject *kobj, struct kobj_attribute *attr,
 			else
 				s += sprintf(s, "%s ", label);
 		}
+	}
 
 	/* Convert the last space to a newline if needed. */
 	if (s != buf)
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 6fcdee7e87a5..827075944d28 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -236,7 +236,8 @@ EXPORT_SYMBOL_GPL(suspend_valid_only_mem);
 
 static bool sleep_state_supported(suspend_state_t state)
 {
-	return state == PM_SUSPEND_TO_IDLE || valid_state(state);
+	return state == PM_SUSPEND_TO_IDLE ||
+	       (valid_state(state) && !cxl_mem_active());
 }
 
 static int platform_suspend_prepare(suspend_state_t state)