* [PATCH v2 0/1] nvdimm: allow exposing RAM as libnvdimm DIMMs
@ 2025-10-15 8:00 Mike Rapoport
2025-10-15 8:00 ` [PATCH v2 1/1] nvdimm: allow exposing RAM carveouts as NVDIMM DIMM devices Mike Rapoport
0 siblings, 1 reply; 6+ messages in thread
From: Mike Rapoport @ 2025-10-15 8:00 UTC (permalink / raw)
To: Dan Williams, Dave Jiang, Ira Weiny, Vishal Verma
Cc: jane.chu, Michał Cłapiński, Mike Rapoport,
Pasha Tatashin, Tyler Hicks, linux-kernel, nvdimm
From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>
Hi,
It's not uncommon that libnvdimm/dax/ndctl are used with normal volatile
memory for a whole bunch of $reasons.
Probably the most common usecase is to back VMs memory with fsdax/devdax,
but there are others as well when there's a requirement to manage memory
separately from the kernel.
The existing mechanisms to expose normal ram as "persistent", such as
memmap=x!y on x86 or dummy pmem-region device tree nodes on DT systems lack
flexibility to dynamically partition a single region without rebooting the
system and sometimes even updating the system firmware. Also, to create
several DAX devices with different properties it's necessary to repeat
the memmap= command line option or add several pmem-region nodes to the
DT.
I propose a new ramdax driver that will create a DIMM device on
E820_TYPE_PRAM/pmem-region and that will allow partitioning that device
dynamically. The label area is kept in the end of that region and managed
by the driver.
v2 changes:
* Change the way driver is bound to a device, following Dan's
suggestion. Instead of forcing mutual exclusion of ramdax and
nr_e820/of-pmem at build time, rely on 'driver_override' attribute to
allow binding ramdax driver to e820_pmem/pmem-region devices.
* Fix build warning reported by kbuild
v1: https://lore.kernel.org/all/20250826080430.1952982-1-rppt@kernel.org
* fix offset calculations in ramdax_{get,set}_config_data
* use a magic constant instead of a random number as nd_set->cookie*
RFC: https://lore.kernel.org/all/20250612083153.48624-1-rppt@kernel.org
Mike Rapoport (Microsoft) (1):
nvdimm: allow exposing RAM carveouts as NVDIMM DIMM devices
drivers/nvdimm/Kconfig | 17 +++
drivers/nvdimm/Makefile | 1 +
drivers/nvdimm/ramdax.c | 272 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 290 insertions(+)
create mode 100644 drivers/nvdimm/ramdax.c
base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
--
2.50.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/1] nvdimm: allow exposing RAM carveouts as NVDIMM DIMM devices
2025-10-15 8:00 [PATCH v2 0/1] nvdimm: allow exposing RAM as libnvdimm DIMMs Mike Rapoport
@ 2025-10-15 8:00 ` Mike Rapoport
2025-10-18 0:08 ` dan.j.williams
0 siblings, 1 reply; 6+ messages in thread
From: Mike Rapoport @ 2025-10-15 8:00 UTC (permalink / raw)
To: Dan Williams, Dave Jiang, Ira Weiny, Vishal Verma
Cc: jane.chu, Michał Cłapiński, Mike Rapoport,
Pasha Tatashin, Tyler Hicks, linux-kernel, nvdimm
From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>
There are use cases, for example virtual machine hosts, that create
"persistent" memory regions using memmap= option on x86 or dummy
pmem-region device tree nodes on DT based systems.
Both these options are inflexible because they create static regions and
the layout of the "persistent" memory cannot be adjusted without reboot
and sometimes they even require firmware update.
Add a ramdax driver that allows creation of DIMM devices on top of
E820_TYPE_PRAM regions and devicetree pmem-region nodes.
The DIMMs support label space management on the "device" and provide a
flexible way to access RAM using fsdax and devdax.
Signed-off-by: Mike Rapoport (Mircosoft) <rppt@kernel.org>
---
drivers/nvdimm/Kconfig | 17 +++
drivers/nvdimm/Makefile | 1 +
drivers/nvdimm/ramdax.c | 272 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 290 insertions(+)
create mode 100644 drivers/nvdimm/ramdax.c
diff --git a/drivers/nvdimm/Kconfig b/drivers/nvdimm/Kconfig
index fde3e17c836c..9ac96a7cd773 100644
--- a/drivers/nvdimm/Kconfig
+++ b/drivers/nvdimm/Kconfig
@@ -97,6 +97,23 @@ config OF_PMEM
Select Y if unsure.
+config RAMDAX
+ tristate "Support persistent memory interfaces on RAM carveouts"
+ depends on OF || X86
+ select X86_PMEM_LEGACY_DEVICE
+ default LIBNVDIMM
+ help
+ Allows creation of DAX devices on RAM carveouts.
+
+ Memory ranges that are manually specified by the
+ 'memmap=nn[KMG]!ss[KMG]' kernel command line or defined by dummy
+ pmem-region device tree nodes would be managed by this driver as DIMM
+ devices with support for dynamic layout of namespaces.
+ The driver can be bound to e820_pmem or pmem-region platform
+ devices using 'driver_override' device attribute.
+
+ Select N if unsure.
+
config NVDIMM_KEYS
def_bool y
depends on ENCRYPTED_KEYS
diff --git a/drivers/nvdimm/Makefile b/drivers/nvdimm/Makefile
index ba0296dca9db..8c268814936c 100644
--- a/drivers/nvdimm/Makefile
+++ b/drivers/nvdimm/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_ND_BTT) += nd_btt.o
obj-$(CONFIG_X86_PMEM_LEGACY) += nd_e820.o
obj-$(CONFIG_OF_PMEM) += of_pmem.o
obj-$(CONFIG_VIRTIO_PMEM) += virtio_pmem.o nd_virtio.o
+obj-$(CONFIG_RAMDAX) += ramdax.o
nd_pmem-y := pmem.o
diff --git a/drivers/nvdimm/ramdax.c b/drivers/nvdimm/ramdax.c
new file mode 100644
index 000000000000..9f3c2685d9b5
--- /dev/null
+++ b/drivers/nvdimm/ramdax.c
@@ -0,0 +1,272 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2025, Mike Rapoport, Microsoft
+ *
+ * Based on e820 pmem driver:
+ * Copyright (c) 2015, Christoph Hellwig.
+ * Copyright (c) 2015, Intel Corporation.
+ */
+#include <linux/platform_device.h>
+#include <linux/memory_hotplug.h>
+#include <linux/libnvdimm.h>
+#include <linux/module.h>
+#include <linux/numa.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/of.h>
+
+#include <uapi/linux/ndctl.h>
+
+#define LABEL_AREA_SIZE SZ_128K
+
+struct ramdax_dimm {
+ struct nvdimm *nvdimm;
+ void *label_area;
+};
+
+static void ramdax_remove(struct platform_device *pdev)
+{
+ struct nvdimm_bus *nvdimm_bus = platform_get_drvdata(pdev);
+
+ nvdimm_bus_unregister(nvdimm_bus);
+}
+
+static int ramdax_register_region(struct resource *res,
+ struct nvdimm *nvdimm,
+ struct nvdimm_bus *nvdimm_bus)
+{
+ struct nd_mapping_desc mapping;
+ struct nd_region_desc ndr_desc;
+ struct nd_interleave_set *nd_set;
+ int nid = phys_to_target_node(res->start);
+
+ nd_set = kzalloc(sizeof(*nd_set), GFP_KERNEL);
+ if (!nd_set)
+ return -ENOMEM;
+
+ nd_set->cookie1 = 0xcafebeefcafebeef;
+ nd_set->cookie2 = nd_set->cookie1;
+ nd_set->altcookie = nd_set->cookie1;
+
+ memset(&mapping, 0, sizeof(mapping));
+ mapping.nvdimm = nvdimm;
+ mapping.start = 0;
+ mapping.size = resource_size(res) - LABEL_AREA_SIZE;
+
+ memset(&ndr_desc, 0, sizeof(ndr_desc));
+ ndr_desc.res = res;
+ ndr_desc.numa_node = numa_map_to_online_node(nid);
+ ndr_desc.target_node = nid;
+ ndr_desc.num_mappings = 1;
+ ndr_desc.mapping = &mapping;
+ ndr_desc.nd_set = nd_set;
+
+ if (!nvdimm_pmem_region_create(nvdimm_bus, &ndr_desc))
+ goto err_free_nd_set;
+
+ return 0;
+
+err_free_nd_set:
+ kfree(nd_set);
+ return -ENXIO;
+}
+
+static int ramdax_register_dimm(struct resource *res, void *data)
+{
+ resource_size_t start = res->start;
+ resource_size_t size = resource_size(res);
+ unsigned long flags = 0, cmd_mask = 0;
+ struct nvdimm_bus *nvdimm_bus = data;
+ struct ramdax_dimm *dimm;
+ int err;
+
+ dimm = kzalloc(sizeof(*dimm), GFP_KERNEL);
+ if (!dimm)
+ return -ENOMEM;
+
+ dimm->label_area = memremap(start + size - LABEL_AREA_SIZE,
+ LABEL_AREA_SIZE, MEMREMAP_WB);
+ if (!dimm->label_area) {
+ err = -ENOMEM;
+ goto err_free_dimm;
+ }
+
+ set_bit(NDD_LABELING, &flags);
+ set_bit(NDD_REGISTER_SYNC, &flags);
+ set_bit(ND_CMD_GET_CONFIG_SIZE, &cmd_mask);
+ set_bit(ND_CMD_GET_CONFIG_DATA, &cmd_mask);
+ set_bit(ND_CMD_SET_CONFIG_DATA, &cmd_mask);
+ dimm->nvdimm = nvdimm_create(nvdimm_bus, dimm,
+ /* dimm_attribute_groups */ NULL,
+ flags, cmd_mask, 0, NULL);
+ if (!dimm->nvdimm) {
+ err = -ENOMEM;
+ goto err_unmap_label;
+ }
+
+ err = ramdax_register_region(res, dimm->nvdimm, nvdimm_bus);
+ if (err)
+ goto err_remove_nvdimm;
+
+ return 0;
+
+err_remove_nvdimm:
+ nvdimm_delete(dimm->nvdimm);
+err_unmap_label:
+ memunmap(dimm->label_area);
+err_free_dimm:
+ kfree(dimm);
+ return err;
+}
+
+static int ramdax_get_config_size(struct nvdimm *nvdimm, int buf_len,
+ struct nd_cmd_get_config_size *cmd)
+{
+ if (sizeof(*cmd) > buf_len)
+ return -EINVAL;
+
+ *cmd = (struct nd_cmd_get_config_size){
+ .status = 0,
+ .config_size = LABEL_AREA_SIZE,
+ .max_xfer = 8,
+ };
+
+ return 0;
+}
+
+static int ramdax_get_config_data(struct nvdimm *nvdimm, int buf_len,
+ struct nd_cmd_get_config_data_hdr *cmd)
+{
+ struct ramdax_dimm *dimm = nvdimm_provider_data(nvdimm);
+
+ if (sizeof(*cmd) > buf_len)
+ return -EINVAL;
+ if (struct_size(cmd, out_buf, cmd->in_length) > buf_len)
+ return -EINVAL;
+ if (cmd->in_offset + cmd->in_length > LABEL_AREA_SIZE)
+ return -EINVAL;
+
+ memcpy(cmd->out_buf, dimm->label_area + cmd->in_offset, cmd->in_length);
+
+ return 0;
+}
+
+static int ramdax_set_config_data(struct nvdimm *nvdimm, int buf_len,
+ struct nd_cmd_set_config_hdr *cmd)
+{
+ struct ramdax_dimm *dimm = nvdimm_provider_data(nvdimm);
+
+ if (sizeof(*cmd) > buf_len)
+ return -EINVAL;
+ if (struct_size(cmd, in_buf, cmd->in_length) > buf_len)
+ return -EINVAL;
+ if (cmd->in_offset + cmd->in_length > LABEL_AREA_SIZE)
+ return -EINVAL;
+
+ memcpy(dimm->label_area + cmd->in_offset, cmd->in_buf, cmd->in_length);
+
+ return 0;
+}
+
+static int ramdax_nvdimm_ctl(struct nvdimm *nvdimm, unsigned int cmd,
+ void *buf, unsigned int buf_len)
+{
+ unsigned long cmd_mask = nvdimm_cmd_mask(nvdimm);
+
+ if (!test_bit(cmd, &cmd_mask))
+ return -ENOTTY;
+
+ switch (cmd) {
+ case ND_CMD_GET_CONFIG_SIZE:
+ return ramdax_get_config_size(nvdimm, buf_len, buf);
+ case ND_CMD_GET_CONFIG_DATA:
+ return ramdax_get_config_data(nvdimm, buf_len, buf);
+ case ND_CMD_SET_CONFIG_DATA:
+ return ramdax_set_config_data(nvdimm, buf_len, buf);
+ default:
+ return -ENOTTY;
+ }
+}
+
+static int ramdax_ctl(struct nvdimm_bus_descriptor *nd_desc,
+ struct nvdimm *nvdimm, unsigned int cmd, void *buf,
+ unsigned int buf_len, int *cmd_rc)
+{
+ /*
+ * No firmware response to translate, let the transport error
+ * code take precedence.
+ */
+ *cmd_rc = 0;
+
+ if (!nvdimm)
+ return -ENOTTY;
+ return ramdax_nvdimm_ctl(nvdimm, cmd, buf, buf_len);
+}
+
+static int ramdax_probe_of(struct platform_device *pdev,
+ struct nvdimm_bus *bus, struct device_node *np)
+{
+ int err;
+
+ for (int i = 0; i < pdev->num_resources; i++) {
+ err = ramdax_register_dimm(&pdev->resource[i], bus);
+ if (err)
+ goto err_unregister;
+ }
+
+ return 0;
+
+err_unregister:
+ /*
+ * FIXME: should we unregister the dimms that were registered
+ * successfully
+ */
+ return err;
+}
+
+static int ramdax_probe(struct platform_device *pdev)
+{
+ static struct nvdimm_bus_descriptor nd_desc;
+ struct device *dev = &pdev->dev;
+ struct nvdimm_bus *nvdimm_bus;
+ struct device_node *np;
+ int rc = -ENXIO;
+
+ nd_desc.provider_name = "ramdax";
+ nd_desc.module = THIS_MODULE;
+ nd_desc.ndctl = ramdax_ctl;
+ nvdimm_bus = nvdimm_bus_register(dev, &nd_desc);
+ if (!nvdimm_bus)
+ goto err;
+
+ np = dev_of_node(&pdev->dev);
+ if (np)
+ rc = ramdax_probe_of(pdev, nvdimm_bus, np);
+ else
+ rc = walk_iomem_res_desc(IORES_DESC_PERSISTENT_MEMORY_LEGACY,
+ IORESOURCE_MEM, 0, -1, nvdimm_bus,
+ ramdax_register_dimm);
+ if (rc)
+ goto err;
+
+ platform_set_drvdata(pdev, nvdimm_bus);
+
+ return 0;
+err:
+ nvdimm_bus_unregister(nvdimm_bus);
+ return rc;
+}
+
+static struct platform_driver ramdax_driver = {
+ .probe = ramdax_probe,
+ .remove = ramdax_remove,
+ .driver = {
+ .name = "ramdax",
+ },
+};
+
+module_platform_driver(ramdax_driver);
+
+MODULE_DESCRIPTION("NVDIMM support for e820 type-12 memory and OF pmem-region");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Microsoft Corporation");
--
2.50.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] nvdimm: allow exposing RAM carveouts as NVDIMM DIMM devices
2025-10-15 8:00 ` [PATCH v2 1/1] nvdimm: allow exposing RAM carveouts as NVDIMM DIMM devices Mike Rapoport
@ 2025-10-18 0:08 ` dan.j.williams
2025-10-22 14:47 ` Mike Rapoport
0 siblings, 1 reply; 6+ messages in thread
From: dan.j.williams @ 2025-10-18 0:08 UTC (permalink / raw)
To: Mike Rapoport, Dan Williams, Dave Jiang, Ira Weiny, Vishal Verma
Cc: jane.chu, Michał Cłapiński, Mike Rapoport,
Pasha Tatashin, Tyler Hicks, linux-kernel, nvdimm
Mike Rapoport wrote:
> From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>
>
> There are use cases, for example virtual machine hosts, that create
> "persistent" memory regions using memmap= option on x86 or dummy
> pmem-region device tree nodes on DT based systems.
>
> Both these options are inflexible because they create static regions and
> the layout of the "persistent" memory cannot be adjusted without reboot
> and sometimes they even require firmware update.
>
> Add a ramdax driver that allows creation of DIMM devices on top of
> E820_TYPE_PRAM regions and devicetree pmem-region nodes.
>
> The DIMMs support label space management on the "device" and provide a
> flexible way to access RAM using fsdax and devdax.
>
> Signed-off-by: Mike Rapoport (Mircosoft) <rppt@kernel.org>
> ---
> drivers/nvdimm/Kconfig | 17 +++
> drivers/nvdimm/Makefile | 1 +
> drivers/nvdimm/ramdax.c | 272 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 290 insertions(+)
> create mode 100644 drivers/nvdimm/ramdax.c
>
> diff --git a/drivers/nvdimm/Kconfig b/drivers/nvdimm/Kconfig
> index fde3e17c836c..9ac96a7cd773 100644
> --- a/drivers/nvdimm/Kconfig
> +++ b/drivers/nvdimm/Kconfig
> @@ -97,6 +97,23 @@ config OF_PMEM
>
> Select Y if unsure.
>
> +config RAMDAX
> + tristate "Support persistent memory interfaces on RAM carveouts"
> + depends on OF || X86
I see no compile time dependency for CONFIG_OF. The one call to
dev_of_node() looks like it still builds in the CONFIG_OF=n case. For
CONFIG_X86 the situation is different because the kernel needs
infrastructure to build the device.
So maybe change the dependency to drop OF and make it:
depends on X86_PMEM_LEGACY if X86
> + select X86_PMEM_LEGACY_DEVICE
...and drop this select.
> + default LIBNVDIMM
> + help
> + Allows creation of DAX devices on RAM carveouts.
> +
> + Memory ranges that are manually specified by the
> + 'memmap=nn[KMG]!ss[KMG]' kernel command line or defined by dummy
> + pmem-region device tree nodes would be managed by this driver as DIMM
> + devices with support for dynamic layout of namespaces.
> + The driver can be bound to e820_pmem or pmem-region platform
> + devices using 'driver_override' device attribute.
Maybe some notes for details like:
* 128K stolen at the end of the memmap range
* supports 509 namespaces (see 'ndctl create-namespace --help')
* must be force bound via driver_override
[..]
> +static int ramdax_probe(struct platform_device *pdev)
> +{
> + static struct nvdimm_bus_descriptor nd_desc;
> + struct device *dev = &pdev->dev;
> + struct nvdimm_bus *nvdimm_bus;
> + struct device_node *np;
> + int rc = -ENXIO;
> +
> + nd_desc.provider_name = "ramdax";
> + nd_desc.module = THIS_MODULE;
> + nd_desc.ndctl = ramdax_ctl;
> + nvdimm_bus = nvdimm_bus_register(dev, &nd_desc);
> + if (!nvdimm_bus)
> + goto err;
> +
> + np = dev_of_node(&pdev->dev);
> + if (np)
> + rc = ramdax_probe_of(pdev, nvdimm_bus, np);
Hmm, I do not see any confirmation that this node is actually a
"pmem-region". If you attach the kernel to the wrong device I think you
get fireworks that could be avoided with a manual of_match_node() check
of the same device_id list as the of_pmem driver.
That still would not require a "depends on OF" given of_match_node()
compiles away in the CONFIG_OF=n case.
[..]
This looks good to me. With the above comments addressed you can add:
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] nvdimm: allow exposing RAM carveouts as NVDIMM DIMM devices
2025-10-18 0:08 ` dan.j.williams
@ 2025-10-22 14:47 ` Mike Rapoport
2025-10-22 23:29 ` dan.j.williams
0 siblings, 1 reply; 6+ messages in thread
From: Mike Rapoport @ 2025-10-22 14:47 UTC (permalink / raw)
To: dan.j.williams
Cc: Dave Jiang, Ira Weiny, Vishal Verma, jane.chu,
Michał Cłapiński, Pasha Tatashin, Tyler Hicks,
linux-kernel, nvdimm
On Fri, Oct 17, 2025 at 05:08:11PM -0700, dan.j.williams@intel.com wrote:
> Mike Rapoport wrote:
> > From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>
> >
> > There are use cases, for example virtual machine hosts, that create
> > "persistent" memory regions using memmap= option on x86 or dummy
> > pmem-region device tree nodes on DT based systems.
> >
> > Both these options are inflexible because they create static regions and
> > the layout of the "persistent" memory cannot be adjusted without reboot
> > and sometimes they even require firmware update.
> >
> > Add a ramdax driver that allows creation of DIMM devices on top of
> > E820_TYPE_PRAM regions and devicetree pmem-region nodes.
> >
> > The DIMMs support label space management on the "device" and provide a
> > flexible way to access RAM using fsdax and devdax.
> >
> > Signed-off-by: Mike Rapoport (Mircosoft) <rppt@kernel.org>
> > ---
> > drivers/nvdimm/Kconfig | 17 +++
> > drivers/nvdimm/Makefile | 1 +
> > drivers/nvdimm/ramdax.c | 272 ++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 290 insertions(+)
> > create mode 100644 drivers/nvdimm/ramdax.c
> >
> > diff --git a/drivers/nvdimm/Kconfig b/drivers/nvdimm/Kconfig
> > index fde3e17c836c..9ac96a7cd773 100644
> > --- a/drivers/nvdimm/Kconfig
> > +++ b/drivers/nvdimm/Kconfig
> > @@ -97,6 +97,23 @@ config OF_PMEM
> >
> > Select Y if unsure.
> >
> > +config RAMDAX
> > + tristate "Support persistent memory interfaces on RAM carveouts"
> > + depends on OF || X86
>
> I see no compile time dependency for CONFIG_OF. The one call to
> dev_of_node() looks like it still builds in the CONFIG_OF=n case. For
> CONFIG_X86 the situation is different because the kernel needs
> infrastructure to build the device.
>
> So maybe change the dependency to drop OF and make it:
>
> depends on X86_PMEM_LEGACY if X86
We can't put if in a depends statement :(
My intention with "depends on OF || X86" was that if it's not really
possible to use this driver if it's not X86 or OF because there's nothing
to define a platform device for ramdax to bind.
Maybe what we actually need is
select X86_PMEM_LEGACY_DEVICE if X86
default n
so that it could be only explicitly enabled in the configuration and if it
is, it will also enable X86_PMEM_LEGACY_DEVICE on x86.
With default set to no it won't be build "accidentailly", but OTOH cloud
providers can disable X86_PMEM_LEGACY and enable RAMDAX and distros can
build them as modules on x86 and architectures that support OF.
What do you think?
> > + select X86_PMEM_LEGACY_DEVICE
>
> ...and drop this select.
>
> > + default LIBNVDIMM
> > + help
> > + Allows creation of DAX devices on RAM carveouts.
> > +
> > + Memory ranges that are manually specified by the
> > + 'memmap=nn[KMG]!ss[KMG]' kernel command line or defined by dummy
> > + pmem-region device tree nodes would be managed by this driver as DIMM
> > + devices with support for dynamic layout of namespaces.
> > + The driver can be bound to e820_pmem or pmem-region platform
> > + devices using 'driver_override' device attribute.
>
> Maybe some notes for details like:
>
> * 128K stolen at the end of the memmap range
> * supports 509 namespaces (see 'ndctl create-namespace --help')
> * must be force bound via driver_override
Sure.
> [..]
> > +static int ramdax_probe(struct platform_device *pdev)
> > +{
> > + static struct nvdimm_bus_descriptor nd_desc;
> > + struct device *dev = &pdev->dev;
> > + struct nvdimm_bus *nvdimm_bus;
> > + struct device_node *np;
> > + int rc = -ENXIO;
> > +
> > + nd_desc.provider_name = "ramdax";
> > + nd_desc.module = THIS_MODULE;
> > + nd_desc.ndctl = ramdax_ctl;
> > + nvdimm_bus = nvdimm_bus_register(dev, &nd_desc);
> > + if (!nvdimm_bus)
> > + goto err;
> > +
> > + np = dev_of_node(&pdev->dev);
> > + if (np)
> > + rc = ramdax_probe_of(pdev, nvdimm_bus, np);
>
> Hmm, I do not see any confirmation that this node is actually a
> "pmem-region". If you attach the kernel to the wrong device I think you
> get fireworks that could be avoided with a manual of_match_node() check
> of the same device_id list as the of_pmem driver.
>
> That still would not require a "depends on OF" given of_match_node()
> compiles away in the CONFIG_OF=n case.
With how driver_override is implemented it's possible to get fireworks with
any platform device :)
I'll add a manual check for of_match_node() to be on the safer side.
> [..]
>
> This looks good to me. With the above comments addressed you can add:
>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
--
Sincerely yours,
Mike.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] nvdimm: allow exposing RAM carveouts as NVDIMM DIMM devices
2025-10-22 14:47 ` Mike Rapoport
@ 2025-10-22 23:29 ` dan.j.williams
2025-10-23 7:39 ` Mike Rapoport
0 siblings, 1 reply; 6+ messages in thread
From: dan.j.williams @ 2025-10-22 23:29 UTC (permalink / raw)
To: Mike Rapoport, dan.j.williams
Cc: Dave Jiang, Ira Weiny, Vishal Verma, jane.chu,
Michał Cłapiński, Pasha Tatashin, Tyler Hicks,
linux-kernel, nvdimm
Mike Rapoport wrote:
[..]
> > > +config RAMDAX
> > > + tristate "Support persistent memory interfaces on RAM carveouts"
> > > + depends on OF || X86
> >
> > I see no compile time dependency for CONFIG_OF. The one call to
> > dev_of_node() looks like it still builds in the CONFIG_OF=n case. For
> > CONFIG_X86 the situation is different because the kernel needs
> > infrastructure to build the device.
> >
> > So maybe change the dependency to drop OF and make it:
> >
> > depends on X86_PMEM_LEGACY if X86
>
> We can't put if in a depends statement :(
Ugh, yeah, whoops.
> My intention with "depends on OF || X86" was that if it's not really
> possible to use this driver if it's not X86 or OF because there's nothing
> to define a platform device for ramdax to bind.
>
> Maybe what we actually need is
>
> select X86_PMEM_LEGACY_DEVICE if X86
> default n
> so that it could be only explicitly enabled in the configuration and if it
> is, it will also enable X86_PMEM_LEGACY_DEVICE on x86.
> With default set to no it won't be build "accidentailly", but OTOH cloud
> providers can disable X86_PMEM_LEGACY and enable RAMDAX and distros can
> build them as modules on x86 and architectures that support OF.
>
> What do you think?
Perhaps:
depends on X86_PMEM_LEGACY || OF || COMPILE_TEST
...because it is awkward to select symbols that has dependencies that
may be missing, and it shows that this driver has no compile time
dependencies on those symbols.
[..]
> With how driver_override is implemented it's possible to get fireworks with
> any platform device :)
True.
> I'll add a manual check for of_match_node() to be on the safer side.
Sounds good.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] nvdimm: allow exposing RAM carveouts as NVDIMM DIMM devices
2025-10-22 23:29 ` dan.j.williams
@ 2025-10-23 7:39 ` Mike Rapoport
0 siblings, 0 replies; 6+ messages in thread
From: Mike Rapoport @ 2025-10-23 7:39 UTC (permalink / raw)
To: dan.j.williams
Cc: Dave Jiang, Ira Weiny, Vishal Verma, jane.chu,
Michał Cłapiński, Pasha Tatashin, Tyler Hicks,
linux-kernel, nvdimm
On Wed, Oct 22, 2025 at 04:29:23PM -0700, dan.j.williams@intel.com wrote:
> Mike Rapoport wrote:
> [..]
> > > > +config RAMDAX
> > > > + tristate "Support persistent memory interfaces on RAM carveouts"
> > > > + depends on OF || X86
> > >
> > > I see no compile time dependency for CONFIG_OF. The one call to
> > > dev_of_node() looks like it still builds in the CONFIG_OF=n case. For
> > > CONFIG_X86 the situation is different because the kernel needs
> > > infrastructure to build the device.
> > >
> > > So maybe change the dependency to drop OF and make it:
> > >
> > > depends on X86_PMEM_LEGACY if X86
> >
> > We can't put if in a depends statement :(
>
> Ugh, yeah, whoops.
>
> > My intention with "depends on OF || X86" was that if it's not really
> > possible to use this driver if it's not X86 or OF because there's nothing
> > to define a platform device for ramdax to bind.
> >
> > Maybe what we actually need is
> >
> > select X86_PMEM_LEGACY_DEVICE if X86
> > default n
> > so that it could be only explicitly enabled in the configuration and if it
> > is, it will also enable X86_PMEM_LEGACY_DEVICE on x86.
> > With default set to no it won't be build "accidentailly", but OTOH cloud
> > providers can disable X86_PMEM_LEGACY and enable RAMDAX and distros can
> > build them as modules on x86 and architectures that support OF.
> >
> > What do you think?
>
> Perhaps:
>
> depends on X86_PMEM_LEGACY || OF || COMPILE_TEST
Works for me :)
> ...because it is awkward to select symbols that has dependencies that
> may be missing, and it shows that this driver has no compile time
> dependencies on those symbols.
--
Sincerely yours,
Mike.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-10-23 7:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-15 8:00 [PATCH v2 0/1] nvdimm: allow exposing RAM as libnvdimm DIMMs Mike Rapoport
2025-10-15 8:00 ` [PATCH v2 1/1] nvdimm: allow exposing RAM carveouts as NVDIMM DIMM devices Mike Rapoport
2025-10-18 0:08 ` dan.j.williams
2025-10-22 14:47 ` Mike Rapoport
2025-10-22 23:29 ` dan.j.williams
2025-10-23 7:39 ` Mike Rapoport
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox