Re: [ndctl V5 4/4] Use page size as alignment value
On Thu, 2021-05-13 at 11:42 +0530, Santosh Sivaraj wrote:
> The alignment sizes passed to ndctl in the tests are all hardcoded to 4k,
> the default page size on x86. Change those to the default page size on that
> architecture (sysconf/getconf). No functional changes otherwise.
>
> Signed-off-by: Santosh Sivaraj
> ---
> test/dpa-alloc.c| 15 ---
> test/multi-dax.sh | 6 --
> test/sector-mode.sh | 4 +++-
> 3 files changed, 15 insertions(+), 10 deletions(-)
Thanks for the updates, these look good - I've applied them and pushed
out on 'pending'.
>
> diff --git a/test/dpa-alloc.c b/test/dpa-alloc.c
> index 0b3bb7a..59185cf 100644
> --- a/test/dpa-alloc.c
> +++ b/test/dpa-alloc.c
> @@ -38,12 +38,13 @@ static int do_test(struct ndctl_ctx *ctx, struct
> ndctl_test *test)
> struct ndctl_region *region, *blk_region = NULL;
> struct ndctl_namespace *ndns;
> struct ndctl_dimm *dimm;
> - unsigned long size;
> + unsigned long size, page_size;
> struct ndctl_bus *bus;
> char uuid_str[40];
> int round;
> int rc;
>
> + page_size = sysconf(_SC_PAGESIZE);
> /* disable nfit_test.1, not used in this test */
> bus = ndctl_bus_get_by_provider(ctx, NFIT_PROVIDER1);
> if (!bus)
> @@ -124,11 +125,11 @@ static int do_test(struct ndctl_ctx *ctx, struct
> ndctl_test *test)
> return rc;
> }
> ndctl_namespace_disable_invalidate(ndns);
> - rc = ndctl_namespace_set_size(ndns, SZ_4K);
> + rc = ndctl_namespace_set_size(ndns, page_size);
> if (rc) {
> - fprintf(stderr, "failed to init %s to size: %d\n",
> + fprintf(stderr, "failed to init %s to size: %lu\n",
> ndctl_namespace_get_devname(ndns),
> - SZ_4K);
> + page_size);
> return rc;
> }
> namespaces[i].ndns = ndns;
> @@ -150,7 +151,7 @@ static int do_test(struct ndctl_ctx *ctx, struct
> ndctl_test *test)
> ndns = namespaces[i % ARRAY_SIZE(namespaces)].ndns;
> if (i % ARRAY_SIZE(namespaces) == 0)
> round++;
> - size = SZ_4K * round;
> + size = page_size * round;
> rc = ndctl_namespace_set_size(ndns, size);
> if (rc) {
> fprintf(stderr, "%s: set_size: %lx failed: %d\n",
> @@ -166,7 +167,7 @@ static int do_test(struct ndctl_ctx *ctx, struct
> ndctl_test *test)
> i--;
> round++;
> ndns = namespaces[i % ARRAY_SIZE(namespaces)].ndns;
> - size = SZ_4K * round;
> + size = page_size * round;
> rc = ndctl_namespace_set_size(ndns, size);
> if (rc) {
> fprintf(stderr, "%s failed to update while labels full\n",
> @@ -175,7 +176,7 @@ static int do_test(struct ndctl_ctx *ctx, struct
> ndctl_test *test)
> }
>
> round--;
> - size = SZ_4K * round;
> + size = page_size * round;
> rc = ndctl_namespace_set_size(ndns, size);
> if (rc) {
> fprintf(stderr, "%s failed to reduce size while labels full\n",
> diff --git a/test/multi-dax.sh b/test/multi-dax.sh
> index e932569..9451ed0 100755
> --- a/test/multi-dax.sh
> +++ b/test/multi-dax.sh
> @@ -12,6 +12,8 @@ check_min_kver "4.13" || do_skip "may lack multi-dax
> support"
>
> trap 'err $LINENO' ERR
>
> +ALIGN_SIZE=`getconf PAGESIZE`
> +
> # setup (reset nfit_test dimms)
> modprobe nfit_test
> $NDCTL disable-region -b $NFIT_TEST_BUS0 all
> @@ -22,9 +24,9 @@ rc=1
> query=". | sort_by(.available_size) | reverse | .[0].dev"
> region=$($NDCTL list -b $NFIT_TEST_BUS0 -t pmem -Ri | jq -r "$query")
>
> -json=$($NDCTL create-namespace -b $NFIT_TEST_BUS0 -r $region -t pmem -m
> devdax -a 4096 -s 16M)
> +json=$($NDCTL create-namespace -b $NFIT_TEST_BUS0 -r $region -t pmem -m
> devdax -a $ALIGN_SIZE -s 16M)
> chardev1=$(echo $json | jq ". | select(.mode == \"devdax\") |
> .daxregion.devices[0].chardev")
> -json=$($NDCTL create-namespace -b $NFIT_TEST_BUS0 -r $region -t pmem -m
> devdax -a 4096 -s 16M)
> +json=$($NDCTL create-namespace -b $NFIT_TEST_BUS0 -r $region -t pmem -m
> devdax -a $ALIGN_SIZE -s 16M)
> chardev2=$(echo $json | jq ". | select(.mode == \"devdax\") |
> .daxregion.devices[0].chardev")
>
> _cleanup
> diff --git a/test/sector-mode.sh b/test/sector-mode.sh
> index dd7013e..d03c0ca 100755
> --- a/test/sector-mode.sh
> +++ b/test/sector-mode.sh
> @@ -9,6 +9,8 @@ rc=77
> set -e
> trap 'err $LINENO' ERR
>
> +ALIGN_SIZE=`getconf PAGESIZE`
> +
> # setup (reset nfit_test dimms)
> modprobe nfit_test
> $NDCTL disable-region -b $NFIT_TEST_BUS0 all
> @@ -25,7 +27,7 @@ NAMESPACE=$($NDCTL list -b $NFIT_TEST_BUS1 -N | jq -r
> "$query")
> REGION=$($NDCTL list -R --namespace=$NAMESPACE | jq -r "(.[]) | .dev")
> echo 0 >
Re: [PATCH 2/4] test: Don't skip tests if nfit modules are missing
On Wed, 2021-05-12 at 21:00 +, Verma, Vishal L wrote:
>
> Did you mean for the errno check to be if (errno != ENOENT) ?
> This is what was causing the unit test failure for me. This patch on
> top fixes it for me:
>
> diff --git a/test/core.c b/test/core.c
> index 44cb277..698bb66 100644
> --- a/test/core.c
> +++ b/test/core.c
> @@ -139,7 +139,7 @@ int ndctl_test_init(struct kmod_ctx **ctx, struct
> kmod_module **mod,
> * determine from the environment variable NVDIMM_TEST_FAMILY
> */
> if (access("/sys/bus/acpi", F_OK) == 0) {
> - if (errno == ENOENT)
> + if (errno != ENOENT)
> family = NVDIMM_FAMILY_INTEL;
> }
Also, looks like for access(, F_OK), it should be sufficient to
just test for the return value instead of return value and errno.
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH 2/4] test: Don't skip tests if nfit modules are missing
On Sat, 2021-05-01 at 11:57 +0530, Santosh Sivaraj wrote: > "Verma, Vishal L" writes: > > Hi Vishal, > > > On Sun, 2021-03-28 at 07:39 +0530, Santosh Sivaraj wrote: > > > For NFIT to be available ACPI is a must, so don't fail when nfit modules > > > are missing on a platform that doesn't support ACPI. > > > > > > Signed-off-by: Santosh Sivaraj > > > --- > > > test.h| 2 +- > > > test/ack-shutdown-count-set.c | 2 +- > > > test/blk_namespaces.c | 2 +- > > > test/core.c | 30 -- > > > test/dpa-alloc.c | 2 +- > > > test/dsm-fail.c | 2 +- > > > test/libndctl.c | 2 +- > > > test/multi-pmem.c | 2 +- > > > test/parent-uuid.c| 2 +- > > > test/pmem_namespaces.c| 2 +- > > > 10 files changed, 37 insertions(+), 11 deletions(-) > > > > > > > I haven't looked deeper, but this seems to fail the blk-ns test with: > > > > ACPI.NFIT unavailable falling back to nfit_test > > test/init: ndctl_test_init: Cannot determine NVDIMM family > > __ndctl_test_skip: explicit skip test_blk_namespaces:235 > > nfit_test unavailable skipping tests > > The first message will be emitted even without the changes if the bus is not > found. The second error will be emitted when check "/sys/bus/acpi" is not > found. We fail for all other buses by default except for NFIT as before and > PAPR > tests are enabled only when NVDIMM_TEST_FAMILY is set to "PAPR". See below on this. > > All tests pass in my setup (x86_64 qemu guest) with the recent upstream > kernel, > except for the the below warning from drivers/acpi/nfit/core.c: Hm I've not seen this with 5.11 or 5.12. What's the qemu command line and is it just triggered from a unit test tun? > > [ 2426.727584] [ cut here ] > [ 2426.728405] WARNING: CPU: 2 PID: 47504 at > tools/testing/nvdimm/../../../drivers/acpi/nfit/core.c:3879 nfit_exit+0x] > [ 2426.730264] Modules linked in: dax_pmem(O) nd_pmem(O) nfit(O-) kmem > dax_pmem_compat(O) nd_blk(O) dax_pmem_core(O) ] > [ 2426.733209] CPU: 2 PID: 47504 Comm: modprobe Tainted: GW O > 5.12.0+ #3 > [ 2426.734472] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.o4 > [ 2426.736305] RIP: 0010:nfit_exit+0x2c/0x703 [nfit] > [ 2426.737099] Code: fd ff ff 48 c7 c7 00 f0 39 c0 e8 52 a1 38 da 48 8b 3d 6b > 46 00 00 e8 e6 88 ee d9 48 8b 05 5f 3c 0 > [ 2426.740046] RSP: 0018:a8e800b77ed8 EFLAGS: 00010287 > [ 2426.740990] RAX: 95b7e51935b0 RBX: 0800 RCX: > 9b4a36a8 > [ 2426.742236] RDX: RSI: 0083 RDI: > 95b7c03e1554 > [ 2426.743404] RBP: c039f740 R08: 0400 R09: > 95b7c03e0e50 > [ 2426.744617] R10: 95b7fbd296f0 R11: 00895440 R12: > a8e800b77f58 > [ 2426.745792] R13: R14: R15: > > [ 2426.746946] FS: 7f48297e3740() GS:95b7fbd0() > knlGS: > [ 2426.748250] CS: 0010 DS: ES: CR0: 80050033 > [ 2426.749198] CR2: 56072aadc9f8 CR3: 000118b08000 CR4: > 06e0 > [ 2426.750349] Call Trace: > [ 2426.750754] __do_sys_delete_module+0x19d/0x240 > [ 2426.751472] ? task_work_run+0x5c/0x90 > [ 2426.751964] ? exit_to_user_mode_prepare+0x2a/0x130 > [ 2426.752637] do_syscall_64+0x40/0x80 > [ 2426.753121] entry_SYSCALL_64_after_hwframe+0x44/0xae > [ 2426.753810] RIP: 0033:0x7f482991361b > [ 2426.754274] Code: 73 01 c3 48 8b 0d 5d 18 0c 00 f7 d8 64 89 01 48 83 c8 ff > c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 8 > [ 2426.756668] RSP: 002b:7ffd46c89b98 EFLAGS: 0206 ORIG_RAX: > 00b0 > [ 2426.757676] RAX: ffda RBX: 56072aad8f90 RCX: > 7f482991361b > [ 2426.758618] RDX: RSI: 0800 RDI: > 56072aad8ff8 > [ 2426.759563] RBP: 56072aad8f90 R08: R09: > > [ 2426.760513] R10: 7f4829987ac0 R11: 0206 R12: > 56072aad8ff8 > [ 2426.761463] R13: R14: 56072aadb4e8 R15: > 7ffd46c89d18 > [ 2426.762405] ---[ end trace 14a8748cda8b4777 ]--- > > This was not seen with the 5.11 kernel. > > Thanks, > Santosh > > > > > diff --git a/test.h b/test.h > > > index cba8d41..7de13fe 100644 > > > --- a/test.h > > > +++ b/test.h > > > @@ -20,7 +20,7 @@
Re: [PATCH 2/4] test: Don't skip tests if nfit modules are missing
On Sun, 2021-03-28 at 07:39 +0530, Santosh Sivaraj wrote:
> For NFIT to be available ACPI is a must, so don't fail when nfit modules
> are missing on a platform that doesn't support ACPI.
>
> Signed-off-by: Santosh Sivaraj
> ---
> test.h| 2 +-
> test/ack-shutdown-count-set.c | 2 +-
> test/blk_namespaces.c | 2 +-
> test/core.c | 30 --
> test/dpa-alloc.c | 2 +-
> test/dsm-fail.c | 2 +-
> test/libndctl.c | 2 +-
> test/multi-pmem.c | 2 +-
> test/parent-uuid.c| 2 +-
> test/pmem_namespaces.c| 2 +-
> 10 files changed, 37 insertions(+), 11 deletions(-)
>
I haven't looked deeper, but this seems to fail the blk-ns test with:
ACPI.NFIT unavailable falling back to nfit_test
test/init: ndctl_test_init: Cannot determine NVDIMM family
__ndctl_test_skip: explicit skip test_blk_namespaces:235
nfit_test unavailable skipping tests
> diff --git a/test.h b/test.h
> index cba8d41..7de13fe 100644
> --- a/test.h
> +++ b/test.h
> @@ -20,7 +20,7 @@ void builtin_xaction_namespace_reset(void);
>
>
> struct kmod_ctx;
> struct kmod_module;
> -int nfit_test_init(struct kmod_ctx **ctx, struct kmod_module **mod,
> +int ndctl_test_init(struct kmod_ctx **ctx, struct kmod_module **mod,
> struct ndctl_ctx *nd_ctx, int log_level,
> struct ndctl_test *test);
>
>
> diff --git a/test/ack-shutdown-count-set.c b/test/ack-shutdown-count-set.c
> index fb1d82b..c561ff3 100644
> --- a/test/ack-shutdown-count-set.c
> +++ b/test/ack-shutdown-count-set.c
> @@ -99,7 +99,7 @@ static int test_ack_shutdown_count_set(int loglevel, struct
> ndctl_test *test,
> int result = EXIT_FAILURE, err;
>
>
> ndctl_set_log_priority(ctx, loglevel);
> - err = nfit_test_init(_ctx, , NULL, loglevel, test);
> + err = ndctl_test_init(_ctx, , NULL, loglevel, test);
> if (err < 0) {
> result = 77;
> ndctl_test_skip(test);
> diff --git a/test/blk_namespaces.c b/test/blk_namespaces.c
> index d7f00cb..f076e85 100644
> --- a/test/blk_namespaces.c
> +++ b/test/blk_namespaces.c
> @@ -228,7 +228,7 @@ int test_blk_namespaces(int log_level, struct ndctl_test
> *test,
>
>
> if (!bus) {
> fprintf(stderr, "ACPI.NFIT unavailable falling back to
> nfit_test\n");
> - rc = nfit_test_init(_ctx, , NULL, log_level, test);
> + rc = ndctl_test_init(_ctx, , NULL, log_level, test);
> ndctl_invalidate(ctx);
> bus = ndctl_bus_get_by_provider(ctx, "nfit_test.0");
> if (rc < 0 || !bus) {
> diff --git a/test/core.c b/test/core.c
> index cc7d8d9..44cb277 100644
> --- a/test/core.c
> +++ b/test/core.c
> @@ -11,6 +11,7 @@
> #include
> #include
> #include
> +#include
> #include
>
>
> #define KVER_STRLEN 20
> @@ -106,11 +107,11 @@ int ndctl_test_get_skipped(struct ndctl_test *test)
> return test->skip;
> }
>
>
> -int nfit_test_init(struct kmod_ctx **ctx, struct kmod_module **mod,
> +int ndctl_test_init(struct kmod_ctx **ctx, struct kmod_module **mod,
> struct ndctl_ctx *nd_ctx, int log_level,
> struct ndctl_test *test)
> {
> - int rc;
> + int rc, family = -1;
> unsigned int i;
> const char *name;
> struct ndctl_bus *bus;
> @@ -127,10 +128,30 @@ int nfit_test_init(struct kmod_ctx **ctx, struct
> kmod_module **mod,
> "nd_e820",
> "nd_pmem",
> };
> + char *test_env;
>
>
> log_init(_ctx, "test/init", "NDCTL_TEST");
> log_ctx.log_priority = log_level;
>
>
> + /*
> + * The following two checks determine the platform family. For
> + * Intel/platforms which support ACPI, check sysfs; for other platforms
> + * determine from the environment variable NVDIMM_TEST_FAMILY
> + */
> + if (access("/sys/bus/acpi", F_OK) == 0) {
> + if (errno == ENOENT)
> + family = NVDIMM_FAMILY_INTEL;
> + }
> +
> + test_env = getenv("NDCTL_TEST_FAMILY");
> + if (test_env && strcmp(test_env, "PAPR") == 0)
> + family = NVDIMM_FAMILY_PAPR;
> +
> + if (family == -1) {
> + log_err(_ctx, "Cannot determine NVDIMM family\n");
> + return -ENOTSUP;
> + }
> +
> *ctx = kmod_new(NULL, NULL);
> if (!*ctx)
> return -ENXIO;
> @@ -185,6 +206,11 @@ retry:
>
>
> path = kmod_module_get_path(*mod);
> if (!path) {
> + if (family != NVDIMM_FAMILY_INTEL &&
> + (strcmp(name, "nfit") == 0 ||
> + strcmp(name, "nd_e820") == 0))
> + continue;
> +
> log_err(_ctx, "%s.ko: failed to get path\n", name);
> break;
> }
> diff --git a/test/dpa-alloc.c
Re: [ndctl PATCH v3 2/4] test: Don't skip tests if nfit modules are missing
On Fri, 2021-03-19 at 11:20 +0530, Santosh Sivaraj wrote:
> "Verma, Vishal L" writes:
[..]
> >
> > fix multi line comment to the right formatting:
> > /*
> > * line 1, etc
> > */
> >
>
> Will fix that.
>
> > > + if (access("/sys/bus/acpi", F_OK) == -1) {
> > > + if (errno == ENOENT)
> > > + family = NVDIMM_FAMILY_PAPR;
> > > + }
> >
> > Instead of a blind default, can we perform a similar check for presence of
> > PAPR too?
> >
>
> Yes, I wanted to do that, but there is no reliable way of check that; there is
> no ofnode before module load, and there won't be any PAPR specific DT entries
> if
> the platform is not Power.
>
> I also test the 'ndtest' module on x86 with NDCTL_TEST_FAMILY environment
> variable. I can let the default be nfit_test (NVDIMM_FAMILY_INTEL) and only
> load
> PAPR module when the environment variable is set. Thoughts?
>
The env variable seems reasonable to me. If there is ever a third
'family' adding tests, having an arbitrary default might be awkward.
I may suggest - if acpi is detected, use NFIT. If env has something that
is known, e.g. PAPR, use that. If env is unset or doesn't match anything
we know about, then bail with an error message. Does that sound
reasonable?
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH v3 3/4] papr: Add support to parse save_fail flag for dimm
On Thu, 2021-03-11 at 13:16 +0530, Santosh Sivaraj wrote: > This will help in getting the dimm fail tests to run on papr family too. > Also add nvdimm_test compatibility string for recognizing the test module. > > Signed-off-by: Santosh Sivaraj > --- > ndctl/lib/libndctl.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c > index 26b9317..dd1a5fc 100644 > --- a/ndctl/lib/libndctl.c > +++ b/ndctl/lib/libndctl.c > @@ -805,6 +805,8 @@ static void parse_papr_flags(struct ndctl_dimm *dimm, > char *flags) > dimm->flags.f_restore = 1; > else if (strcmp(start, "smart_notify") == 0) > dimm->flags.f_smart = 1; > + else if (strcmp(start, "save_fail") == 0) > + dimm->flags.f_save = 1; > start = end + 1; > } > if (end != start) > @@ -1035,7 +1037,8 @@ NDCTL_EXPORT int ndctl_bus_is_papr_scm(struct ndctl_bus > *bus) > if (sysfs_read_attr(bus->ctx, bus->bus_buf, buf) < 0) > return 0; > > - return (strcmp(buf, "ibm,pmemory") == 0); > + return (strcmp(buf, "ibm,pmemory") == 0 || > + strcmp(buf, "nvdimm_test") == 0); I'm guessing this name comes from the kernel? It would be nice to make it symmetrical with 'nfit_test' by calling the bus 'papr_test' maybe, but no worries if it is too late for that. > } > > /** ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH v3 2/4] test: Don't skip tests if nfit modules are missing
On Thu, 2021-03-11 at 13:16 +0530, Santosh Sivaraj wrote:
> For NFIT to be available ACPI is a must, so don't fail when nfit modules
> are missing on a platform that doesn't support ACPI.
>
> Signed-off-by: Santosh Sivaraj
> ---
> test.h| 2 +-
> test/ack-shutdown-count-set.c | 2 +-
> test/blk_namespaces.c | 2 +-
> test/core.c | 23 +--
> test/dpa-alloc.c | 2 +-
> test/dsm-fail.c | 2 +-
> test/libndctl.c | 2 +-
> test/multi-pmem.c | 2 +-
> test/parent-uuid.c| 2 +-
> test/pmem_namespaces.c| 2 +-
> 10 files changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/test.h b/test.h
> index cba8d41..7de13fe 100644
> --- a/test.h
> +++ b/test.h
> @@ -20,7 +20,7 @@ void builtin_xaction_namespace_reset(void);
>
>
> struct kmod_ctx;
> struct kmod_module;
> -int nfit_test_init(struct kmod_ctx **ctx, struct kmod_module **mod,
> +int ndctl_test_init(struct kmod_ctx **ctx, struct kmod_module **mod,
> struct ndctl_ctx *nd_ctx, int log_level,
> struct ndctl_test *test);
>
>
> diff --git a/test/ack-shutdown-count-set.c b/test/ack-shutdown-count-set.c
> index fb1d82b..c561ff3 100644
> --- a/test/ack-shutdown-count-set.c
> +++ b/test/ack-shutdown-count-set.c
> @@ -99,7 +99,7 @@ static int test_ack_shutdown_count_set(int loglevel, struct
> ndctl_test *test,
> int result = EXIT_FAILURE, err;
>
>
> ndctl_set_log_priority(ctx, loglevel);
> - err = nfit_test_init(_ctx, , NULL, loglevel, test);
> + err = ndctl_test_init(_ctx, , NULL, loglevel, test);
> if (err < 0) {
> result = 77;
> ndctl_test_skip(test);
> diff --git a/test/blk_namespaces.c b/test/blk_namespaces.c
> index d7f00cb..f076e85 100644
> --- a/test/blk_namespaces.c
> +++ b/test/blk_namespaces.c
> @@ -228,7 +228,7 @@ int test_blk_namespaces(int log_level, struct ndctl_test
> *test,
>
>
> if (!bus) {
> fprintf(stderr, "ACPI.NFIT unavailable falling back to
> nfit_test\n");
> - rc = nfit_test_init(_ctx, , NULL, log_level, test);
> + rc = ndctl_test_init(_ctx, , NULL, log_level, test);
> ndctl_invalidate(ctx);
> bus = ndctl_bus_get_by_provider(ctx, "nfit_test.0");
> if (rc < 0 || !bus) {
> diff --git a/test/core.c b/test/core.c
> index cc7d8d9..903034a 100644
> --- a/test/core.c
> +++ b/test/core.c
> @@ -11,6 +11,7 @@
> #include
> #include
> #include
> +#include
> #include
>
>
> #define KVER_STRLEN 20
> @@ -106,11 +107,11 @@ int ndctl_test_get_skipped(struct ndctl_test *test)
> return test->skip;
> }
>
>
> -int nfit_test_init(struct kmod_ctx **ctx, struct kmod_module **mod,
> +int ndctl_test_init(struct kmod_ctx **ctx, struct kmod_module **mod,
> struct ndctl_ctx *nd_ctx, int log_level,
> struct ndctl_test *test)
> {
> - int rc;
> + int rc, family = NVDIMM_FAMILY_INTEL;
> unsigned int i;
> const char *name;
> struct ndctl_bus *bus;
> @@ -127,6 +128,19 @@ int nfit_test_init(struct kmod_ctx **ctx, struct
> kmod_module **mod,
> "nd_e820",
> "nd_pmem",
> };
> + char *test_env;
> +
> + /* Do we want to force test PAPR? */
> + test_env = getenv("NDCTL_TEST_FAMILY");
> + if (test_env && strcmp(test_env, "PAPR") == 0)
> + family = NVDIMM_FAMILY_PAPR;
> +
> + /* ACPI is a must for nfit, so if ACPI is not available let's default to
> + * PAPR */
fix multi line comment to the right formatting:
/*
* line 1, etc
*/
> + if (access("/sys/bus/acpi", F_OK) == -1) {
> + if (errno == ENOENT)
> + family = NVDIMM_FAMILY_PAPR;
> + }
Instead of a blind default, can we perform a similar check for presence of
PAPR too?
>
>
> log_init(_ctx, "test/init", "NDCTL_TEST");
> log_ctx.log_priority = log_level;
> @@ -185,6 +199,11 @@ retry:
>
>
> path = kmod_module_get_path(*mod);
> if (!path) {
> + if (family == NVDIMM_FAMILY_PAPR &&
> + (strcmp(name, "nfit") == 0 ||
> + strcmp(name, "nd_e820") == 0))
> + continue;
> +
> log_err(_ctx, "%s.ko: failed to get path\n", name);
> break;
> }
> diff --git a/test/dpa-alloc.c b/test/dpa-alloc.c
> index e922009..0b3bb7a 100644
> --- a/test/dpa-alloc.c
> +++ b/test/dpa-alloc.c
> @@ -289,7 +289,7 @@ int test_dpa_alloc(int loglevel, struct ndctl_test *test,
> struct ndctl_ctx *ctx)
> return 77;
>
>
> ndctl_set_log_priority(ctx, loglevel);
> - err = nfit_test_init(_ctx, , NULL, loglevel, test);
> + err = ndctl_test_init(_ctx, , NULL, loglevel, test);
> if (err < 0) {
>
Re: [ndctl PATCH v3 1/4] libndctl: test enablement for non-nfit devices
On Thu, 2021-03-11 at 13:16 +0530, Santosh Sivaraj wrote:
> Unify adding dimms for papr and nfit families, this will help in adding
Minor nit, but it seems like the subject line and the first sentence in
the body should be swapped. The one-line description of what's happening
in this patch is "Unify adding dimms for papr and nfit families", and
the body can go into more detail about why - i.e. in preparation for
enabling tests on non-nfit devices.
> all attributes needed for the unit tests too. We don't fail adding a dimm
> if some of the dimm attributes are missing, so this will work fine on PAPR
> platforms where most dimm attributes are provided.
Does this mean 'most - but not all'? Might be a good clarification to
make here.
>
> Signed-off-by: Santosh Sivaraj
> ---
> ndctl/lib/libndctl.c | 103 ---
> 1 file changed, 38 insertions(+), 65 deletions(-)
>
> v3:
> * Drop patch which skips SMART tests, smart test enablement will be posted
> soon.
>
> v2:
> * Patch 2: Fix a bug, I skip erroring out if PAPR family, but condition had
> INTEL family instead. That change was there to test the same code on x86,
> but
> accidently committed. Now have a environment variable to force test PAPR
> family on x86.
>
> * Patch 4: Remove stray code, artifact of refactoring in patch 1.
>
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index 36fb6fe..26b9317 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -1646,41 +1646,9 @@ static int ndctl_bind(struct ndctl_ctx *ctx, struct
> kmod_module *module,
> static int ndctl_unbind(struct ndctl_ctx *ctx, const char *devpath);
> static struct kmod_module *to_module(struct ndctl_ctx *ctx, const char
> *alias);
>
> -static int add_papr_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
> -{
> - int rc = -ENODEV;
> - char buf[SYSFS_ATTR_SIZE];
> - struct ndctl_ctx *ctx = dimm->bus->ctx;
> - char *path = calloc(1, strlen(dimm_base) + 100);
> - const char * const devname = ndctl_dimm_get_devname(dimm);
> -
> - dbg(ctx, "%s: Probing of_pmem dimm at %s\n", devname, dimm_base);
> -
> - if (!path)
> - return -ENOMEM;
> -
> - /* construct path to the papr compatible dimm flags file */
> - sprintf(path, "%s/papr/flags", dimm_base);
> -
> - if (ndctl_bus_is_papr_scm(dimm->bus) &&
> - sysfs_read_attr(ctx, path, buf) == 0) {
> -
> - dbg(ctx, "%s: Adding papr-scm dimm flags:\"%s\"\n", devname,
> buf);
> - dimm->cmd_family = NVDIMM_FAMILY_PAPR;
> -
> - /* Parse dimm flags */
> - parse_papr_flags(dimm, buf);
> -
> - /* Allocate monitor mode fd */
> - dimm->health_eventfd = open(path, O_RDONLY|O_CLOEXEC);
> - rc = 0;
> - }
> -
> - free(path);
> - return rc;
> -}
> -
> -static int add_nfit_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
> +static int populate_dimm_attributes(struct ndctl_dimm *dimm,
> + const char *dimm_base,
> + const char *bus_prefix)
> {
> int i, rc = -1;
> char buf[SYSFS_ATTR_SIZE];
> @@ -1694,7 +1662,7 @@ static int add_nfit_dimm(struct ndctl_dimm *dimm, const
> char *dimm_base)
> * 'unique_id' may not be available on older kernels, so don't
> * fail if the read fails.
> */
> - sprintf(path, "%s/nfit/id", dimm_base);
> + sprintf(path, "%s/%s/id", dimm_base, bus_prefix);
> if (sysfs_read_attr(ctx, path, buf) == 0) {
> unsigned int b[9];
>
> @@ -1709,68 +1677,74 @@ static int add_nfit_dimm(struct ndctl_dimm *dimm,
> const char *dimm_base)
> }
> }
>
> - sprintf(path, "%s/nfit/handle", dimm_base);
> + sprintf(path, "%s/%s/handle", dimm_base, bus_prefix);
> if (sysfs_read_attr(ctx, path, buf) < 0)
> goto err_read;
> dimm->handle = strtoul(buf, NULL, 0);
>
> - sprintf(path, "%s/nfit/phys_id", dimm_base);
> + sprintf(path, "%s/%s/phys_id", dimm_base, bus_prefix);
> if (sysfs_read_attr(ctx, path, buf) < 0)
> goto err_read;
> dimm->phys_id = strtoul(buf, NULL, 0);
>
> - sprintf(path, "%s/nfit/serial", dimm_base);
> + sprintf(path, "%s/%s/serial", dimm_base, bus_prefix);
> if (sysfs_read_attr(ctx, path, buf) == 0)
> dimm->serial = strtoul(buf, NULL, 0);
>
> - sprintf(path, "%s/nfit/vendor", dimm_base);
> + sprintf(path, "%s/%s/vendor", dimm_base, bus_prefix);
> if (sysfs_read_attr(ctx, path, buf) == 0)
> dimm->vendor_id = strtoul(buf, NULL, 0);
>
> - sprintf(path, "%s/nfit/device", dimm_base);
> + sprintf(path, "%s/%s/device", dimm_base, bus_prefix);
> if (sysfs_read_attr(ctx, path, buf) == 0)
> dimm->device_id = strtoul(buf, NULL, 0);
>
> - sprintf(path, "%s/nfit/rev_id", dimm_base);
> + sprintf(path,
Re: [PATCH v2] libnvdimm: Notify disk drivers to revalidate region read-only
On Tue, 2021-03-09 at 17:43 -0800, Dan Williams wrote:
> Previous kernels allowed the BLKROSET to override the disk's read-only
> status. With that situation fixed the pmem driver needs to rely on
> notification events to reevaluate the disk read-only status after the
> host region has been marked read-write.
>
> Recall that when libnvdimm determines that the persistent memory has
> lost persistence (for example lack of energy to flush from DRAM to FLASH
> on an NVDIMM-N device) it marks the region read-only, but that state can
> be overridden by the user via:
>
> echo 0 > /sys/bus/nd/devices/regionX/read_only
>
> ...to date there is no notification that the region has restored
> persistence, so the user override is the only recovery.
>
> Fixes: 52f019d43c22 ("block: add a hard-readonly flag to struct gendisk")
> Cc: Christoph Hellwig
> Cc: Ming Lei
> Cc: Martin K. Petersen
> Cc: Hannes Reinecke
> Cc: Jens Axboe
> Reported-by: kernel test robot
> Reported-by: Vishal Verma
> Signed-off-by: Dan Williams
> ---
> Changes since v1 [1]:
> - Move from the sinking ship of revalidate_disk() to the local hotness
> of nd_pmem_notify() (hch).
>
> [1]:
> http://lore.kernel.org/r/161527286194.446794.5215036039655765042.st...@dwillia2-desk3.amr.corp.intel.com
>
> drivers/nvdimm/bus.c | 14 ++
> drivers/nvdimm/pmem.c| 37 +
> drivers/nvdimm/region_devs.c |7 +++
> include/linux/nd.h |1 +
> 4 files changed, 47 insertions(+), 12 deletions(-)
With the update to the unit test applied, and this, everything passes
for me. You can add:
Tested-by: Vishal Verma
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH 1/2] configure: add checking jq command
On Tue, 2021-03-02 at 02:25 +0900, QI Fuli wrote: > Add checking jq command since it is needed to validate tests > > Cc: Santosh Sivaraj > Signed-off-by: QI Fuli > Link: https://github.com/pmem/ndctl/issues/141 > --- > configure.ac | 6 ++ > 1 file changed, 6 insertions(+) Hm, I think I prefer how you did it in v1. i.e. no configure.ac check. In my view, configure.ac tests are for the core things needed to /run/ ndctl on a system. Development unit tests can just continue to use check_prereq as you did. So I'll pick up v1 of this for now - if you want me to do something else please let me know! > > diff --git a/configure.ac b/configure.ac > index 5ec8d2f..839836b 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -65,6 +65,12 @@ fi > AC_SUBST([XMLTO]) > fi > > +AC_CHECK_PROG(JQ, [jq], [$(which jq)], [missing]) > +if test "x$JQ" = xmissing; then > + AC_MSG_ERROR([jq command needed to validate tests]) > +fi > +AC_SUBST([JQ]) > + > AC_C_TYPEOF > AC_DEFINE([HAVE_STATEMENT_EXPR], 1, [Define to 1 if you have statement > expressions.]) > > -- > 2.29.2 > ___ > Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org > To unsubscribe send an email to linux-nvdimm-le...@lists.01.org ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH v2] Expose ndctl_bus_nfit_translate_spa as a public function.
On Wed, 2021-03-03 at 13:10 -0800, Tsaur, Erwin wrote:
> The motivation is to allow access to ACPI defined NVDIMM Root Device
> _DSM Function Index 5(Translate SPA). The rest of the _DSM functions,
> which are mostly ARS related, are already public.
>
> Basically move ndctl_bus_nfit_translate_spa declaration from private.h
> to libndctl.h.
>
> Changes from V1:
> - Group function declaration in libndctl.h with other ndctl_bus_* functions.
>
> Reviewed-by: Dan Williams
> Reviewed-by: Verma, Vishal L
Hi Erwin,
I forgot to mention this for v1 but you're also missing your own Signed-
off-by. If you commit using 'git commit -s', that should automatically
add it for you.
Do you mind sending a v3 with that, and I'll get it queued up.
There's also a malformed email CC - I think you want to quote the --cc
line to git-send-email like: --cc="name "
> ---
> ndctl/lib/libndctl.sym | 4
> ndctl/lib/nfit.c | 2 +-
> ndctl/lib/private.h| 2 --
> ndctl/libndctl.h | 2 ++
> 4 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
> index 0a82616..58afb74 100644
> --- a/ndctl/lib/libndctl.sym
> +++ b/ndctl/lib/libndctl.sym
> @@ -451,3 +451,7 @@ LIBNDCTL_25 {
> ndctl_bus_clear_fw_activate_nosuspend;
> ndctl_bus_activate_firmware;
> } LIBNDCTL_24;
> +
> +LIBNDCTL_26 {
> + ndctl_bus_nfit_translate_spa;
> +} LIBNDCTL_25;
> diff --git a/ndctl/lib/nfit.c b/ndctl/lib/nfit.c
> index 6f68fcf..d85682f 100644
> --- a/ndctl/lib/nfit.c
> +++ b/ndctl/lib/nfit.c
> @@ -114,7 +114,7 @@ static int is_valid_spa(struct ndctl_bus *bus, unsigned
> long long spa)
> *
> * If success, returns zero, store dimm's @handle, and @dpa.
> */
> -int ndctl_bus_nfit_translate_spa(struct ndctl_bus *bus,
> +NDCTL_EXPORT int ndctl_bus_nfit_translate_spa(struct ndctl_bus *bus,
> unsigned long long address, unsigned int *handle, unsigned long long
> *dpa)
> {
>
>
>
>
> diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h
> index ede1300..8f4510e 100644
> --- a/ndctl/lib/private.h
> +++ b/ndctl/lib/private.h
> @@ -370,8 +370,6 @@ static inline int check_kmod(struct kmod_ctx *kmod_ctx)
> return kmod_ctx ? 0 : -ENXIO;
> }
>
>
>
>
> -int ndctl_bus_nfit_translate_spa(struct ndctl_bus *bus, unsigned long long
> addr,
> - unsigned int *handle, unsigned long long *dpa);
> struct ndctl_cmd *ndctl_bus_cmd_new_err_inj(struct ndctl_bus *bus);
> struct ndctl_cmd *ndctl_bus_cmd_new_err_inj_clr(struct ndctl_bus *bus);
> struct ndctl_cmd *ndctl_bus_cmd_new_err_inj_stat(struct ndctl_bus *bus,
> diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
> index 60e1288..87d07b7 100644
> --- a/ndctl/libndctl.h
> +++ b/ndctl/libndctl.h
> @@ -152,6 +152,8 @@ int ndctl_bus_clear_fw_activate_noidle(struct ndctl_bus
> *bus);
> int ndctl_bus_set_fw_activate_nosuspend(struct ndctl_bus *bus);
> int ndctl_bus_clear_fw_activate_nosuspend(struct ndctl_bus *bus);
> int ndctl_bus_activate_firmware(struct ndctl_bus *bus, enum ndctl_fwa_method
> method);
> +int ndctl_bus_nfit_translate_spa(struct ndctl_bus *bus, unsigned long long
> addr,
> + unsigned int *handle, unsigned long long *dpa);
>
>
>
>
> struct ndctl_dimm;
> struct ndctl_dimm *ndctl_dimm_get_first(struct ndctl_bus *bus);
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH] Expose ndctl_bus_nfit_translate_spa as a public function.
On Wed, 2021-02-24 at 15:48 -0800, Tsaur, Erwin wrote:
> The motivation is to allow access to ACPI defined NVDIMM Root Device _DSM
> Function Index 5(Translate SPA). The rest of the _DSM functions, which are
> mostly ARS related, are already public.
>
> Basically move ndctl_bus_nfit_translate_spa declaration from private.h to
> libndctl.h.
> ---
> ndctl/lib/libndctl.sym | 4
> ndctl/lib/nfit.c | 2 +-
> ndctl/lib/private.h| 2 --
> ndctl/libndctl.h | 2 ++
> 4 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
> index 0a82616..58afb74 100644
> --- a/ndctl/lib/libndctl.sym
> +++ b/ndctl/lib/libndctl.sym
> @@ -451,3 +451,7 @@ LIBNDCTL_25 {
> ndctl_bus_clear_fw_activate_nosuspend;
> ndctl_bus_activate_firmware;
> } LIBNDCTL_24;
> +
> +LIBNDCTL_26 {
> + ndctl_bus_nfit_translate_spa;
> +} LIBNDCTL_25;
> diff --git a/ndctl/lib/nfit.c b/ndctl/lib/nfit.c
> index 6f68fcf..d85682f 100644
> --- a/ndctl/lib/nfit.c
> +++ b/ndctl/lib/nfit.c
> @@ -114,7 +114,7 @@ static int is_valid_spa(struct ndctl_bus *bus, unsigned
> long long spa)
> *
> * If success, returns zero, store dimm's @handle, and @dpa.
> */
> -int ndctl_bus_nfit_translate_spa(struct ndctl_bus *bus,
> +NDCTL_EXPORT int ndctl_bus_nfit_translate_spa(struct ndctl_bus *bus,
> unsigned long long address, unsigned int *handle, unsigned long long
> *dpa)
> {
>
>
>
>
> diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h
> index ede1300..8f4510e 100644
> --- a/ndctl/lib/private.h
> +++ b/ndctl/lib/private.h
> @@ -370,8 +370,6 @@ static inline int check_kmod(struct kmod_ctx *kmod_ctx)
> return kmod_ctx ? 0 : -ENXIO;
> }
>
>
>
>
> -int ndctl_bus_nfit_translate_spa(struct ndctl_bus *bus, unsigned long long
> addr,
> - unsigned int *handle, unsigned long long *dpa);
> struct ndctl_cmd *ndctl_bus_cmd_new_err_inj(struct ndctl_bus *bus);
> struct ndctl_cmd *ndctl_bus_cmd_new_err_inj_clr(struct ndctl_bus *bus);
> struct ndctl_cmd *ndctl_bus_cmd_new_err_inj_stat(struct ndctl_bus *bus,
> diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
> index 60e1288..ee517a7 100644
> --- a/ndctl/libndctl.h
> +++ b/ndctl/libndctl.h
> @@ -237,6 +237,8 @@ unsigned long long ndctl_cmd_clear_error_get_cleared(
> struct ndctl_cmd *clear_err);
> unsigned int ndctl_cmd_ars_cap_get_clear_unit(struct ndctl_cmd *ars_cap);
> int ndctl_cmd_ars_stat_get_flag_overflow(struct ndctl_cmd *ars_stat);
> +int ndctl_bus_nfit_translate_spa(struct ndctl_bus *bus, unsigned long long
> addr,
> + unsigned int *handle, unsigned long long *dpa);
>
One nit here: can you group this with the other ndctl_bus_* function
declarations in this file?
Everything else looks good to me.
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH v2 01/13] cxl: add a cxl utility and libcxl library
On Mon, 2021-02-22 at 13:36 -0800, Ben Widawsky wrote:
[..]
>
> > +SYNOPSIS
> > +
> > +[verse]
> > +'cxl list' []
> > +
> > +Walk the CXL capable device hierarchy in the system and list all device
> > +instances along with some of their major attributes.
>
> This doesn't seem to match the above. Here it's just devices and above you
> talk
> about bridges and switches as well.
Good catch - those can be added in later when we have a sysfs
representation for them. I'll change it to say just devices for now.
[..]
> > +
> > +static void *add_cxl_memdev(void *parent, int id, const char *cxlmem_base)
> > +{
> > + const char *devname = devpath_to_devname(cxlmem_base);
> > + char *path = calloc(1, strlen(cxlmem_base) + 100);
> > + struct cxl_ctx *ctx = parent;
> > + struct cxl_memdev *memdev, *memdev_dup;
> > + char buf[SYSFS_ATTR_SIZE];
> > + struct stat st;
> > +
> > + if (!path)
> > + return NULL;
> > + dbg(ctx, "%s: base: \'%s\'\n", __func__, cxlmem_base);
> > +
> > + memdev = calloc(1, sizeof(*memdev));
> > + if (!memdev)
> > + goto err_dev;
> > + memdev->id = id;
> > + memdev->ctx = ctx;
> > +
> > + sprintf(path, "/dev/cxl/%s", devname);
> > + if (stat(path, ) < 0)
> > + goto err_read;
> > + memdev->major = major(st.st_rdev);
> > + memdev->minor = minor(st.st_rdev);
> > +
> > + sprintf(path, "%s/pmem/size", cxlmem_base);
> > + if (sysfs_read_attr(ctx, path, buf) < 0)
> > + goto err_read;
> > + memdev->pmem_size = strtoull(buf, NULL, 0);
>
> For strtoull usage and below - it certainly doesn't matter much but maybe
> using
> 10 for base would better since sysfs is ABI and therefore anything other than
> base 10 is incorrect.
Hm, I followed what libndctl does, but I think there is value in
accepting valid hex even if it is technically 'wrong' per the robustness
principle. How much do we want libcxl/libndctl to be a kernel validation
vehicle vs. just work if you can?
[..]
> > +
> > +static int cmd_help(int argc, const char **argv, struct cxl_ctx *ctx)
> > +{
> > + const char * const builtin_help_subcommands[] = {
> > + "list", NULL,
> > + };
>
> Move NULL to newline.
Yep.
>
> > +int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx)
> > +{
> > + const struct option options[] = {
> > + OPT_STRING('d', "memdev", , "memory device name",
> > + "filter by CXL memory device name"),
> > + OPT_BOOLEAN('D', "memdevs", ,
> > + "include CXL memory device info"),
> > + OPT_BOOLEAN('i', "idle", , "include idle devices"),
> > + OPT_BOOLEAN('u', "human", ,
> > + "use human friendly number formats "),
> > + OPT_END(),
> > + };
> > + const char * const u[] = {
> > + "cxl list []",
> > + NULL
> > + };
> > + struct json_object *jdevs = NULL;
> > + unsigned long list_flags;
> > + struct cxl_memdev *memdev;
> > + int i;
> > +
> > +argc = parse_options(argc, argv, options, u, 0);
>
> Tab.
>
> /me looks for .clang-format
Thanks - let me see if I can quickly adapt the kernel's .clang-format
for this and add it in for the next revision.
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH] ACPI: NFIT: Fix flexible_array.cocci warnings
On Tue, 2021-01-05 at 13:03 -0800, Dan Williams wrote:
> Julia and 0day report:
>
> Zero-length and one-element arrays are deprecated, see
> Documentation/process/deprecated.rst
> Flexible-array members should be used instead.
>
> However, a straight conversion to flexible arrays yields:
>
> drivers/acpi/nfit/core.c:2276:4: error: flexible array member in a struct
> with no named members
> drivers/acpi/nfit/core.c:2287:4: error: flexible array member in a struct
> with no named members
>
> Instead, just use plain arrays not embedded a flexible arrays.
This reads a bit awkwardly, maybe:
"Just use plain arrays instead of embedded flexible arrays."
Other than that, the patch looks looks good:
Reviewed-by: Vishal Verma
>
> Cc: Denis Efremov
> Reported-by: kernel test robot
> Reported-by: Julia Lawall
> Signed-off-by: Dan Williams
> ---
> drivers/acpi/nfit/core.c | 75
> +-
> 1 file changed, 28 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index b11b08a60684..8c5dde628405 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -2269,40 +2269,24 @@ static const struct attribute_group
> *acpi_nfit_region_attribute_groups[] = {
>
>
>
>
> /* enough info to uniquely specify an interleave set */
> struct nfit_set_info {
> - struct nfit_set_info_map {
> - u64 region_offset;
> - u32 serial_number;
> - u32 pad;
> - } mapping[0];
> + u64 region_offset;
> + u32 serial_number;
> + u32 pad;
> };
>
>
>
>
> struct nfit_set_info2 {
> - struct nfit_set_info_map2 {
> - u64 region_offset;
> - u32 serial_number;
> - u16 vendor_id;
> - u16 manufacturing_date;
> - u8 manufacturing_location;
> - u8 reserved[31];
> - } mapping[0];
> + u64 region_offset;
> + u32 serial_number;
> + u16 vendor_id;
> + u16 manufacturing_date;
> + u8 manufacturing_location;
> + u8 reserved[31];
> };
>
>
>
>
> -static size_t sizeof_nfit_set_info(int num_mappings)
> -{
> - return sizeof(struct nfit_set_info)
> - + num_mappings * sizeof(struct nfit_set_info_map);
> -}
> -
> -static size_t sizeof_nfit_set_info2(int num_mappings)
> -{
> - return sizeof(struct nfit_set_info2)
> - + num_mappings * sizeof(struct nfit_set_info_map2);
> -}
> -
> static int cmp_map_compat(const void *m0, const void *m1)
> {
> - const struct nfit_set_info_map *map0 = m0;
> - const struct nfit_set_info_map *map1 = m1;
> + const struct nfit_set_info *map0 = m0;
> + const struct nfit_set_info *map1 = m1;
>
>
>
>
> return memcmp(>region_offset, >region_offset,
> sizeof(u64));
> @@ -2310,8 +2294,8 @@ static int cmp_map_compat(const void *m0, const void
> *m1)
>
>
>
>
> static int cmp_map(const void *m0, const void *m1)
> {
> - const struct nfit_set_info_map *map0 = m0;
> - const struct nfit_set_info_map *map1 = m1;
> + const struct nfit_set_info *map0 = m0;
> + const struct nfit_set_info *map1 = m1;
>
>
>
>
> if (map0->region_offset < map1->region_offset)
> return -1;
> @@ -2322,8 +2306,8 @@ static int cmp_map(const void *m0, const void *m1)
>
>
>
>
> static int cmp_map2(const void *m0, const void *m1)
> {
> - const struct nfit_set_info_map2 *map0 = m0;
> - const struct nfit_set_info_map2 *map1 = m1;
> + const struct nfit_set_info2 *map0 = m0;
> + const struct nfit_set_info2 *map1 = m1;
>
>
>
>
> if (map0->region_offset < map1->region_offset)
> return -1;
> @@ -2361,22 +2345,22 @@ static int acpi_nfit_init_interleave_set(struct
> acpi_nfit_desc *acpi_desc,
> return -ENOMEM;
> import_guid(_set->type_guid, spa->range_guid);
>
>
>
>
> - info = devm_kzalloc(dev, sizeof_nfit_set_info(nr), GFP_KERNEL);
> + info = devm_kcalloc(dev, nr, sizeof(*info), GFP_KERNEL);
> if (!info)
> return -ENOMEM;
>
>
>
>
> - info2 = devm_kzalloc(dev, sizeof_nfit_set_info2(nr), GFP_KERNEL);
> + info2 = devm_kcalloc(dev, nr, sizeof(*info2), GFP_KERNEL);
> if (!info2)
> return -ENOMEM;
>
>
>
>
> for (i = 0; i < nr; i++) {
> struct nd_mapping_desc *mapping = _desc->mapping[i];
> - struct nfit_set_info_map *map = >mapping[i];
> - struct nfit_set_info_map2 *map2 = >mapping[i];
> struct nvdimm *nvdimm = mapping->nvdimm;
> struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
> - struct acpi_nfit_memory_map *memdev = memdev_from_spa(acpi_desc,
> - spa->range_index, i);
> + struct nfit_set_info *map = [i];
> + struct nfit_set_info2 *map2 = [i];
> + struct
[ANNOUNCE] ndctl v71
A new ndctl release is available[1].
Highlights include support for the new device-dax subdivision
functionality added in Linux in v5.10, including ways to create smaller
devdax devices using daxctl/libdaxctl, as well as creating, listing, and
restoring from a config dump, 'mappings' on these devices. Other updates
include several static analysis fixups, reworking the license
identification scheme for different sub-components, and a fix for the
reconfigure-in-place workflow which tries to retain device names.
A shortlog is appended below.
[1]: https://github.com/pmem/ndctl/releases/tag/v71
Aneesh Kumar K.V (1):
daxctl: phys_index value 0 is valid
Dan Williams (6):
build: Use asciidoc instead of asciidoctor on RHEL
ndctl/namespace: Catch attempts to sub-divide legacy / label-less capacity
Clarify COPYING
daxctl: Cleanup whitespace
Rework license identification
ndctl/namespace: Reconfigure in-place
Joao Martins (20):
libdaxctl: add daxctl_dev_set_size()
daxctl: add resize support in reconfigure-device
daxctl: add command to disable devdax device
daxctl: add command to enable devdax device
libdaxctl: add daxctl_region_create_dev()
daxctl: add command to create device
libdaxctl: add daxctl_region_destroy_dev()
daxctl: add command to destroy device
daxctl/test: Add tests for dynamic dax regions
test/daxctl-create.sh: Validate @size versus mappingX sizes
daxctl: add daxctl_dev_{get,set}_align()
util/json: Print device align
daxctl: add align support in reconfigure-device
daxctl: add align support in create-device
daxctl/test: Add a test for daxctl-create with align
libdaxctl: add mapping iterator APIs
daxctl: include mappings when listing
libdaxctl: add daxctl_dev_set_mapping()
daxctl: allow creating devices from input json
daxctl/test: add a test for daxctl-create with input file
Vishal Verma (3):
Documentation/daxctl: use option includes in reconfigure-device
daxctl/device: fix a memory leak in create-device
ndctl.spec.in: update for license reworks
Zhiqiang Liu (8):
namespace: check whether pfn|dax|btt is NULL in setup_namespace
lib/libndctl: fix memory leakage problem in add_bus
libdaxctl: fix memory leakage in add_dax_region()
dimm: fix potential fd leakage in dimm_action()
util/help: check whether strdup returns NULL in exec_man_konqueror
lib/inject: check whether cmd is created successfully
Check whether ndctl_btt_get_namespace returns NULL in callers
Check whether seed is NULL in validate_namespace_options
signature.asc
Description: This is a digitally signed message part
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH daxctl v2 0/5] daxctl: range mapping allocation
On Fri, 2020-12-18 at 02:14 +, Joao Martins wrote: > Hey, > > This series adds support for: > > 1) Listing mappings when passing -M to ´daxctl list´. These are ommited > by default. > > 2) Iteration APIs for the mappings. > > 3) Allow passing an input JSON file with the manually selected ranges > to be used when creating the device-dax instance. > > This applies on top of 'jm/devdax_subdiv' branch in github.com:pmem/ndctl.git > > Testing requires a 5.10+ kernel. > > v1 -> v2: > * List mappings only with -M|--mappings option > * Adds a unit test for --input file (while testing with -M listing too) > * Rename --restore to --input > * Add Documentation for -M and for --input > > Joao Martins (5): > libdaxctl: add mapping iterator APIs > daxctl: include mappings when listing > libdaxctl: add daxctl_dev_set_mapping() > daxctl: allow creating devices from input json > daxctl/test: add a test for daxctl-create with input file > > Documentation/daxctl/daxctl-create-device.txt | 13 +++ > Documentation/daxctl/daxctl-list.txt | 4 + > daxctl/device.c | 128 > +- > daxctl/lib/libdaxctl-private.h| 8 ++ > daxctl/lib/libdaxctl.c| 118 +++- > daxctl/lib/libdaxctl.sym | 7 ++ > daxctl/libdaxctl.h| 14 +++ > daxctl/list.c | 4 + > test/daxctl-create.sh | 31 ++- > util/json.c | 57 +++- > util/json.h | 4 + > 11 files changed, 380 insertions(+), 8 deletions(-) > Hi Joao, These all look good, applied. Thanks for the quick turnaround! -Vishal ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH ndctl v1 0/8] daxctl: Add device align and range mapping allocation
On Thu, 2020-12-17 at 11:23 +, Joao Martins wrote: > > The provisioning flow additions has some questions open about the daxctl > user interface. To summarize: > > 1) Should we always list mappings, or only list them with a -v option? Or > maybe instead of -v to use instead a new -M option which enables the > listing of mappings? > > The reason being that it can get quite verbose with a device picks a lot > of mappings, hence I would imagine this info isn't necessary for the casual > daxctl listing. I think hiding them behind a new option is probably best. And then we can have different verbosity levels turn on or off. The verbosity levels stuff is implemented in ndctl, but I don't think it is in daxctl yet, so we can just do the specific option to display mappings for now, and then revisit verbosity levels in the future if we feel like the number of options is getting out of hand. > > 2) Does the '--restore ' should instead be called it > instead '--device'? I feel the name '--restore' is too tied to one specific > way of using it when the feature can be used by a tool which wants to manage Hm, I looked at other commands that take an input file - write labels just calls it --input, so there might be value in staying consistent with that. But write-infoblock just uses stdin - so that could be another option. I'd be fine with either of those. > its own range mappings. Additionally, I was thinking that if one wants to > manually add/fixup ranges more easily that we would add > a --mapping :- sort of syntax. But I suppose this could > be added later if its really desired. Agreed with adding this later if needed. > > And with these clarified, I could respin it over. Oh and I'm missing a > mappings test as well. Sounds good I'll wait to get these in. > > It's worth mentioning that kexec will need fixing, as dax_hmem regions > created with HMAT or manually assigned with efi_fake_mem= get lost on > kexec because we do not pass the EFI_MEMMAP conventional memory ranges > to the second kernel (only runtime code/boot services). I have a > RFC patch for x86 efi handling, but should get that conversation started > after holidays. > > Joao ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH V2 0/8] fix serverl issues reported by Coverity
On Wed, 2020-11-25 at 09:00 +0800, Zhiqiang Liu wrote: > Changes: V1->V2 > - add one empty line in 1/8 patch as suggested by Jeff Moyer > . > > > Recently, we use Coverity to analysis the ndctl package. > Several issues should be resolved to make Coverity happy. > > Zhiqiang Liu (8): > namespace: check whether pfn|dax|btt is NULL in setup_namespace > lib/libndctl: fix memory leakage problem in add_bus > libdaxctl: fix memory leakage in add_dax_region() > dimm: fix potential fd leakage in dimm_action() > util/help: check whether strdup returns NULL in exec_man_konqueror > lib/inject: check whether cmd is created successfully > libndctl: check whether ndctl_btt_get_namespace returns NULL in > callers > namespace: check whether seed is NULL in validate_namespace_options > > daxctl/lib/libdaxctl.c | 3 +++ > ndctl/dimm.c | 12 +++- > ndctl/lib/inject.c | 8 > ndctl/lib/libndctl.c | 1 + > ndctl/namespace.c | 23 ++- > test/libndctl.c| 16 +++- > test/parent-uuid.c | 2 +- > util/help.c| 8 +++- > util/json.c| 3 +++ > 9 files changed, 59 insertions(+), 17 deletions(-) > Hi Zhiquiang, The patches look good, and I've applied them for v71. However one thing to note: If you're sending a v2, it is preferable to respin the whole series, even if you're only changing a subset of (even a single) patch in the series. That allows tools like 'b4' to just Do The Right Thing, and make sure all the latest patches are grabbed. In this case, especially, your cover letter promises 8 patches (0/8), but there is only one that follows. This confuses 'b4': ERROR: missing [2/8]! ERROR: missing [3/8]! ERROR: missing [4/8]! ...etc I've fixed it up manually for this, but just some things to consider for the future. Thanks, -Vishal ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH 6/8] lib/inject: check whether cmd is created successfully
On Fri, 2020-11-06 at 17:27 +0800, Zhiqiang Liu wrote:
> ndctl_bus_cmd_new_ars_cp() is called to create cmd,
> which may return NULL. We need to check whether it
> is NULL in callers, such as ndctl_namespace_get_clear_uint
> and ndctl_namespace_injection_status.
>
> Signed-off-by: Zhiqiang Liu
> ---
> ndctl/lib/inject.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/ndctl/lib/inject.c b/ndctl/lib/inject.c
> index 815f254..b543fc7 100644
> --- a/ndctl/lib/inject.c
> +++ b/ndctl/lib/inject.c
> @@ -114,6 +114,10 @@ static int ndctl_namespace_get_clear_unit(struct
> ndctl_namespace *ndns)
> if (rc)
> return rc;
> cmd = ndctl_bus_cmd_new_ars_cap(bus, ns_offset, ns_size);
> + if (!cmd) {
> + err(ctx, "bus: %s failed to create cmd\n",
> ndctl_bus_get_provider(bus));
> + return -ENOTTY;
> + }
> rc = ndctl_cmd_submit(cmd);
> if (rc < 0) {
> dbg(ctx, "Error submitting ars_cap: %d\n", rc);
> @@ -457,6 +461,10 @@ NDCTL_EXPORT int ndctl_namespace_injection_status(struct
> ndctl_namespace *ndns)
> return rc;
>
> cmd = ndctl_bus_cmd_new_ars_cap(bus, ns_offset, ns_size);
> + if (!cmd) {
> + err(ctx, "bus: %s failed to create cmd\n",
> ndctl_bus_get_provider(bus));
> + return -ENOTTY;
> + }
> rc = ndctl_cmd_submit(cmd);
> if (rc < 0) {
> dbg(ctx, "Error submitting ars_cap: %d\n", rc);
This looks good in general, but I made some small fixups while applying.
Printing the bus provider here isn't as useful - I replaced it with
printing the namespace 'devname':
- err(ctx, "bus: %s failed to create cmd\n",
ndctl_bus_get_provider(bus));
+ err(ctx, "%s: failed to create cmd\n",
+ ndctl_namespace_get_devname(ndns));
Also fixed up a couple of typos in commit messages, but otherwise the
series looks good and I've applied it for v71.
Thanks,
-Vishal
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH daxctl v2 0/5] daxctl: device align support
On Wed, 2020-12-16 at 22:48 +, Joao Martins wrote:
> Hey,
>
> This series adds support for:
>
> 1) {create,reconfigure}-device for selecting @align (hugepage size).
> Here we add a '-a|--align 4K|2M|1G' option to the existing commands;
>
> 2) Listing now displays align (if supported).
>
> Testing requires a 5.10+ kernel.
>
> v1 -> v2:
> * Fix listing of dax devices on kernels that don't support align;
> * Adds a unit test for align;
> * Remove the mapping part to a later series;
>
These all look good to me, I've applied them and pushed a branch out,
'jm/devdax_subdiv'. As Dan mentioned in the other thread, if you had a
respin of the provisioning flows patches, we can add them in.
Thanks for the quick turnaround on these!
> Joao Martins (5):
> daxctl: add daxctl_dev_{get,set}_align()
> util/json: Print device align
> daxctl: add align support in reconfigure-device
> daxctl: add align support in create-device
> daxctl/test: Add a test for daxctl-create with align
>
> Documentation/daxctl/daxctl-create-device.txt | 8 +
> Documentation/daxctl/daxctl-reconfigure-device.txt | 12
> daxctl/device.c| 32 ---
> daxctl/lib/libdaxctl-private.h | 1 +
> daxctl/lib/libdaxctl.c | 36
> ++
> daxctl/lib/libdaxctl.sym | 2 ++
> daxctl/libdaxctl.h | 2 ++
> test/daxctl-create.sh | 29 +
> util/json.c| 9 +-
> 9 files changed, 125 insertions(+), 6 deletions(-)
>
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH ndctl v1 0/8] daxctl: Add device align and range mapping allocation
On Wed, 2020-12-16 at 11:39 +, Joao Martins wrote:
> On 7/16/20 7:46 PM, Joao Martins wrote:
> > Hey,
> >
> > This series builds on top of this one[0] and does the following improvements
> > to the Soft-Reserved subdivision:
> >
> > 1) Support for {create,reconfigure}-device for selecting @align (hugepage
> > size).
> > Here we add a '-a|--align 4K|2M|1G' option to the existing commands;
> >
> > 2) Listing improvements for device alignment and mappings;
> > Note: Perhaps it is better to hide the mappings by default, and only
> >print with -v|--verbose. This would align with ndctl, as the mappings
> >info can be quite large.
> >
> > 3) Allow creating devices from selecting ranges. This allows to keep the
> >same GPA->HPA mapping as before we kexec the hypervisor with running
> > guests:
> >
> >daxctl list -d dax0.1 > /var/log/dax0.1.json
> >kexec -d -l bzImage
> >systemctl kexec
> >daxctl create -u --restore /var/log/dax0.1.json
> >
> >The JSON was what I though it would be easier for an user, given that it
> > is
> >the data format daxctl outputs. Alternatives could be adding multiple:
> > --mapping :-
> >
> >But that could end up in a gigantic line and a little more
> >unmanageable I think.
> >
> > This series requires this series[0] on top of Dan's patches[1]:
> >
> > [0]
> > https://lore.kernel.org/linux-nvdimm/20200716172913.19658-1-joao.m.mart...@oracle.com/
> > [1]
> > https://lore.kernel.org/linux-nvdimm/159457116473.754248.7879464730875147365.st...@dwillia2-desk3.amr.corp.intel.com/
> >
> > The only TODO here is docs and improving tests to validate mappings, and
> > test
> > the restore path.
> >
> > Suggestions/comments are welcome.
> >
> There's a couple of issues in this series regarding daxctl-reconfigure
> options and
> breakage of ndctl with kernels (<5.10) that do not supply a device @align
> upon testing
> with NVDIMMs. Plus it is missing daxctl-create.sh unit test for @align.
>
> I will fix those and respin, and probably take out the last patch as it's
> more RFC-ish and
> in need of feedback.
Sounds good. Any objections to releasing v70 with the initial support,
and then adding this series on for the next one? I'm thinking I'll do a
much quicker v72 release say in early January with this and anything
else that missed v71.
>
> Joao
>
> > Joao Martins (8):
> > daxctl: add daxctl_dev_{get,set}_align()
> > util/json: Print device align
> > daxctl: add align support in reconfigure-device
> > daxctl: add align support in create-device
> > libdaxctl: add mapping iterator APIs
> > daxctl: include mappings when listing
> > libdaxctl: add daxctl_dev_set_mapping()
> > daxctl: Allow restore devices from JSON metadata
> >
> > daxctl/device.c| 154
> > +++--
> > daxctl/lib/libdaxctl-private.h | 9 +++
> > daxctl/lib/libdaxctl.c | 152
> > +++-
> > daxctl/lib/libdaxctl.sym | 9 +++
> > daxctl/libdaxctl.h | 16 +
> > util/json.c| 63 -
> > util/json.h| 3 +
> > 7 files changed, 396 insertions(+), 10 deletions(-)
> >
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH ndctl v2 10/10] daxctl/test: Add tests for dynamic dax regions
On Thu, 2020-12-10 at 15:01 +, Joao Martins wrote: > On 7/21/20 5:49 PM, Joao Martins wrote: > > On 7/13/20 5:08 PM, Joao Martins wrote: > > > Add a couple tests which exercise the new sysfs based > > > interface for Soft-Reserved regions (by EFI/HMAT, or > > > efi_fake_mem). > > > > > > The tests exercise the daxctl orchestration surrounding > > > for creating/disabling/destroying/reconfiguring devices. > > > Furthermore it exercises dax region space allocation > > > code paths particularly: > > > > > > 1) zeroing out and reconfiguring a dax device from > > > its current size to be max available and back to initial > > > size > > > > > > 2) creates devices from holes in the beginning, > > > middle of the region. > > > > > > 3) reconfigures devices in a interleaving fashion > > > > > > 4) test adjust of the region towards beginning and end > > > > > > The tests assume you pass a valid efi_fake_mem parameter > > > marked as EFI_MEMORY_SP e.g. > > > > > > efi_fake_mem=112G@16G:0x4 > > > > > > Naturally it bails out from the test if hmem device driver > > > isn't loaded or builtin. If hmem regions are found, only > > > region 0 is used, and the others remain untouched. > > > > > > Signed-off-by: Joao Martins > > > > Following your suggestion[0], I added a couple more validations > > to this test suite, covering the mappings. So on top of this patch > > I added the following snip below the scissors mark. Mainly, I check > > that the size calculated by mapping is the same as advertised by > > the sysfs size attribute, thus looping through all the mappings. > > > > Perhaps it would be enough to have just such validation in setup_dev() > > to catch the bug in [0]. But I went ahead and also validated the test > > cases where a certain amount of mappings are meant to be created. > > > > My only worry is the last piece in daxctl_test_adjust() where we might > > be tying too much on how a kernel version picks space from the region; > > should this logic change in an unforeseeable future (e.g. allowing space > > at the beginning to be adjusted). Otherwise, if this no concern, let me > > know and I can resend a v3 with the adjustment below. > > > > Ping? Hi Joao, Thanks for the patience on these, I've gone through the patches in preparation for the next release, and they all look mostly fine. I had a few minor fixups - to the documentation and the test (fixup module name, and shellcheck complaints). I've appended a diff below of all the fixups I added. I've also included the patch below for the mapping size validation. I think the concern for future kernel layout changes is valid, but if/when that happens, we can always come back and relax or adjust the test as needed. So for now, I think having a pickier test should be better than not having one. > > > ->8-- > > Subject: Validate @size versus mappingX sizes > > > > [0] > > https://lore.kernel.org/linux-nvdimm/CAPcyv4hFS7JS9s7cUY=2ru2kutrsesxwx1pgnnc_tudjjod...@mail.gmail.com/ > > > > --- > > > > test/daxctl-create.sh | 64 > > +++- > > 1 file changed, 63 insertions(+), 1 deletion(-) > > My fixups: --- Documentation/daxctl/daxctl-create-device.txt | 18 +++--- Documentation/daxctl/daxctl-destroy-device.txt | 22 -- Documentation/daxctl/daxctl-disable-device.txt | 22 -- Documentation/daxctl/daxctl-enable-device.txt | 22 -- Documentation/daxctl/daxctl-reconfigure-device.txt | 19 --- Documentation/daxctl/human-option.txt | 8 +++ Documentation/daxctl/region-option.txt | 8 +++ Documentation/daxctl/verbose-option.txt| 5 util/filter.c | 2 +- test/daxctl-create.sh | 76 ++-- 10 files changed, 82 insertions(+), 120 deletions(-) --- diff --git a/Documentation/daxctl/daxctl-create-device.txt b/Documentation/daxctl/daxctl-create-device.txt index 648d254..70029ab 100644 --- a/Documentation/daxctl/daxctl-create-device.txt +++ b/Documentation/daxctl/daxctl-create-device.txt @@ -71,12 +71,7 @@ EFI memory map with EFI_MEMORY_SP. The resultant ranges mean that it's OPTIONS --- --r:: ---region=:: - Restrict the operation to devices belonging to the specified region(s). - A device-dax region is a contiguous range of memory that hosts one or - more /dev/daxX.Y devices, where X is the region id and Y is the device - instance id. +include::region-option.txt[] -s:: --size=:: @@ -87,16 +82,9 @@ OPTIONS The size must be a multiple of the region alignment. --u:: ---human:: - By default the command will output machine-friendly raw-integer - data. Instead, with this flag, numbers representing storage size - will be formatted as human readable
Re: [PATCH ndctl v2 10/10] daxctl/test: Add tests for dynamic dax regions
On Thu, 2020-12-10 at 15:01 +, Joao Martins wrote: > On 7/21/20 5:49 PM, Joao Martins wrote: > > On 7/13/20 5:08 PM, Joao Martins wrote: > > > Add a couple tests which exercise the new sysfs based > > > interface for Soft-Reserved regions (by EFI/HMAT, or > > > efi_fake_mem). > > > > > > The tests exercise the daxctl orchestration surrounding > > > for creating/disabling/destroying/reconfiguring devices. > > > Furthermore it exercises dax region space allocation > > > code paths particularly: > > > > > > 1) zeroing out and reconfiguring a dax device from > > > its current size to be max available and back to initial > > > size > > > > > > 2) creates devices from holes in the beginning, > > > middle of the region. > > > > > > 3) reconfigures devices in a interleaving fashion > > > > > > 4) test adjust of the region towards beginning and end > > > > > > The tests assume you pass a valid efi_fake_mem parameter > > > marked as EFI_MEMORY_SP e.g. > > > > > > efi_fake_mem=112G@16G:0x4 > > > > > > Naturally it bails out from the test if hmem device driver > > > isn't loaded or builtin. If hmem regions are found, only > > > region 0 is used, and the others remain untouched. > > > > > > Signed-off-by: Joao Martins > > > > Following your suggestion[0], I added a couple more validations > > to this test suite, covering the mappings. So on top of this patch > > I added the following snip below the scissors mark. Mainly, I check > > that the size calculated by mapping is the same as advertised by > > the sysfs size attribute, thus looping through all the mappings. > > > > Perhaps it would be enough to have just such validation in setup_dev() > > to catch the bug in [0]. But I went ahead and also validated the test > > cases where a certain amount of mappings are meant to be created. > > > > My only worry is the last piece in daxctl_test_adjust() where we might > > be tying too much on how a kernel version picks space from the region; > > should this logic change in an unforeseeable future (e.g. allowing space > > at the beginning to be adjusted). Otherwise, if this no concern, let me > > know and I can resend a v3 with the adjustment below. > > > > Ping? Hi Joao, Thanks for the patience on these, I've gone through the patches in preparation for the next release, and they all look mostly fine. I had a few minor fixups - to the documentation and the test (fixup module name, and shellcheck complaints). I've appended a diff below of all the fixups I added. I've also included the patch below for the mapping size validation. I think the concern for future kernel layout changes is valid, but if/when that happens, we can always come back and relax or adjust the test as needed. So for now, I think having a pickier test should be better than not having one. > > > ->8-- > > Subject: Validate @size versus mappingX sizes > > > > [0] > > https://lore.kernel.org/linux-nvdimm/CAPcyv4hFS7JS9s7cUY=2ru2kutrsesxwx1pgnnc_tudjjod...@mail.gmail.com/ > > > > --- > > > > test/daxctl-create.sh | 64 > > +++- > > 1 file changed, 63 insertions(+), 1 deletion(-) > > My fixups: --- Documentation/daxctl/daxctl-create-device.txt | 18 +++--- Documentation/daxctl/daxctl-destroy-device.txt | 22 -- Documentation/daxctl/daxctl-disable-device.txt | 22 -- Documentation/daxctl/daxctl-enable-device.txt | 22 -- Documentation/daxctl/daxctl-reconfigure-device.txt | 19 --- Documentation/daxctl/human-option.txt | 8 +++ Documentation/daxctl/region-option.txt | 8 +++ Documentation/daxctl/verbose-option.txt| 5 util/filter.c | 2 +- test/daxctl-create.sh | 76 ++-- 10 files changed, 82 insertions(+), 120 deletions(-) --- diff --git a/Documentation/daxctl/daxctl-create-device.txt b/Documentation/daxctl/daxctl-create-device.txt index 648d254..70029ab 100644 --- a/Documentation/daxctl/daxctl-create-device.txt +++ b/Documentation/daxctl/daxctl-create-device.txt @@ -71,12 +71,7 @@ EFI memory map with EFI_MEMORY_SP. The resultant ranges mean that it's OPTIONS --- --r:: ---region=:: - Restrict the operation to devices belonging to the specified region(s). - A device-dax region is a contiguous range of memory that hosts one or - more /dev/daxX.Y devices, where X is the region id and Y is the device - instance id. +include::region-option.txt[] -s:: --size=:: @@ -87,16 +82,9 @@ OPTIONS The size must be a multiple of the region alignment. --u:: ---human:: - By default the command will output machine-friendly raw-integer - data. Instead, with this flag, numbers representing storage size - will be formatted as human readable
Re: [ndctl PATCH 0/8] fix serverl issues reported by Coverity
On Fri, 2020-11-20 at 10:45 +0800, Zhiqiang Liu wrote: > Friendly ping... > > I just wonder if this kind of patches will not be reviewed > and processed. > > I`d be very happy to receive any feedback. Hi Zhiqiang, These are definitely on my list to look at, I've just not had the time to do so yet. I'm hoping to clear up my ndctl backlog in the next couple of weeks though and get a v71 release out. Thanks for the patience! -Vishal > > On 2020/11/6 17:23, Zhiqiang Liu wrote: > > Recently, we use Coverity to analysis the ndctl package. > > Several issues should be resolved to make Coverity happy. > > > > lihaotian9 (8): > > namespace: check whether pfn|dax|btt is NULL in setup_namespace > > lib/libndctl: fix memory leakage problem in add_bus > > libdaxctl: fix memory leakage in add_dax_region() > > dimm: fix potential fd leakage in dimm_action() > > util/help: check whether strdup returns NULL in exec_man_konqueror > > lib/inject: check whether cmd is created successfully > > libndctl: check whether ndctl_btt_get_namespace returns NULL in > > callers > > namespace: check whether seed is NULL in validate_namespace_options > > > > daxctl/lib/libdaxctl.c | 3 +++ > > ndctl/dimm.c | 12 +++- > > ndctl/lib/inject.c | 8 > > ndctl/lib/libndctl.c | 1 + > > ndctl/namespace.c | 23 ++- > > test/libndctl.c| 16 +++- > > test/parent-uuid.c | 2 +- > > util/help.c| 8 +++- > > util/json.c| 3 +++ > > 9 files changed, 59 insertions(+), 17 deletions(-) > > > ___ > Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org > To unsubscribe send an email to linux-nvdimm-le...@lists.01.org ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH 1/1] ACPI/nfit: correct the badrange to be reported in nfit_handle_mce()
On Wed, 2020-11-18 at 16:41 +0800, Zhen Lei wrote: > The badrange to be reported should always cover mce->addr. > > Signed-off-by: Zhen Lei > --- > drivers/acpi/nfit/mce.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Ah good find, agreed with Dan that this is stable material. Minor nit, I'd recommend rewording the subject line to something like: "acpi/nfit: fix badrange insertion in nfit_handle_mce()" Otherwise, looks good to me. Reviewed-by: Vishal Verma > > diff --git a/drivers/acpi/nfit/mce.c b/drivers/acpi/nfit/mce.c > index ee8d9973f60b..053e719c7bea 100644 > --- a/drivers/acpi/nfit/mce.c > +++ b/drivers/acpi/nfit/mce.c > @@ -63,7 +63,7 @@ static int nfit_handle_mce(struct notifier_block *nb, > unsigned long val, > > /* If this fails due to an -ENOMEM, there is little we can do */ > nvdimm_bus_add_badrange(acpi_desc->nvdimm_bus, > - ALIGN(mce->addr, L1_CACHE_BYTES), > + ALIGN_DOWN(mce->addr, L1_CACHE_BYTES), > L1_CACHE_BYTES); > nvdimm_region_notify(nfit_spa->nd_region, > NVDIMM_REVALIDATE_POISON); ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH -next] ACPI: NFIT: Fix judgment of rc is '-ENXIO'
On Tue, 2020-10-27 at 21:49 +0800, Zhang Qilong wrote: > Initial value of rc is '-ENXIO', and we should > use the initial value to check it. > > Signed-off-by: Zhang Qilong > --- > drivers/acpi/nfit/core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Looks good, Reviewed-by: Vishal Verma > > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c > index 756227837b3b..3a3c209ed3d3 100644 > --- a/drivers/acpi/nfit/core.c > +++ b/drivers/acpi/nfit/core.c > @@ -1564,7 +1564,7 @@ static ssize_t format1_show(struct device *dev, > le16_to_cpu(nfit_dcr->dcr->code)); > break; > } > - if (rc != ENXIO) > + if (rc != -ENXIO) > break; > } > mutex_unlock(_desc->init_mutex); ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH 0/4] Remove nrexceptional tracking
On Thu, 2020-10-08 at 20:33 +0100, Matthew Wilcox wrote: > On Thu, Aug 06, 2020 at 08:16:02PM +0000, Verma, Vishal L wrote: > > On Thu, 2020-08-06 at 19:44 +0000, Verma, Vishal L wrote: > > > > I'm running xfstests on this patchset right now. If one of the DAX > > > > people could try it out, that'd be fantastic. > > > > > > > > Matthew Wilcox (Oracle) (4): > > > > mm: Introduce and use page_cache_empty > > > > mm: Stop accounting shadow entries > > > > dax: Account DAX entries as nrpages > > > > mm: Remove nrexceptional from inode > > > > > > Hi Matthew, > > > > > > I applied these on top of 5.8 and ran them through the nvdimm unit test > > > suite, and saw some test failures. The first failing test signature is: > > > > > > + umount test_dax_mnt > > > ./dax-ext4.sh: line 62: 15749 Segmentation fault umount $MNT > > > FAIL dax-ext4.sh (exit status: 139) > > Thanks. Fixed: > > +++ b/fs/dax.c > @@ -644,7 +644,7 @@ static int __dax_invalidate_entry(struct address_space > *mapping, > goto out; > dax_disassociate_entry(entry, mapping, trunc); > xas_store(, NULL); > - mapping->nrpages -= dax_entry_order(entry); > + mapping->nrpages -= 1UL << dax_entry_order(entry); > ret = 1; > out: > put_unlocked_entry(, entry); > > Updated git tree at > https://git.infradead.org/users/willy/pagecache.git/ I ran this tree through the unit tests, and everything passes. (Well, while the tests passed, this tree as-is did have an RCU warning splat. I rebased to v5.9-rc8 and that was fine). Feel free to add: Tested-by: Vishal Verma ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH] libndctl: Fix probe of non-nfit nvdimms
On Fri, 2020-10-09 at 17:30 +0530, Vaibhav Jain wrote:
> commit 107a24ff429f ("ndctl/list: Add firmware activation
> enumeration") introduced changes in add_dimm() to enumerate the status
> of firmware activation. However a branch added in that commit broke
> the probe for non-nfit nvdimms like one provided by papr-scm. This
> cause an error reported when listing namespaces like below:
>
> $ sudo ndctl list
> libndctl: add_dimm: nmem0: probe failed: No such device
> libndctl: __sysfs_device_parse: nmem0: add_dev() failed
>
> Do a fix for this by removing the offending branch in the add_dimm()
> patch. This continues the flow of add_dimm() probe even if the nfit is
> not detected on the associated bus.
>
> Fixes: 107a24ff429fa("ndctl/list: Add firmware activation enumeration")
> Signed-off-by: Vaibhav Jain
> ---
> ndctl/lib/libndctl.c | 3 ---
> 1 file changed, 3 deletions(-)
Ah apologies - this snuck in when I reflowed Dan's patches on top of the
papr work for v70.
I expect you'd like a point release with this fix asap?
Is there a way for me to incorporate some papr unit tests into my
release workflow so I can avoid breaking things like this again?
I'll also try to do a better job of pushing things out to the pending
branch more frequently so if you're monitoring that branch, hopefully
things like this will get caught before a release happens :)
>
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index 554696386f48..ad521d365ee4 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -1875,9 +1875,6 @@ static void *add_dimm(void *parent, int id, const char
> *dimm_base)
> else
> dimm->fwa_result = fwa_result_to_result(buf);
>
> - if (!ndctl_bus_has_nfit(bus))
> - goto out;
> -
> /* Check if the given dimm supports nfit */
> if (ndctl_bus_has_nfit(bus)) {
> dimm->formats = formats;
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH] build: Use asciidoc instead of asciidoctor on RHEL
On Tue, 2020-10-06 at 17:15 -0700, Dan Williams wrote:
> Until RHEL moves to asciidoctor fallback to the old asciidoc for RHEL
> builds.
>
> Signed-off-by: Dan Williams
> ---
> ndctl.spec.in |7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
Thanks Dan - looks good!
I applied this and tested with a copr build for el8, and it all looks
good:
https://download.copr.fedorainfracloud.org/results/djbw/ndctl/epel-8-x86_64/01698754-ndctl/
> diff --git a/ndctl.spec.in b/ndctl.spec.in
> index 94e15ad309c5..056c53069082 100644
> --- a/ndctl.spec.in
> +++ b/ndctl.spec.in
> @@ -9,7 +9,12 @@ Source0:
> https://github.com/pmem/%{name}/archive/v%{version}.tar.gz#/%{name}-%{v
> Requires:LNAME%{?_isa} = %{version}-%{release}
> Requires:DAX_LNAME%{?_isa} = %{version}-%{release}
> BuildRequires: autoconf
> +%if 0%{?rhel} < 9
> +BuildRequires: asciidoc
> +%define asciidoc --disable-asciidoctor
> +%else
> BuildRequires: rubygem-asciidoctor
> +%endif
> BuildRequires: xmlto
> BuildRequires: automake
> BuildRequires: libtool
> @@ -86,7 +91,7 @@ control API for these devices.
> %build
> echo %{version} > version
> ./autogen.sh
> -%configure --disable-static --disable-silent-rules
> +%configure --disable-static --disable-silent-rules %{?asciidoc}
> make %{?_smp_mflags}
>
> %install
>
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
[ANNOUNCE] ndctl v70
A new release of ndctl is available [1]. Highlights include support for the new firmware activation facility, a new 'split-acpi' command in 'daxctl'to aid testing and debugging, and other minor fixes. A shortlog is appended below. [1]: https://github.com/pmem/ndctl/releases/tag/v70 Dan Williams (15): ndctl/dimm: Fix chatty status messages ndctl/list: Indicate firmware update capability ndctl/dimm: Detect firmware-update vs ARS conflict ndctl/dimm: Improve firmware-update failure message ndctl/dimm: Prepare to emit dimm json object after firmware update ndctl/dimm: Emit dimm firmware details after update ndctl/list: Add firmware activation enumeration ndctl/dimm: Auto-arm firmware activation ndctl/bus: Add 'activate-firmware' command ndctl/test: Test firmware-activation interface ndctl/docs: Update copyright date test: Validate strict iomem protections of pmem ndctl: Refactor nfit.h to acpi.h daxctl: Add 'split-acpi' command to generate custom ACPI tables test/ndctl: mremap pmd confusion Santosh Sivaraj (1): test: Remove a redundant ndctl_namespace_foreach Vishal Verma (3): ndctl/contrib: update 'prepare-release' for merge workflow libndctl: fix a potential buffer overflow ndctl/inject-error: remove logically dead code ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: 回复：regression caused by patch 6180bb446ab624b9ab8bf201ed251ca87f07b413?? ("dax: fix detection of dax support for non-persistent memory block?? devices")
On Tue, 2020-09-15 at 10:01 +0200, Jan Kara wrote:
> Hi!
>
> On Tue 15-09-20 11:03:29, col...@suse.de wrote:
> > Could you please to take a look? I am offline in the next two weeks.
>
> I just had a look into this. IMHO the justification in 6180bb446a "dax: fix
> detection of dax support for non-persistent memory block devices" is just
> bogus and people got confused by the previous condition
>
> if (!dax_dev && !bdev_dax_supported(bdev, blocksize))
>
> which was bogus as well. bdev_dax_supported() always returns false for bdev
> that doesn't have dax_dev (naturally so). So in the original condition
> there was no point in calling bdev_dax_supported() if we know dax_dev is
> NULL.
>
> Then this was changed to:
>
> if (!dax_dev || !bdev_dax_supported(bdev, blocksize))
>
> which looks more sensible at the first sight. But only at the first sight -
> if you look at wider context, __generic_fsdax_supported() is the bulk of
> code that decides whether a device supports DAX so calling
> bdev_dax_supported() from it indeed doesn't look as such a great idea. So
> IMO the condition should be just:
>
> if (!dax_dev)
>
> I'll send a fix for this.
>
> Also there's the process question how this patch could get to Linus when
> any attempt to use DAX would immediately kill the machine like Mikulas
> spotted. This shows the that patch was untested with DAX by anybody on the
> path from the developer to Linus...
Hi Jan,
This was entirely my fault, and I apologize. I got confused as to what
state my branches were in, and I thought this had cleared our unit tests
etc, when it obviously hadn't. I'm going to take a harder look at my
personal branch/patch management process to make sure it doesn't happen
again!
Thanks for taking a look.
-Vishal
>
> Honza
>
> > 原始邮件
> > 发件人： Mikulas Patocka
> > 日期： 2020年9月14日周一半夜11:48
> > 收件人： Coly Li , Dan Williams ,
> > Dave Jiang
> > 抄送： Jan Kara , Vishal Verma ,
> > Adrian Huang , Ira Weiny , Mike
> > Snitzer , Pankaj Gupta ,
> > linux-nvdimm@lists.01.org
> > 主题： regression caused by patch 6180bb446ab624b9ab8bf201ed251ca87f07b413
> > ("dax: fix detection of dax support for non-persistent memory block
> > devices")
> >
> > Hi
> >
> > The patch 6180bb446ab624b9ab8bf201ed251ca87f07b413 ("dax: fix detection
> > of
> > dax support for non-persistent memory block devices") causes crash when
> > attempting to mount the ext4 filesystem on /dev/pmem0 ("mkfs.ext4
> > /dev/pmem0; mount -t ext4 /dev/pmem0 /mnt/test"). The device /dev/pmem0
> > is
> > emulated using the "memmap" kernel parameter.
> >
> > The patch causes infinite recursion and double-fault exception:
> >
> > __generic_fsdax_supported
> > bdev_dax_supported
> > __bdev_dax_supported
> > dax_supported
> > dax_dev->ops->dax_supported
> > generic_fsdax_supported
> > __generic_fsdax_supported
> >
> > Mikulas
> >
> >
> >
> > [ 17.500619] traps: PANIC: double fault, error_code: 0x0
> > [ 17.500619] double fault: [#1] PREEMPT SMP
> > [ 17.500620] CPU: 0 PID: 1326 Comm: mount Not tainted
> > 5.9.0-rc1-bisect #
> > 10
> > [ 17.500620] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
> > [ 17.500621] RIP: 0010:__generic_fsdax_supported+0x6a/0x500
> > [ 17.500622] Code: ff ff ff ff ff 7f 00 48 21 f3 48 01 c3 48 c1 e3 09
> > f6
> > c7 0e 0f 85 fa 01 00 00 48 85 ff 49 89 fd 74 11 be 00 10 00 00 4c 89 e7
> > b1 fe ff ff 84 c0 75 11 31 c0 48 83 c4 48 5b 5d 41 5c 41 5d 41
> > [ 17.500623] RSP: 0018:88940b4fdff8 EFLAGS: 00010286
> > [ 17.500624] RAX: RBX: 0007f000 RCX:
> >
> > [ 17.500625] RDX: 1000 RSI: 1000 RDI:
> > 88940b34c300
> > [ 17.500625] RBP: R08: 0400 R09:
> > 8080808080808080
> > [ 17.500626] R10: R11: fefefefefefefeff R12:
> > 88940b34c300
> > [ 17.500626] R13: 88940b3dc000 R14: 88940badd000 R15:
> > 0001
> > [ 17.500627] FS: f7c25780() GS:88940fa0()
> > knlGS:
> > [ 17.500628] CS: 0010 DS: 002b ES: 002b CR0: 80050033
> > [ 17.500628] CR2: 88940b4fdfe8 CR3: 00140bd15000 CR4:
> > 06b0
> > [ 17.500628] Call Trace:
> > [ 17.500629] Modules linked in: uvesafb cfbfillrect cfbimgblt cn
> > cfbcopyarea fb fbdev ipv6 tun autofs4 binfmt_misc configfs af_packet
> > virtio_rng rng_core mousedev evdev pcspkr virtio_balloon button raid10
> > raid456 async_raid6_recov async_memcpy async_pq raid6_pq async_xor xor
> > async_tx libcrc32c raid1 raid0 md_mod sd_mod t10_pi virtio_scsi
> > virtio_net
> > net_failover psmouse scsi_mod failover
> > [ 17.500638] ---[ end trace 3c877fcb5b865459 ]---
> > [
[GIT PULL] libnvdimm fix for v5.9-rc5
Hi Linus, please pull from:
git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git
tags/libnvdimm-fix-v5.9-rc5
...to receive another fix for detection of DAX support for block
devices. The previous fix in this area (merged in -rc3) was incomplete,
and this should finally address all the problems.
---
The following changes since commit c2affe920b0e0669650943ac086215cf6519be34:
dax: do not print error message for non-persistent memory block device
(2020-08-20 11:43:18 -0600)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git
tags/libnvdimm-fix-v5.9-rc5
for you to fetch changes up to 6180bb446ab624b9ab8bf201ed251ca87f07b413:
dax: fix detection of dax support for non-persistent memory block devices
(2020-09-03 12:28:03 -0600)
libnvdimm fix for v5.9-rc5
Fix decetion of dax support for block devices. Previous fixes in this
area, which only affected printing of debug messages, had an incorrect
condition for detection of dax. This fix should finally do the right thing.
Coly Li (1):
dax: fix detection of dax support for non-persistent memory block devices
drivers/dax/super.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dax/super.c b/drivers/dax/super.c
index 32642634c1bb..e5767c83ea23 100644
--- a/drivers/dax/super.c
+++ b/drivers/dax/super.c
@@ -100,7 +100,7 @@ bool __generic_fsdax_supported(struct dax_device *dax_dev,
return false;
}
- if (!dax_dev && !bdev_dax_supported(bdev, blocksize)) {
+ if (!dax_dev || !bdev_dax_supported(bdev, blocksize)) {
pr_debug("%s: error: dax unsupported by block device\n",
bdevname(bdev, buf));
return false;
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: flood of "dm-X: error: dax access failed" due to 5.9 commit 231609785cbfb
On Thu, 2020-09-03 at 00:40 +0800, Coly Li wrote:
> On 2020/9/3 00:04, Mike Snitzer wrote:
> > 5.9 commit 231609785cbfb ("dax: print error message by pr_info() in
> > __generic_fsdax_supported()") switched from pr_debug() to pr_info().
> >
> > The justification in the commit header is really inadequate. If there
> > is a problem that you need to drill in on, repeat the testing after
> > enabling the dynamic debugging.
> >
> > Otherwise, now all DM devices that aren't layered on DAX capable devices
> > spew really confusing noise to users when they simply activate their
> > non-DAX DM devices:
> >
> > [66567.129798] dm-6: error: dax access failed (-5)
> > [66567.134400] dm-6: error: dax access failed (-5)
> > [66567.139152] dm-6: error: dax access failed (-5)
> > [66567.314546] dm-2: error: dax access failed (-95)
> > [66567.319380] dm-2: error: dax access failed (-95)
> > [66567.324254] dm-2: error: dax access failed (-95)
> > [66567.479025] dm-2: error: dax access failed (-95)
> > [66567.483713] dm-2: error: dax access failed (-95)
> > [66567.488722] dm-2: error: dax access failed (-95)
> > [66567.494061] dm-2: error: dax access failed (-95)
> > [66567.498823] dm-2: error: dax access failed (-95)
> > [66567.503693] dm-2: error: dax access failed (-95)
> >
> > commit 231609785cbfb must be reverted.
> >
> > Please advise, thanks.
>
> Adrian Huang from Lenovo posted a patch, which titled: dax: do not print
> error message for non-persistent memory block device
>
> It fixes the issue, but no response for now. Maybe we should take this fix.
>
Mike, Coly,
I applied Adrians patch, and submitted it - it is already in v5.9-rc3 -
c2affe920b0e dax: do not print error message for non-persistent memory block
device
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
[GIT PULL] libnvdimm fixes for v5.9-rc3
Hi Linus, please pull from:
git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git
tags/libnvdimm-fix-v5.9-rc3
...to receive a couple of minor fixes for things merged in 5.9-rc1. One
is an out-of-bounds access caught by KASAN, and the second is a tweak to
some overzealous logging about dax support even for traditional block
devices which was unnecessary. These have appeared in -next without any
problems.
---
The following changes since commit 9123e3a74ec7b934a4a099e98af6a61c2f80bbf5:
Linux 5.9-rc1 (2020-08-16 13:04:57 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git
tags/libnvdimm-fix-v5.9-rc3
for you to fetch changes up to c2affe920b0e0669650943ac086215cf6519be34:
dax: do not print error message for non-persistent memory block device
(2020-08-20 11:43:18 -0600)
libnvdimm fixes for v5.9-rc3
Fix an out-of-bounds access introduced in libnvdimm v5.9-rc1
dax: do not print error message for non-persistent memory block device
Adrian Huang (1):
dax: do not print error message for non-persistent memory block device
Zqiang (1):
libnvdimm: KASAN: global-out-of-bounds Read in internal_create_group
drivers/dax/super.c| 6 ++
drivers/nvdimm/dimm_devs.c | 1 +
2 files changed, 7 insertions(+)
diff --git a/drivers/dax/super.c b/drivers/dax/super.c
index c82cbcb64202..32642634c1bb 100644
--- a/drivers/dax/super.c
+++ b/drivers/dax/super.c
@@ -100,6 +100,12 @@ bool __generic_fsdax_supported(struct dax_device *dax_dev,
return false;
}
+ if (!dax_dev && !bdev_dax_supported(bdev, blocksize)) {
+ pr_debug("%s: error: dax unsupported by block device\n",
+ bdevname(bdev, buf));
+ return false;
+ }
+
id = dax_read_lock();
len = dax_direct_access(dax_dev, pgoff, 1, , );
len2 = dax_direct_access(dax_dev, pgoff_end, 1, _kaddr, _pfn);
diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index 61374def5155..b59032e0859b 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -529,6 +529,7 @@ static DEVICE_ATTR_ADMIN_RW(activate);
static struct attribute *nvdimm_firmware_attributes[] = {
_attr_activate.attr,
_attr_result.attr,
+ NULL,
};
static umode_t nvdimm_firmware_visible(struct kobject *kobj, struct attribute
*a, int n)
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: 回复: [PATCH v2] libnvdimm: KASAN: global-out-of-bounds Read in internal_create_group
On Wed, 2020-08-19 at 03:23 +, Zhang, Qiang wrote:
> cc: Dan Williams
> Please review.
Hi Qiang,
I've got this queued up, I'll submit it for -rc2.
Thanks,
-Vishal
>
>
> 发件人: linux-kernel-ow...@vger.kernel.org
> 代表 qiang.zh...@windriver.com
> 发送时间: 2020年8月12日 16:55
> 收件人: dan.j.willi...@intel.com; vishal.l.ve...@intel.com;
> dave.ji...@intel.com; ira.we...@intel.com
> 抄送: linux-nvdimm@lists.01.org; linux-ker...@vger.kernel.org
> 主题: [PATCH v2] libnvdimm: KASAN: global-out-of-bounds Read in
> internal_create_group
>
> From: Zqiang
>
> Because the last member of the "nvdimm_firmware_attributes" array
> was not assigned a null ptr, when traversal of "grp->attrs" array
> is out of bounds in "create_files" func.
>
> func:
> create_files:
> ->for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++)
> ->
>
> BUG: KASAN: global-out-of-bounds in create_files fs/sysfs/group.c:43 [inline]
> BUG: KASAN: global-out-of-bounds in internal_create_group+0x9d8/0xb20
> fs/sysfs/group.c:149
> Read of size 8 at addr 8a2e4cf0 by task kworker/u17:10/959
>
> CPU: 2 PID: 959 Comm: kworker/u17:10 Not tainted 5.8.0-syzkaller #0
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
> BIOS rel-1.12.0-59-gc9ba5276e321-prebuilt.qemu.org 04/01/2014
> Workqueue: events_unbound async_run_entry_fn
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x18f/0x20d lib/dump_stack.c:118
> print_address_description.constprop.0.cold+0x5/0x497 mm/kasan/report.c:383
> __kasan_report mm/kasan/report.c:513 [inline]
> kasan_report.cold+0x1f/0x37 mm/kasan/report.c:530
> create_files fs/sysfs/group.c:43 [inline]
> internal_create_group+0x9d8/0xb20 fs/sysfs/group.c:149
> internal_create_groups.part.0+0x90/0x140 fs/sysfs/group.c:189
> internal_create_groups fs/sysfs/group.c:185 [inline]
> sysfs_create_groups+0x25/0x50 fs/sysfs/group.c:215
> device_add_groups drivers/base/core.c:2024 [inline]
> device_add_attrs drivers/base/core.c:2178 [inline]
> device_add+0x7fd/0x1c40 drivers/base/core.c:2881
> nd_async_device_register+0x12/0x80 drivers/nvdimm/bus.c:506
> async_run_entry_fn+0x121/0x530 kernel/async.c:123
> process_one_work+0x94c/0x1670 kernel/workqueue.c:2269
> worker_thread+0x64c/0x1120 kernel/workqueue.c:2415
> kthread+0x3b5/0x4a0 kernel/kthread.c:292
> ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:294
>
> The buggy address belongs to the variable:
> nvdimm_firmware_attributes+0x10/0x40
>
> Reported-by: syzbot+1cf0ffe61aecf46f5...@syzkaller.appspotmail.com
> Signed-off-by: Zqiang
> ---
> v1->v2:
> Modify the description of the error.
>
> drivers/nvdimm/dimm_devs.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
> index 61374def5155..b59032e0859b 100644
> --- a/drivers/nvdimm/dimm_devs.c
> +++ b/drivers/nvdimm/dimm_devs.c
> @@ -529,6 +529,7 @@ static DEVICE_ATTR_ADMIN_RW(activate);
> static struct attribute *nvdimm_firmware_attributes[] = {
> _attr_activate.attr,
> _attr_result.attr,
> + NULL,
> };
>
> static umode_t nvdimm_firmware_visible(struct kobject *kobj, struct
> attribute *a, int n)
> --
> 2.17.1
>
> ___
> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH] libnvdimm: Add a NULL entry to 'nvdimm_firmware_attributes'
On Fri, 2020-08-14 at 10:10 -0700, Ira Weiny wrote:
> On Fri, Aug 14, 2020 at 08:35:09PM +0530, Vaibhav Jain wrote:
> > We recently discovered a kernel oops with 'papr_scm' module while
> > booting ppc64 phyp guest with following back-trace:
> >
> > BUG: Kernel NULL pointer dereference on write at 0x0188
> > Faulting instruction address: 0xc05d7084
> > Oops: Kernel access of bad area, sig: 11 [#1]
> >
> > Call Trace:
> > internal_create_group+0x128/0x4c0 (unreliable)
> > internal_create_groups.part.4+0x70/0x130
> > device_add+0x458/0x9c0
> > nd_async_device_register+0x28/0xa0 [libnvdimm]
> > async_run_entry_fn+0x78/0x1f0
> > process_one_work+0x2c0/0x5b0
> > worker_thread+0x88/0x650
> > kthread+0x1a8/0x1b0
> > ret_from_kernel_thread+0x5c/0x6c
> >
> > A bisect lead to the 'commit 48001ea50d17f ("PM, libnvdimm: Add runtime
> > firmware activation support")' and on investigation discovered that
> > the newly introduced 'struct attribute *nvdimm_firmware_attributes[]'
> > is missing a terminating NULL entry in the array. This causes a loop
> > in sysfs's 'create_files()' to read garbage beyond bounds of
> > 'nvdimm_firmware_attributes' and trigger the oops.
> >
> > Fixes: 48001ea50d17f ("PM, libnvdimm: Add runtime firmware activation
> > support")
> > Reported-by: Sandipan Das
> > Signed-off-by: Vaibhav Jain
>
> Reviewed-by: Ira Weiny
Thanks Vaibhav and Ira. I see this was also reported and fixed by Zqiang
a couple days ago. I'll pick that, merge these trailers and add it to
the fixes queue.
>
> > ---
> > drivers/nvdimm/dimm_devs.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
> > index 61374def51555..b59032e0859b7 100644
> > --- a/drivers/nvdimm/dimm_devs.c
> > +++ b/drivers/nvdimm/dimm_devs.c
> > @@ -529,6 +529,7 @@ static DEVICE_ATTR_ADMIN_RW(activate);
> > static struct attribute *nvdimm_firmware_attributes[] = {
> > _attr_activate.attr,
> > _attr_result.attr,
> > + NULL,
> > };
> >
> > static umode_t nvdimm_firmware_visible(struct kobject *kobj, struct
> > attribute *a, int n)
> > --
> > 2.26.2
> >
> ___
> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH] acpi/nfit: Use kobj_to_dev() instead
On Fri, 2020-08-14 at 17:28 +0200, Rafael J. Wysocki wrote:
> On Thu, Aug 13, 2020 at 4:54 AM Wang Qing wrote:
> > Use kobj_to_dev() instead of container_of()
> >
> > Signed-off-by: Wang Qing
>
> LGTM
>
> Dan, any objections?
Looks good to me - you can add:
Acked-by: Vishal Verma
>
> > ---
> > drivers/acpi/nfit/core.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> > index fa4500f..3bb350b
> > --- a/drivers/acpi/nfit/core.c
> > +++ b/drivers/acpi/nfit/core.c
> > @@ -1382,7 +1382,7 @@ static bool ars_supported(struct nvdimm_bus
> > *nvdimm_bus)
> >
> > static umode_t nfit_visible(struct kobject *kobj, struct attribute *a, int
> > n)
> > {
> > - struct device *dev = container_of(kobj, struct device, kobj);
> > + struct device *dev = kobj_to_dev(kobj);
> > struct nvdimm_bus *nvdimm_bus = to_nvdimm_bus(dev);
> >
> > if (a == _attr_scrub.attr && !ars_supported(nvdimm_bus))
> > @@ -1667,7 +1667,7 @@ static struct attribute *acpi_nfit_dimm_attributes[]
> > = {
> > static umode_t acpi_nfit_dimm_attr_visible(struct kobject *kobj,
> > struct attribute *a, int n)
> > {
> > - struct device *dev = container_of(kobj, struct device, kobj);
> > + struct device *dev = kobj_to_dev(kobj);
> > struct nvdimm *nvdimm = to_nvdimm(dev);
> > struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
> >
> > --
> > 2.7.4
> >
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
[GIT PULL] libnvdimm for v5.9
Hi Linus, please pull from:
git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/
tags/libnvdimm-for-5.9
...to receive a new feature in libnvdimm - 'Runtime Firmware
Activation', and a few small cleanups and fixes in libnvdimm and DAX.
You'd normally receive this pull request from Dan Williams, but he's
busy watching a newborn (Congrats Dan!), so I'm watching libnvdimm this
cycle.
I'd originally intended to make separate topic-based pull requests - one
for libnvdimm, and one for DAX, but some of the DAX material fell out
since it wasn't quite ready. I ended up merging the two branches into
one, and hence a couple of 'internal' merges - I hope these are ok. If
you prefer that I should've handled this differently please let me know!
I was also expecting a potential conflict - I was assuming Greg had
pulled in one of Dan's patches[1] through driver-core, but I don't see
it in his tree, and a test merge with the current master went through
cleanly.
There were a small handful of late fixes, but everything has had at
least a week of -next soak time without any reported issues. We've also
received a positive build notification from 0-day.
[1]: https://lore.kernel.org/linux-nvdimm/20200721104442.gf1676...@kroah.com/
---
The following changes since commit 48778464bb7d346b47157d21ffde2af6b2d39110:
Linux 5.8-rc2 (2020-06-21 15:45:29 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/
tags/libnvdimm-for-5.9
for you to fetch changes up to 7f674025d9f7321dea11b802cc0ab3f09cbe51c5:
libnvdimm/security: ensure sysfs poll thread woke up and fetch updated attr
(2020-08-03 18:54:13 -0600)
libnvdimm for 5.9
- Add 'Runtime Firmware Activation' support for NVDIMMs that advertise
the relevant capability
- Misc libnvdimm and DAX cleanups
Coly Li (1):
dax: print error message by pr_info() in __generic_fsdax_supported()
Dan Williams (12):
libnvdimm: Validate command family indices
ACPI: NFIT: Move bus_dsm_mask out of generic nvdimm_bus_descriptor
ACPI: NFIT: Define runtime firmware activation commands
tools/testing/nvdimm: Cleanup dimm index passing
tools/testing/nvdimm: Add command debug messages
tools/testing/nvdimm: Prepare nfit_ctl_test() for ND_CMD_CALL emulation
tools/testing/nvdimm: Emulate firmware activation commands
driver-core: Introduce DEVICE_ATTR_ADMIN_{RO,RW}
libnvdimm: Convert to DEVICE_ATTR_ADMIN_RO()
PM, libnvdimm: Add runtime firmware activation support
ACPI: NFIT: Add runtime firmware activate support
ACPI: NFIT: Fix ARS zero-sized allocation
Hao Li (1):
dax: Fix incorrect argument passed to xas_set_err()
Ira Weiny (2):
fs/dax: Remove unused size parameter
drivers/dax: Expand lock scope to cover the use of addresses
Jane Chu (3):
libnvdimm/security: fix a typo
libnvdimm/security: the 'security' attr never show 'overwrite' state
libnvdimm/security: ensure sysfs poll thread woke up and fetch updated
attr
Vishal Verma (2):
Merge branch 'for-5.9/dax' into libnvdimm-for-next
Merge branch 'for-5.9/firmware-activate' into libnvdimm-for-next
Documentation/ABI/testing/sysfs-bus-nfit | 19 +
Documentation/ABI/testing/sysfs-bus-nvdimm | 2 +
.../driver-api/nvdimm/firmware-activate.rst| 86 +
drivers/acpi/nfit/core.c | 157 ++---
drivers/acpi/nfit/intel.c | 386 +
drivers/acpi/nfit/intel.h | 61
drivers/acpi/nfit/nfit.h | 38 +-
drivers/dax/super.c| 13 +-
drivers/nvdimm/bus.c | 16 +
drivers/nvdimm/core.c | 149
drivers/nvdimm/dimm_devs.c | 123 ++-
drivers/nvdimm/namespace_devs.c| 2 +-
drivers/nvdimm/nd-core.h | 1 +
drivers/nvdimm/pfn_devs.c | 2 +-
drivers/nvdimm/region_devs.c | 2 +-
drivers/nvdimm/security.c | 13 +-
fs/dax.c | 15 +-
include/linux/device.h | 4 +
include/linux/libnvdimm.h | 52 ++-
include/linux/suspend.h| 6 +
include/linux/sysfs.h | 7 +
include/uapi/linux/ndctl.h | 5 +
kernel/power/hibernate.c | 97 ++
tools/testing/nvdimm/test/nfit.c | 367
24 files changed, 1486 insertions(+), 137 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-bus-nvdimm
create mode
Re: [PATCH 0/4] Remove nrexceptional tracking
On Thu, 2020-08-06 at 19:44 +, Verma, Vishal L wrote: > > > > I'm running xfstests on this patchset right now. If one of the DAX > > people could try it out, that'd be fantastic. > > > > Matthew Wilcox (Oracle) (4): > > mm: Introduce and use page_cache_empty > > mm: Stop accounting shadow entries > > dax: Account DAX entries as nrpages > > mm: Remove nrexceptional from inode > > Hi Matthew, > > I applied these on top of 5.8 and ran them through the nvdimm unit test > suite, and saw some test failures. The first failing test signature is: > > + umount test_dax_mnt > ./dax-ext4.sh: line 62: 15749 Segmentation fault umount $MNT > FAIL dax-ext4.sh (exit status: 139) > > The line is: https://github.com/pmem/ndctl/blob/master/test/dax.sh#L79 > And the failing umount happens right after 'run_test', which calls this: > https://github.com/pmem/ndctl/blob/master/test/dax-pmd.c > > And here is the associated kernel splat: [ 89.570142] EXT4-fs (pmem4): DAX enabled. Warning: EXPERIMENTAL, use at your own risk [ 89.573407] EXT4-fs (pmem4): mounted filesystem with ordered data mode. Opts: dax [ 90.450644] Injecting memory failure for pfn 0x148900 at process virtual address 0x7f2ec9b0 [ 90.452421] Memory failure: 0x148900: Sending SIGBUS to dax-pmd:16886 due to hardware memory corruption [ 90.454067] Memory failure: 0x148900: recovery action for dax page: Recovered [ 91.656624] [ cut here ] [ 91.657822] kernel BUG at fs/inode.c:530! [ 91.658850] invalid opcode: [#1] SMP PTI [ 91.659883] CPU: 4 PID: 16891 Comm: umount Tainted: G O 5.8.0-4-g6161e2d6ac48 #38 [ 91.661861] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014 [ 91.664920] RIP: 0010:clear_inode+0x78/0x90 [ 91.665934] Code: 00 a8 20 74 2b a8 40 75 29 48 8b 93 f0 01 00 00 48 8d 83 f0 01 00 00 48 39 c2 75 18 48 c7 83 e0 00 00 00 60 00 00 00 5b 5d c3 <0f> 0b 0f 0b 0f 0b 0f 0b 0f 0b 0f 0b 66 66 2e 0f 1f 84 00 00 00 00 [ 91.669979] RSP: 0018:c9000212fd98 EFLAGS: 00010002 [ 91.671151] RAX: 0004 RBX: 88810e33d470 RCX: 8c6318c6318c6319 [ 91.672689] RDX: 888110b48040 RSI: 0004 RDI: 0046 [ 91.674208] RBP: 88810e33d6b8 R08: R09: 0001 [ 91.675760] R10: 0001 R11: R12: 88810e33d6b0 [ 91.677332] R13: 88811014c828 R14: 88811014c9c0 R15: 88810e3398c0 [ 91.678940] FS: 7f86a7e74c80() GS:888117c0() knlGS: [ 91.680745] CS: 0010 DS: ES: CR0: 80050033 [ 91.682001] CR2: 55bcfb4c3008 CR3: 000110b7c000 CR4: 06e0 [ 91.683581] Call Trace: [ 91.684268] ext4_clear_inode+0x16/0x80 [ 91.685298] ext4_evict_inode+0x5f/0x610 [ 91.686245] evict+0xcf/0x1f0 [ 91.687017] dispose_list+0x48/0x70 [ 91.687937] evict_inodes+0x152/0x190 [ 91.688918] generic_shutdown_super+0x37/0x100 [ 91.689880] kill_block_super+0x21/0x50 [ 91.690784] deactivate_locked_super+0x36/0xa0 [ 91.691790] cleanup_mnt+0x12d/0x190 [ 91.692658] task_work_run+0x5c/0xa0 [ 91.694739] __prepare_exit_to_usermode+0x1b6/0x1c0 [ 91.695806] do_syscall_64+0x5e/0xb0 [ 91.696656] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 91.697798] RIP: 0033:0x7f86a80c694b [ 91.698650] Code: 15 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 90 f3 0f 1e fa 31 f6 e9 05 00 00 00 0f 1f 44 00 00 f3 0f 1e fa b8 a6 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 1d 15 0c 00 f7 d8 64 89 01 48 [ 91.702545] RSP: 002b:7ffd49a8cb98 EFLAGS: 0246 ORIG_RAX: 00a6 [ 91.704204] RAX: RBX: 7f86a81ee204 RCX: 7f86a80c694b [ 91.705732] RDX: ff78 RSI: RDI: 55bcfb4bf2a0 [ 91.707207] RBP: 55bcfb4bafa0 R08: R09: 7f86a8188a40 [ 91.708809] R10: 55bcfb4c1550 R11: 0246 R12: [ 91.710040] R13: 55bcfb4bf2a0 R14: 55bcfb4bb0b0 R15: 55bcfb4bb1d0 [ 91.711193] Modules linked in: nd_e820(O) nd_blk(O) nfit(O) kmem nd_pmem(O) nd_btt(O) dax_pmem(O) dax_pmem_core(O) libnvdimm(O) device_dax(O) nfit_test_iomap(O) [last unloaded: nfit_test_iomap] [ 91.714154] ---[ end trace 016cb116e8654993 ]--- [ 91.715011] RIP: 0010:clear_inode+0x78/0x90 [ 91.715803] Code: 00 a8 20 74 2b a8 40 75 29 48 8b 93 f0 01 00 00 48 8d 83 f0 01 00 00 48 39 c2 75 18 48 c7 83 e0 00 00 00 60 00 00 00 5b 5d c3 <0f> 0b 0f 0b 0f 0b 0f 0b 0f 0b 0f 0b 66 66 2e 0f 1f 84 00 00 00 00 [ 91.718950] RSP: 0018:c9000212fd98 EFLAGS: 00010002 [ 91.719976] RAX: 0004 RBX: 88810e33d470 RCX: 8c6318c6318c6319 [ 91.721187] RDX: 888110b48040 RSI: 0004 RDI: 0046 [
Re: [PATCH 0/4] Remove nrexceptional tracking
On Tue, 2020-08-04 at 17:17 +0100, Matthew Wilcox (Oracle) wrote: > We actually use nrexceptional for very little these days. It's a > constant > source of pain with the THP patches because we don't know how large a > shadow entry is, so either we have to ask the xarray how many indices > it covers, or store that information in the shadow entry (and reduce > the amount of other information in the shadow entry proportionally). > While tracking down the most recent case of "evict tells me I've got > the accounting wrong again", I wondered if it might not be simpler to > just remove it. So here's a patch set to do just that. I think each > of these patches is an improvement in isolation, but the combination > of > all four is larger than the sum of its parts. > > I'm running xfstests on this patchset right now. If one of the DAX > people could try it out, that'd be fantastic. > > Matthew Wilcox (Oracle) (4): > mm: Introduce and use page_cache_empty > mm: Stop accounting shadow entries > dax: Account DAX entries as nrpages > mm: Remove nrexceptional from inode Hi Matthew, I applied these on top of 5.8 and ran them through the nvdimm unit test suite, and saw some test failures. The first failing test signature is: + umount test_dax_mnt ./dax-ext4.sh: line 62: 15749 Segmentation fault umount $MNT FAIL dax-ext4.sh (exit status: 139) The line is: https://github.com/pmem/ndctl/blob/master/test/dax.sh#L79 And the failing umount happens right after 'run_test', which calls this: https://github.com/pmem/ndctl/blob/master/test/dax-pmd.c > > fs/block_dev.c | 2 +- > fs/dax.c| 8 > fs/inode.c | 2 +- > include/linux/fs.h | 2 -- > include/linux/pagemap.h | 5 + > mm/filemap.c| 15 --- > mm/truncate.c | 19 +++ > mm/workingset.c | 1 - > 8 files changed, 14 insertions(+), 40 deletions(-) > ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
[ANNOUNCE] ndctl v69
A new ndctl release is available[1]. Highlights include support for 'PAPR' NVDIMMs, a build fix for zero-length array warnings in GCC10, a new option for ndctl-monitor allowing for a timeout for epoll, and misc unit test and documentation fixes. A shortlog is appended below. [1]: https://github.com/pmem/ndctl/releases/tag/v69 Dan Williams (1): ndctl/build: Fix zero-length array warnings Michal Suchanek (1): Documentation: use includes in more ndctl command pages. Santosh Sivaraj (2): Skip region filtering if numa_node attribute is not present infoblock: Set the default alignment to the platform alignment Steve Scargall (1): ndctl/doc: Fix a typo in Documentation/daxctl/movable-options.txt Vaibhav Jain (8): libndctl: Refactor out add_dimm() to handle NFIT specific init libncdtl: Add initial support for NVDIMM_FAMILY_PAPR nvdimm family libndctl,papr_scm: Add definitions for PAPR nvdimm specific methods papr: Add scaffolding to issue and handle PDSM requests libndctl,papr_scm: Implement support for PAPR_PDSM_HEALTH monitor: Add epoll timeout for forcing a full dimm health check libndctl/papr_scm: Add support for reporting "life_used_percentage" metric papr: Check for command type in papr_xlat_firmware_status() Vishal Verma (5): ndctl/test: Fix region selection in align.sh ndctl/test: fix align.sh to not expect initialized labels ndctl/README: Add CONFIG_ENCRYPTED_KEYS to the config items list ndctl/namespace: fix a resource leak in file_write_infoblock() ndctl/lib: fix new symbol location in the symbol script ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH] papr: Check for command type in papr_xlat_firmware_status()
On Wed, 2020-07-22 at 07:29 +0530, Vaibhav Jain wrote:
> Vishal Verma writes:
>
>
> > > static int papr_xlat_firmware_status(struct ndctl_cmd *cmd)
> > > {
> > > - const struct nd_pkg_pdsm *pcmd = to_pdsm(cmd);
> > > -
> > > - return pcmd->cmd_status;
> > > + return (cmd->type == ND_CMD_CALL) ? to_pdsm(cmd)->cmd_status :
> > > 0;
> >
> > Is this correct? -- for non ND_CMD_CALL commands this will always
> > return a 0,
> > and it seems like you will lose any error state for commands
> > like ND_CMD_SET_CONFIG_DATA.
> This behaviour is similar to what ndctl_cmd_xlat_firmware_status()
> does
> when corresponding dimm-ops are missing the 'xlat_firmware_status'
> callback.
>
> Also ndctl_cmd_submit_xlat() returns the rc from ndctl_cmd_submit()
> in case ndctl_cmd_xlat_firmware_status() returns '0', which
> corresponds
> to 'ndctl_cmd.status' field. So any error codes
> returned from ndctl_cmd_submit() are still returned back to the caller
> even though papr_xlat_firmware_status() returned '0'.
Ah yes you're right. I'll queue this up for v69 and we can do the more
involved fix in the next cycle.
Thanks!
-Vishal
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH] ACPI: Replace HTTP links with HTTPS ones
On Fri, 2020-07-17 at 20:24 +0200, Alexander A. Klimov wrote: > Rationale: > Reduces attack surface on kernel devs opening the links for MITM > as HTTPS traffic is much harder to manipulate. > > Deterministic algorithm: > For each file: > If not .svg: > For each line: > If doesn't contain `\bxmlns\b`: > For each link, `\bhttp://[^# \t\r\n]*(?:\w|/)`: > If neither `\bgnu\.org/license`, nor `\bmozilla\.org/MPL\b`: > If both the HTTP and HTTPS versions > return 200 OK and serve the same content: > Replace HTTP with HTTPS. > > Signed-off-by: Alexander A. Klimov > --- > Continuing my work started at 93431e0607e5. > See also: git log --oneline '--author=Alexander A. Klimov > ' v5.7..master > > If there are any URLs to be removed completely or at least not just > HTTPSified: > Just clearly say so and I'll *undo my change*. > See also: https://lkml.org/lkml/2020/6/27/64 > > If there are any valid, but yet not changed URLs: > See: https://lkml.org/lkml/2020/6/26/837 > > If you apply the patch, please let me know. > > Sorry again to all maintainers who complained about subject lines. > Now I realized that you want an actually perfect prefixes, > not just subsystem ones. > I tried my best... > And yes, *I could* (at least half-)automate it. > Impossible is nothing! :) > > > .../firmware-guide/acpi/DSD-properties-rules.rst | 4 ++-- > .../firmware-guide/acpi/dsd/data-node-references.rst | 4 ++-- > Documentation/firmware-guide/acpi/dsd/graph.rst| 10 +- > Documentation/firmware-guide/acpi/dsd/leds.rst | 6 +++--- > Documentation/firmware-guide/acpi/lpit.rst | 2 +- > drivers/acpi/Kconfig | 2 +- > drivers/acpi/nfit/nfit.h | 2 +- > 7 files changed, 15 insertions(+), 15 deletions(-) For nfit/nfit.h, Acked-by: Vishal Verma > diff --git a/drivers/acpi/nfit/nfit.h b/drivers/acpi/nfit/nfit.h > index f5525f8bb770..a303f0123394 100644 > --- a/drivers/acpi/nfit/nfit.h > +++ b/drivers/acpi/nfit/nfit.h > @@ -16,7 +16,7 @@ > /* ACPI 6.1 */ > #define UUID_NFIT_BUS "2f10e7a4-9e91-11e4-89d3-123b93f75cba" > > -/* http://pmem.io/documents/NVDIMM_DSM_Interface-V1.6.pdf */ > +/* https://pmem.io/documents/NVDIMM_DSM_Interface-V1.6.pdf */ > #define UUID_NFIT_DIMM "4309ac30-0d11-11e4-9191-0800200c9a66" > > /* https://github.com/HewlettPackard/hpe-nvm/blob/master/Documentation/ */ > -- > 2.27.0 > ___ > Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org > To unsubscribe send an email to linux-nvdimm-le...@lists.01.org ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH v4 2/2] ACPICA: Preserve memory opregion mappings
On Mon, 2020-06-29 at 18:33 +0200, Rafael J. Wysocki wrote: > From: "Rafael J. Wysocki" > > The ACPICA's strategy with respect to the handling of memory mappings > associated with memory operation regions is to avoid mapping the > entire region at once which may be problematic at least in principle > (for example, it may lead to conflicts with overlapping mappings > having different attributes created by drivers). It may also be > wasteful, because memory opregions on some systems take up vast > chunks of address space while the fields in those regions actually > accessed by AML are sparsely distributed. > > For this reason, a one-page "window" is mapped for a given opregion > on the first memory access through it and if that "window" does not > cover an address range accessed through that opregion subsequently, > it is unmapped and a new "window" is mapped to replace it. Next, > if the new "window" is not sufficient to acess memory through the > opregion in question in the future, it will be replaced with yet > another "window" and so on. That may lead to a suboptimal sequence > of memory mapping and unmapping operations, for example if two fields > in one opregion separated from each other by a sufficiently wide > chunk of unused address space are accessed in an alternating pattern. > > The situation may still be suboptimal if the deferred unmapping > introduced previously is supported by the OS layer. For instance, > the alternating memory access pattern mentioned above may produce > a relatively long list of mappings to release with substantial > duplication among the entries in it, which could be avoided if > acpi_ex_system_memory_space_handler() did not release the mapping > used by it previously as soon as the current access was not covered > by it. > > In order to improve that, modify acpi_ex_system_memory_space_handler() > to preserve all of the memory mappings created by it until the memory > regions associated with them go away. > > Accordingly, update acpi_ev_system_memory_region_setup() to unmap all > memory associated with memory opregions that go away. > > Reported-by: Dan Williams > Signed-off-by: Rafael J. Wysocki > --- > drivers/acpi/acpica/evrgnini.c | 14 > drivers/acpi/acpica/exregion.c | 65 -- > include/acpi/actypes.h | 12 +-- > 3 files changed, 64 insertions(+), 27 deletions(-) > Hi Rafael, Picking up from Dan while he's out - I had these patches tested by the original reporter, and they work fine. I see you had them staged in the acpica-osl branch. Is that slated to go in during the 5.9 merge window? You can add: Tested-by: Xiang Li ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH v6 2/5] libncdtl: Add initial support for NVDIMM_FAMILY_PAPR nvdimm family
On Wed, 2020-06-17 at 22:12 +0530, Vaibhav Jain wrote: > Hi Vishal, > > Thanks for reviewing this patch. I will be addressing your review > comments in v7 of this patch series. > > ~ Vaibhav > Thanks Vaibhav. The rest of the series looks good to me, so once you send the update, I'll queue it up for v69. Thanks, -Vishal ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH v6 2/5] libncdtl: Add initial support for NVDIMM_FAMILY_PAPR nvdimm family
On Tue, 2020-06-16 at 11:00 +0530, Vaibhav Jain wrote:
> Add necessary scaffolding in libndctl for dimms that support papr_scm
support /the/ papr_scm specification
> specification[1]. Since there can be platforms that support
> Open-Firmware[2] but not the papr_scm specification, hence the changes
s/hence//
> proposed first add support for probing if the dimm bus supports
> Open-Firmware. This is done via querying for sysfs attribute 'of_node'
This is done /by/ querying for /the/ sysfs attribute
> in dimm device sysfs directory. If available newly introduced member
> 'struct ndctl_bus.has_of_node' is set. During the probe of the dimm
> and execution of add_dimm(), the newly introduced add_papr_dimm()
During 'add_dimm()', the newly introduced..
> is called if dimm bus reports supports Open-Firmware.
>
> Function add_papr_dimm() queries the 'compatible' device tree
> attribute via newly introduced ndctl_bus_is_papr_scm() and based on
> its value assign NVDIMM_FAMILY_PAPR to the dimm command family. In
based on its value, assigns NVDIM_..
> future, based on the contents of 'compatible' attribute more of_pmem
In /the/ future
> dimm families can be queried.
>
> We also add support for parsing the dimm flags for
'We' can be ambiguous. Say something like: "Additionally, add support.."
> NVDIMM_FAMILY_PAPR supporting nvdimms as described at [3]. A newly
> introduced function parse_papr_flags() reads the contents of this
> flag file and sets appropriate flag bits in 'struct
> ndctl_dimm.flags'.
>
> Also we advertise support for monitor mode by allocating a file
"Advertise support for monitor mode.."
> descriptor to the dimm 'flags' file and assigning it to 'struct
> ndctl_dimm.health_event_fd'.
>
> The dimm-ops implementation for NVDIMM_FAMILY_PAPR is
> available in global variable 'papr_dimm_ops' which points to
> skeleton implementation in newly introduced file 'lib/papr.c'.
This paragraph can just be dropped - it's a minor implementation detail,
and doesn't add much to the commit message. The same actually goes for
the part above that talks about setting flags.
>
> References:
> [1] Documentation/powerpc/papr_hcalls.rst
> https://lore.kernel.org/linux-nvdimm/20200529214719.223344-2-vaib...@linux.ibm.com
>
> [2] https://en.wikipedia.org/wiki/Open_Firmware
>
> [3] Documentation/ABI/testing/sysfs-bus-papr-pmem
> https://lore.kernel.org/linux-nvdimm/20200529214719.223344-4-vaib...@linux.ibm.com
Not a huge deal, but the lore links can probably be updated to the
latest posting.
>
> Signed-off-by: Vaibhav Jain
> ---
>
[..]
> diff --git a/ndctl/lib/papr.c b/ndctl/lib/papr.c
> new file mode 100644
> index ..4b6ce8beccab
> --- /dev/null
> +++ b/ndctl/lib/papr.c
> @@ -0,0 +1,22 @@
> +// SPDX-License-Identifier: LGPL-2.1
I'm not sure if you intended to drop the copyright line here :)
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +static bool papr_cmd_is_supported(struct ndctl_dimm *dimm, int cmd)
> +{
> + /* Handle this separately to support monitor mode */
> + if (cmd == ND_CMD_SMART)
> + return true;
> +
> + return !!(dimm->cmd_mask & (1ULL << cmd));
> +}
> +
> +struct ndctl_dimm_ops * const papr_dimm_ops = &(struct ndctl_dimm_ops) {
> + .cmd_is_supported = papr_cmd_is_supported,
> +};
>
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH v6 1/5] libndctl: Refactor out add_dimm() to handle NFIT specific init
On Tue, 2020-06-16 at 11:00 +0530, Vaibhav Jain wrote:
> Presently add_dimm() only probes dimms that support NFIT/ACPI. Hence
"...probes NVDIMMs for platforms that support the ACPI NFIT"
The NFIT is a platform firmware thing, not directly related to the DIMMs
themselves.
> this patch refactors this functionality into two functions namely
s/Hence this patch refactors/Refactor/
> add_dimm() and add_nfit_dimm(). Function add_dimm() performs
> allocation and common 'struct ndctl_dimm' initialization and depending
> on whether the dimm-bus supports NIFT, calls add_nfit_dimm(). Once
> the probe is completed based on the value of 'ndctl_dimm.cmd_family'
> appropriate dimm-ops are assigned to the dimm.
>
> In case dimm-bus is of unknown type or doesn't support NFIT the
> initialization still continues, with no dimm-ops assigned to the
> 'struct ndctl_dimm' there-by limiting the functionality available.
No need to hyphenate 'thereby'
>
> This patch shouldn't introduce any behavioral change.
>
> Signed-off-by: Vaibhav Jain
> ---
> Changelog:
>
> v5..v6:
> * Changed a return code for add_nfit_dimm() in case a allocation
> failed. [ Vishal ]
> * Updated an error message in error path of add_dimm() [ Vishal ]
>
> v4..v5:
> * None
>
> v3..v4:
> * None
>
> v2..v3:
> * None
>
> v1..v2:
> * None
> ---
> ndctl/lib/libndctl.c | 196 +--
> 1 file changed, 115 insertions(+), 81 deletions(-)
>
[..]
> +
> + /* Assign dimm-ops based on command family */
> + if (dimm->cmd_family == NVDIMM_FAMILY_INTEL)
> + dimm->ops = intel_dimm_ops;
> + if (dimm->cmd_family == NVDIMM_FAMILY_HPE1)
> + dimm->ops = hpe1_dimm_ops;
> + if (dimm->cmd_family == NVDIMM_FAMILY_MSFT)
> + dimm->ops = msft_dimm_ops;
> + if (dimm->cmd_family == NVDIMM_FAMILY_HYPERV)
> + dimm->ops = hyperv_dimm_ops;
> out:
> + if (rc) {
> + /* Ensure dimm_uninit() is not called during free_dimm() */
> + dimm->ops = NULL;
I think the above assignment and comment are now stale..
> + err(ctx, "%s: probe failed: %s\n", ndctl_dimm_get_devname(dimm),
> + strerror(-rc));
> + goto err_read;
> + }
> +
> list_add(>dimms, >list);
> free(path);
>
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH v5 3/6] libndctl: Introduce new dimm-ops dimm_init() & dimm_uninit()
On Sat, 2020-05-30 at 03:35 +0530, Vaibhav Jain wrote:
> There are scenarios when a dimm-provider need to allocate some
> per-dimm data that can be quickly retrieved. This data can be used to
> cache data that spans multiple 'struct ndctl_cmd' submissions.
>
> Unfortunately currently in libnvdimm there is no easy way to implement
> this. Even if this data is some how stored in some field of 'struct
> ndctl_dimm', managing its lifetime is a challenge.
>
> To solve this problem, the patch proposes a new member 'struct
> ndctl_dimm.dimm_user_data' to store per-dimm data interpretation of
> which is specific to a dimm-provider. Also two new dimm-ops namely
> dimm_init() & dimm_uninit() are introduced that can be used to manage
> the lifetime of this per-dimm data.
>
> Semantics
> =
> int (*dimm_init)(struct ndctl_dimm *):
>
> This callback will be called just after dimm-probe inside add_dimm()
> is completed. Dimm-providers should use this callback to allocate
> per-dimm data and assign it to 'struct ndctl_dimm.dimm_user_data'
> member. In case this function returns an error, dimm initialization is
> halted and errors out.
>
> void (*dimm_uninit)(struct ndctl_dimm *):
>
> This callback will be called during free_dimm() and is only called if
> previous call to 'dimm_ops->dimm_init()' had reported no
> error. Dimm-providers should use this callback to unallocate and
> cleanup 'dimm_user_data'.
I'm not sure I fully understand the need for this whole paradigm - of
creating a private caching area in ndctl_dimm, and having these
init/uninit functions to set it up.
Looking ahead at subsequent patches, the data you're stashing there is
already cached in the kernel or the command payloads. It seems the
caching is maybe just avoiding some ioctl round trips - is that correct?
If so , why not just make all the data retrieval synchronous to the
operation that's requesting it? Health retrieval is generally not a fast
path of any sort, and doing everything synchronously seems much simpler,
and is also what existing nvdimm families do.
>
> Signed-off-by: Vaibhav Jain
> ---
> Changelog:
>
> v4..v5:
> * None
>
> v3..v4:
> * None
>
> v2..v3:
> * None
>
> v1..v2:
> * Changed the patch order
> ---
> ndctl/lib/libndctl.c | 11 +++
> ndctl/lib/private.h | 5 +
> 2 files changed, 16 insertions(+)
>
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index a52c2e208fbe..8d226abf7495 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -598,6 +598,11 @@ static void free_dimm(struct ndctl_dimm *dimm)
> {
> if (!dimm)
> return;
> +
> + /* If needed call the dimm uninitialization function */
> + if (dimm->ops && dimm->ops->dimm_uninit)
> + dimm->ops->dimm_uninit(dimm);
> +
> free(dimm->unique_id);
> free(dimm->dimm_buf);
> free(dimm->dimm_path);
> @@ -1714,8 +1719,14 @@ static void *add_dimm(void *parent, int id, const char
> *dimm_base)
> if (dimm->cmd_family == NVDIMM_FAMILY_PAPR)
> dimm->ops = papr_dimm_ops;
>
> + /* Call the dimm initialization function if needed */
> + if (!rc && dimm->ops && dimm->ops->dimm_init)
> + rc = dimm->ops->dimm_init(dimm);
> +
> out:
> if (rc) {
> + /* Ensure dimm_uninit() is not called during free_dimm() */
> + dimm->ops = NULL;
> err(ctx, "Unable to probe dimm:%d. Err:%d\n", id, rc);
> goto err_read;
> }
> diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h
> index d90236b1f98b..d0188a97d673 100644
> --- a/ndctl/lib/private.h
> +++ b/ndctl/lib/private.h
> @@ -99,6 +99,7 @@ struct ndctl_dimm {
> } flags;
> int locked;
> int aliased;
> + void *dimm_user_data;
> struct list_node list;
> int formats;
> int format[0];
> @@ -347,6 +348,10 @@ struct ndctl_dimm_ops {
> int (*fw_update_supported)(struct ndctl_dimm *);
> int (*xlat_firmware_status)(struct ndctl_cmd *);
> u32 (*get_firmware_status)(struct ndctl_cmd *);
> + /* Called just after dimm is initialized and probed */
> + int (*dimm_init)(struct ndctl_dimm *);
> + /* Called just before struct ndctl_dimm is de-allocated */
> + void (*dimm_uninit)(struct ndctl_dimm *);
> };
>
> extern struct ndctl_dimm_ops * const intel_dimm_ops;
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH v5 2/6] libncdtl: Add initial support for NVDIMM_FAMILY_PAPR nvdimm family
On Wed, 2020-06-03 at 15:27 +0530, Vaibhav Jain wrote: > > > > Two things here: > > 1. Why not use the new ndctl_bus_has_of_node helper here? and > > 2. This looks redundant. add_papr_dimm() is only called if > > ndctl_bus_has_of_node() during add_dimm. > Presently we have two different nvdimm implementations: > > * papr-scm: handled by arch/powerpc/platforms/pseries/papr_scm kernel module. > * nvdimm-n: handled by drivers/nvdimm/of_pmem kernel module. > > Both nvdimms are exposed to the kernel via device tree nodes but different > 'compatible' properties. This patchset only adds support for 'papr-scm' > compatible nvdimms. > > 'ndctl_bus_has_of_node()' simply indicates if the nvdimm has an > open-firmware compatible devicetree node associated with it and doesnt > necessarily indicate if its papr-scm compliant. > > Hence validating the 'compatible' attribute value is necessary here. > Please see a more detailed info below regarding the 'compatible' sysfs > attribute. > Understood - one more question: Would it be useful to wrap the 'compatible' check into an API similar to _has_of_node - say ndctl_bus_is_papr_compatible()? I'm not too strongly attached this, there is only one user so far after all, but it seemed like an easy thing that might get copy-pasted around in the future. ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH v5 2/6] libncdtl: Add initial support for NVDIMM_FAMILY_PAPR nvdimm family
On Sat, 2020-05-30 at 03:35 +0530, Vaibhav Jain wrote:
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index d76dbf7e17de..a52c2e208fbe 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -799,6 +799,28 @@ static void parse_nfit_mem_flags(struct ndctl_dimm
> *dimm, char *flags)
> ndctl_dimm_get_devname(dimm), flags);
> }
>
> +static void parse_papr_flags(struct ndctl_dimm *dimm, char *flags)
> +{
> + char *start, *end;
> +
> + start = flags;
> + while ((end = strchr(start, ' '))) {
> + *end = '\0';
> + if (strcmp(start, "not_armed") == 0)
> + dimm->flags.f_arm = 1;
> + else if (strcmp(start, "flush_fail") == 0)
> + dimm->flags.f_flush = 1;
> + else if (strcmp(start, "restore_fail") == 0)
> + dimm->flags.f_restore = 1;
> + else if (strcmp(start, "smart_notify") == 0)
> + dimm->flags.f_smart = 1;
> + start = end + 1;
> + }
> + if (end != start)
> + dbg(ndctl_dimm_get_ctx(dimm), "%s: %s\n",
> + ndctl_dimm_get_devname(dimm), flags);
I think this would be more readable if ctx was obtained and saved in a
'ctx' variable at the start. Also, it might be better if 'flags' was
included in the string - something like "%s flags: %s\n"
> +}
> +
> static void parse_dimm_flags(struct ndctl_dimm *dimm, char *flags)
> {
> char *start, *end;
> @@ -856,6 +878,12 @@ static void *add_bus(void *parent, int id, const char
> *ctl_base)
> bus->revision = strtoul(buf, NULL, 0);
> }
>
> + sprintf(path, "%s/device/of_node/compatible", ctl_base);
> + if (sysfs_read_attr(ctx, path, buf) < 0)
> + bus->has_of_node = 0;
> + else
> + bus->has_of_node = 1;
> +
> sprintf(path, "%s/device/nfit/dsm_mask", ctl_base);
> if (sysfs_read_attr(ctx, path, buf) < 0)
> bus->nfit_dsm_mask = 0;
> @@ -964,6 +992,10 @@ NDCTL_EXPORT int ndctl_bus_has_nfit(struct ndctl_bus
> *bus)
> return bus->has_nfit;
> }
>
> +NDCTL_EXPORT int ndctl_bus_has_of_node(struct ndctl_bus *bus)
> +{
> + return bus->has_of_node;
> +}
New libndctl APIs need to update the 'symbol script' -
ndctl/lib/libndctl.sym. Create a new 'section' at the bottom, and add
all new symbols to that section. The new section would be 'LIBNDCTL_24'
(Everything going into a given release gets a new section in that file,
so any subsequent patches - throughout the release cycle - that add new
APIs can add them to this new section).
> /**
> * ndctl_bus_get_major - nd bus character device major number
> * @bus: ndctl_bus instance returned from ndctl_bus_get_{first|next}
> @@ -1441,6 +1473,47 @@ static int ndctl_bind(struct ndctl_ctx *ctx, struct
> kmod_module *module,
> static int ndctl_unbind(struct ndctl_ctx *ctx, const char *devpath);
> static struct kmod_module *to_module(struct ndctl_ctx *ctx, const char
> *alias);
>
> +static int add_papr_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
> +{
> + int rc = -ENODEV;
> + char buf[SYSFS_ATTR_SIZE];
> + struct ndctl_ctx *ctx = dimm->bus->ctx;
> + char *path = calloc(1, strlen(dimm_base) + 100);
> +
> + dbg(ctx, "Probing of_pmem dimm %d at %s\n", dimm->id, dimm_base);
> +
> + if (!path)
> + return -ENOMEM;
> +
> + sprintf(path, "%s/../of_node/compatible", dimm_base);
> + if (sysfs_read_attr(ctx, path, buf) < 0)
> + goto out;
Two things here:
1. Why not use the new ndctl_bus_has_of_node helper here? and
2. This looks redundant. add_papr_dimm() is only called if
ndctl_bus_has_of_node() during add_dimm.
> +
> +
Nit: double newline here
> + dbg(ctx, "Compatible of_pmem dimm %d at %s\n", dimm->id, buf);
> + /* construct path to the papr compatible dimm flags file */
> + sprintf(path, "%s/papr/flags", dimm_base);
> +
> + if (strcmp(buf, "ibm,pmemory") == 0 &&
> + sysfs_read_attr(ctx, path, buf) == 0) {
> +
> + dbg(ctx, "Found papr dimm %d at %s\n", dimm->id, buf);
Throughout the series - it would be better to print messages such as:
"%s: adding papr dimm with flags: %s\n", devname, buf
This would result in the canonical dimm names - nmem1, nmem2 and so on
being used instead of "dimm 1" which can be confusing just from a
convention standpoint.
Similarly for the print a few lines above this:
"%s: of_pmem compatible dimm: %s\n", devname, buf
(In this case, what does the 'compatible' sysfs attrubute contain? Is it
useful to print here? - I havent't looked, just asking).
> diff --git a/ndctl/lib/papr.c b/ndctl/lib/papr.c
> new file mode 100644
> index ..5ddf2755a950
> --- /dev/null
> +++ b/ndctl/lib/papr.c
> @@ -0,0 +1,32 @@
> +/*
> + * Copyright (C) 2020 IBM Corporation
> + *
> + * This program is free software; you can redistribute it
Re: [ndctl PATCH v5 1/6] libndctl: Refactor out add_dimm() to handle NFIT specific init
On Sat, 2020-05-30 at 03:35 +0530, Vaibhav Jain wrote:
> Presently add_dimm() only probes dimms that support NFIT/ACPI. Hence
> this patch refactors this functionality into two functions namely
> add_dimm() and add_nfit_dimm(). Function add_dimm() performs
> allocation and common 'struct ndctl_dimm' initialization and depending
> on whether the dimm-bus supports NIFT, calls add_nfit_dimm(). Once
> the probe is completed based on the value of 'ndctl_dimm.cmd_family'
> appropriate dimm-ops are assigned to the dimm.
>
> In case dimm-bus is of unknown type or doesn't support NFIT the
> initialization still continues, with no dimm-ops assigned to the
> 'struct ndctl_dimm' there-by limiting the functionality available.
>
> This patch shouldn't introduce any behavioral change.
>
> Signed-off-by: Vaibhav Jain
> ---
> Changelog:
>
> v4..v5:
> * None
>
> v3..v4:
> * None
>
> v2..v3:
> * None
>
> v1..v2:
> * None
> ---
> ndctl/lib/libndctl.c | 193 +-
> -
> 1 file changed, 112 insertions(+), 81 deletions(-)
Hi Vaibhav,
This mostly looks good, one minor nit below.
>
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index ee737cbbfe3e..d76dbf7e17de 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -1441,82 +1441,15 @@ static int ndctl_bind(struct ndctl_ctx *ctx, struct
> kmod_module *module,
> static int ndctl_unbind(struct ndctl_ctx *ctx, const char *devpath);
> static struct kmod_module *to_module(struct ndctl_ctx *ctx, const char
> *alias);
>
> -static void *add_dimm(void *parent, int id, const char *dimm_base)
> +static int add_nfit_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
> {
> - int formats, i;
> - struct ndctl_dimm *dimm;
> + int i, rc = -1;
> char buf[SYSFS_ATTR_SIZE];
> - struct ndctl_bus *bus = parent;
> - struct ndctl_ctx *ctx = bus->ctx;
> + struct ndctl_ctx *ctx = dimm->bus->ctx;
> char *path = calloc(1, strlen(dimm_base) + 100);
>
> if (!path)
> + return -1;
You should generally avoid returning a plain '-1'. This really wants to
return an -ENOMEM.
>
> /*
>* 'unique_id' may not be available on older kernels, so don't
> @@ -1582,24 +1515,15 @@ static void *add_dimm(void *parent, int id, const
> char *dimm_base)
> sprintf(path, "%s/nfit/family", dimm_base);
> if (sysfs_read_attr(ctx, path, buf) == 0)
> dimm->cmd_family = strtoul(buf, NULL, 0);
> - if (dimm->cmd_family == NVDIMM_FAMILY_INTEL)
> - dimm->ops = intel_dimm_ops;
> - if (dimm->cmd_family == NVDIMM_FAMILY_HPE1)
> - dimm->ops = hpe1_dimm_ops;
> - if (dimm->cmd_family == NVDIMM_FAMILY_MSFT)
> - dimm->ops = msft_dimm_ops;
> - if (dimm->cmd_family == NVDIMM_FAMILY_HYPERV)
> - dimm->ops = hyperv_dimm_ops;
>
> sprintf(path, "%s/nfit/dsm_mask", dimm_base);
> if (sysfs_read_attr(ctx, path, buf) == 0)
> dimm->nfit_dsm_mask = strtoul(buf, NULL, 0);
>
> - dimm->formats = formats;
> sprintf(path, "%s/nfit/format", dimm_base);
> if (sysfs_read_attr(ctx, path, buf) == 0)
> dimm->format[0] = strtoul(buf, NULL, 0);
> - for (i = 1; i < formats; i++) {
> + for (i = 1; i < dimm->formats; i++) {
> sprintf(path, "%s/nfit/format%d", dimm_base, i);
> if (sysfs_read_attr(ctx, path, buf) == 0)
> dimm->format[i] = strtoul(buf, NULL, 0);
> @@ -1610,7 +1534,114 @@ static void *add_dimm(void *parent, int id, const
> char *dimm_base)
> out:
> + if (rc) {
> + err(ctx, "Unable to probe dimm:%d. Err:%d\n", id, rc);
Returning -1 being awkward is especially true when it might end up
getting printed as part of an error message like this. Indeed, you
shouldn't even print 'rc' as a number, which then needs to get looked up
- use strerror to turn it into a string.
Additionally, "dimm:" is pretty nonstandard in ndctl, we
normally use the 'devname' for error messages. How about something like:
err(ctx, "%s: probe failed: %s\n", ndctl_dimm_get_devname(dimm),
strerror(-rc));
> + goto err_read;
> + }
> +
> list_add(>dimms, >list);
> free(path);
>
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH v5] mm/memory_hotplug: refrain from adding memory into an impossible node
On Fri, 2020-04-17 at 08:38 +0200, Michal Hocko wrote:
> On Thu 16-04-20 16:54:38, Vishal Verma wrote:
> > A misbehaving qemu created a situation where the ACPI SRAT table
> > advertised one fewer proximity domains than intended. The NFIT table did
> > describe all the expected proximity domains. This caused the device dax
> > driver to assign an impossible target_node to the device, and when
> > hotplugged as system memory, this would fail with the following
> > signature:
> >
> > [ +0.001627] BUG: kernel NULL pointer dereference, address:
> > 0088
> > [ +0.001331] #PF: supervisor read access in kernel mode
> > [ +0.000975] #PF: error_code(0x) - not-present page
> > [ +0.000976] PGD 8001767d4067 P4D 8001767d4067 PUD 10e0c4067 PMD > > 0
> > [ +0.001338] Oops: [#1] SMP PTI
> > [ +0.000676] CPU: 4 PID: 22737 Comm: kswapd3 Tainted: G O
> > 5.6.0-rc5 #9
> > [ +0.001457] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> > BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
> > [ +0.001990] RIP: 0010:prepare_kswapd_sleep+0x7c/0xc0
> > [ +0.000780] Code: 89 df e8 87 fd ff ff 89 c2 31 c0 84 d2 74 e6 0f 1f 44
> > 00 00 48 8b 05 fb af 7a 01 48 63 93 88 1d 01 00 48 8b
> > 84 d0 20 0f 00 00 <48> 3b 98 88 00 00 00 75 28 f0 80 a0
> > 80 00 00 00 fe f0 80 a3 38 20
> > [ +0.002877] RSP: 0018:c900017a3e78 EFLAGS: 00010202
> > [ +0.000805] RAX: RBX: 8881209e RCX:
> >
> > [ +0.001115] RDX: 0003 RSI: RDI:
> > 8881209e0e80
> > [ +0.001098] RBP: R08: R09:
> > 8000
> > [ +0.001092] R10: R11: 0003 R12:
> > 0003
> > [ +0.001092] R13: 0003 R14: R15:
> > c900017a3ec8
> > [ +0.001091] FS: () GS:888318c0()
> > knlGS:
> > [ +0.001275] CS: 0010 DS: ES: CR0: 80050033
> > [ +0.000882] CR2: 0088 CR3: 000120b50002 CR4:
> > 001606e0
> > [ +0.001095] Call Trace:
> > [ +0.000388] kswapd+0x103/0x520
> > [ +0.000494] ? finish_wait+0x80/0x80
> > [ +0.000547] ? balance_pgdat+0x5a0/0x5a0
> > [ +0.000607] kthread+0x120/0x140
> > [ +0.000508] ? kthread_create_on_node+0x60/0x60
> > [ +0.000706] ret_from_fork+0x3a/0x50
> >
> > Add a check in the add_memory path to fail if the node to which we
> > are adding memory is in the node_possible_map
> >
> > Cc: Michal Hocko
> > Cc: David Hildenbrand
> > Cc: Dan Williams
> > Cc: Dave Hansen
> > Acked-by: David Hildenbrand
> > Signed-off-by: Vishal Verma
>
> Acked-by: Michal Hocko
>
> We can start thiking on how to handle such a misconfiguration more
> gracefully when we see this hitting in real world and find out more why
> that happens. E.g. if a FW/BIOS are not fixable then we can implement
> some fallback strategy but this should be a good start.
>
> Thanks!
Thank you for the review Michal.
Should this go via Andrew and the mm tree?
>
> > ---
> > mm/memory_hotplug.c | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > v2:
> > - Centralize the check in the add_memory path (David)
> > - Instead of failing, add the memory to a nearby node, while warning
> > (and tainting) to call out attention to the firmware bug (Dan)
> >
> > v3:
> > - Fix the CONFIG_NUMA=n case, and use node 0 as the final fallback (Dan)
> >
> > v4:
> > - Error out instead of being smart about picking a node that wasn't
> > asked for (Michal)
> >
> > v5:
> > - Change the return code to -EINVAL (David)
> >
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index 0a54ffac8c68..e07b80d149db 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -1005,6 +1005,11 @@ int __ref add_memory_resource(int nid, struct
> > resource *res)
> > if (ret)
> > return ret;
> >
> > + if (!node_possible(nid)) {
> > + WARN(1, "node %d was absent from the node_possible_map\n", nid);
> > + return -EINVAL;
> > + }
> > +
> > mem_hotplug_begin();
> >
> > /*
> > --
> > 2.21.1
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH v4] mm/memory_hotplug: refrain from adding memory into an impossible node
On Thu, 2020-04-16 at 19:53 +0200, David Hildenbrand wrote: > > > > > > > Hm, I'm happy to make the changes, but EINVAL to me suggests there is a > > > problem in the way this was called by the user. And in this case there > > > really might not be much the user can change in case fo buggy firmware. > > > > Yeah, but introducing new return codes callers might not expected might > > create IMHO other issues. > > > > > Same thing with the WARN - make the potential firmware bug much more > > > obvious and visible. > > > > > > > Yeah, but I doubt this is really necessary. No strong feelings. > > > > Forgot to > > Acked-by: David Hildenbrand > Thanks for the review David. I'll change the return code, and keep the WARN, and send a new version. ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH v4] mm/memory_hotplug: refrain from adding memory into an impossible node
On Thu, 2020-04-16 at 19:12 +0200, David Hildenbrand wrote:
> On 16.04.20 19:10, Vishal Verma wrote:
> >
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index 0a54ffac8c68..ddd3347edd54 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -1005,6 +1005,11 @@ int __ref add_memory_resource(int nid, struct
> > resource *res)
> > if (ret)
> > return ret;
> >
> > + if (!node_possible(nid)) {
> > + WARN(1, "node %d was absent from the node_possible_map\n", nid);
> > + return -ENXIO;
>
> Nit: I suggest using "-EINVAL" instead (e.g., returned via
> check_hotplug_memory_range).
>
> Not sure if we should pr_err() instead of WARN (see e.g.,
> check_hotplug_memory_range)
>
Hm, I'm happy to make the changes, but EINVAL to me suggests there is a
problem in the way this was called by the user. And in this case there
really might not be much the user can change in case fo buggy firmware.
Same thing with the WARN - make the potential firmware bug much more
obvious and visible.
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH v3] mm/memory_hotplug: refrain from adding memory into an impossible node
On Thu, 2020-04-16 at 18:16 +0200, David Hildenbrand wrote: > > > > > > Doing that papers over something that is clearly a FW issue and makes > > > it "my performance is suboptimal" deal with it OS problem. Really, is > > > this something we have to care about. Your changelog talks about a Qemu > > > misconfiguration which is trivial to fix. Has this ever been observed > > > with a real HW? > > > > > Well - more of a qemu bug I think - I can share the details, but it just > > looked like it was producing a bogus SRAT. I think it is plausible that > > such a firmware bug can happen out in the wild. The NFIT tables would > > just need to reference a 'proximity domain' that the SRAT hasn't > > previously described, and hotplug will happily go add memory from the > > NFIT and the backing node related data structures would be missing. > > > > I'm not too opposed to erroring out, so long as we are ok with the fact > > that we will leave some memory stranded until there's a firmware fix. > > So let's reject it and print a warning, so we know it's a thing. If this > actually shows up often in real live, we have good evidence that we > should tolerate buggy firmwares instead of warning/rejecting. > > (rejecting from inside add_memory() still makes sense IMHO) > Sounds good, I'll send a v4. ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH v3] mm/memory_hotplug: refrain from adding memory into an impossible node
On Thu, 2020-04-16 at 08:19 +0200, Michal Hocko wrote: > On Wed 15-04-20 20:32:00, Verma, Vishal L wrote: > > > > > > I really do not like this. Why should we try to be clever and change the > > > node id requested by the caller? I would just stick with node_possible > > > check and be done with this. > > > > Hi Michal, > > > > Being clever allows us to still use the memory even if it is in a non- > > optimal configuration. Failing here leaves the user no path to add this > > memory until the firmware is fixed. It is the tradeoff between some > > usability vs. how loud we want to be for the failure. > > Doing that papers over something that is clearly a FW issue and makes > it "my performance is suboptimal" deal with it OS problem. Really, is > this something we have to care about. Your changelog talks about a Qemu > misconfiguration which is trivial to fix. Has this ever been observed > with a real HW? > Well - more of a qemu bug I think - I can share the details, but it just looked like it was producing a bogus SRAT. I think it is plausible that such a firmware bug can happen out in the wild. The NFIT tables would just need to reference a 'proximity domain' that the SRAT hasn't previously described, and hotplug will happily go add memory from the NFIT and the backing node related data structures would be missing. I'm not too opposed to erroring out, so long as we are ok with the fact that we will leave some memory stranded until there's a firmware fix. ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH v3] mm/memory_hotplug: refrain from adding memory into an impossible node
On Wed, 2020-04-15 at 12:43 +0200, Michal Hocko wrote:
> On Tue 14-04-20 17:58:12, Vishal Verma wrote:
> [...]
> > +static int check_hotplug_node(int nid)
> > +{
> > + int alt_nid;
> > +
> > + if (node_possible(nid))
> > + return nid;
> > +
> > + alt_nid = numa_map_to_online_node(nid);
> > + if (alt_nid == NUMA_NO_NODE)
> > + alt_nid = first_online_node;
> > + WARN_TAINT(1, TAINT_FIRMWARE_WORKAROUND,
> > + "node %d expected, but was absent from the
> > node_possible_map, using %d instead\n",
> > + nid, alt_nid);
>
> I really do not like this. Why should we try to be clever and change the
> node id requested by the caller? I would just stick with node_possible
> check and be done with this.
Hi Michal,
Being clever allows us to still use the memory even if it is in a non-
optimal configuration. Failing here leaves the user no path to add this
memory until the firmware is fixed. It is the tradeoff between some
usability vs. how loud we want to be for the failure.
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH] ndctl/README: Add a missing config setting
On Thu, 2020-03-26 at 09:25 +, Dorau, Lukasz wrote: > Add a missing config setting to README.md: > CONFIG_DEV_DAX_PMEM_COMPAT=m > > Cc: Williams, Dan J > Cc: Verma, Vishal L > Signed-off-by: Lukasz Dorau > --- > README.md | 1 + > 1 file changed, 1 insertion(+) > Thanks, looks good, I've pushed this out to a 'ld/readme' topic branch, and it will make its way through pending and master. -Vishal ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
[ANNOUNCE] ndctl v68
This release incorporates functionality up to the 5.6 kernel.
Highlights for this release include new commands to read-infoblock and
write-infoblock, improvements and tests related to alignment
constraints, misc build/compilation related fixes, and misc usability
and documentation fixes.
The shortlog for this release is appended below:
Auke Kok (3):
ndctl/build: Do not use `check-news` when `NEWS` file is absent entirely.
ndctl/build: Ensure header and other misc files are listed.
ndctl/build: Add `header` as a prereq to Make rule where it is consumed.
Dan Williams (39):
ndctl/namespace: Clarify that 'reconfigure' == 'destroy+create'
ndctl/namespace: Fixup man page indentation
ndctl/list: Add 'target_node' to region and namespace verbose listings
ndctl/docs: Fix mailing list sign-up link
ndctl/list: Drop named list objects from verbose listing
daxctl/list: Avoid memory operations without resource data
ndctl/build: Fix distcheck
ndctl/namespace: Fix destroy-namespace accounting relative to seed devices
ndctl/region: Support ndctl_region_{get, set}_align()
ndctl/namespace: Emit better errors on failure
ndctl/namespace: Check for region alignment violations
ndctl/util: Up-level is_power_of_2() and introduce IS_ALIGNED
ndctl/namespace: Validate resource alignment for dax-mode namespaces
ndctl/namespace: Add read-infoblock command
ndctl/test: Update dax-dev to handle multiple e820 ranges
ndctl/namespace: Always zero info-blocks
ndctl/namespace: Disable autorecovery of create-namespace failures
ndctl/build: Fix EXTRA_DIST already defined errors
ndctl/test: Checkout device-mapper + dax operation
ndctl/test: Exercise sub-section sized namespace creation/deletion
ndctl/namespace: Kill off the legacy mode names
ndctl/namespace: Introduce mode-to-name and name-to-mode helpers
ndctl/namespace: Validate namespace size within
validate_namespace_options()
ndctl/namespace: Clarify 16M minimum size requirement
ndctl/test: Regression test 'failed to track'
ndctl/dimm: Rework dimm command status reporting
ndctl/dimm: Rework iteration to drop unaligned pointers
ndctl/test: Fix typos / loss of tpm.handle in security test
ndctl/test: Cleanup test-vs-production nvdimm module detection
ndctl/test: Relax dax_pmem_compat requirement
ndctl/namespace: Fix namespace-action vs namespace-mode confusion
ndctl/namespace: Update 'pfn' infoblock definition
ndctl/util: Return 0 for NULL arguments to parse_size64()
ndctl/namespace: Fix read-info-block vs read-infoblock
ndctl/namespace: Parse infoblocks from stdin
ndctl/namespace: Add write-infoblock command
ndctl/list: Add option to list configured + disabled namespaces
ndctl/lib/namespace: Fix resource retrieval after size change
ndctl/test: Regression test misaligned namespaces
Ira Weiny (1):
ndctl: Clean up loop logic in query_fw_finish_status
Santosh Sivaraj (2):
ndctl/namespace: Fix enable-namespace error for seed namespaces
ndctl/zero-labels: Display error if regions are active
Vaibhav Jain (1):
namespace/create: Don't create multiple namespaces unless greedy
Vishal Verma (7):
ndctl/namespace: remove open coded is_namespace_active()
ndctl/namespace: introduce ndctl_namespace_is_configuration_idle()
ndctl/README: Update kernel documentation URL
ndctl/lib: fix symbol redefinitions reported by GCC10
ndctl: add util/filter.{c,h} to ndctl_SOURCES in Makefile.am
ndctl/namespace: Fix a resource leak in file_write_infoblock
ndctl: release v68
Yi Zhang (1):
ndctl/test: add bus-id parameter for start-scrub/wait-scrub operation
redhairer (2):
ndctl/test: add UUID_LIBS for blk_namespaces/pmem_namespaces/device_dax
daxctl: Change region input type from INTEGER to STRING.
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH 00/36] Multiple topics / backlog for v68
On Sat, 2020-02-29 at 12:20 -0800, Dan Williams wrote: > Changes from review: > - Add NDCTL_LIST_LINT to not regress list output (Jeff) > - Add kernel-doc description for ndctl_region_set_align() (Jeff) > > --- > > About half of these have been posted previously, but have been reworked > and revised as they have percolated in my tree relative to other > arriving features. Yes, it is quite a lot to ingest at once, but given > the interdependencies and need to catch up I decided to post it all > together. > > The recommendation on how to review is to start with the new tests, > those introduce some new commands, and those new commands introduce some > new library routines. The rest are miscellaneous updates, fixes, and > cleanups. > This all looks good to me - you can add: Reviewed-by: Vishal Verma ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH] ndctl/list: Drop named list objects from verbose listing
On Wed, 2020-02-19 at 12:09 -0800, Dan Williams wrote: > > > > > > Let's do a compromise, because users also hate nonsensical legacy that > > > they can't avoid. How about an environment variable, > > > "NDCTL_LIST_LINT", that users can set to opt into the latest / > > > cleanest output format with the understanding that the clean up may > > > regress scripts that were dependent on the old bugs. > > > > > Hm, this sounds good in concept, but how about waiting for this cleanup > > to go in after the (yes, long pending) config rework. Then this can just > > be a global config setting, and we won't have config things coming from > > the environment as well (which this would be a first of). > > That does make some sense, but I notice that git deals with "cosmetic" > environment variables (GIT_EDITOR, GIT_PAGER, etc) in addition to its > config file. So if we're borrowing from git, I'd also borrow that > config vs environment logic. True, that's reasonable. I guess I was hoping to avoid, if we can, suddenly having a multitude of config sources, but env variables are pretty standard and it should be fine to add them. ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH] ndctl/list: Drop named list objects from verbose listing
On Wed, 2020-02-19 at 10:53 -0800, Dan Williams wrote: > > > > > Will this break existing code that parses the javascript output? > > > > > > Always a potential for that. That said, I'd rather attempt to make it > > > symmetric and replace it if someone screams, rather than let this > > > quirk persist because it makes it impossible to ingest region data > > > with the same script across -R and -Rv. > > > > Yeah, I see where you're coming from. However, script authors will > > still have to deal with older versions of ndctl in the wild (for many > > years). If the decision was up to me, I'd live with the wart in favor > > of not breaking scripts when ndctl gets updated. Users hate that. > > Let's do a compromise, because users also hate nonsensical legacy that > they can't avoid. How about an environment variable, > "NDCTL_LIST_LINT", that users can set to opt into the latest / > cleanest output format with the understanding that the clean up may > regress scripts that were dependent on the old bugs. > Hm, this sounds good in concept, but how about waiting for this cleanup to go in after the (yes, long pending) config rework. Then this can just be a global config setting, and we won't have config things coming from the environment as well (which this would be a first of). ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH] ndctl/lib: make dimm_ops in private.h extern
On Thu, 2020-01-30 at 11:03 -0800, Dan Williams wrote: > On Thu, Jan 30, 2020 at 10:56 AM Vishal Verma > wrote: > > A toolchain update in Fedora 32 caused new compile errors due to > > multiple definitions of dimm_ops structures. The declarations in > > 'private.h' for the various NFIT families are present so that libndctl > > can find all the per-family dimm-ops. However they need to be declared > > as extern because the actual definitions are in .c > > Looks good to me. Does this quiet the build spew? I'm testing that now - it at least doesn't introduce any new errors in my F30 environment and makes sense. ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH v2 1/2] ndctl/namespace: Rework counts reported by enable-namespace
On Tue, 2019-11-05 at 10:27 -0700, Vishal Verma wrote: > Add detection of 'seed' namespaces > (ndctl_namespace_is_configuration_idle()) to the enable-namespace > operatiuon and libndctl API. In libndctl, return a '1' for seed > namespaces. In namespace.c, reinterpret a '1' based on a check for a > seed namespace, and decide on skip vs success accordingly. Collect this > into a new namespace_enable helper, and make the reported count > consistent by also skipping namespaces that were already enabled. > > Link: https://github.com/pmem/ndctl/issues/119 > Reported-by: Aneesh Kumar K.V > Cc: Dan Williams > Signed-off-by: Vishal Verma > --- > > Changes in v2: > - The kernel is the ultimate authority on enabling namespaces, so we > should let it make the decision of how to handle seed namespaces > instead of preemptively skipping them. Let the kernel make that > decision, and fix up error reporting after the fact. These break the unit tests, so I'll have to take another look at this. I suspect the added complexity is probably not worth the few extra prints from seed namespaces for operations using the 'all' keyword. ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH v2 2/2] ndctl/namespace: introduce ndctl_namespace_is_configuration_idle()
On Mon, 2019-11-04 at 13:08 -0700, Vishal Verma wrote: > > diff --git a/util/sysfs.c b/util/sysfs.c > index 9f7bc1f..81cd055 100644 > --- a/util/sysfs.c > +++ b/util/sysfs.c > @@ -20,6 +20,7 @@ > #include > #include > #include > +#include These are stray lines from the previous version, I'll resend removing these. Sorry for the noise. > #include > #include > #include > diff --git a/util/sysfs.h b/util/sysfs.h > index fb169c6..b2d28f1 100644 > --- a/util/sysfs.h > +++ b/util/sysfs.h > @@ -14,6 +14,7 @@ > #define __UTIL_SYSFS_H__ > > #include > +#include > > typedef void *(*add_dev_fn)(void *parent, int id, const char *dev_path); > ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH 2/2] ndctl/namespace: introduce ndctl_namespace_is_configurable()
On Mon, 2019-11-04 at 10:58 -0800, Dan Williams wrote: > > > > > Also, how about > > > s/ndctl_namespace_is_configurable/ndctl_namespace_is_configuration_idle/? > > > Because to me all namespaces are always "configurable", but some may > > > have active non-default properties set. > > > > Sounds reasonable, but how about ndctl_namespace_has_partial_config(), > > which I think describes the situation better, and makes it > > straightforward for a potential future --really-force (or -ff) option > > where we might still want to blow away anything on this namespace and > > reclaim it. > > Does "_has_partial_config()" imply "_has_full_config()"? In other > words, what ndctl cares about are namespaces that can effectively be > used as seeds with no existing configuration parameters having been > set. So "_is_configuration_idle()" seems unambiguous where > "_has_partial_config()" does not. Good point, I'll go with _is_configuration_idle() ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH 2/2] ndctl/namespace: introduce ndctl_namespace_is_configurable()
On Mon, 2019-11-04 at 08:35 -0800, Dan Williams wrote: > On Fri, Nov 1, 2019 at 1:27 PM Vishal Verma wrote: > > The motivation for this change is that we want to refrain from > > (re)configuring what appear to be partially configured namespaces. > > Namespaces may end up in a state that looks partially configured when > > the kernel isn't able to enable a namespace due to mismatched > > PAGE_SIZE expectations. In such cases, ndctl should not treat those > > as unconfigured 'seed' namespaces, and reuse them. > > > > Add a new ndctl_namespace_is_configurable API, whish tests whether a > > namespaces is active, and whether it is partially configured. If neither > > are true, then it can be used for (re)configuration. Additionally, deal > > with the corner case of ND_DEVICE_NAMESPACE_IO (legacy PMEM) namespaces > > by testing whether the size attribute is read-only (which indicates such > > a namespace). Legacy namespaces always appear as configured, since their > > size cannot be changed, but they are also always re-configurable. > > > > Use this API in namespace_reconfig() and namespace_create() to enable > > this partial configuration detection. > > A couple comments I think it's probably sufficient to just check > for ND_DEVICE_NAMESPACE_IO as I don't anticipate we'll ever have more > than one namespace type with a read-only size attribute. Yep I did notice I could do that, but I decided to go to the source of it instead of adding another ND_DEVICE_NAMESPACE_IO assumption. Also I had already written sysfs_attr_writable() before I noticed that :) But there are already assumptions around ND_DEVICE_NAMESPACE_IO, so adding another one here is fine. > Also, how about > s/ndctl_namespace_is_configurable/ndctl_namespace_is_configuration_idle/? > Because to me all namespaces are always "configurable", but some may > have active non-default properties set. Sounds reasonable, but how about ndctl_namespace_has_partial_config(), which I think describes the situation better, and makes it straightforward for a potential future --really-force (or -ff) option where we might still want to blow away anything on this namespace and reclaim it. ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH] libnvdimm/pmem: Delete include of nd-core.h
On Thu, 2019-10-31 at 17:31 -0700, Dan Williams wrote:
> The entire point of nd-core.h is to hide functionality that no leaf
> driver should touch. In fact, the commit that added it had no need to
> include it.
>
> Fixes: 06e8ccdab15f ("acpi: nfit: Add support for detect platform...")
> Cc: Ira Weiny
> Cc: Dave Jiang
> Cc: Vishal Verma
> Signed-off-by: Dan Williams
> ---
> drivers/nvdimm/pmem.c |1 -
> 1 file changed, 1 deletion(-)
Looks good,
Reviewed-by: Vishal Verma
>
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index 7a6f4501dcda..ad8e4df1282b 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -28,7 +28,6 @@
> #include "pmem.h"
> #include "pfn.h"
> #include "nd.h"
> -#include "nd-core.h"
>
> static struct device *to_dev(struct pmem_device *pmem)
> {
>
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH v3] nvdimm/btt: fix variable 'rc' set but not used
On Thu, 2019-10-31 at 10:05 -0400, Qian Cai wrote:
> drivers/nvdimm/btt.c: In function 'btt_read_pg':
> drivers/nvdimm/btt.c:1264:8: warning: variable 'rc' set but not used
> [-Wunused-but-set-variable]
> int rc;
> ^~
>
> Add a ratelimited message in case a storm of errors is encountered.
>
> Fixes: d9b83c756953 ("libnvdimm, btt: rework error clearing")
> Signed-off-by: Qian Cai
> ---
> v3: remove the unused "rc" per Vishal.
> v2: include the block address that is returning an error per Dan.
>
> drivers/nvdimm/btt.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
Looks good,
Reviewed-by: Vishal Verma
>
> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
> index 3e9f45aec8d1..5129543a0473 100644
> --- a/drivers/nvdimm/btt.c
> +++ b/drivers/nvdimm/btt.c
> @@ -1261,11 +1261,11 @@ static int btt_read_pg(struct btt *btt, struct
> bio_integrity_payload *bip,
>
> ret = btt_data_read(arena, page, off, postmap, cur_len);
> if (ret) {
> - int rc;
> -
> /* Media error - set the e_flag */
> - rc = btt_map_write(arena, premap, postmap, 0, 1,
> - NVDIMM_IO_ATOMIC);
> + if (btt_map_write(arena, premap, postmap, 0, 1,
> NVDIMM_IO_ATOMIC))
> + dev_warn_ratelimited(to_dev(arena),
> + "Error persistently tracking bad blocks
> at %#x\n",
> + premap);
> goto out_rtt;
> }
>
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH] Consider namespace with size as active namespace
On Thu, 2019-10-17 at 08:35 +0530, Aneesh Kumar K.V wrote: > > > > --- > > > ndctl/namespace.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/ndctl/namespace.c b/ndctl/namespace.c > > > index 58a9e3c53474..1f212a2b3a9b 100644 > > > --- a/ndctl/namespace.c > > > +++ b/ndctl/namespace.c > > > @@ -455,7 +455,8 @@ static int is_namespace_active(struct ndctl_namespace > > > *ndns) > > > return ndns && (ndctl_namespace_is_enabled(ndns) > > > || ndctl_namespace_get_pfn(ndns) > > > || ndctl_namespace_get_dax(ndns) > > > - || ndctl_namespace_get_btt(ndns)); > > > + || ndctl_namespace_get_btt(ndns) > > > + || ndctl_namespace_get_size(ndns)); > > > } > > > > > > /* [..] > > > The failing unit tests are sector-mode.sh and dax.sh > > > > I will see if i can run them on ppc64. We still had issues in getting > ndctl check to be running on ppc64. > I dug into this a bit more. The failure happens on 'legacy' namespaces (ND_DEVICE_NAMESPACE_IO). There is an assumption that legacy namespaces cannot be fully deleted, so as part of a reconfigure, when it comes time to delete the namespace (ndctl_namespace_delete()), we refuse to do that, and bail, before setting the size to zero. libndctl.c:4467 case ND_DEVICE_NAMESPACE_BLK: break; default: dbg(ctx, "%s: nstype: %d not deletable\n", ndctl_namespace_get_devname(ndns), ndctl_namespace_get_type(ndns)); return 0; } rc = namespace_set_size(ndns, 0); ... Indeed, destroy namespace wouldn't even get to that point, because that assumption is repeated in namespace_destroy(), where we switch on namespace type, and potentially skip over the ndctl_namespace_destroy call entirely. If setting the size to zero is now significant we'd need to rework both of these sites. In destroy_namespace(), delay the did_zero checking until after ndctl_namespace_delete(), and in ndctl_namespace_delete(), set the size to zero before the type check. Dan, does the above make sense - was there reason to refrain from touching the size on legacy namespaces? -Vishal ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH v2] nvdimm/btt: fix variable 'rc' set but not used
On Wed, 2019-10-30 at 17:28 -0400, Qian Cai wrote:
> drivers/nvdimm/btt.c: In function 'btt_read_pg':
> drivers/nvdimm/btt.c:1264:8: warning: variable 'rc' set but not used
> [-Wunused-but-set-variable]
> int rc;
> ^~
>
> Add a ratelimited message in case a storm of errors is encountered.
>
> Fixes: d9b83c756953 ("libnvdimm, btt: rework error clearing")
> Signed-off-by: Qian Cai
> ---
>
> v2: include the block address that is returning an error per Dan.
>
> drivers/nvdimm/btt.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
> index 3e9f45aec8d1..10313be78221 100644
> --- a/drivers/nvdimm/btt.c
> +++ b/drivers/nvdimm/btt.c
> @@ -1266,6 +1266,12 @@ static int btt_read_pg(struct btt *btt, struct
> bio_integrity_payload *bip,
> /* Media error - set the e_flag */
> rc = btt_map_write(arena, premap, postmap, 0, 1,
> NVDIMM_IO_ATOMIC);
> +
> + if (rc)
> + dev_warn_ratelimited(to_dev(arena),
> + "Error persistently tracking bad blocks
> at %#x\n",
> + premap);
> +
> goto out_rtt;
> }
Good find! Since we're not really using rc later, we should just
simplify this to:
if (btt_map_write(...))
dev_warn_ratelimited(...)
goto out_rtt;
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH] ndctl/namespace: Fixup man page indentation
On Tue, 2019-10-29 at 10:56 -0700, Dan Williams wrote:
> Text that follows a list tends to continue the list indentation. Use a
> bare "::" to end the list indentation.
>
> Signed-off-by: Dan Williams
> ---
> Documentation/ndctl/ndctl-create-namespace.txt |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/ndctl/ndctl-create-namespace.txt
> b/Documentation/ndctl/ndctl-create-namespace.txt
> index 45a4c4c2f408..8cd80fa789c1 100644
> --- a/Documentation/ndctl/ndctl-create-namespace.txt
> +++ b/Documentation/ndctl/ndctl-create-namespace.txt
> @@ -155,7 +155,7 @@ OPTIONS
> per-page metadata. The allocation can be drawn from either:
> - "mem": typical system memory
> - "dev": persistent memory reserved from the namespace
> -
> +::
> Given relative capacities of "Persistent Memory" to "System
> RAM" the allocation defaults to reserving space out of the
> namespace directly ("--map=dev"). The overhead is 64-bytes per
>
Good find!
I see how this (ab)uses the list marker by creating a new list item
without a new 'term' associated with it. I disliked creating a new list
item when the paragraph logically belongs in the previous list, and I
tried to use various combinations of list continuation markers, play
around with list starts/ends, but couldn't get the paragraph to
dissociate from the 'dev/mem' sublist and associate it back to the
'parent' list.
So I'll take this :)
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
[ANNOUNCE] ndctl v67
This release incorporates functionality up to the 5.4 kernel, and adds a number of bug fixes, and improvements. Highlights include small changes for PowerPC compatibility, improvements to the dax.sh unit test to detect failures in mapping huge pages, support for the 'security frozen' attribute, user experience improvements for the daxctl-reconfigure-device command, including an option to specify movable vs. non-movable state for onlining memory, and an option to allow create-namespaces to create a maximal configuration until it exhausts all available region capacity. The shortlog for this release is appended below. Aneesh Kumar K.V (1): ndctl: Reuse the align value from the original namespace on reconfiguration Dan Williams (10): ndctl/lib: Fix duplicate bus detection ndctl/test: Add xfs reflink dependency daxctl/test: Skip daxctl-devices.sh on older kernels ndctl/dimm: Add support for separate security-frozen attribute ndctl/namespace: Fix 'clear-error -s' excessive scrubbing test/dax.sh: Fix failure reporting / handling test/dax.sh: Fix xfs 2M alignment test/dax.sh: Validate huge page mappings test/dax.sh: Make dax.sh more robust vs small namespaces test/dax.sh: Split into ext4 and xfs tests Jeff Moyer (4): util/abspath: cleanup prefix_filename fix building of tags tables query_fw_finish_status: get rid of redundant variable load-keys: get rid of duplicate assignment Naoya Horiguchi (1): Documentation/ndctl: fix typo in ndctl-clear-errors.txt Vishal Verma (21): Documentation/namespace-description: Clarify label-less restrictions ndctl/check-namespace: improve error message in absence of a BTT libdaxctl: point to migrate-device-model for dax-class errors Documentation: refactor 'bus options' into its own include Documentation: clarify bus/dimm/region filtering ndctl/namespace: add a --continue option to create namespaces greedily libdaxctl: fix the system-ram capability check libdaxctl: fix device reconfiguration with builtin drivers libndctl: Fix a potentially non NUL-terminated string operation libdaxctl: fix memory leaks with daxctl_memory objects libdaxctl: refactor path construction in op_for_one_memblock() libdaxctl: refactor memblock_is_online() checks daxctl/device.c: fix json output omission for reconfigure-device libdaxctl: add an API to determine if memory is movable libdaxctl: allow memblock_in_dev() to return an error daxctl: show a 'movable' attribute in device listings daxctl: detect races when onlining memory blocks Documentation: clarify memory movablity for reconfigure-device libdaxctl: add an API to online memory in a non-movable state daxctl: add --no-movable option for onlining memory ndctl: release v67 ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl patch 3/4] query_fw_finish_status: get rid of redundant variable
On Fri, 2019-10-25 at 15:21 -0700, Ira Weiny wrote:
> How about this patch instead? Untested.
>
> Ira
Not a big deal, but just a quick note - if you include a scissors line
here, I can easily apply it via git am --scissors
--8<--
Otherwise this looks good in principle.
I've already got Jeff's original (less intrusive) patch queued for v67 -
maybe we can rebase this to be its own refactoring patch, and get some
testing etc. for 68?
>
> From 24511b6a9f1b5e5c9e36c70ef6a03da5100cf4c7 Mon Sep 17 00:00:00 2001
> From: Ira Weiny
> Date: Fri, 25 Oct 2019 15:16:13 -0700
> Subject: [PATCH] ndctl: Clean up loop logic in query_fw_finish_status
>
> This gets rid of a redundant variable as originally pointed out by Jeff
> Moyer[1]
>
> Also, while we are here change the printf's to fprintf(stderr, ...)
>
> [1] https://patchwork.kernel.org/patch/11199557/
>
> Suggested-by: Jeff Moyer
> Signed-off-by: Ira Weiny
> ---
> ndctl/dimm.c | 142 +--
> 1 file changed, 70 insertions(+), 72 deletions(-)
>
> diff --git a/ndctl/dimm.c b/ndctl/dimm.c
> index 5e6fa19bab15..84de014e93d6 100644
> --- a/ndctl/dimm.c
> +++ b/ndctl/dimm.c
> @@ -682,7 +682,6 @@ static int query_fw_finish_status(struct ndctl_dimm *dimm,
> struct ndctl_cmd *cmd;
> int rc;
> enum ND_FW_STATUS status;
> - bool done = false;
> struct timespec now, before, after;
> uint64_t ver;
>
> @@ -692,88 +691,87 @@ static int query_fw_finish_status(struct ndctl_dimm
> *dimm,
>
> rc = clock_gettime(CLOCK_MONOTONIC, );
> if (rc < 0)
> - goto out;
> + goto unref;
>
> now.tv_nsec = fw->query_interval / 1000;
> now.tv_sec = 0;
>
> - do {
> - rc = ndctl_cmd_submit(cmd);
> - if (rc < 0)
> - break;
> +again:
> + rc = ndctl_cmd_submit(cmd);
> + if (rc < 0)
> + goto unref;
>
> - status = ndctl_cmd_fw_xlat_firmware_status(cmd);
> - switch (status) {
> - case FW_SUCCESS:
> - ver = ndctl_cmd_fw_fquery_get_fw_rev(cmd);
> - if (ver == 0) {
> - fprintf(stderr, "No firmware updated.\n");
> - rc = -ENXIO;
> - goto out;
> - }
> + status = ndctl_cmd_fw_xlat_firmware_status(cmd);
> + if (status == FW_EBUSY) {
> + /* Still on going, continue */
> + rc = clock_gettime(CLOCK_MONOTONIC, );
> + if (rc < 0) {
> + rc = -errno;
> + goto unref;
> + }
>
> - printf("Image updated successfully to DIMM %s.\n",
> - ndctl_dimm_get_devname(dimm));
> - printf("Firmware version %#lx.\n", ver);
> - printf("Cold reboot to activate.\n");
> - done = true;
> - rc = 0;
> - break;
> - case FW_EBUSY:
> - /* Still on going, continue */
> - rc = clock_gettime(CLOCK_MONOTONIC, );
> - if (rc < 0) {
> - rc = -errno;
> - goto out;
> - }
> + /*
> + * If we expire max query time,
> + * we timed out
> + */
> + if (after.tv_sec - before.tv_sec >
> + fw->max_query / 100) {
> + rc = -ETIMEDOUT;
> + goto unref;
> + }
>
> - /*
> - * If we expire max query time,
> - * we timed out
> - */
> - if (after.tv_sec - before.tv_sec >
> - fw->max_query / 100) {
> - rc = -ETIMEDOUT;
> - goto out;
> - }
> + /*
> + * Sleep the interval dictated by firmware
> + * before query again.
> + */
> + rc = nanosleep(, NULL);
> + if (rc < 0) {
> + rc = -errno;
> + goto unref;
> + }
> + goto again;
> + }
>
> - /*
> - * Sleep the interval dictated by firmware
> - * before query again.
> - */
> - rc = nanosleep(, NULL);
> - if (rc < 0) {
> - rc = -errno;
> - goto out;
> - }
> - break;
> - case FW_EBADFW:
> - fprintf(stderr,
> - "Firmware failed to verify by DIMM %s.\n",
> -
Re: [PATCH] uapi: Add the BSD-2-Clause license to ndctl.h
On Fri, 2019-10-25 at 15:45 -0700, Dan Williams wrote: > On Fri, Oct 25, 2019 at 10:55 AM D Scott Phillips > wrote: > > Allow ndctl.h to be licensed with BSD-2-Clause so that other > > operating systems can provide the same user level interface. > > --- > > > > I've been working on nvdimm support in FreeBSD and would like to > > offer the same ndctl API there to ease porting of application > > code. Here I'm proposing to add the BSD-2-Clause license to this > > header file, so that it can later be copied into FreeBSD. > > > > I believe that all the authors of changes to this file (in the To: > > list) would need to agree to this change before it could be > > accepted, so any signed-off-by is intentionally ommited for now. > > Thanks, > > I have no problem with this change, but let's take the opportunity to > let SPDX do its job and drop the full license text. This is fine by me too, barring the full license text vs. SPDX caveat Dan mentions. ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH 3/4] test/dax.sh: Validate huge page mappings
> On Wed, 2019-10-23 at 19:33 +0000, Verma, Vishal L wrote:
> > > @@ -91,4 +111,4 @@ json=$($NDCTL create-namespace -m raw -f -e $dev)
> > > eval $(json2var <<< "$json")
> > > [ $mode != "fsdax" ] && echo "fail: $LINENO" && exit 1
> >
> > same comment about quoting "$mode". If 'mode' happens to be empty for
> > soem reason, we want to fail with the error message - instead the above
> > will fail with a syntax error.
>
> Sorry ignore this - that was a context line..
>
Hey Dan,
I've applied patches 1-3, with the following fixup to patch 3.
Patch 4 didn't apply cleanly, if you can resend that I'll queue it up too.
--8<--
>From 9d6c43d5240ddb18b5540b3064f2f90c25dcf574 Mon Sep 17 00:00:00 2001
From: Vishal Verma
Date: Fri, 25 Oct 2019 16:41:22 -0600
Subject: [ndctl PATCH] fixup! test/dax.sh: Validate huge page mappings
---
test/dax.sh | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/test/dax.sh b/test/dax.sh
index 157b398..45c2027 100755
--- a/test/dax.sh
+++ b/test/dax.sh
@@ -32,8 +32,8 @@ run_test() {
rc=0
if ! trace-cmd record -e fs_dax:dax_pmd_fault_done ./dax-pmd
$MNT/$FILE; then
rc=$?
- if [ $rc -ne 77 -a $rc -ne 0 ]; then
- cleanup $1
+ if [ "$rc" -ne 77 ] && [ "$rc" -ne 0 ]; then
+ cleanup "$1"
fi
fi
@@ -44,17 +44,18 @@ run_test() {
# result of success (NOPAGE).
count=0
rc=1
- for p in $(trace-cmd report | awk '{ print $21 }')
- do
- if [ $count -lt 10 ]; then
- if [ $p != "0x100" -a $p != "NOPAGE" ]; then
- cleanup $1
+ while read -r p; do
+ [[ $p ]] || continue
+ if [ "$count" -lt 10 ]; then
+ if [ "$p" != "0x100" ] && [ "$p" != "NOPAGE" ]; then
+ cleanup "$1"
fi
fi
count=$((count + 1))
- done
+ done < <(trace-cmd report | awk '{ print $21 }')
+
if [ $count -lt 10 ]; then
- cleanup $1
+ cleanup "$1"
fi
}
--
2.20.1
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl patch 3/4] query_fw_finish_status: get rid of redundant variable
On Wed, 2019-10-23 at 22:28 +, Verma, Vishal L wrote:
> On Fri, 2019-10-18 at 17:06 -0400, Jeff Moyer wrote:
> > Ira Weiny writes:
> > > On Fri, Oct 18, 2019 at 04:23:01PM -0400, Jeff Moyer wrote:
> > > > The 'done' variable only adds confusion.
> > > >
> > > > Signed-off-by: Jeff Moyer
> > > > ---
> > > > ndctl/dimm.c | 7 +--
> > > > 1 file changed, 1 insertion(+), 6 deletions(-)
> > > >
> > > > diff --git a/ndctl/dimm.c b/ndctl/dimm.c
> > > > index c8821d6..f28b9c1 100644
> > > > --- a/ndctl/dimm.c
> > > > +++ b/ndctl/dimm.c
> > > > @@ -682,7 +682,6 @@ static int query_fw_finish_status(struct ndctl_dimm
> > > > *dimm,
> > > > struct ndctl_cmd *cmd;
> > > > int rc;
> > > > enum ND_FW_STATUS status;
> > > > - bool done = false;
> > > > struct timespec now, before, after;
> > > > uint64_t ver;
> > > >
> > > > @@ -716,7 +715,6 @@ static int query_fw_finish_status(struct ndctl_dimm
> > > > *dimm,
> > > > ndctl_dimm_get_devname(dimm));
> > > > printf("Firmware version %#lx.\n", ver);
> > > > printf("Cold reboot to activate.\n");
> > > > - done = true;
> > > > rc = 0;
> > >
> > > Do we need "goto out" here?
> >
> > Yes, I missed that one. Thanks.
>
> This actually looks fine, since there is a 'break' down below.
>
> > > > break;
> > > > case FW_EBUSY:
(Watching the unit test run fall into an infinite loop..) Nope, the
break is in the switch scope, the while loop needs the 'goto out'.
Yes this bit definitely needs to be refactored :)
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl patch 3/4] query_fw_finish_status: get rid of redundant variable
On Fri, 2019-10-18 at 17:06 -0400, Jeff Moyer wrote:
> Ira Weiny writes:
> > On Fri, Oct 18, 2019 at 04:23:01PM -0400, Jeff Moyer wrote:
> > > The 'done' variable only adds confusion.
> > >
> > > Signed-off-by: Jeff Moyer
> > > ---
> > > ndctl/dimm.c | 7 +--
> > > 1 file changed, 1 insertion(+), 6 deletions(-)
> > >
> > > diff --git a/ndctl/dimm.c b/ndctl/dimm.c
> > > index c8821d6..f28b9c1 100644
> > > --- a/ndctl/dimm.c
> > > +++ b/ndctl/dimm.c
> > > @@ -682,7 +682,6 @@ static int query_fw_finish_status(struct ndctl_dimm
> > > *dimm,
> > > struct ndctl_cmd *cmd;
> > > int rc;
> > > enum ND_FW_STATUS status;
> > > - bool done = false;
> > > struct timespec now, before, after;
> > > uint64_t ver;
> > >
> > > @@ -716,7 +715,6 @@ static int query_fw_finish_status(struct ndctl_dimm
> > > *dimm,
> > > ndctl_dimm_get_devname(dimm));
> > > printf("Firmware version %#lx.\n", ver);
> > > printf("Cold reboot to activate.\n");
> > > - done = true;
> > > rc = 0;
> >
> > Do we need "goto out" here?
>
> Yes, I missed that one. Thanks.
This actually looks fine, since there is a 'break' down below.
>
> > > break;
> > > case FW_EBUSY:
[..]
> > > - } while (!done);
> > > + } while (true);
> >
> > I'm not a fan of "while (true)". But I'm not the maintainer. The Logic
> > seems
> > fine otherwise.
>
> The way things stand today is a mashup of goto vs. break. I'll
> follow-up with fixed up patch next week if there is consensus on the
> change. If you have a suggestion for a better way, that's welcome as
> well.
>
I've applied this as is for v67, we can look at a refactoring for the
while (true) later.
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH 3/4] test/dax.sh: Validate huge page mappings
On Wed, 2019-10-23 at 19:33 +, Verma, Vishal L wrote: > > > @@ -91,4 +111,4 @@ json=$($NDCTL create-namespace -m raw -f -e $dev) > > eval $(json2var <<< "$json") > > [ $mode != "fsdax" ] && echo "fail: $LINENO" && exit 1 > > same comment about quoting "$mode". If 'mode' happens to be empty for > soem reason, we want to fail with the error message - instead the above > will fail with a syntax error. Sorry ignore this - that was a context line.. ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH 3/4] test/dax.sh: Validate huge page mappings
On Sat, 2019-10-19 at 09:39 -0700, Dan Williams wrote:
> Using trace-cmd to validate the expectations of the huge page faults
> generated by the dax-pmd.c test.
>
> Signed-off-by: Dan Williams
> ---
> test/dax.sh | 24 ++--
> 1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/test/dax.sh b/test/dax.sh
> index 59d5eafadae8..e5945fc3e335 100755
> --- a/test/dax.sh
> +++ b/test/dax.sh
> @@ -30,12 +30,32 @@ cleanup() {
>
> run_test() {
> rc=0
> - if ! ./dax-pmd $MNT/$FILE; then
> + if ! trace-cmd record -e fs_dax:dax_pmd_fault_done ./dax-pmd
> $MNT/$FILE; then
> rc=$?
> if [ $rc -ne 77 -a $rc -ne 0 ]; then
> cleanup $1
> fi
> fi
> +
> + # Fragile hack to double check the kernel services this test
> + # with successful pmd faults. If dax-pmd.c ever changes the
> + # number of times the dax_pmd_fault_done trace point fires the
> + # hack needs to be updated from 10 expected firings and the
> + # result of success (NOPAGE).
> + count=0
> + rc=1
> + for p in $(trace-cmd report | awk '{ print $21 }')
> + do
> + if [ $count -lt 10 ]; then
> + if [ $p != "0x100" -a $p != "NOPAGE" ]; then
> + cleanup $1
> + fi
> + fi
> + count=$((count + 1))
> + done
We shouldn't read lines with 'for' - if the command in $() happens to
spit out any glob metacharacters, we can get unexpected iterations.
It should instead be:
while read -r p; do
...
done < <(trace-cmd report | awk '{ print $21 }')
> + if [ $count -lt 10 ]; then
within [ ], any $variables should be quoted. Generally true for just
about all variables. If [[ ]] is used (bash-specific), then the quoting
may be omitted, but since it is a math comparison, the easiest might be:
if ((count < 10)); then
...
fi
> + cleanup $1
> + fi
> }
>
> set -e
> @@ -91,4 +111,4 @@ json=$($NDCTL create-namespace -m raw -f -e $dev)
> eval $(json2var <<< "$json")
> [ $mode != "fsdax" ] && echo "fail: $LINENO" && exit 1
same comment about quoting "$mode". If 'mode' happens to be empty for
soem reason, we want to fail with the error message - instead the above
will fail with a syntax error.
>
> -exit $rc
> +exit 0
> ___
> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH 10/10] daxctl: add --no-movable option for onlining memory
On Fri, 2019-10-18 at 13:58 -0700, Dan Williams wrote: > > > +++ b/Documentation/daxctl/movable-options.txt > > @@ -0,0 +1,10 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +-M:: > > +--no-movable:: > > If --movable is the default I would expect -M to be associated with > --movable. Don't we otherwise get the --no- handling for free > with boolean options? I otherwise don't think we need a short option > for the "no" case. Yep we get the inverse options for booleans for free, but we can define them either way - i.e. define it as --no-movable, and --movable is implied, or the other way round. But I agree it makes sense to remove the short option here. ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH 08/10] Documentation: clarify memory movablity for reconfigure-device
On Fri, 2019-10-18 at 13:46 -0700, Dan Williams wrote: > > > +'daxctl-reconfigure-device' nominally expects that it will online new > > memory > > +blocks as 'movable', so that kernel data doesn't make it into this memory. > > +However, there are other potential agents that may be configured to > > +automatically online new hot-plugged memory as it appears. Most notably, > > +these are the '/sys/devices/system/memory/auto_online_blocks' > > configuration, > > +or system udev rules. If such an agent races to online memory sections, > > daxctl > > +checks if the blocks were onlined as 'movable' memory. If this was not the > > +case, and the memory blocks are found to be in a different zone, then a > > +warning is displayed. If it is desired that a different agent control the > > +onlining of memory blocks, and the associated memory zone, then it is > > +recommended to use the --no-online option described below. This will > > abridge > > +the device reconfiguration operation to just hotplugging the memory, and > > +refrain from then onlining it. > > Oh here's that full description that calls out udev. > > I think just "See daxctl reconfigure-device --help" is sufficient for > those warnings in the previous patch. Yep I've truncated the runtime warning down to point here. ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH] ndctl: Use the same align value as original namespace on reconfigure
On Fri, 2019-10-18 at 15:55 +0530, Aneesh Kumar K.V wrote:
> Aneesh Kumar K.V writes:
>
> > "Verma, Vishal L" writes:
> >
> > > On Wed, 2019-08-07 at 10:14 +0530, Aneesh Kumar K.V wrote:
> > > > When using reconfigure command to add a `name` to the namespace we end
> > > > up updating the align attribute. Avoid this by using the value from
> > > > the original namespace. Do this only if we are keeping the namespace
> > > > mode
> > > > same.
> > > >
> > > > Signed-off-by: Aneesh Kumar K.V
> > > > ---
> > > > ndctl/namespace.c | 16
> > > > 1 file changed, 16 insertions(+)
> > >
> > > Hi Aneesh,
> > >
> > > A few comments below:
> > >
> > > > diff --git a/ndctl/namespace.c b/ndctl/namespace.c
> > > > index 1f212a2b3a9b..24e51bb35ae1 100644
> > > > --- a/ndctl/namespace.c
> > > > +++ b/ndctl/namespace.c
> > > > @@ -596,6 +596,22 @@ static int validate_namespace_options(struct
> > > > ndctl_region *region,
> > > > return -ENXIO;
> > > > }
> > > > } else {
> > > > +
> > > > + /*
> > > > +* If we are tryint to reconfigure with the same
> > > > namespace mode
> > >
> > > ^trying
> > >
> > > > +* Use the align details from the origin namespace.
> > > > Otherwise
> > >
> > > s/origin/original/
> > >
> > > > +* pick the align details from seed namespace
> > > > +*/
> > > > + if (ndns && p->mode == ndctl_namespace_get_mode(ndns)) {
> > >
> > > Do we need to depend on the mode here?
> > >
> > > I'm thinking it should be sufficient to do:
> > > 1. Check We're in 'reconfigure'
> > > 2. Check param.align was not supplied
> > > 3. Get alignment from the pfn/dax personality, and just use that.
> > >
> > > Does this make sense (Maybe I'm missing something).
> >
> > We want to use the align value from the seed when we are trying
> > to reconfigure a namespace with a different mode. ie, if we are moving a
> > fsdax namespace with align value 64K to a devdax, IMHO we should pick 16M
> > as alignment for devdax.
> >
> > > > + struct ndctl_pfn *ns_pfn =
> > > > ndctl_namespace_get_pfn(ndns);
> > > > + struct ndctl_dax *ns_dax =
> > > > ndctl_namespace_get_dax(ndns);
> > > > + if (ns_pfn)
> > > > + p->align = ndctl_pfn_get_align(ns_pfn);
> > > > + else if (ns_dax)
> > > > + p->align = ndctl_dax_get_align(ns_dax);
> > > > + else
> > > > + p->align = sysconf(_SC_PAGE_SIZE);
> > >
> > > Do we need the page size fallback here - there are other checks after
> > > this point that also do a similar fallback, do they not catch the
> > > default case?
> >
> > I did that to simplify the code with that `else if`
> >
> > > > + } else
> > > > /*
> > > > * Use the seed namespace alignment as the default if
> > > > we need
> > > > * one. If we don't then use PAGE_SIZE so the size_align
>
> Any update on this.?
Yes - I've applied it to the pending branch, and it will be in v67.
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [ndctl PATCH] libdaxctl: fix memory leaks with daxctl_memory objects
On Tue, 2019-10-01 at 16:19 -0700, Ira Weiny wrote:
>
> > diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
> > index 8abfd64..639224c 100644
> > --- a/daxctl/lib/libdaxctl.c
> > +++ b/daxctl/lib/libdaxctl.c
> > @@ -204,8 +204,9 @@ DAXCTL_EXPORT void daxctl_region_get_uuid(struct
> > daxctl_region *region, uuid_t u
> >
> > static void free_mem(struct daxctl_dev *dev)
> > {
> > - if (dev && dev->mem) {
> > + if (dev->mem) {
>
> There is a comment in daxctl_dev_disable() which says:
>
> /* If there is a memory object, first free that */
>
> I'm 100% sure that dev can't be NULL there. So that comment no longer
> applies.
>
> May want to remove that comment.
>
Hm, I'm not sure I follow.
Yes, dev can't be null there, but 'mem' can me. Hence the comment saying
if /mem/ is present, free it.
The check in free_mem() only checks for non-NULL mem too.
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH] bnvdimm/namsepace: Don't set claim_class on error
On Wed, 2019-09-25 at 11:10 -0700, ira.we...@intel.com wrote:
> From: Ira Weiny
>
> Don't leave claim_class set to an invalid value if an error occurs in
> btt_claim_class().
>
> While we are here change the return type of __holder_class_store() to be
> clear about the values it is returning.
>
> This was found via code inspection.
>
> Signed-off-by: Ira Weiny
> ---
> drivers/nvdimm/namespace_devs.c | 19 +--
> 1 file changed, 9 insertions(+), 10 deletions(-)
One minor nit below, but otherwise it looks good to me:
Reviewed-by: Vishal Verma
>
> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
> index 5b22cecefc99..a020c166e1e7 100644
> --- a/drivers/nvdimm/namespace_devs.c
> +++ b/drivers/nvdimm/namespace_devs.c
> @@ -1510,16 +1510,19 @@ static ssize_t holder_show(struct device *dev,
> }
> static DEVICE_ATTR_RO(holder);
>
> -static ssize_t __holder_class_store(struct device *dev, const char *buf)
> +static int __holder_class_store(struct device *dev, const char *buf)
> {
> struct nd_namespace_common *ndns = to_ndns(dev);
>
> if (dev->driver || ndns->claim)
> return -EBUSY;
>
> - if (sysfs_streq(buf, "btt"))
> - ndns->claim_class = btt_claim_class(dev);
> - else if (sysfs_streq(buf, "pfn"))
> + if (sysfs_streq(buf, "btt")) {
> + int rc = btt_claim_class(dev);
Need a blank line here separating variable declarations and code.
> + if (rc < NVDIMM_CCLASS_NONE)
> + return rc;
> + ndns->claim_class = rc;
> + } else if (sysfs_streq(buf, "pfn"))
> ndns->claim_class = NVDIMM_CCLASS_PFN;
> else if (sysfs_streq(buf, "dax"))
> ndns->claim_class = NVDIMM_CCLASS_DAX;
> @@ -1528,10 +1531,6 @@ static ssize_t __holder_class_store(struct device
> *dev, const char *buf)
> else
> return -EINVAL;
>
> - /* btt_claim_class() could've returned an error */
> - if ((int)ndns->claim_class < 0)
> - return ndns->claim_class;
> -
> return 0;
> }
>
> @@ -1539,7 +1538,7 @@ static ssize_t holder_class_store(struct device *dev,
> struct device_attribute *attr, const char *buf, size_t len)
> {
> struct nd_region *nd_region = to_nd_region(dev->parent);
> - ssize_t rc;
> + int rc;
>
> nd_device_lock(dev);
> nvdimm_bus_lock(dev);
> @@ -1547,7 +1546,7 @@ static ssize_t holder_class_store(struct device *dev,
> rc = __holder_class_store(dev, buf);
> if (rc >= 0)
> rc = nd_namespace_label_update(nd_region, dev);
> - dev_dbg(dev, "%s(%zd)\n", rc < 0 ? "fail " : "", rc);
> + dev_dbg(dev, "%s(%d)\n", rc < 0 ? "fail " : "", rc);
> nvdimm_bus_unlock(dev);
> nd_device_unlock(dev);
>
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] libnvdimm/namespace: Fix a signedness bug in __holder_class_store()
On Wed, 2019-09-25 at 11:25 -0600, Weiny, Ira wrote:
> > On Wed, 2019-09-25 at 14:00 +0300, Dan Carpenter wrote:
> > > The "ndns->claim_class" variable is an enum but in this case GCC will
> > > treat it as an unsigned int so the error handling is never triggered.
> > >
> > > Fixes: 14e494542636 ("libnvdimm, btt: BTT updates for UEFI 2.7
> > > format")
> > > Signed-off-by: Dan Carpenter
> > > ---
> > > drivers/nvdimm/namespace_devs.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/nvdimm/namespace_devs.c
> > > b/drivers/nvdimm/namespace_devs.c index cca0a3ba1d2c..669985527716
> > > 100644
> > > --- a/drivers/nvdimm/namespace_devs.c
> > > +++ b/drivers/nvdimm/namespace_devs.c
> > > @@ -1529,7 +1529,7 @@ static ssize_t __holder_class_store(struct device
> > *dev, const char *buf)
> > > return -EINVAL;
> > >
> > > /* btt_claim_class() could've returned an error */
> > > - if (ndns->claim_class < 0)
> > > + if ((int)ndns->claim_class < 0)
> > > return ndns->claim_class;
> > >
> > > return 0;
> >
> > Looks straightforward, and a good catch.
> > Reviewed-by: Vishal Verma
>
> I'm not sure this is really a good fix. This leaves ndns->claim_class set to
> an invalid value. Is that ok?
>
Hm, it is just in a store operation for the holder_class sysfs
attribute. if claim_class was negative, that store will just fail.
>From the userspace side, this means the 'mode' enforcement API will
fail. Typically, 'ndctl' doesn't require the enforcement to succeed,
since we can disambiguate namespaces even without it, so it doesn't
block namespace creation etc.
On the kernel side, claim_class gets used by btt_devs.c during probe,
and looks like this case, as it currently stands, would fail the BTT
probe (nd_btt_probe()). I think this is what we want?
I guess it can be made a bit clearer by storing NVDIMM_CCLASS_UNKNOWN
explicitly in holder_class_store(), but that can be a separate
improvement from what this patch is addressing.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] libnvdimm/namespace: Fix a signedness bug in __holder_class_store()
On Wed, 2019-09-25 at 14:00 +0300, Dan Carpenter wrote:
> The "ndns->claim_class" variable is an enum but in this case GCC will
> treat it as an unsigned int so the error handling is never triggered.
>
> Fixes: 14e494542636 ("libnvdimm, btt: BTT updates for UEFI 2.7 format")
> Signed-off-by: Dan Carpenter
> ---
> drivers/nvdimm/namespace_devs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
> index cca0a3ba1d2c..669985527716 100644
> --- a/drivers/nvdimm/namespace_devs.c
> +++ b/drivers/nvdimm/namespace_devs.c
> @@ -1529,7 +1529,7 @@ static ssize_t __holder_class_store(struct device *dev,
> const char *buf)
> return -EINVAL;
>
> /* btt_claim_class() could've returned an error */
> - if (ndns->claim_class < 0)
> + if ((int)ndns->claim_class < 0)
> return ndns->claim_class;
>
> return 0;
Looks straightforward, and a good catch.
Reviewed-by: Vishal Verma
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 11/13] nvdimm: Use more common logic testing styles and bare ; positions
On Wed, 2019-09-11 at 19:54 -0700, Joe Perches wrote:
> Avoid using uncommon logic testing styles to make the code a
> bit more like other kernel code.
>
> e.g.:
> if (foo) {
> ;
> } else {
>
> }
>
> is typically written
>
> if (!foo) {
>
> }
>
A lot of times the excessive inversions seem to result in a net loss of
readability - e.g.:
> diff --git a/drivers/nvdimm/region_devs.c
> b/drivers/nvdimm/region_devs.c
> index 65df07481909..6861e0997d21 100644
> --- a/drivers/nvdimm/region_devs.c
> +++ b/drivers/nvdimm/region_devs.c
> @@ -320,9 +320,7 @@ static ssize_t set_cookie_show(struct device *dev,
> struct nd_interleave_set *nd_set = nd_region->nd_set;
> ssize_t rc = 0;
>
> - if (is_memory(dev) && nd_set)
> - /* pass, should be precluded by region_visible */;
For one, the comment is lost
> - else
> + if (!(is_memory(dev) && nd_set))
And it takes a moment to resolve between things such as:
if (!(A && B))
vs.
if (!(A) && B)
And this is especially true if 'A' and 'B' are longer function calls,
split over multiple lines, or are themselves compound 'sections'.
I'm not opposed to /all/ such transformations -- for example, the ones
where the logical inversion can be 'consumed' by toggling a comparision
operator, as you have a few times in this patch, don't sacrifice any
readibility, and perhaps even improve it.
> return -ENXIO;
>
> /*
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [Ksummit-discuss] [PATCH v2 2/3] Maintainer Handbook: Maintainer Entry Profile
On Wed, 2019-09-11 at 08:48 -0700, Dan Williams wrote: > diff --git a/MAINTAINERS b/MAINTAINERS > index 3f171339df53..e5d111a86e61 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -98,6 +98,10 @@ Descriptions of section entries: > Obsolete:Old code. Something tagged obsolete generally means > it has been replaced by a better system and you > should be using that. > + P: Subsystem Profile document for more details submitting > +patches to the given subsystem. This is either an in-tree file, > +or a URI. See Documentation/maintainer/maintainer-entry-profile.rst > +for details. Considering each maintainer entry or driver may not have a subdirectory under Documentation/ to put these under, would it be better to collect them in one place, say Documentation/maintainer-entry-profiles/ ? ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [ndctl PATCH 2/2] libdaxctl: fix device reconfiguration with builtin drivers
On Wed, 2019-09-04 at 14:01 -0700, Dan Williams wrote: > On Wed, Sep 4, 2019 at 1:27 PM Verma, Vishal L > wrote: > > On Tue, 2019-09-03 at 19:20 -0700, Dan Williams wrote: > > > > > > Hmm, why wait until now to check if this list is NULL. How about fall > > > back to kmod_module_new_from_name() at to_module_list() time? That > > > would seem to simplify this follow up routine to not need to worry > > > about working around a NULL list. > > > > So we moved the list checking to later in the process around v4 of the > > original series, so that we don't unnecessarily fail add_dax_dev() if > > for some reason a list wasn't created. > > Ah true, I forgot that wrinkle, however... > > > Also, we use mod_name = dax_modules[mode] during an 'enable' to > > determine the module name to use for the fallback - we wouldn't have > > this at add_dax_dev() time. > > Since modalias is already not reliable it seems the implementation > should go ahead never do module lookups and just do everything based > on module names. > > In other words the libndctl panacea of not needing to hard code module > names is already lost in libdaxctl land. If the code drops modalias > usage does that clean up some of these flows? Yep I think so - we use modalias to construct a lookup list, but we still have to use the name to resolve to the final module based on the mode. I think we can remove the list lookup and replace it with simply: kmod_module_new_from_name(ctx->kmod_ctx, mod_name, ); It would clean up the module related flows, but is there any disadvantage to doing this? ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [ndctl PATCH 2/2] libdaxctl: fix device reconfiguration with builtin drivers
On Tue, 2019-09-03 at 19:20 -0700, Dan Williams wrote:
>
> > +static int try_kmod_builtin(struct daxctl_dev *dev, const char *mod_name)
> > +{
> > + const char *devname = daxctl_dev_get_devname(dev);
> > + struct daxctl_ctx *ctx = daxctl_dev_get_ctx(dev);
> > + struct kmod_module *kmod;
> > + int rc = -ENXIO;
> > +
> > + rc = kmod_module_new_from_name(ctx->kmod_ctx, mod_name, );
> > + if (rc < 0) {
> > + err(ctx, "%s: failed getting module for: %s: %s\n",
> > + devname, mod_name, strerror(-rc));
> > + return rc;
> > + }
> > +
> > + if (kmod_module_get_initstate(kmod) != KMOD_MODULE_BUILTIN)
> > + return -ENXIO;
> > +
> > + dbg(ctx, "%s inserting module: %s\n", devname,
> > + kmod_module_get_name(kmod));
> > + rc = kmod_module_probe_insert_module(kmod,
> > + KMOD_PROBE_APPLY_BLACKLIST,
> > + NULL, NULL, NULL, NULL);
> > + if (rc < 0) {
> > + err(ctx, "%s: insert failure: %d\n", devname, rc);
> > + return rc;
> > + }
> > + dev->module = kmod;
> > +
> > + return 0;
> > +}
> > +
> > static int daxctl_insert_kmod_for_mode(struct daxctl_dev *dev,
> > const char *mod_name)
> > {
> > @@ -877,6 +908,8 @@ static int daxctl_insert_kmod_for_mode(struct
> > daxctl_dev *dev,
> > int rc = -ENXIO;
> >
> > if (dev->kmod_list == NULL) {
>
> Hmm, why wait until now to check if this list is NULL. How about fall
> back to kmod_module_new_from_name() at to_module_list() time? That
> would seem to simplify this follow up routine to not need to worry
> about working around a NULL list.
So we moved the list checking to later in the process around v4 of the
original series, so that we don't unnecessarily fail add_dax_dev() if
for some reason a list wasn't created.
Also, we use mod_name = dax_modules[mode] during an 'enable' to
determine the module name to use for the fallback - we wouldn't have
this at add_dax_dev() time.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: daxctl fails to reconfigure to system-ram when DAX modules built-in?
On Tue, 2019-09-03 at 23:20 +0200, Brice Goglin wrote: > Hello > > It looks like daxctl fails to reconfigure to system-ram when > DAX modules are built-in the kernel: > > $ daxctl reconfigure-device --mode=system-ram dax1.0 -v > libdaxctl: daxctl_insert_kmod_for_mode: dax1.0: a modalias lookup list > was not created > error reconfiguring devices: No such device or address > reconfigured 0 devices > > It looks like things were failing in kmod_module_new_from_lookup() Hi Brice, Thanks for the report - Dave Hansen also hit this recently, and I'll take a look at kmod builtin detection. I've created a github issue for this: https://github.com/pmem/ndctl/issues/108 -Vishal ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [ndctl PATCH 3/3] ndctl/namespace: add a --continue option to create namespaces greedily
On Thu, 2019-08-29 at 10:38 -0700, jane@oracle.com wrote: > Hi, Vishal, > > > On 8/28/19 5:17 PM, Vishal Verma wrote: > > Add a --continue option to ndctl-create-namespaces to allow the creation > > of as many namespaces as possible, that meet the given filter > > restrictions. > > > > The creation loop will be aborted if a failure is encountered at any > > point. > > Just wondering what is the motivation behind providing this option? Hi Jane, See Steve's email here: https://lists.01.org/pipermail/linux-nvdimm/2019-August/023390.html Essentially it lets sysadmins create a simple, maximal configuration without everyone having to script it. ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [ndctl PATCH 3/3] ndctl/namespace: add a --continue option to create namespaces greedily
On Wed, 2019-08-28 at 19:34 -0700, Dan Williams wrote: > On Wed, Aug 28, 2019 at 5:17 PM Vishal Verma wrote: > > Add a --continue option to ndctl-create-namespaces to allow the creation > > of as many namespaces as possible, that meet the given filter > > restrictions. > > > > The creation loop will be aborted if a failure is encountered at any > > point. > > > > Link: https://github.com/pmem/ndctl/issues/106 > > Reported-by: Steve Scargal > > Cc: Jeff Moyer > > Cc: Dan Williams > > Signed-off-by: Vishal Verma > > --- > > .../ndctl/ndctl-create-namespace.txt | 7 ++ > > ndctl/namespace.c | 25 +++ > > 2 files changed, 27 insertions(+), 5 deletions(-) > > > > diff --git a/Documentation/ndctl/ndctl-create-namespace.txt > > b/Documentation/ndctl/ndctl-create-namespace.txt > > index c9ae27c..55a8581 100644 > > --- a/Documentation/ndctl/ndctl-create-namespace.txt > > +++ b/Documentation/ndctl/ndctl-create-namespace.txt > > @@ -215,6 +215,13 @@ include::xable-region-options.txt[] > > --bus=:: > > include::xable-bus-options.txt[] > > > > +-c:: > > +--continue:: > > + Do not stop after creating one namespace. Instead, greedily create > > as > > I like the "greedy" terminology here because it makes the option seem > a bit off-putting. "Do you really want to be greedy?" Ha, I just borrowed it from regular expressions :) > > > + many namespaces as possible within the given --bus and --region > > filter > > + restrictions. This will abort if any creation attempt results in an > > + error. > > Hmm, should "--continue --force" override that policy? Yep that's a good idea, with a big note in the man page that errors could be lost/meaningless in that case. > > Otherwise this looks good to me. Thanks, I'll send a new version with the above. ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [ndctl RFC PATCH] ndctl/namespace: create namespaces greedily
On Wed, 2019-08-28 at 15:38 -0700, Scargall, Steve wrote: > Thanks for the clarification. I have a much better understanding > now. > > Updating the ndctl-create-namespace man page to clarify what ' > --region=all' does and does not do in combination with other arguments > and options would be beneficial. This would actually provide a more > immediate solution to the problem. It sounds like the general > statement used in the other man pages can remain? Or are there other > exceptions? Yep I have a patch clarifying the --region option for create-namespace. 'all' is used in two ways, --=all or ndctl- all. The former acts as a filter, where as the latter acts as a "act on all" directive. So 'ndctl disable-namespace --region=region0 all' disables all namespaces on region0, where as 'ndctl disable-namespace --region=all all' is the same as omitting the --region option (while still acceptable syntax), and acts on all namespaces regardless of the region. > > With the proposed implementation that bails out on first error, ` > --continue` seems reasonable vs. `--greedy`. What do you think about > `--all-regions` instead? It is more meaningful towards its intended > action or use-case, but would it cause confusion with ` > --regions=all`? Documentation could provide the solution here. We > would have to choose an arbitrary short option since `-a` and `-r` are > already taken. I prefer --continue/-c since it automatically extends to buses as well. As a result, with these changes, 'ndctl create-namespace -c' will create namespaces on all possible regions on all possible buses. > > Whatever the final decision, we should test for and return a usage > error for a mutually exclusive set of inputs, i.e. `ndctl create- > namespace --all-regions --region=region0` doesn't make sense - either > you want to perform the create operation on all regions or only in > region0. With --continue this isn't a big problem as the --region restriction will render the --continue meaningless. ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [ndctl RFC PATCH] ndctl/namespace: create namespaces greedily
On Wed, 2019-08-28 at 21:16 +, Verma, Vishal L wrote:
> On Wed, 2019-08-28 at 13:47 -0700, Scargall, Steve wrote:
> > Hi Jeff,
> >
> > The issue is more of repetition. On an 8-socket system, should a
> > user really be expected to type 'ndctl create-namespace' eight times
> > vs. running 'ndctl create-namespace --region=all' once? SAP HANA is
> > an example app the requires one namespace per region. Scripting is a
> > viable solution, but that requires somebody to write the script and do
> > all the error checking & handling. Each OEM/ISV/SysAdmin would have
> > their own script. Pushing the logic to ndctl seems to be a reasonable
> > approach and let the user handle any errors returned by ndctl.
>
> A scripted solution can indeed be really simple - e.g.:
>
> # while read -r region; do ndctl create-namespace --region="$region";
> done < <(ndctl list --bus=nfit_test.0 -R | jq -r '.[].dev')
>
> {
> "dev":"namespace5.0",
> "mode":"fsdax",
> "map":"dev",
> "size":"62.00 MiB (65.01 MB)",
> "uuid":"c8014457-c268-4f22-8eae-6386fbf08ceb",
> "sector_size":512,
> "align":2097152,
> "blockdev":"pmem5"
> }
> {
> "dev":"namespace4.0",
> "mode":"fsdax",
> "map":"dev",
> "size":"30.00 MiB (31.46 MB)",
> "uuid":"f9498ef6-cdd6-46c7-95f1-86469546ecb9",
> "sector_size":512,
> "align":2097152,
> "blockdev":"pmem4"
> }
>
> > The ndctl-man-page implies the 'ndctl create-namespace --region=all'
> > feature exists today:
> >
> >-r, --region=
> >
> >A regionX device name, or a region id number. The keyword
> > all can be specified to carry out the operation on every region in the
> > system, optionally filtered by bus id (see --bus= option).
> >
>
> This is true, but unfortunately, the implementation has treated create-
> namespace as an exception to this since the start, and I agree with Jeff
> that changing its behavior now can cause other Hyrum's law-esqe [1]
> breakage.
>
> I think however it should be easy to make a compromise, and retain the
> legacy behavior of create-namespace, while creating a new create-
> namespace-greedy command with the new semantics.
>
.. And it doesn't even need to be a new command, a simple --greedy
option to create-namespace should be sufficient.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [ndctl RFC PATCH] ndctl/namespace: create namespaces greedily
On Wed, 2019-08-28 at 13:47 -0700, Scargall, Steve wrote:
> Hi Jeff,
>
> The issue is more of repetition. On an 8-socket system, should a
> user really be expected to type 'ndctl create-namespace' eight times
> vs. running 'ndctl create-namespace --region=all' once? SAP HANA is
> an example app the requires one namespace per region. Scripting is a
> viable solution, but that requires somebody to write the script and do
> all the error checking & handling. Each OEM/ISV/SysAdmin would have
> their own script. Pushing the logic to ndctl seems to be a reasonable
> approach and let the user handle any errors returned by ndctl.
A scripted solution can indeed be really simple - e.g.:
# while read -r region; do ndctl create-namespace --region="$region";
done < <(ndctl list --bus=nfit_test.0 -R | jq -r '.[].dev')
{
"dev":"namespace5.0",
"mode":"fsdax",
"map":"dev",
"size":"62.00 MiB (65.01 MB)",
"uuid":"c8014457-c268-4f22-8eae-6386fbf08ceb",
"sector_size":512,
"align":2097152,
"blockdev":"pmem5"
}
{
"dev":"namespace4.0",
"mode":"fsdax",
"map":"dev",
"size":"30.00 MiB (31.46 MB)",
"uuid":"f9498ef6-cdd6-46c7-95f1-86469546ecb9",
"sector_size":512,
"align":2097152,
"blockdev":"pmem4"
}
>
> The ndctl-man-page implies the 'ndctl create-namespace --region=all'
> feature exists today:
>
>-r, --region=
>
>A regionX device name, or a region id number. The keyword
> all can be specified to carry out the operation on every region in the
> system, optionally filtered by bus id (see --bus= option).
>
This is true, but unfortunately, the implementation has treated create-
namespace as an exception to this since the start, and I agree with Jeff
that changing its behavior now can cause other Hyrum's law-esqe [1]
breakage.
I think however it should be easy to make a compromise, and retain the
legacy behavior of create-namespace, while creating a new create-
namespace-greedy command with the new semantics.
Thoughts on that?
Thanks,
-Vishal
[1]: http://www.hyrumslaw.com/
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] libnvdimm, region: Use struct_size() in kzalloc()
On Wed, 2019-08-28 at 14:36 -0500, Gustavo A. R. Silva wrote:
> struct_size() does not apply to those scenarios. See below...
>
> > [1]:
> > https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/tree/drivers/nvdimm/region_devs.c#n1030
>
> struct_size() only applies to structures of the following kind:
>
> struct foo {
>int stuff;
>struct boo entry[];
> };
>
> and this scenario includes two different structures:
>
> struct nd_region {
> ...
> struct nd_mapping mapping[0];
> };
>
> struct nd_blk_region {
> ...
> struct nd_region nd_region;
> };
Yep - I neglected to actually look at the structures involved - you're
right, it doesn't apply here.
>
> > [2]:
> > https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/tree/drivers/nvdimm/region_devs.c#n96
> >
>
> In this scenario struct_size() does not apply directly because of the
> following
> logic before the call to devm_kzalloc():
Agreed, I missed that the calculation was more involved here.
Thanks for the clarifications, you can add:
Reviewed-by: Vishal Verma
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] libnvdimm, region: Use struct_size() in kzalloc()
On Mon, 2019-06-10 at 16:06 -0500, Gustavo A. R. Silva wrote:
> One of the more common cases of allocation size calculations is
> finding
> the size of a structure that has a zero-sized array at the end, along
> with memory for some number of elements for that array. For example:
>
> struct nd_region {
> ...
> struct nd_mapping mapping[0];
> };
>
> instance = kzalloc(sizeof(struct nd_region) + sizeof(struct
> nd_mapping) *
> count, GFP_KERNEL);
>
> Instead of leaving these open-coded and prone to type mistakes, we can
> now use the new struct_size() helper:
>
> instance = kzalloc(struct_size(instance, mapping, count), GFP_KERNEL);
>
> This code was detected with the help of Coccinelle.
>
> Signed-off-by: Gustavo A. R. Silva
> ---
> drivers/nvdimm/region_devs.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/nvdimm/region_devs.c
> b/drivers/nvdimm/region_devs.c
> index b4ef7d9ff22e..88becc87e234 100644
> --- a/drivers/nvdimm/region_devs.c
> +++ b/drivers/nvdimm/region_devs.c
> @@ -1027,10 +1027,9 @@ static struct nd_region
> *nd_region_create(struct nvdimm_bus *nvdimm_bus,
> }
> region_buf = ndbr;
> } else {
> - nd_region = kzalloc(sizeof(struct nd_region)
> - + sizeof(struct nd_mapping)
> - * ndr_desc->num_mappings,
> - GFP_KERNEL);
> + nd_region = kzalloc(struct_size(nd_region, mapping,
> + ndr_desc->num_mappings),
> + GFP_KERNEL);
> region_buf = nd_region;
> }
>
Hi Gustavo,
The patch looks good to me, however it looks like it might've missed
some instances where this replacement can be performed?
One is just a few lines below from the above, in the 'else' block[1].
Additionally, maybe the Coccinelle script can be augmented to catch
devm_kzalloc instances as well - there is one of those in this file[2].
Thanks,
-Vishal
[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/tree/drivers/nvdimm/region_devs.c#n1030
[2]:
https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/tree/drivers/nvdimm/region_devs.c#n96
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [ndctl PATCH] ndctl/namespace: Fix 'clear-error -s' excessive scrubbing
On Tue, 2019-08-27 at 11:58 -0700, Dan Williams wrote:
> Erwin reports:
> The current implementation of ndctl clear-errors takes a very long time,
> because a full scrub happens for every namespace.
>
> Doing a full system ARS scrub is obviously not necessary, it just needs
> to happen once.
>
> Indeed, it only needs to happen once per bus. Clear the 'scrub' option
> after one namespace_clear_bb() invocation, and reset it when looping to
> the next bus.
>
> Link: https://github.com/pmem/ndctl/issues/104
> Reported-by: Erwin Tsaur
> Fixes: 2ba7b8277075 ("ndctl: add a 'clear-errors' command")
> Signed-off-by: Dan Williams
> ---
> ndctl/namespace.c | 18 +-
> 1 file changed, 13 insertions(+), 5 deletions(-)
Looks good, applied.
>
> diff --git a/ndctl/namespace.c b/ndctl/namespace.c
> index bbc9107c6baa..c1af7d0db153 100644
> --- a/ndctl/namespace.c
> +++ b/ndctl/namespace.c
> @@ -1295,7 +1295,7 @@ static int raw_clear_badblocks(struct ndctl_namespace
> *ndns)
> return nstype_clear_badblocks(ndns, devname, begin, size);
> }
>
> -static int namespace_wait_scrub(struct ndctl_namespace *ndns)
> +static int namespace_wait_scrub(struct ndctl_namespace *ndns, bool do_scrub)
> {
> const char *devname = ndctl_namespace_get_devname(ndns);
> struct ndctl_bus *bus = ndctl_namespace_get_bus(ndns);
> @@ -1309,7 +1309,7 @@ static int namespace_wait_scrub(struct ndctl_namespace
> *ndns)
> }
>
> /* start a scrub if asked and if one isn't in progress */
> - if (scrub && (!in_progress)) {
> + if (do_scrub && (!in_progress)) {
> rc = ndctl_bus_start_scrub(bus);
> if (rc) {
> error("%s: Unable to start scrub: %s\n", devname,
> @@ -1332,7 +1332,7 @@ static int namespace_wait_scrub(struct ndctl_namespace
> *ndns)
> return 0;
> }
>
> -static int namespace_clear_bb(struct ndctl_namespace *ndns)
> +static int namespace_clear_bb(struct ndctl_namespace *ndns, bool do_scrub)
> {
> struct ndctl_pfn *pfn = ndctl_namespace_get_pfn(ndns);
> struct ndctl_dax *dax = ndctl_namespace_get_dax(ndns);
> @@ -1347,7 +1347,7 @@ static int namespace_clear_bb(struct ndctl_namespace
> *ndns)
> return 1;
> }
>
> - rc = namespace_wait_scrub(ndns);
> + rc = namespace_wait_scrub(ndns, do_scrub);
> if (rc)
> return rc;
>
> @@ -1716,9 +1716,14 @@ static int do_xaction_namespace(const char *namespace,
> ndctl_set_log_priority(ctx, LOG_DEBUG);
>
> ndctl_bus_foreach(ctx, bus) {
> + bool do_scrub;
> +
> if (!util_bus_filter(bus, param.bus))
> continue;
>
> + /* only request scrubbing once per bus */
> + do_scrub = scrub;
> +
> ndctl_region_foreach(bus, region) {
> if (!util_region_filter(region, param.region))
> continue;
> @@ -1778,7 +1783,10 @@ static int do_xaction_namespace(const char *namespace,
> (*processed)++;
> break;
> case ACTION_CLEAR:
> - rc = namespace_clear_bb(ndns);
> + rc = namespace_clear_bb(ndns, do_scrub);
> +
> + /* one scrub per bus is sufficient */
> + do_scrub = false;
> if (rc == 0)
> (*processed)++;
> break;
>
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
