Re: [PATCH v3 3/3] dt-bindings: pmem-region: Document memory-region
On Sun, Feb 23, 2020 at 06:10:29PM -0800, Alistair Delva wrote:
> From: Kenny Root
>
> Add documentation and example for memory-region in pmem.
>
> Signed-off-by: Kenny Root
> Signed-off-by: Alistair Delva
> Cc: "Oliver O'Halloran"
> Cc: Rob Herring
> Cc: Dan Williams
> Cc: Vishal Verma
> Cc: Dave Jiang
> Cc: Ira Weiny
> Cc: devicet...@vger.kernel.org
> Cc: linux-nvdimm@lists.01.org
> Cc: kernel-t...@android.com
> ---
> [v3: adelva: remove duplicate "From:"]
> .../devicetree/bindings/pmem/pmem-region.txt | 29 +++
> 1 file changed, 29 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pmem/pmem-region.txt
> b/Documentation/devicetree/bindings/pmem/pmem-region.txt
> index 5cfa4f016a00..0ec87bd034e0 100644
> --- a/Documentation/devicetree/bindings/pmem/pmem-region.txt
> +++ b/Documentation/devicetree/bindings/pmem/pmem-region.txt
> @@ -29,6 +29,18 @@ Required properties:
> in a separate device node. Having multiple address ranges in a
> node implies no special relationship between the two ranges.
>
> + This property may be replaced or supplemented with a
> + memory-region property. Only one of reg or memory-region
> + properties is required.
> +
> + - memory-region:
> + Reference to the reserved memory node. The reserved memory
> + node should be defined as per the bindings in
> + reserved-memory.txt
Though we've never enforced it, but /reserved-memory should be within
the bounds of /memory node(s). Is that the intent here? If so, how does
that work? Wouldn't all the memory be persistent then? Or some other
system processor is preserving the contents?
> +
> + This property may be replaced or supplemented with a reg
> + property. Only one of reg or memory-region is required.
> +
> Optional properties:
> - Any relevant NUMA assocativity properties for the target platform.
>
> @@ -63,3 +75,20 @@ Examples:
> volatile;
> };
>
> +
> + /*
> + * This example uses a reserved-memory entry instead of
> + * specifying the memory region directly in the node.
> + */
> +
> + reserved-memory {
> + pmem_1: pmem@5000 {
> + no-map;
Just add 'compatible = "pmem-region";' here and be done with it. Why add
a layer of indirection?
> + reg = <0x5000 0x1000>;
> + };
> + };
> +
> + pmem@1 {
No 'reg', so shouldn't have a unit-address here.
> + compatible = "pmem-region";
> + memory-region = <&pmem_1>;
> + };
> --
> 2.25.0.265.gbab2e86ba0-goog
>
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [RFC v3 18/19] of: unittest: split out a couple of test cases from unittest
Following up from LPC discussions... On Thu, Mar 21, 2019 at 8:30 PM Brendan Higgins wrote: > > On Thu, Mar 21, 2019 at 5:22 PM Frank Rowand wrote: > > > > On 2/27/19 7:52 PM, Brendan Higgins wrote: > > > On Wed, Feb 20, 2019 at 12:45 PM Frank Rowand > > > wrote: > > >> > > >> On 2/18/19 2:25 PM, Frank Rowand wrote: > > >>> On 2/15/19 2:56 AM, Brendan Higgins wrote: > > On Thu, Feb 14, 2019 at 6:05 PM Frank Rowand > > wrote: > > > > > > On 2/14/19 4:56 PM, Brendan Higgins wrote: > > >> On Thu, Feb 14, 2019 at 3:57 PM Frank Rowand > > >> wrote: > > >>> > > >>> On 12/5/18 3:54 PM, Brendan Higgins wrote: > > On Tue, Dec 4, 2018 at 2:58 AM Frank Rowand > > wrote: > < snip > > > >> > > >> In the base version, the order of execution of the test code requires > > >> bouncing back and forth between the test functions and the coding of > > >> of_test_find_node_by_name_cases[]. > > > > > > You shouldn't need to bounce back and forth because the order in which > > > the tests run shouldn't matter. > > > > If one can't guarantee total independence of all of the tests, with no > > side effects, then yes. But that is not my world. To make that > > guarantee, I would need to be able to run just a single test in an > > entire test run. > > > > I actually want to make side effects possible. Whether from other > > tests or from live kernel code that is accessing the live devicetree. > > Any extra stress makes me happier. > > > > I forget the exact term that has been tossed around, but to me the > > devicetree unittests are more like system validation, release tests, > > acceptance tests, and stress tests. Not unit tests in the philosophy > > of KUnit. > > Ah, I understand. I thought that they were actually trying to be unit > tests; that pretty much voids this discussion then. Integration tests > and end to end tests are valuable as long as that is actually what you > are trying to do. There's a mixture. There's a whole bunch of tests that are basically just testing various DT APIs and use a static DT. Those are all unit tests IMO. Then there's all the overlay tests Frank has added. I guess some of those are not unittests in the strictest sense. Regardless, if we're reporting test results, we should align our reporting with what will become the rest of the kernel. > > I do see the value of pure unit tests, and there are rare times that > > my devicetree use case might be better served by that approach. But > > if so, it is very easy for me to add a simple pure test when debugging. > > My general use case does not map onto this model. > > Why do you think it is rare that you would actually want unit tests? I don't. We should have a unittest (or multiple) for every single DT API call and that should be a requirement to add any new APIs. > I mean, if you don't get much code churn, then maybe it's not going to > provide you a ton of value to immediately go and write a bunch of unit > tests right now, but I can't think of a single time where it's hurt. > Unit tests, from my experience, are usually the easiest tests to > maintain, and the most helpful when I am developing. > > Maybe I need to understand your use case better. > > > > > > > >> > > >> In the frank version the order of execution of the test code is obvious. > > > > > > So I know we were arguing before over whether order *does* matter in > > > some of the other test cases (none in the example that you or I > > > posted), but wouldn't it be better if the order of execution didn't > > > matter? If you don't allow a user to depend on the execution of test > > > cases, then arguably these test case dependencies would never form and > > > the order wouldn't matter. > > > > Reality intrudes. Order does matter. > > > > > > >> > > >> It is possible that a test function could be left out of > > >> of_test_find_node_by_name_cases[], in error. This will result in a > > >> compile > > >> warning (I think warning instead of error, but I have not verified that) > > >> so it might be caught or it might be overlooked. > > >> > > >> The base version is 265 lines. The frank version is 208 lines, 57 lines > > >> less. Less is better. > > > > > > I agree that less is better, but there are different kinds of less to > > > consider. I prefer less logic in a function to fewer lines overall. > > > > > > It seems we are in agreement that test cases should be small and > > > simple, so I won't dwell on that point any longer. I agree that the > > > > As a general guide for simple unit tests, sure. > > > > For my case, no. Reality intrudes. > > > > KUnit has a nice architectural view of what a unit test should be. > > Cool, I am glad you think so! That actually means a lot to me. I was > afraid I wasn't conveying the idea properly and that was the root of > this debate. > > > > > The existing devicetree "unittests" are not such unit tests. They > > simply share the same name. > > > > The devicetree unittests do not f
Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile
On Fri, Sep 13, 2019 at 11:42 AM Joe Perches wrote: > > On Fri, 2019-09-13 at 16:46 +0100, Rob Herring wrote: > > On Fri, Sep 13, 2019 at 4:00 PM Randy Dunlap wrote: > > > On 9/13/19 4:48 AM, Dan Carpenter wrote: > > > > > > > > So I'm expecting to take this kind of stuff into Documentation/. My > > > > > own > > > > > personal hope is that it can maybe serve to shame some of these "local > > > > > quirks" out of existence. The evidence from this brief discussion > > > > > suggests > > > > > that this might indeed happen. > > > > > > > > I don't think it's shaming, I think it's validating. Everyone just > > > > insists that since it's written in the Book of Rules then it's our fault > > > > for not reading it. It's like those EULA things where there is more > > > > text than anyone can physically read in a life time. > > > > > > Yes, agreed. > > > > > > > And the documentation doesn't help. For example, I knew people's rules > > > > about capitalizing the subject but I'd just forget. I say that if you > > > > can't be bothered to add it to checkpatch then it means you don't really > > > > care that strongly. > > > > > > If a subsystem requires a certain spelling/capitalization in patch email > > > subjects, it should be added to MAINTAINERS IMO. E.g., > > > E: NuBus > > > > +1 > > > > Better make this a regex to deal with (net|net-next). > > > > We could probably script populating MAINTAINERS with this using how it > > is done manually: git log --oneline > > I made a similar proposal nearly a decade ago to add a grammar > to MAINTAINERS sections for patch subject prefixes. > > https://lore.kernel.org/lkml/1289919077.28741.50.camel@Joe-Laptop/ Perhaps there's more support for it now. I didn't get through all the thread, but the positions seemed to range from "who cares, subjects are easy to edit" to "seems like a good idea and doesn't hurt". I probably would have implemented something, but perl (tacking on to checkpatch and having you tell me everything wrong is about all I can do :)). Rob ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile
On Fri, Sep 13, 2019 at 4:00 PM Randy Dunlap wrote: > > On 9/13/19 4:48 AM, Dan Carpenter wrote: > > >> So I'm expecting to take this kind of stuff into Documentation/. My own > >> personal hope is that it can maybe serve to shame some of these "local > >> quirks" out of existence. The evidence from this brief discussion suggests > >> that this might indeed happen. > > > > I don't think it's shaming, I think it's validating. Everyone just > > insists that since it's written in the Book of Rules then it's our fault > > for not reading it. It's like those EULA things where there is more > > text than anyone can physically read in a life time. > > Yes, agreed. > > > And the documentation doesn't help. For example, I knew people's rules > > about capitalizing the subject but I'd just forget. I say that if you > > can't be bothered to add it to checkpatch then it means you don't really > > care that strongly. > > If a subsystem requires a certain spelling/capitalization in patch email > subjects, it should be added to MAINTAINERS IMO. E.g., > E: NuBus +1 Better make this a regex to deal with (net|net-next). We could probably script populating MAINTAINERS with this using how it is done manually: git log --oneline Rob ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [Ksummit-discuss] [PATCH v2 0/3] Maintainer Entry Profiles
On Fri, Sep 13, 2019 at 3:12 PM Joe Perches wrote: > > On Thu, 2019-09-12 at 13:01 -0700, Bart Van Assche wrote: > > > Another argument in favor of W=1 is that the formatting of kernel-doc > > headers is checked only if W=1 is passed to make. > > Actually, but for the fact there are far too many > to generally enable that warning right now, > (an x86-64 defconfig has more than 1000) > that sounds pretty reasonable to me. It's in the 1000s on arm because W=1 turns on more checks in building .dts files. There are lots of duplicates as most of the dts content is as an include file (e.g. board dts includes soc dts). We could shift the dts checks to W=2 (and what's enabled by W=2 now to 3) I guess. Why not just promote what doesn't give false or still noisy warnings out of W=1 to be the default? Rob ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [RFC v3 17/19] of: unittest: migrate tests to run on KUnit
On Tue, Feb 12, 2019 at 7:44 PM Brendan Higgins
wrote:
>
> On Wed, Nov 28, 2018 at 12:56 PM Rob Herring wrote:
> >
> > On Wed, Nov 28, 2018 at 1:38 PM Brendan Higgins
> > wrote:
> > >
> > > Migrate tests without any cleanup, or modifying test logic in anyway to
> > > run under KUnit using the KUnit expectation and assertion API.
> >
> > Nice! You beat me to it. This is probably going to conflict with what
> > is in the DT tree for 4.21. Also, please Cc the DT list for
> > drivers/of/ changes.
> >
> > Looks good to me, but a few mostly formatting comments below.
>
> I just realized that we never talked about your other comments, and I
> still have some questions. (Sorry, it was the last thing I looked at
> while getting v4 ready.) No worries if you don't get to it before I
> send v4 out, I just didn't want you to think I was ignoring you.
>
> >
> > >
> > > Signed-off-by: Brendan Higgins
> > > ---
> > > drivers/of/Kconfig|1 +
> > > drivers/of/unittest.c | 1405 ++---
> > > 2 files changed, 752 insertions(+), 654 deletions(-)
> > >
>
> > > diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> > > index 41b49716ac75f..a5ef44730ffdb 100644
> > > --- a/drivers/of/unittest.c
> > > +++ b/drivers/of/unittest.c
>
> > > -
> > > -static void __init of_unittest_find_node_by_name(void)
> > > +static void of_unittest_find_node_by_name(struct kunit *test)
> >
> > Why do we have to drop __init everywhere? The tests run later?
>
> From the standpoint of a unit test __init doesn't really make any
> sense, right? I know that right now we are running as part of a
> kernel, but the goal should be that a unit test is not part of a
> kernel and we just include what we need.
Well, the test only runs during boot and better to free the space when
done with it. There was some desire to make it a kernel module and
then we'd also need to get rid of __init too.
> Even so, that's the future. For now, I did not put the KUnit
> infrastructure in the .init section because I didn't think it belonged
> there. In practice, KUnit only knows how to run during the init phase
> of the kernel, but I don't think it should be restricted there. You
> should be able to run tests whenever you want because you should be
> able to test anything right? I figured any restriction on that is
> misleading and will potentially get in the way at worst, and
> unnecessary at best especially since people shouldn't build a
> production kernel with all kinds of unit tests inside.
More folks will run things if they can be enabled on production
kernels. If size is the only issue, modules mitigate that. However,
there's probably APIs to test which we don't want to export to
modules.
I think in general, we change things in the kernel when needed, not
for something in the future. Changing __init is simple enough to do
later.
OTOH, things get copied and maybe this we don't want copied, so we can
remove it if you want to.
>
> > >
> > > -static void __init of_unittest_property_string(void)
> > > +static void of_unittest_property_string(struct kunit *test)
> > > {
> > > const char *strings[4];
> > > struct device_node *np;
> > > int rc;
> > >
> > > np =
> > > of_find_node_by_path("/testcase-data/phandle-tests/consumer-a");
> > > - if (!np) {
> > > - pr_err("No testcase data in device tree\n");
> > > - return;
> > > - }
> > > -
> > > - rc = of_property_match_string(np, "phandle-list-names", "first");
> > > - unittest(rc == 0, "first expected:0 got:%i\n", rc);
> > > - rc = of_property_match_string(np, "phandle-list-names", "second");
> > > - unittest(rc == 1, "second expected:1 got:%i\n", rc);
> > > - rc = of_property_match_string(np, "phandle-list-names", "third");
> > > - unittest(rc == 2, "third expected:2 got:%i\n", rc);
> > > - rc = of_property_match_string(np, "phandle-list-names", "fourth");
> > > - unittest(rc == -ENODATA, "unmatched string; rc=%i\n", rc);
> > > - rc = of_property_match_string(np, "missing-property", "blah");
> > > - unittest(rc == -EINVAL, "missing property; rc=%i\n", rc);
> > > - rc = of_property_match_s
Re: [RFC v3 17/19] of: unittest: migrate tests to run on KUnit
On Wed, Dec 5, 2018 at 5:43 PM Brendan Higgins wrote: > > On Tue, Dec 4, 2018 at 5:41 AM Rob Herring wrote: > > > > On Mon, Dec 3, 2018 at 6:14 PM Brendan Higgins > > wrote: > > > > > > On Thu, Nov 29, 2018 at 4:40 PM Randy Dunlap > > > wrote: > > > > > > > > On 11/28/18 12:56 PM, Rob Herring wrote: > > > > >> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig > > > > >> index ad3fcad4d75b8..f309399deac20 100644 > > > > >> --- a/drivers/of/Kconfig > > > > >> +++ b/drivers/of/Kconfig > > > > >> @@ -15,6 +15,7 @@ if OF > > > > >> config OF_UNITTEST > > > > >> bool "Device Tree runtime unit tests" > > > > >> depends on !SPARC > > > > >> + depends on KUNIT > > > > > Unless KUNIT has depends, better to be a select here. > > > > > > > > That's just style or taste. I would prefer to use depends > > > > instead of select, but that's also just my preference. > > > > > > I prefer depends too, but Rob is the maintainer here. > > > > Well, we should be consistent, not the follow the whims of each maintainer. > > Sorry, I don't think that came out the way I meant it. I don't really > think we are consistent on this point across the kernel, and I don't > feel very strongly about the point, so I was just looking to follow > the path of least resistance. (I also just assumed Rob would keep us > consistent within drivers/of/.) I meant across unittests, we should be consistent. All unittests do either "depends on KUNIT" or "select KUNIT". The question I would ask is does KUNIT need to be user visible or is useful to enable without any unittests enabled? With depends, a user has 2 options to go enable vs. 1 with select. But if you want a global kill switch to turn off all unittests, then depends works better. > I figure if we are running unit tests from the test runner script or > from an automated system, you won't be hunting for dependencies for a > single test every time you want to run a test, so select doesn't make > it easier to configure in most imagined use cases. > > KUNIT hypothetically should not depend on anything, so select should > be safe to use. > > On the other hand, if we end up being wrong on this point and KUnit > gains widespread adoption, I would prefer not to be in a position > where I have to change a bunch of configs all over the kernel because > this example got copied and pasted. You'll be so happy that 100s of tests have been created using kunit, it won't be a big deal. :) In any case, I wouldn't spend more time on this. Rob ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [RFC v3 00/19] kunit: introduce KUnit, the Linux kernel unit testing framework
On Tue, Dec 4, 2018 at 5:40 AM Frank Rowand wrote: > > Hi Brendan, Rob, > > On 11/28/18 11:36 AM, Brendan Higgins wrote: > > This patch set proposes KUnit, a lightweight unit testing and mocking > > framework for the Linux kernel. > > > > Unlike Autotest and kselftest, KUnit is a true unit testing framework; > > it does not require installing the kernel on a test machine or in a VM > > and does not require tests to be written in userspace running on a host > > kernel. Additionally, KUnit is fast: From invocation to completion KUnit > > can run several dozen tests in under a second. Currently, the entire > > KUnit test suite for KUnit runs in under a second from the initial > > invocation (build time excluded). > > > > KUnit is heavily inspired by JUnit, Python's unittest.mock, and > > Googletest/Googlemock for C++. KUnit provides facilities for defining > > unit test cases, grouping related test cases into test suites, providing > > common infrastructure for running tests, mocking, spying, and much more. > > > > ## What's so special about unit testing? > > > > A unit test is supposed to test a single unit of code in isolation, > > hence the name. There should be no dependencies outside the control of > > the test; this means no external dependencies, which makes tests orders > > of magnitudes faster. Likewise, since there are no external dependencies, > > there are no hoops to jump through to run the tests. Additionally, this > > makes unit tests deterministic: a failing unit test always indicates a > > problem. Finally, because unit tests necessarily have finer granularity, > > they are able to test all code paths easily solving the classic problem > > of difficulty in exercising error handling code. > > > > ## Is KUnit trying to replace other testing frameworks for the kernel? > > > > No. Most existing tests for the Linux kernel are end-to-end tests, which > > have their place. A well tested system has lots of unit tests, a > > reasonable number of integration tests, and some end-to-end tests. KUnit > > is just trying to address the unit test space which is currently not > > being addressed. > > > > ## More information on KUnit > > > > There is a bunch of documentation near the end of this patch set that > > describes how to use KUnit and best practices for writing unit tests. > > For convenience I am hosting the compiled docs here: > > https://google.github.io/kunit-docs/third_party/kernel/docs/ > > Additionally for convenience, I have applied these patches to a branch: > > https://kunit.googlesource.com/linux/+/kunit/rfc/4.19/v3 > > The repo may be cloned with: > > git clone https://kunit.googlesource.com/linux > > This patchset is on the kunit/rfc/4.19/v3 branch. > > > > ## Changes Since Last Version > > > > - Changed namespace prefix from `test_*` to `kunit_*` as requested by > >Shuah. > > > > - Started converting/cleaning up the device tree unittest to use KUnit. > > - Started adding KUnit expectations with custom messages. > > > > Sorry I missed your reply to me in the v1 patch thread. I've been > traveling a lot the last few weeks. I'm starting to read messages > that occurred late in the v1 patch thread and the v2 patch thread, > so I'm just coming up to speed on this. > > My comments below are motivated by adding the devicetree unittest to > this version of the patch series. > > Pulling a comment from way back in the v1 patch thread: > > On 10/17/18 3:22 PM, Brendan Higgins wrote: > > On Wed, Oct 17, 2018 at 10:49 AM wrote: > > < snip > > > > The test and the code under test are linked together in the same > > binary and are compiled under Kbuild. Right now I am linking > > everything into a UML kernel, but I would ultimately like to make > > tests compile into completely independent test binaries. So each test > > file would get compiled into its own test binary and would link > > against only the code needed to run the test, but we are a bit of a > > ways off from that. > > I have never used UML, so you should expect naive questions from me, > exhibiting my lack of understanding. > > Does this mean that I have to build a UML architecture kernel to run > the KUnit tests? In this version of the patch series, yes. > *** Rob, if the answer is yes, then it seems like for my workflow, > which is to build for real ARM hardware, my work is doubled (or > worse), because for every patch/commit that I apply, I not only have > to build the ARM kernel and boot on the real hardware to test, I also > have to build the UML kernel and boot in UML. If that is correct > then I see this as a major problem for me. I've already raised this issue elsewhere in the series. Restricting the DT tests to UML is a non-starter. > Brenden, in the above quote you said that in the future you would > like to make the "tests compile into completely independent test > binaries". I am assuming those are intended to run as standalone > user space programs instead of inside UML. Is that correct? If > so, how will
Re: [RFC v3 17/19] of: unittest: migrate tests to run on KUnit
On Mon, Dec 3, 2018 at 6:14 PM Brendan Higgins wrote: > > On Thu, Nov 29, 2018 at 4:40 PM Randy Dunlap wrote: > > > > On 11/28/18 12:56 PM, Rob Herring wrote: > > >> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig > > >> index ad3fcad4d75b8..f309399deac20 100644 > > >> --- a/drivers/of/Kconfig > > >> +++ b/drivers/of/Kconfig > > >> @@ -15,6 +15,7 @@ if OF > > >> config OF_UNITTEST > > >> bool "Device Tree runtime unit tests" > > >> depends on !SPARC > > >> + depends on KUNIT > > > Unless KUNIT has depends, better to be a select here. > > > > That's just style or taste. I would prefer to use depends > > instead of select, but that's also just my preference. > > I prefer depends too, but Rob is the maintainer here. Well, we should be consistent, not the follow the whims of each maintainer. Rob ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [RFC v3 06/19] arch: um: enable running kunit from User Mode Linux
On Thu, Nov 29, 2018 at 9:37 PM Luis Chamberlain wrote: > > On Wed, Nov 28, 2018 at 03:26:03PM -0600, Rob Herring wrote: > > On Wed, Nov 28, 2018 at 1:37 PM Brendan Higgins > > wrote: > > > > > > Make minimum number of changes outside of the KUnit directories for > > > KUnit to build and run using UML. > > > > There's nothing in this patch limiting this to UML. > > Not that one, but the abort thing segv thing is, eventually. > To support other architectures we'd need to make a wrapper to that > hack which Brendan added, and then allow each os to implement > its own call, and add an asm-generic helper. I've not looked into why this is needed, but can't you make the abort support optional and arches can select it when they support it. At least before, the DT unittests didn't need this to run and shouldn't depend on it after converting to kunit. Rob ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [RFC v3 06/19] arch: um: enable running kunit from User Mode Linux
On Wed, Nov 28, 2018 at 1:37 PM Brendan Higgins wrote: > > Make minimum number of changes outside of the KUnit directories for > KUnit to build and run using UML. There's nothing in this patch limiting this to UML. Only patch 1 does that and I would remove that depends. I'd guess most folks will want to run under something other than UML. DRM for instance (though the virtual KMS stuff may work in UML?). Plus you want to make sure this all builds with allmodconfig for x86 (or ARM) because those get the most (and quickest) compile coverage. Rob ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [RFC v3 16/19] arch: um: make UML unflatten device tree when testing
On Wed, Nov 28, 2018 at 1:38 PM Brendan Higgins wrote: > > Make UML unflatten any present device trees when running KUnit tests. > > Signed-off-by: Brendan Higgins > --- > arch/um/kernel/um_arch.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/arch/um/kernel/um_arch.c b/arch/um/kernel/um_arch.c > index a818ccef30ca2..bd58ae3bf4148 100644 > --- a/arch/um/kernel/um_arch.c > +++ b/arch/um/kernel/um_arch.c > @@ -13,6 +13,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -347,6 +348,9 @@ void __init setup_arch(char **cmdline_p) > read_initrd(); > > paging_init(); > +#if IS_ENABLED(CONFIG_OF_UNITTEST) > + unflatten_device_tree(); > +#endif Kind of strange to have this in the arch code. I'd rather have this in the unittest code if possible. Can we have an initcall conditional on CONFIG_UM in the unittest do this? Side note, use a C if with IS_ENABLED() whenever possible instead of pre-processor #if. I'll take a fix separately as it was on my todo to fix. I've got the unit tests running in a gitlab CI job now[1]. Rob [1] https://gitlab.com/robherring/linux-dt-unittest/pipelines ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [RFC v3 17/19] of: unittest: migrate tests to run on KUnit
On Wed, Nov 28, 2018 at 1:38 PM Brendan Higgins
wrote:
>
> Migrate tests without any cleanup, or modifying test logic in anyway to
> run under KUnit using the KUnit expectation and assertion API.
Nice! You beat me to it. This is probably going to conflict with what
is in the DT tree for 4.21. Also, please Cc the DT list for
drivers/of/ changes.
Looks good to me, but a few mostly formatting comments below.
>
> Signed-off-by: Brendan Higgins
> ---
> drivers/of/Kconfig|1 +
> drivers/of/unittest.c | 1405 ++---
> 2 files changed, 752 insertions(+), 654 deletions(-)
>
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index ad3fcad4d75b8..f309399deac20 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -15,6 +15,7 @@ if OF
> config OF_UNITTEST
> bool "Device Tree runtime unit tests"
> depends on !SPARC
> + depends on KUNIT
Unless KUNIT has depends, better to be a select here.
> select IRQ_DOMAIN
> select OF_EARLY_FLATTREE
> select OF_RESOLVE
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index 41b49716ac75f..a5ef44730ffdb 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -26,186 +26,187 @@
>
> #include
>
> +#include
> +
> #include "of_private.h"
>
> -static struct unittest_results {
> - int passed;
> - int failed;
> -} unittest_results;
> -
> -#define unittest(result, fmt, ...) ({ \
> - bool failed = !(result); \
> - if (failed) { \
> - unittest_results.failed++; \
> - pr_err("FAIL %s():%i " fmt, __func__, __LINE__,
> ##__VA_ARGS__); \
> - } else { \
> - unittest_results.passed++; \
> - pr_debug("pass %s():%i\n", __func__, __LINE__); \
> - } \
> - failed; \
> -})
> -
> -static void __init of_unittest_find_node_by_name(void)
> +static void of_unittest_find_node_by_name(struct kunit *test)
Why do we have to drop __init everywhere? The tests run later?
> {
> struct device_node *np;
> const char *options, *name;
>
> np = of_find_node_by_path("/testcase-data");
> name = kasprintf(GFP_KERNEL, "%pOF", np);
> - unittest(np && !strcmp("/testcase-data", name),
> - "find /testcase-data failed\n");
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
> + KUNIT_EXPECT_STREQ_MSG(test, "/testcase-data", name,
> + "find /testcase-data failed\n");
> of_node_put(np);
> kfree(name);
>
> /* Test if trailing '/' works */
> - np = of_find_node_by_path("/testcase-data/");
> - unittest(!np, "trailing '/' on /testcase-data/ should fail\n");
> + KUNIT_EXPECT_EQ_MSG(test, of_find_node_by_path("/testcase-data/"),
> NULL,
> + "trailing '/' on /testcase-data/ should fail\n");
>
> np = of_find_node_by_path("/testcase-data/phandle-tests/consumer-a");
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
> name = kasprintf(GFP_KERNEL, "%pOF", np);
> - unittest(np && !strcmp("/testcase-data/phandle-tests/consumer-a",
> name),
> - "find /testcase-data/phandle-tests/consumer-a failed\n");
> + KUNIT_EXPECT_STREQ_MSG(test,
> + "/testcase-data/phandle-tests/consumer-a",
> name,
> + "find /testcase-data/phandle-tests/consumer-a
> failed\n");
> of_node_put(np);
> kfree(name);
>
> np = of_find_node_by_path("testcase-alias");
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
> name = kasprintf(GFP_KERNEL, "%pOF", np);
> - unittest(np && !strcmp("/testcase-data", name),
> - "find testcase-alias failed\n");
> + KUNIT_EXPECT_STREQ_MSG(test, "/testcase-data", name,
> + "find testcase-alias failed\n");
> of_node_put(np);
> kfree(name);
>
> /* Test if trailing '/' works on aliases */
> - np = of_find_node_by_path("testcase-alias/");
> - unittest(!np, "trailing '/' on testcase-alias/ should fail\n");
> + KUNIT_EXPECT_EQ_MSG(test, of_find_node_by_path("testcase-alias/"),
> NULL,
> + "trailing '/' on testcase-alias/ should fail\n");
>
> np = of_find_node_by_path("testcase-alias/phandle-tests/consumer-a");
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
> name = kasprintf(GFP_KERNEL, "%pOF", np);
> - unittest(np && !strcmp("/testcase-data/phandle-tests/consumer-a",
> name),
> - "find testcase-alias/phandle-tests/consumer-a failed\n");
> + KUNIT_EXPECT_STREQ_MSG(test,
> + "/testcase-data/phandle-tests/consumer-a",
> name,
> + "find testcase-alias/phandle-tests/consumer-a
> failed\n");
> of_node_put(np);
> kfree(name);
>
> - np = of_find_node_by_path("/testcase-data/missing-path")
Re: [Ksummit-discuss] [RFC PATCH 2/3] MAINTAINERS, Handbook: Subsystem Profile
On Fri, Nov 16, 2018 at 11:57 AM Joe Perches wrote: > > On Fri, 2018-11-16 at 14:44 +0200, Jani Nikula wrote: > > I quickly cooked up this script to produce the top-5 commit prefixes for > > the given files over the arbitrary last 200 commits. It'll give you a > > pretty good idea if you're even close. > > > > --- > > #!/bin/sh > > # usage: subject-prefix FILE [...] > > # show top 5 subject prefixes for FILEs > > > > git log --format=%s -n 200 -- "$@" |\ > > grep -v "^Merge " |\ --no-merges in git log can replace this line. > > sed 's/\(.*\):.*/\1/' |\ > > sort | uniq -c | sort -nr | sed 's/ *[0-9]\+ //' |\ > > head -n 5 > > --- > > > > Someone who knows perl could turn that into a checkpatch check: See if > > the patch subject prefix is one of the top-5 for all files changed by > > the patch, and ask the user to double check if it isn't. Or some > > heuristics thereof. > > This won't work when a patch contains multiple files > from different paths, or even multiple files from a > single driver. Different paths is often, but not always a sign that patches may need to be split up. Maybe that is something checkpatch should point out. > Perhaps it's better to use a generic mechanism like > > basename $(dirname $filename): > > with some exceptions and add an override patch subject > grammar to appropriate various sections of MAINTAINERS. Perhaps just use the script as a starting point to populate MAINTAINERS as it may never be that accurate. > I also think it's better to use a separate script like > scripts/spdxcheck.py and tie any necessary checkpatch > use to that script. Yes, checkpatch is getting pretty unwieldy. Rob ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH] libnvdimm, of_pmem: use dev_to_node() instead of of_node_to_nid()
Remove the direct dependency on of_node_to_nid() by using dev_to_node()
instead. Any DT platform device will have its NUMA node id set when the
device is created.
With this, commit 291717b6fbdb ("libnvdimm, of_pmem: workaround OF_NUMA=n
build error") can be reverted.
Cc: Dan Williams
Cc: Oliver O'Halloran
Cc: linux-nvdimm@lists.01.org
Signed-off-by: Rob Herring
---
drivers/nvdimm/of_pmem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
index 85013bad35de..0a701837dfc0 100644
--- a/drivers/nvdimm/of_pmem.c
+++ b/drivers/nvdimm/of_pmem.c
@@ -67,7 +67,7 @@ static int of_pmem_region_probe(struct platform_device *pdev)
*/
memset(&ndr_desc, 0, sizeof(ndr_desc));
ndr_desc.attr_groups = region_attr_groups;
- ndr_desc.numa_node = of_node_to_nid(np);
+ ndr_desc.numa_node = dev_to_node(&pdev->dev);
ndr_desc.res = &pdev->resource[i];
ndr_desc.of_node = np;
set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
--
2.14.1
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] drivers/of: Introduce ARCH_HAS_OWN_OF_NUMA
On Mon, Apr 9, 2018 at 4:05 PM, Dan Williams wrote: > On Mon, Apr 9, 2018 at 1:52 PM, Rob Herring wrote: >> On Mon, Apr 9, 2018 at 2:46 AM, Oliver O'Halloran wrote: >>> Some OF platforms (pseries and some SPARC systems) has their own >>> implementations of NUMA affinity detection rather than using the generic >>> OF_NUMA driver, which mainly exists for arm64. For other platforms one >>> of two fallbacks provided by the base OF driver are used depending on >>> CONFIG_NUMA. >>> >>> In the CONFIG_NUMA=n case the fallback is an inline function in of.h. >>> In the =y case the fallback is a real function which is defined as a >>> weak symbol so that it may be overwritten by the architecture if desired. >>> >>> The problem with this arrangement is that the real implementations all >>> export of_node_to_nid(). Unfortunately it's not possible to export the >>> fallback since it would clash with the non-weak version. As a result >>> we get build failures when: >>> >>> a) CONFIG_NUMA=y && CONFIG_OF=y, and >>> b) The platform doesn't implement of_node_to_nid(), and >>> c) A module uses of_node_to_nid() >>> >>> Given b) will be true for most platforms this is fairly easy to hit >>> and has been observed on ia64 and x86. >> >> How specifically do we hit this? The only module I see using >> of_node_to_nid in mainline is Cell EDAC driver. > > The of_pmem driver is using it currently pending for a 4.17 pull > request. Stephen hit the compile failure in -next. I took a look at this. The correct fix here is to use dev_to_node() instead: diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c index 85013bad35de..0a701837dfc0 100644 --- a/drivers/nvdimm/of_pmem.c +++ b/drivers/nvdimm/of_pmem.c @@ -67,7 +67,7 @@ static int of_pmem_region_probe(struct platform_device *pdev) */ memset(&ndr_desc, 0, sizeof(ndr_desc)); ndr_desc.attr_groups = region_attr_groups; - ndr_desc.numa_node = of_node_to_nid(np); + ndr_desc.numa_node = dev_to_node(&pdev->dev); ndr_desc.res = &pdev->resource[i]; ndr_desc.of_node = np; set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags); And we should remove the exported symbol. I'll send a proper patch. Rob ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] drivers/of: Introduce ARCH_HAS_OWN_OF_NUMA
On Mon, Apr 9, 2018 at 4:05 PM, Dan Williams wrote: > On Mon, Apr 9, 2018 at 1:52 PM, Rob Herring wrote: >> On Mon, Apr 9, 2018 at 2:46 AM, Oliver O'Halloran wrote: >>> Some OF platforms (pseries and some SPARC systems) has their own >>> implementations of NUMA affinity detection rather than using the generic >>> OF_NUMA driver, which mainly exists for arm64. For other platforms one >>> of two fallbacks provided by the base OF driver are used depending on >>> CONFIG_NUMA. >>> >>> In the CONFIG_NUMA=n case the fallback is an inline function in of.h. >>> In the =y case the fallback is a real function which is defined as a >>> weak symbol so that it may be overwritten by the architecture if desired. >>> >>> The problem with this arrangement is that the real implementations all >>> export of_node_to_nid(). Unfortunately it's not possible to export the >>> fallback since it would clash with the non-weak version. As a result >>> we get build failures when: >>> >>> a) CONFIG_NUMA=y && CONFIG_OF=y, and >>> b) The platform doesn't implement of_node_to_nid(), and >>> c) A module uses of_node_to_nid() >>> >>> Given b) will be true for most platforms this is fairly easy to hit >>> and has been observed on ia64 and x86. >> >> How specifically do we hit this? The only module I see using >> of_node_to_nid in mainline is Cell EDAC driver. > > The of_pmem driver is using it currently pending for a 4.17 pull > request. Stephen hit the compile failure in -next. You mean the stuff reviewed last week in the middle of the merge window? Sounds like 4.18 material to me. Rob ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] drivers/of: Introduce ARCH_HAS_OWN_OF_NUMA
On Mon, Apr 9, 2018 at 2:46 AM, Oliver O'Halloran wrote: > Some OF platforms (pseries and some SPARC systems) has their own > implementations of NUMA affinity detection rather than using the generic > OF_NUMA driver, which mainly exists for arm64. For other platforms one > of two fallbacks provided by the base OF driver are used depending on > CONFIG_NUMA. > > In the CONFIG_NUMA=n case the fallback is an inline function in of.h. > In the =y case the fallback is a real function which is defined as a > weak symbol so that it may be overwritten by the architecture if desired. > > The problem with this arrangement is that the real implementations all > export of_node_to_nid(). Unfortunately it's not possible to export the > fallback since it would clash with the non-weak version. As a result > we get build failures when: > > a) CONFIG_NUMA=y && CONFIG_OF=y, and > b) The platform doesn't implement of_node_to_nid(), and > c) A module uses of_node_to_nid() > > Given b) will be true for most platforms this is fairly easy to hit > and has been observed on ia64 and x86. How specifically do we hit this? The only module I see using of_node_to_nid in mainline is Cell EDAC driver. > This patch remedies the problem by introducing the ARCH_HAS_OWN_OF_NUMA > Kconfig option which is selected if an architecture provides an > implementation of of_node_to_nid(). If a platform does not use it's own, > or the generic OF_NUMA, then always use the inline fallback in of.h so > we don't need to futz around with exports. I'm more inclined to figure out how to remove the export and provide a non DT specific function if drivers need to know this. Rob ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v3 1/3] resource: Use list_head to link resource sibling
+Nico who has been working on tinification of the kernel.
On Mon, Apr 9, 2018 at 4:08 AM, Baoquan He wrote:
> The struct resource uses singly linked list to link siblings. It's not
> easy to do reverse iteration on sibling list. So replace it with list_head.
Why is reverse iteration needed?
> And code refactoring makes codes in kernel/resource.c more readable than
> pointer operation.
resource_for_each_* helpers could solve that without the size increase.
> Besides, type of member variables of struct resource, sibling and child, are
> changed from 'struct resource *' to 'struct list_head'. Kernel size will
> increase because of those statically defined struct resource instances.
The DT struct device_node also has the same tree structure with
parent, child, sibling pointers and converting to list_head had been
on the todo list for a while. ACPI also has some tree walking
functions (drivers/acpi/acpica/pstree.c). Perhaps there should be a
common tree struct and helpers defined either on top of list_head or a
new struct if that saves some size.
>
> Signed-off-by: Baoquan He
> ---
> v2->v3:
> Make sibling() and first_child() global so that they can be called
> out of kernel/resource.c to simplify code.
These should probably be inline functions. Or exported if not.
>
> Fix several code bugs found by kbuild test robot.
>
> Got report from lkp that kernel size increased. It's on purpose since
> the type change of sibling and child inside struct resource{}. For
> each struct resource variable, it will cost another 16 bytes on x86 64.
The size increase should be mentioned in the commit log. More
generally, the size increase is 2 pointers.
Rob
>
> arch/sparc/kernel/ioport.c | 2 +-
> drivers/gpu/drm/drm_memory.c| 3 +-
> drivers/gpu/drm/gma500/gtt.c| 5 +-
> drivers/hv/vmbus_drv.c | 52
> drivers/input/joystick/iforce/iforce-main.c | 4 +-
> drivers/nvdimm/e820.c | 2 +-
> drivers/nvdimm/namespace_devs.c | 6 +-
> drivers/nvdimm/nd.h | 5 +-
> drivers/of/address.c| 4 +-
> drivers/parisc/lba_pci.c| 4 +-
> drivers/pci/host/vmd.c | 8 +-
> drivers/pci/probe.c | 2 +
> drivers/pci/setup-bus.c | 2 +-
> include/linux/ioport.h | 6 +-
> kernel/resource.c | 193
> ++--
> 15 files changed, 149 insertions(+), 149 deletions(-)
>
> diff --git a/arch/sparc/kernel/ioport.c b/arch/sparc/kernel/ioport.c
> index 3bcef9ce74df..4e91fbbbedcc 100644
> --- a/arch/sparc/kernel/ioport.c
> +++ b/arch/sparc/kernel/ioport.c
> @@ -669,7 +669,7 @@ static int sparc_io_proc_show(struct seq_file *m, void *v)
> struct resource *root = m->private, *r;
> const char *nm;
>
> - for (r = root->child; r != NULL; r = r->sibling) {
> + list_for_each_entry(r, &root->child, sibling) {
> if ((nm = r->name) == NULL) nm = "???";
> seq_printf(m, "%016llx-%016llx: %s\n",
> (unsigned long long)r->start,
> diff --git a/drivers/gpu/drm/drm_memory.c b/drivers/gpu/drm/drm_memory.c
> index 3c54044214db..53e300a993dc 100644
> --- a/drivers/gpu/drm/drm_memory.c
> +++ b/drivers/gpu/drm/drm_memory.c
> @@ -155,9 +155,8 @@ u64 drm_get_max_iomem(void)
> struct resource *tmp;
> resource_size_t max_iomem = 0;
>
> - for (tmp = iomem_resource.child; tmp; tmp = tmp->sibling) {
> + list_for_each_entry(tmp, &iomem_resource.child, sibling)
> max_iomem = max(max_iomem, tmp->end);
> - }
>
> return max_iomem;
> }
> diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
> index 3949b0990916..addd3bc009af 100644
> --- a/drivers/gpu/drm/gma500/gtt.c
> +++ b/drivers/gpu/drm/gma500/gtt.c
> @@ -565,7 +565,7 @@ int psb_gtt_init(struct drm_device *dev, int resume)
> int psb_gtt_restore(struct drm_device *dev)
> {
> struct drm_psb_private *dev_priv = dev->dev_private;
> - struct resource *r = dev_priv->gtt_mem->child;
> + struct resource *r;
> struct gtt_range *range;
> unsigned int restored = 0, total = 0, size = 0;
>
> @@ -573,14 +573,13 @@ int psb_gtt_restore(struct drm_device *dev)
> mutex_lock(&dev_priv->gtt_mutex);
> psb_gtt_init(dev, 1);
>
> - while (r != NULL) {
> + list_for_each_entry(r, &dev_priv->gtt_mem->child, sibling) {
> range = container_of(r, struct gtt_range, resource);
> if (range->pages) {
> psb_gtt_insert(dev, range, 1);
> size += range->resource.end - range->resource.start;
> restored++;
> }
> - r = r->sibling;
> total++;
>
Re: [PATCH 6/6] doc/devicetree: NVDIMM region documentation
On Tue, Mar 27, 2018 at 9:53 AM, Oliver wrote:
> On Tue, Mar 27, 2018 at 9:24 AM, Rob Herring wrote:
>> On Fri, Mar 23, 2018 at 07:12:09PM +1100, Oliver O'Halloran wrote:
>>> Add device-tree binding documentation for the nvdimm region driver.
>>>
>>> Cc: devicet...@vger.kernel.org
>>> Signed-off-by: Oliver O'Halloran
>>> ---
>>> .../devicetree/bindings/nvdimm/nvdimm-region.txt | 45
>>> ++
>>> 1 file changed, 45 insertions(+)
>>> create mode 100644
>>> Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
>>> b/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
>>> new file mode 100644
>>> index ..02091117ff16
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
>>> @@ -0,0 +1,45 @@
>>> +Device-tree bindings for NVDIMM memory regions
>>> +-
>>> +
>>> +Non-volatile DIMMs are memory modules used to provide (cacheable) main
>>> memory
>>
>> Are DIMMs always going to be the only form factor for NV memory?
>>
>> And if you have multiple DIMMs, does each DT node correspond to a DIMM?
>
> A nvdimm-region might correspond to a single NVDIMM, a set of
> interleaved NVDIMMs, or it might just be a chunk of normal memory that
> you want treated as a NVDIMM for some reason. The last case is useful
> for provisioning install media on servers since it allows you do
> download a DVD image, turn it into an nvdimm-region, and kexec into
> the installer which can use it as a root disk. That may seem a little
> esoteric, but it's handy and we're using a full linux environment for
> our boot loader so it's easy to make use of.
I'm really just asking if we should drop the "dimm" name because it is
not always a DIMM. Maybe pmem instead? I don't know, naming is
hard(TM).
>> If not, then what if we want/need to provide power control to a DIMM?
>
> That would require a DIMM (and probably memory controller) specific
> driver. I've deliberately left out how regions are mapped back to
> DIMMs from the binding since it's not really clear to me how that
> should work. A phandle array pointing to each DIMM device (which could
> be anything) would do the trick, but I've found that a bit awkward to
> plumb into the model that libnvdimm expects.
>
>>> +that retains its contents across power cycles. In more practical terms,
>>> they
>>> +are kind of storage device where the contents can be accessed by the CPU
>>> +directly, rather than indirectly via a storage controller or similar. The
>>> an
>>> +nvdimm-region specifies a physical address range that is hosted on an
>>> NVDIMM
>>> +device.
>>> +
>>> +Bindings for the region nodes:
>>> +-
>>> +
>>> +Required properties:
>>> + - compatible = "nvdimm-region"
>>> +
>>> + - reg = ;
>>> + The system physical address range of this nvdimm region.
>>> +
>>> +Optional properties:
>>> + - Any relevant NUMA assocativity properties for the target platform.
>>> + - A "volatile" property indicating that this region is actually in
>>> + normal DRAM and does not require cache flushes after each write.
>>> +
>>> +A complete example:
>>> +
>>> +
>>> +/ {
>>> + #size-cells = <2>;
>>> + #address-cells = <2>;
>>> +
>>> + platform {
>>
>> Perhaps we need a more well defined node here. Like we have 'memory' for
>> memory nodes.
>
> I think treating it as a platform device is fine. Memory nodes are
Platform device is a Linux term...
> special since the OS needs to know where it can allocate early in boot
> and I don't see non-volatile memory as being similarly significant.
> Fundamentally an NVDIMM is just a memory mapped storage device so we
> should be able to defer looking at them until later in boot.
It's not clear if 'platform' is just an example or random name or what
the node is required to be called. In the latter case, we should be
much more specific because 'platform' could be anything. In the former
case, then we have no way to find or validate the node because the
name could be anything and there's no compatible property either.
"region&q
Re: [PATCH 6/6] doc/devicetree: NVDIMM region documentation
On Fri, Mar 23, 2018 at 07:12:09PM +1100, Oliver O'Halloran wrote:
> Add device-tree binding documentation for the nvdimm region driver.
>
> Cc: devicet...@vger.kernel.org
> Signed-off-by: Oliver O'Halloran
> ---
> .../devicetree/bindings/nvdimm/nvdimm-region.txt | 45
> ++
> 1 file changed, 45 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
>
> diff --git a/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
> b/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
> new file mode 100644
> index ..02091117ff16
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
> @@ -0,0 +1,45 @@
> +Device-tree bindings for NVDIMM memory regions
> +-
> +
> +Non-volatile DIMMs are memory modules used to provide (cacheable) main memory
Are DIMMs always going to be the only form factor for NV memory?
And if you have multiple DIMMs, does each DT node correspond to a DIMM?
If not, then what if we want/need to provide power control to a DIMM?
> +that retains its contents across power cycles. In more practical terms, they
> +are kind of storage device where the contents can be accessed by the CPU
> +directly, rather than indirectly via a storage controller or similar. The an
> +nvdimm-region specifies a physical address range that is hosted on an NVDIMM
> +device.
> +
> +Bindings for the region nodes:
> +-
> +
> +Required properties:
> + - compatible = "nvdimm-region"
> +
> + - reg = ;
> + The system physical address range of this nvdimm region.
> +
> +Optional properties:
> + - Any relevant NUMA assocativity properties for the target platform.
> + - A "volatile" property indicating that this region is actually in
> + normal DRAM and does not require cache flushes after each write.
> +
> +A complete example:
> +
> +
> +/ {
> + #size-cells = <2>;
> + #address-cells = <2>;
> +
> + platform {
Perhaps we need a more well defined node here. Like we have 'memory' for
memory nodes.
> + region@5000 {
> + compatible = "nvdimm-region;
> + reg = <0x0001 0x 0x 0x4000>
> +
> + };
> +
> + region@6000 {
> + compatible = "nvdimm-region";
> + reg = <0x0001 0x 0x 0x4000>
Your reg property and unit-address don't match and you have overlapping
regions.
> + volatile;
> + };
> + };
> +};
> --
> 2.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 2/3] libnvdimm: Add a device-tree interface
On Thu, Nov 16, 2017 at 04:51:30AM +1100, Oliver O'Halloran wrote: > A fairly bare-bones set of device-tree bindings so libnvdimm can be used > on powerpc and other device-tree based platforms. > > Cc: devicet...@vger.kernel.org > Signed-off-by: Oliver O'Halloran > --- > .../devicetree/bindings/nvdimm/nvdimm-bus.txt | 69 +++ Also, please split bindings to a separate patch. > MAINTAINERS| 8 + > drivers/nvdimm/Kconfig | 10 + > drivers/nvdimm/Makefile| 1 + > drivers/nvdimm/of_nvdimm.c | 202 > + > 5 files changed, 290 insertions(+) > create mode 100644 Documentation/devicetree/bindings/nvdimm/nvdimm-bus.txt > create mode 100644 drivers/nvdimm/of_nvdimm.c ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 2/3] libnvdimm: Add a device-tree interface
On Thu, Nov 16, 2017 at 04:51:30AM +1100, Oliver O'Halloran wrote:
> A fairly bare-bones set of device-tree bindings so libnvdimm can be used
> on powerpc and other device-tree based platforms.
>
> Cc: devicet...@vger.kernel.org
> Signed-off-by: Oliver O'Halloran
> ---
> .../devicetree/bindings/nvdimm/nvdimm-bus.txt | 69 +++
> MAINTAINERS| 8 +
> drivers/nvdimm/Kconfig | 10 +
> drivers/nvdimm/Makefile| 1 +
> drivers/nvdimm/of_nvdimm.c | 202
> +
> 5 files changed, 290 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/nvdimm/nvdimm-bus.txt
> create mode 100644 drivers/nvdimm/of_nvdimm.c
>
> diff --git a/Documentation/devicetree/bindings/nvdimm/nvdimm-bus.txt
> b/Documentation/devicetree/bindings/nvdimm/nvdimm-bus.txt
> new file mode 100644
> index ..491e7c4900ed
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvdimm/nvdimm-bus.txt
> @@ -0,0 +1,69 @@
> +Device-tree bindings for nonvolatile-memory / NVDIMMs
> +-
> +
> +Non-volatile DIMMs are memory modules used to provide (cacheable) main
> memory that
> +retains its contents across power cycles. In more practical terms, they are
> kind
> +of storage device where the contents can be accessed by the CPU directly,
> +rather than indirectly via a storage controller or similar. This can provide
> +substantial performance improvements for applications designed to take
> +advantage of in-memory storage.
> +
> +This binding provides a way to describe memory regions that should be managed
> +by an NVDIMM storage driver (libNVDIMM in Linux) and some of the associated
> +metadata. The binding itself is split into two main parts: A container bus
> and
> +its sub-nodes which describe which memory address ranges corresponding to
> +NVDIMM backed memory.
> +
> +Bindings for the container bus:
> +--
> +
> +Required properties:
> + - compatible = "nvdimm-bus";
> + - ranges;
> + A blank ranges property is required because the sub-nodes have
> + addresses in the system's physical address space.
> +
> +The use of a container bus is mainly to handle future expansion of the
> binding. For
> +comparison the ACPI equivalent of this binding (NFIT) describes: Memory
> regions, DIMM
> +control structures, Block mode DIMM control structures, interleave sets, and
> more. Some
> +of these structures cross reference each other so everyone should be happier
> if we keep
> +it relatively self contained.
Will adding any of these things need unit addresses and colide with the
existing nodes below? IOW, at one level there's only 1 number space.
> +
> +Bindings for the region nodes:
> +-
> +
> +Required properties:
> + - compatible = "nvdimm-persistent" or "nvdimm-volatile"
> +
> + The "nvdimm-persistent" region type indicates that this memory
> region
> + is actually a persistent region. The volatile type is mainly
> useful
> + for testing and RAM disks that can persist across kexec.
> +
> + - reg = ;
> + The reg property should only contain one address range.
> +
> +Optional properties:
> + - Any relevant NUMA assocativity properties for the target platform.
> +
> +A complete example:
> +
> +
> +/ {
> + #size-cells = <1>;
> + #address-cells = <1>;
> +
> + nonvolatile-memory {
> + compatible = "nonvolatile-memory";
> + ranges;
> +
> + region@5000 {
> + compatible = "nvdimm-persistent";
> + reg = <0x5000 0x1000>;
> + };
> +
> + region@6000 {
> + compatible = "nvdimm-volatile";
> + reg = <0x6000 0x1000>;
> + };
> + };
> +};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 65eff7857ec3..0350bf5a94d2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7875,6 +7875,14 @@ Q:
> https://patchwork.kernel.org/project/linux-nvdimm/list/
> S: Supported
> F: drivers/nvdimm/pmem*
>
> +LIBNVDIMM: DEVICETREE BINDINGS
> +M: Oliver O'Halloran
> +L: linux-nvdimm@lists.01.org
> +Q: https://patchwork.kernel.org/project/linux-nvdimm/list/
> +S: Supported
> +F: drivers/nvdimm/of_nvdimm.c
> +F: Documentation/devicetree/bindings/nvdimm/nvdimm-bus.txt
> +
> LIBNVDIMM: NON-VOLATILE MEMORY DEVICE SUBSYSTEM
> M: Dan Williams
> L: linux-nvdimm@lists.01.org
> diff --git a/drivers/nvdimm/Kconfig b/drivers/nvdimm/Kconfig
> index 5bdd499b5f4f..72d147b55596 100644
> --- a/drivers/nvdimm/Kconfig
> +++ b/drivers/nvdimm/Kconfig
> @@ -102,4 +102,14 @@ config NVDIMM_DAX
>
> Select Y if unsure
>
> +config OF_NVDIMM
> + tristate "Device-tree support for NVDIMMs"
> + depends on OF
> + default LIBNVDIMM
> +
