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/
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 h...@lst.de 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/
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 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 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 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 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 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 asm/e820.h. 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 h...@lst.de 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 asm/e820.h. 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 8:58 AM, Christoph Hellwig h...@lst.de 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 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/
