[RFC PATCH 5/7] libnvdimm, e820: make CONFIG_X86_PMEM_LEGACY a tristate option
Purely for ease of testing, with this in place we can run the unit test
alongside any tests that depend on the memmap=ss!nn kernel parameter.
The unit test mocking implementation requires that libnvdimm be a module
and not built-in.
A nice side effect is the implementation is a bit more generic as it no
longer depends on .
Cc: Christoph Hellwig
Signed-off-by: Dan Williams
---
arch/x86/Kconfig |6 ++-
arch/x86/include/uapi/asm/e820.h |2 -
arch/x86/kernel/Makefile |2 -
arch/x86/kernel/pmem.c | 79 ---
drivers/nvdimm/Makefile |3 +
drivers/nvdimm/e820.c| 86 ++
tools/testing/nvdimm/Kbuild |4 ++
7 files changed, 108 insertions(+), 74 deletions(-)
create mode 100644 drivers/nvdimm/e820.c
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 64829b17980b..5d6994f62c4d 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1439,10 +1439,14 @@ config ILLEGAL_POINTER_VALUE
source "mm/Kconfig"
+config X86_PMEM_LEGACY_DEVICE
+ bool
+
config X86_PMEM_LEGACY
- bool "Support non-standard NVDIMMs and ADR protected memory"
+ tristate "Support non-standard NVDIMMs and ADR protected memory"
depends on PHYS_ADDR_T_64BIT
depends on BLK_DEV
+ select X86_PMEM_LEGACY_DEVICE
select LIBNVDIMM
help
Treat memory marked using the non-standard e820 type of 12 as used
diff --git a/arch/x86/include/uapi/asm/e820.h b/arch/x86/include/uapi/asm/e820.h
index 0f457e6eab18..9dafe59cf6e2 100644
--- a/arch/x86/include/uapi/asm/e820.h
+++ b/arch/x86/include/uapi/asm/e820.h
@@ -37,7 +37,7 @@
/*
* This is a non-standardized way to represent ADR or NVDIMM regions that
* persist over a reboot. The kernel will ignore their special capabilities
- * unless the CONFIG_X86_PMEM_LEGACY=y option is set.
+ * unless the CONFIG_X86_PMEM_LEGACY option is set.
*
* ( Note that older platforms also used 6 for the same type of memory,
* but newer versions switched to 12 as 6 was assigned differently. Some
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 0f15af41bd80..ac2bb7e28ba2 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -92,7 +92,7 @@ obj-$(CONFIG_KVM_GUEST) += kvm.o kvmclock.o
obj-$(CONFIG_PARAVIRT) += paravirt.o paravirt_patch_$(BITS).o
obj-$(CONFIG_PARAVIRT_SPINLOCKS)+= paravirt-spinlocks.o
obj-$(CONFIG_PARAVIRT_CLOCK) += pvclock.o
-obj-$(CONFIG_X86_PMEM_LEGACY) += pmem.o
+obj-$(CONFIG_X86_PMEM_LEGACY_DEVICE) += pmem.o
obj-$(CONFIG_PCSPKR_PLATFORM) += pcspeaker.o
diff --git a/arch/x86/kernel/pmem.c b/arch/x86/kernel/pmem.c
index 64f90f53bb85..4f00b63d7ff3 100644
--- a/arch/x86/kernel/pmem.c
+++ b/arch/x86/kernel/pmem.c
@@ -3,80 +3,17 @@
* Copyright (c) 2015, Intel Corporation.
*/
#include
-#include
#include
-#include
-
-static void e820_pmem_release(struct device *dev)
-{
- struct nvdimm_bus *nvdimm_bus = dev->platform_data;
-
- if (nvdimm_bus)
- nvdimm_bus_unregister(nvdimm_bus);
-}
-
-static struct platform_device e820_pmem = {
- .name = "e820_pmem",
- .id = -1,
- .dev = {
- .release = e820_pmem_release,
- },
-};
-
-static const struct attribute_group *e820_pmem_attribute_groups[] = {
- &nvdimm_bus_attribute_group,
- NULL,
-};
-
-static const struct attribute_group *e820_pmem_region_attribute_groups[] = {
- &nd_region_attribute_group,
- &nd_device_attribute_group,
- NULL,
-};
static __init int register_e820_pmem(void)
{
- static struct nvdimm_bus_descriptor nd_desc;
- struct device *dev = &e820_pmem.dev;
- struct nvdimm_bus *nvdimm_bus;
- int rc, i;
-
- rc = platform_device_register(&e820_pmem);
- if (rc)
- return rc;
-
- nd_desc.attr_groups = e820_pmem_attribute_groups;
- nd_desc.provider_name = "e820";
- nvdimm_bus = nvdimm_bus_register(dev, &nd_desc);
- if (!nvdimm_bus)
- goto err;
- dev->platform_data = nvdimm_bus;
-
- for (i = 0; i < e820.nr_map; i++) {
- struct e820entry *ei = &e820.map[i];
- struct resource res = {
- .flags = IORESOURCE_MEM,
- .start = ei->addr,
- .end= ei->addr + ei->size - 1,
- };
- struct nd_region_desc ndr_desc;
-
- if (ei->type != E820_PRAM)
- continue;
-
- memset(&ndr_desc, 0, sizeof(ndr_desc));
- ndr_desc.res = &res;
- ndr_desc.attr_groups = e820_pmem_region_attribute_groups;
- ndr_desc.numa_node = NUMA_NO_NODE;
- if (!nvdimm_pmem_region_create(nvdimm_bus, &ndr_desc))
- goto err;
- }
-
- return 0;
-
- err:
- dev_err(dev, "f
Re: [RFC PATCH 5/7] libnvdimm, e820: make CONFIG_X86_PMEM_LEGACY a tristate option
On Wed, Aug 12, 2015 at 11:50:29PM -0400, Dan Williams wrote: > Purely for ease of testing, with this in place we can run the unit test > alongside any tests that depend on the memmap=ss!nn kernel parameter. > The unit test mocking implementation requires that libnvdimm be a module > and not built-in. > > A nice side effect is the implementation is a bit more generic as it no > longer depends on . I really don't like this artifical split, and I also don't like how your weird "unit tests" force even more ugliness on the kernel. Almost reminds of the python projects spending more effort on getting their class mockable than actually producing results.. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 5/7] libnvdimm, e820: make CONFIG_X86_PMEM_LEGACY a tristate option
On Sat, Aug 15, 2015 at 2:06 AM, Christoph Hellwig wrote: > On Wed, Aug 12, 2015 at 11:50:29PM -0400, Dan Williams wrote: >> Purely for ease of testing, with this in place we can run the unit test >> alongside any tests that depend on the memmap=ss!nn kernel parameter. >> The unit test mocking implementation requires that libnvdimm be a module >> and not built-in. >> >> A nice side effect is the implementation is a bit more generic as it no >> longer depends on . > > I really don't like this artifical split, and I also don't like how > your weird "unit tests" force even more ugliness on the kernel. Almost > reminds of the python projects spending more effort on getting their > class mockable than actually producing results.. Well, the minute you see a 'struct DeviceFactory' appear in the kernel source you can delete all the unit tests and come take away my keyboard. Until then can we please push probing platform resources to a device driver ->probe() method where it belongs? Also given the type-7 type-12 confusion I'm just waiting for some firmware to describe persistent memory with type-12 at the e820 level and expect an ACPI-NFIT to be able to sub-divide it. In that case you'd want to blacklist either 'nd_e820.ko' or 'nfit.ko' to resolve the conflict. I'm not grokking the argument against allowing this functionality to be modular. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 5/7] libnvdimm, e820: make CONFIG_X86_PMEM_LEGACY a tristate option
On Sat, Aug 15, 2015 at 08:28:35AM -0700, Dan Williams wrote: > I'm not grokking the argument against allowing this functionality to > be modular. You're adding a another layer of platform_devices just to make a tivially small piece of code modular so that you can hook into it. I don't think that's a good reason, and neither is the after thought of preventing potentially future buggy firmware. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 5/7] libnvdimm, e820: make CONFIG_X86_PMEM_LEGACY a tristate option
On Sat, Aug 15, 2015 at 8:58 AM, Christoph Hellwig wrote: > On Sat, Aug 15, 2015 at 08:28:35AM -0700, Dan Williams wrote: >> I'm not grokking the argument against allowing this functionality to >> be modular. > > You're adding a another layer of platform_devices just to make a tivially > small piece of code modular so that you can hook into it. I don't think > that's a good reason, and neither is the after thought of preventing > potentially future buggy firmware. What other layer? /sys/devices/platform/e820_pmem is that exact same device we had before this patch. We just have a proper driver for it now. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 5/7] libnvdimm, e820: make CONFIG_X86_PMEM_LEGACY a tristate option
On Sat, Aug 15, 2015 at 09:04:02AM -0700, Dan Williams wrote: > What other layer? /sys/devices/platform/e820_pmem is that exact same > device we had before this patch. We just have a proper driver for it > now. We're adding another layer of indirection between the old e820 file and the new module. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 5/7] libnvdimm, e820: make CONFIG_X86_PMEM_LEGACY a tristate option
On Mon, Aug 17, 2015 at 8:01 AM, Christoph Hellwig wrote: > On Sat, Aug 15, 2015 at 09:04:02AM -0700, Dan Williams wrote: >> What other layer? /sys/devices/platform/e820_pmem is that exact same >> device we had before this patch. We just have a proper driver for it >> now. > > We're adding another layer of indirection between the old e820 file > and the new module. Ok, yes, I was confused by "another layer of platform_devices". That said here are the non-unit-test related reasons for this change that I would include in a new changelog: --- We currently register a platform device for e820 type-12 memory and register a nvdimm bus beneath it. Registering the platform device triggers the device-core machinery to probe for a driver, but that search currently comes up empty. Building the nvdimm-bus registration into the e820_pmem platform device registration in this way forces libnvdimm to be built-in. Instead, convert the built-in portion of CONFIG_X86_PMEM_LEGACY to simply register a platform device and move the rest of the logic to the driver for e820_pmem, for the following reasons: 1/ Letting libnvdimm be a module allows building and testing libnvdimm.ko changes without rebooting 2/ All the normal policy around modules can be applied to e820_pmem (unbind to disable and/or blacklisting the module from loading by default) 3/ Moving the driver to a generic location and converting it to scan "iomem_resource" rather than "e820.map" means any other architecture can take advantage of this simple nvdimm resource discovery mechanism by registering a resource named "Persistent Memory (legacy)" -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
