Re: [PATCH v3 1/3] resource: Use list_head to link resource sibling

[Baoquan He]

Hi Rob,

Thanks a lot for looking into this and involve Nico to this thread!

On 04/09/18 at 09:49am, Rob Herring wrote:
> +Nico who has been working on tinification of the kernel.
> 
> On Mon, Apr 9, 2018 at 4:08 AM, Baoquan He  wrote:
> > The struct resource uses singly linked list to link siblings. It's not
> > easy to do reverse iteration on sibling list. So replace it with list_head.
> 
> Why is reverse iteration needed?

This is the explanation I made when Andrew helped to review the v1 post:
https://lkml.org/lkml/2018/3/23/78

Because we have been using kexec-tools utility to search available
System RAM space for loading kernel/initrd/purgatory from top to down.
That is done in user space by searching /proc/iomem. While later added
kexec_file interface, the searching code happened in kernel, and it
only search System RAM region bottom up, then take an area in that found
RAM region from top to down. We need unify these two interfaces on
behaviour since they are the same on essense from the users' point of
view, though implementation is different. As you know, the singly linked
list implementation of the current resource's sibling linking, makes the
searching from top to down very hard to satisfy people. 

Below is the v1 post, we make an temporary array to copy iomem_resource's
first level of children, then iterate the array reversedly. Andrew
suggested me to try list_head after reviewing. In fact we can optimize
that patch to only copy resource pointer into array, still the way is
ugly.
https://lkml.org/lkml/2018/3/21/952

Then Wei pasted a patch he had made as below. He didn't mention if he
also has requirement on reversed iteration of resource. That is an O(n*n)
way, from personal feelings, hard to say if it's bettern than v1 post.
https://lkml.org/lkml/2018/3/24/157

That's why I would like to have a try of the list_head linking.

> 
> > And code refactoring makes codes in kernel/resource.c more readable than
> > pointer operation.
> 
> resource_for_each_* helpers could solve that without the size increase.

Nico mentioned llist, that is a linux kernel standard singly linked
list, very elegant code existed. Still can not satisfy the reversed
iteration requirement.

> 
> > Besides, type of member variables of struct resource, sibling and child, are
> > changed from 'struct resource *' to 'struct list_head'. Kernel size will
> > increase because of those statically defined struct resource instances.
> 
> The DT struct device_node also has the same tree structure with
> parent, child, sibling pointers and converting to list_head had been
> on the todo list for a while. ACPI also has some tree walking
> functions (drivers/acpi/acpica/pstree.c). Perhaps there should be a
> common tree struct and helpers defined either on top of list_head or a
> new struct if that saves some size.

We have had many this kind of trees in kernel, the obvious examples is 
task_struct. And struct task_group {} too. They have used list_head
already.
struct task_struct {
...
/* Real parent process: */  
   
struct task_struct __rcu*real_parent;   
   

   
/* Recipient of SIGCHLD, wait4() reports: */
   
struct task_struct __rcu*parent;
struct list_headchildren;   
   
struct list_headsibling;
...
}

root
///   |   \\\
   ///|\\\
  /// | \\\
 ///  |  \\\
  (child)<->(child)<->(child)
 ///   |   \\\
 ///   |  \\\
   /// |  \\\
///| \\\
 (grandchild)<->(grandchild)<->(grandchild)

If define an new struct, , like tree_list, or tlist?

struct tlist {
void*parent;
struct list_headsibling;
 
struct list_headchild;
}

Just a rough idea, feel it might not bring too much benefit compared
with direct list operation.

> 
> >
> > Signed-off-by: Baoquan He 
> > ---
> > v2->v3:
> >   Make sibling() and first_child() global so that they can be called
> >   out of kernel/resource.c to simplify code.
> 
> These should probably be inline functions. Or exported if not.
> 
> >
> >   Fix several code bugs found by kbuild test robot.
> >
> >   Got report from lkp that kernel size increased. It's on purpose since
> >   the type change of sibling and 

Re: [PATCH] ndctl: fix libdaxctl memory leak

[Dan Williams]

On Tue, Apr 10, 2018 at 9:56 AM, Dave Jiang  wrote:
> When daxctl_unref is releasing the context, we should make sure that the
> regions and devices are also being released.
>
> Signed-off-by: Dave Jiang 
> ---
>  daxctl/lib/libdaxctl.c |7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
> index 9e503201..0552f6d7 100644
> --- a/daxctl/lib/libdaxctl.c
> +++ b/daxctl/lib/libdaxctl.c
> @@ -119,11 +119,18 @@ DAXCTL_EXPORT struct daxctl_ctx *daxctl_ref(struct 
> daxctl_ctx *ctx)
>   */
>  DAXCTL_EXPORT void daxctl_unref(struct daxctl_ctx *ctx)
>  {
> +   struct daxctl_region *region;
> +
> if (ctx == NULL)
> return;
> ctx->refcount--;
> if (ctx->refcount > 0)
> return;
> +
> +   while ((region = list_top(>regions, struct daxctl_region, list)) 
> !=
> +   NULL)
> +   daxctl_region_unref(region);
> +

Sorry, should have mentioned this before. Why not use
list_for_each_entry_safe() to iterate and delete regions like all the
other free routines?
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v2] ndctl: fix libdaxctl memory leak

[Dave Jiang]

When daxctl_unref is releasing the context, we should make sure that the
regions and devices are also being released. free_region() will free
all the devices under the region.

Signed-off-by: Dave Jiang 
---

v2: Use list_for_each_safe() for region removal. (Dan)

 daxctl/lib/libdaxctl.c |8 
 1 file changed, 8 insertions(+)

diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
index 9e503201..22f4210a 100644
--- a/daxctl/lib/libdaxctl.c
+++ b/daxctl/lib/libdaxctl.c
@@ -29,6 +29,8 @@
 
 static const char *attrs = "dax_region";
 
+static void free_region(struct daxctl_region *region, struct list_head *head);
+
 /**
  * struct daxctl_ctx - library user context to find "nd" instances
  *
@@ -119,11 +121,17 @@ DAXCTL_EXPORT struct daxctl_ctx *daxctl_ref(struct 
daxctl_ctx *ctx)
  */
 DAXCTL_EXPORT void daxctl_unref(struct daxctl_ctx *ctx)
 {
+   struct daxctl_region *region, *_r;
+
if (ctx == NULL)
return;
ctx->refcount--;
if (ctx->refcount > 0)
return;
+
+   list_for_each_safe(>regions, region, _r, list)
+   free_region(region, >regions);
+
info(ctx, "context %p released\n", ctx);
free(ctx);
 }

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] ndctl: fix daxctl list memory leak

[Dave Jiang]

On 04/09/2018 07:39 PM, Dan Williams wrote:
> On Mon, Apr 9, 2018 at 5:13 PM, Dave Jiang  wrote:
>> daxctl list is not calling daxctl_unref() when executed succesfully. At the 
>> same
>> time, daxctl_region_unref() is not being called when daxctl_unref() executes.
>> Valgrind is reporting unfreed memory. Adding the appropriate calls to make 
>> sure
>> all memory allocated are freed for daxctl list.
>>
>> Signed-off-by: Dave Jiang 
>> ---
>>  daxctl/lib/libdaxctl.c |7 +++
>>  daxctl/list.c  |1 +
>>  2 files changed, 8 insertions(+)
>>
>> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
>> index 9e503201..0552f6d7 100644
>> --- a/daxctl/lib/libdaxctl.c
>> +++ b/daxctl/lib/libdaxctl.c
>> @@ -119,11 +119,18 @@ DAXCTL_EXPORT struct daxctl_ctx *daxctl_ref(struct 
>> daxctl_ctx *ctx)
>>   */
>>  DAXCTL_EXPORT void daxctl_unref(struct daxctl_ctx *ctx)
>>  {
>> +   struct daxctl_region *region;
>> +
>> if (ctx == NULL)
>> return;
>> ctx->refcount--;
>> if (ctx->refcount > 0)
>> return;
>> +
>> +   while ((region = list_top(>regions, struct daxctl_region, 
>> list)) !=
>> +   NULL)
>> +   daxctl_region_unref(region);
>> +
>> info(ctx, "context %p released\n", ctx);
>> free(ctx);
>>  }
>> diff --git a/daxctl/list.c b/daxctl/list.c
>> index 254f0ac9..9b18ae8d 100644
>> --- a/daxctl/list.c
>> +++ b/daxctl/list.c
>> @@ -134,6 +134,7 @@ int cmd_list(int argc, const char **argv, void *ctx)
>> else if (jdevs)
>> util_display_json_array(stdout, jdevs, jflag);
>>
>> +   daxctl_unref(ctx);
> 
> This should move to daxctl/daxctl.c similar to the ndctl_unref() in
> ndctl/ndctl.c
> 
With the way things are coded right now that's the exit function and we
can't have it in daxctl.c. main_handle_internal_commands gets called and
that invokes exit() on the matched commands. We never return from that.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[ndctl PATCH v2] ndctl: complete move to "fsdax" and "devdax"

[Ross Zwisler]

Add on to the work started by:

commit ebb4fb605e68 ("ndctl, create-namespace: introduce "fsdax" and "devdax" 
modes")

and change some more user visible places to use "fsdax" and "devdax" modes
instead of "memory" and "dax", respectively.  Having multiple terms for the
same mode is confusing for users.

We will continue to accept "memory" and "dax" as parameters, but all output
and man pages will now use the updated terms.

Note that after the above referenced commit we still printed the old names
in the default 'ndctl list' output for backward compatibility with scripts.
This patch intentionally breaks that backward compatibility in favor of
avoiding confusion and using the new mode names everywhere.

Signed-off-by: Ross Zwisler 
---
 Documentation/ndctl/ndctl-inject-error.txt |  2 +-
 Documentation/ndctl/ndctl-list.txt |  6 +++---
 ndctl/namespace.c  | 16 
 util/json.c| 10 ++
 4 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/Documentation/ndctl/ndctl-inject-error.txt 
b/Documentation/ndctl/ndctl-inject-error.txt
index 01f6c22..94c4e69 100644
--- a/Documentation/ndctl/ndctl-inject-error.txt
+++ b/Documentation/ndctl/ndctl-inject-error.txt
@@ -45,7 +45,7 @@ OPTIONS
 
NOTE: The offset is interpreted in different ways based on the "mode"
of the namespace. For "raw" mode, the offset is the base namespace
-   offset. For "memory" mode (i.e. a "pfn" namespace), the offset is
+   offset. For "fsdax" mode (i.e. a "pfn" namespace), the offset is
relative to the user-visible part of the namespace, and the offset
introduced by the kernel's metadata will be accounted for. For a
"sector" mode namespace (i.e. a "BTT" namespace), the offset is
diff --git a/Documentation/ndctl/ndctl-list.txt 
b/Documentation/ndctl/ndctl-list.txt
index 04affc4..2abc572 100644
--- a/Documentation/ndctl/ndctl-list.txt
+++ b/Documentation/ndctl/ndctl-list.txt
@@ -49,7 +49,7 @@ EXAMPLE
   "namespaces":[
 {
   "dev":"namespace0.0",
-  "mode":"memory",
+  "mode":"fsdax",
   "size":8589934592,
   "blockdev":"pmem0"
 }
@@ -132,11 +132,11 @@ include::xable-region-options.txt[]
 -X::
 --device-dax::
Include device-dax ("daxregion") details when a namespace is in
-   "dax" mode.
+   "devdax" mode.
 [verse]
 {
   "dev":"namespace0.0",
-  "mode":"dax",
+  "mode":"devdax",
   "size":4225761280,
   "uuid":"18ae1bbb-bb62-4efc-86df-4a5caacb5dcc",
   "daxregion":{
diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index f2c5644..fe86d82 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -103,7 +103,7 @@ OPT_STRING('n', "name", , "name", \
 OPT_STRING('s', "size", , "size", \
"specify the namespace size in bytes (default: available capacity)"), \
 OPT_STRING('m', "mode", , "operation-mode", \
-   "specify a mode for the namespace, 'sector', 'memory', or 'raw'"), \
+   "specify a mode for the namespace, 'sector', 'fsdax', 'devdax' or 
'raw'"), \
 OPT_STRING('M', "map", , "memmap-location", \
"specify 'mem' or 'dev' for the location of the memmap"), \
 OPT_STRING('l', "sector-size", _size, "lba-size", \
@@ -533,7 +533,7 @@ static int validate_namespace_options(struct ndctl_region 
*region,
 * supported a 2M default alignment when
 * ndctl_pfn_has_align() returns false.
 */
-   debug("%s not support 'align' for memory mode\n",
+   debug("%s not support 'align' for fsdax mode\n",
region_name);
return -EAGAIN;
} else if (p->mode == NDCTL_NS_MODE_DAX
@@ -542,7 +542,7 @@ static int validate_namespace_options(struct ndctl_region 
*region,
 * Unlike the pfn case, we require the kernel to
 * have 'align' support for device-dax.
 */
-   debug("%s not support 'align' for dax mode\n",
+   debug("%s not support 'align' for devdax mode\n",
region_name);
return -EAGAIN;
} else if (!param.align_default
@@ -696,7 +696,7 @@ static int validate_namespace_options(struct ndctl_region 
*region,
 
if (ndns && p->mode != NDCTL_NS_MODE_MEMORY
&& p->mode != NDCTL_NS_MODE_DAX) {
-   debug("%s: --map= only valid for memory mode 
namespace\n",
+   debug("%s: --map= only valid for fsdax mode 
namespace\n",
ndctl_namespace_get_devname(ndns));
return -EINVAL;
}
@@ -709,10 +709,10 @@ static int validate_namespace_options(struct ndctl_region 
*region,
struct ndctl_pfn *pfn = 

Re: [PATCH] ndctl: fix libdaxctl memory leak

[Dave Jiang]

On 04/10/2018 10:01 AM, Dan Williams wrote:
> On Tue, Apr 10, 2018 at 9:56 AM, Dave Jiang  wrote:
>> When daxctl_unref is releasing the context, we should make sure that the
>> regions and devices are also being released.
>>
>> Signed-off-by: Dave Jiang 
>> ---
>>  daxctl/lib/libdaxctl.c |7 +++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
>> index 9e503201..0552f6d7 100644
>> --- a/daxctl/lib/libdaxctl.c
>> +++ b/daxctl/lib/libdaxctl.c
>> @@ -119,11 +119,18 @@ DAXCTL_EXPORT struct daxctl_ctx *daxctl_ref(struct 
>> daxctl_ctx *ctx)
>>   */
>>  DAXCTL_EXPORT void daxctl_unref(struct daxctl_ctx *ctx)
>>  {
>> +   struct daxctl_region *region;
>> +
>> if (ctx == NULL)
>> return;
>> ctx->refcount--;
>> if (ctx->refcount > 0)
>> return;
>> +
>> +   while ((region = list_top(>regions, struct daxctl_region, 
>> list)) !=
>> +   NULL)
>> +   daxctl_region_unref(region);
>> +
> 
> Sorry, should have mentioned this before. Why not use
> list_for_each_entry_safe() to iterate and delete regions like all the
> other free routines?
> 

Somehow I missed that even though I was looking for it. Will fix.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v2] ndctl: fix libdaxctl memory leak

[Dave Jiang]

On 04/10/2018 10:38 AM, Plewa, Lukasz wrote:
> On Tue, Apr 10, 2018 at 7:17 PM, Dave Jiang  wrote:
>> When daxctl_unref is releasing the context, we should make sure that the
>> regions and devices are also being released. free_region() will free all the
>> devices under the region.
>>
>> Signed-off-by: Dave Jiang 
>> ---
>>
>> v2: Use list_for_each_safe() for region removal. (Dan)
>>
>>  daxctl/lib/libdaxctl.c |8 
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c index
>> 9e503201..22f4210a 100644
>> --- a/daxctl/lib/libdaxctl.c
>> +++ b/daxctl/lib/libdaxctl.c
>> @@ -29,6 +29,8 @@
>>
>>  static const char *attrs = "dax_region";
>>
>> +static void free_region(struct daxctl_region *region, struct list_head
>> +*head);
>> +
>>  /**
>>   * struct daxctl_ctx - library user context to find "nd" instances
>>   *
>> @@ -119,11 +121,17 @@ DAXCTL_EXPORT struct daxctl_ctx
>> *daxctl_ref(struct daxctl_ctx *ctx)
>>   */
>>  DAXCTL_EXPORT void daxctl_unref(struct daxctl_ctx *ctx)  {
>> +struct daxctl_region *region, *_r;
>> +
>>  if (ctx == NULL)
>>  return;
>>  ctx->refcount--;
>>  if (ctx->refcount > 0)
>>  return;
>> +
>> +list_for_each_safe(>regions, region, _r, list)
>> +free_region(region, >regions);
>> +
>>  info(ctx, "context %p released\n", ctx);
>>  free(ctx);
>>  }
> 
> As daxctl_region has its own refcount, you need daxctl_ref() in 
> add_dax_region() and daxctl_unref() in free_region().( or daxctl_ref() in 
> daxctl_region_ref() and daxctl_unref() in daxctl_region_unref())
> 
> Without it, this code will cause segfault:
>  
> daxctl_new();
> region = daxctl_new_region(...);
> daxctl_region_ref(region);
> daxctl_unref(ctx);
> daxctl_region_unref(region);

Shouldn't it go in this order:

daxctl_region_unref(region);
daxctl_unref(ctx);

In this case, you won't segfault right? I think ctx should be the very
last thing you free.

> 
> Łukasz
> 
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] ndctl: fix daxctl list memory leak

[Dan Williams]

On Tue, Apr 10, 2018 at 8:59 AM, Dave Jiang  wrote:
>
>
> On 04/09/2018 07:39 PM, Dan Williams wrote:
>> On Mon, Apr 9, 2018 at 5:13 PM, Dave Jiang  wrote:
>>> daxctl list is not calling daxctl_unref() when executed succesfully. At the 
>>> same
>>> time, daxctl_region_unref() is not being called when daxctl_unref() 
>>> executes.
>>> Valgrind is reporting unfreed memory. Adding the appropriate calls to make 
>>> sure
>>> all memory allocated are freed for daxctl list.
>>>
>>> Signed-off-by: Dave Jiang 
>>> ---
>>>  daxctl/lib/libdaxctl.c |7 +++
>>>  daxctl/list.c  |1 +
>>>  2 files changed, 8 insertions(+)
>>>
>>> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
>>> index 9e503201..0552f6d7 100644
>>> --- a/daxctl/lib/libdaxctl.c
>>> +++ b/daxctl/lib/libdaxctl.c
>>> @@ -119,11 +119,18 @@ DAXCTL_EXPORT struct daxctl_ctx *daxctl_ref(struct 
>>> daxctl_ctx *ctx)
>>>   */
>>>  DAXCTL_EXPORT void daxctl_unref(struct daxctl_ctx *ctx)
>>>  {
>>> +   struct daxctl_region *region;
>>> +
>>> if (ctx == NULL)
>>> return;
>>> ctx->refcount--;
>>> if (ctx->refcount > 0)
>>> return;
>>> +
>>> +   while ((region = list_top(>regions, struct daxctl_region, 
>>> list)) !=
>>> +   NULL)
>>> +   daxctl_region_unref(region);
>>> +
>>> info(ctx, "context %p released\n", ctx);
>>> free(ctx);
>>>  }
>>> diff --git a/daxctl/list.c b/daxctl/list.c
>>> index 254f0ac9..9b18ae8d 100644
>>> --- a/daxctl/list.c
>>> +++ b/daxctl/list.c
>>> @@ -134,6 +134,7 @@ int cmd_list(int argc, const char **argv, void *ctx)
>>> else if (jdevs)
>>> util_display_json_array(stdout, jdevs, jflag);
>>>
>>> +   daxctl_unref(ctx);
>>
>> This should move to daxctl/daxctl.c similar to the ndctl_unref() in
>> ndctl/ndctl.c
>>
> With the way things are coded right now that's the exit function and we
> can't have it in daxctl.c. main_handle_internal_commands gets called and
> that invokes exit() on the matched commands. We never return from that.

Then we need to fix that. cmd_list() should not be releasing
references that it did not take. How is daxctl::cmd_list() leaky but
ndctl::cmd_list() is not?
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v2] ndctl: add support in libndctl to provide deep flush

[Dave Jiang]

Providing an API call in libndctl to support accessing the region deep_flush
in sysfs.

Signed-off-by: Dave Jiang 
---

v2: Cover case where deep_flush doesn't exist, i.e. memmap=nn!ss. (Dan)

 ndctl/lib/libndctl.c   |   35 +++
 ndctl/lib/libndctl.sym |1 +
 ndctl/libndctl.h   |1 +
 3 files changed, 37 insertions(+)

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index 580a450e..c34f1e09 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -181,6 +181,8 @@ struct ndctl_region {
FILE *badblocks;
struct badblock bb;
enum ndctl_persistence_domain persistence_domain;
+   /* file descriptor for deep flush sysfs entry */
+   int flush_fd;
 };
 
 /**
@@ -511,6 +513,8 @@ static void free_region(struct ndctl_region *region)
free(region->region_path);
if (region->badblocks)
fclose(region->badblocks);
+   if (region->flush_fd > 0)
+   close(region->flush_fd);
free(region);
 }
 
@@ -1049,6 +1053,14 @@ NDCTL_EXPORT unsigned long long 
ndctl_region_get_resource(struct ndctl_region *r
return strtoull(buf, NULL, 0);
 }
 
+NDCTL_EXPORT int ndctl_region_deep_flush(struct ndctl_region *region)
+{
+   int rc = pwrite(region->flush_fd, "1", 1, 0);
+
+   return (rc == -1) ? -errno : 0;
+}
+
+
 NDCTL_EXPORT const char *ndctl_bus_get_cmd_name(struct ndctl_bus *bus, int cmd)
 {
return nvdimm_bus_cmd_name(cmd);
@@ -1791,6 +1803,7 @@ static void *add_region(void *parent, int id, const char 
*region_base)
struct ndctl_bus *bus = parent;
struct ndctl_ctx *ctx = bus->ctx;
char *path = calloc(1, strlen(region_base) + 100);
+   int perm;
 
if (!path)
return NULL;
@@ -1866,6 +1879,28 @@ static void *add_region(void *parent, int id, const char 
*region_base)
else
region->persistence_domain = region_get_pd_type(buf);
 
+   sprintf(path, "%s/deep_flush", region_base);
+   region->flush_fd = open(path, O_RDWR);
+   if (region->flush_fd == -1) {
+   /* for those that do not export deep_flush sysfs file */
+   if (errno == ENOENT)
+   goto out;
+   else
+   goto err_read;
+   }
+
+   if (pread(region->flush_fd, buf, 1, 0) == -1) {
+   close(region->flush_fd);
+   goto err_read;
+   }
+
+   perm = strtol(buf, NULL, 0);
+   if (perm == 0) {
+   region->flush_fd = -1;
+   close(region->flush_fd);
+   }
+
+ out:
free(path);
return region;
 
diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
index 3209aefe..38cc3b9a 100644
--- a/ndctl/lib/libndctl.sym
+++ b/ndctl/lib/libndctl.sym
@@ -352,4 +352,5 @@ global:
ndctl_dimm_fw_update_supported;
ndctl_region_get_persistence_domain;
ndctl_bus_get_persistence_domain;
+   ndctl_region_deep_flush;
 } LIBNDCTL_14;
diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
index cf6a77fd..1a622ae5 100644
--- a/ndctl/libndctl.h
+++ b/ndctl/libndctl.h
@@ -365,6 +365,7 @@ int ndctl_region_enable(struct ndctl_region *region);
 int ndctl_region_disable_invalidate(struct ndctl_region *region);
 int ndctl_region_disable_preserve(struct ndctl_region *region);
 void ndctl_region_cleanup(struct ndctl_region *region);
+int ndctl_region_deep_flush(struct ndctl_region *region);
 
 struct ndctl_interleave_set;
 struct ndctl_interleave_set *ndctl_region_get_interleave_set(

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 5/5] tools/testing/nvdimm: enable labels for nfit_test.1 dimms

[Dan Williams]

Enable test cases for the kernel's fallback to label-less mode.

Signed-off-by: Dan Williams 
---
 tools/testing/nvdimm/test/nfit.c |3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
index 51fc41c24a77..4ea385be528f 100644
--- a/tools/testing/nvdimm/test/nfit.c
+++ b/tools/testing/nvdimm/test/nfit.c
@@ -2265,6 +2265,9 @@ static void nfit_test1_setup(struct nfit_test *t)
set_bit(ND_CMD_ARS_STATUS, _desc->bus_cmd_force_en);
set_bit(ND_CMD_CLEAR_ERROR, _desc->bus_cmd_force_en);
set_bit(ND_INTEL_ENABLE_LSS_STATUS, _desc->dimm_cmd_force_en);
+   set_bit(ND_CMD_GET_CONFIG_SIZE, _desc->dimm_cmd_force_en);
+   set_bit(ND_CMD_GET_CONFIG_DATA, _desc->dimm_cmd_force_en);
+   set_bit(ND_CMD_SET_CONFIG_DATA, _desc->dimm_cmd_force_en);
 }
 
 static int nfit_test_blk_do_io(struct nd_blk_region *ndbr, resource_size_t dpa,

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 4/5] tools/testing/nvdimm: fix missing newline in nfit_test_dimm 'handle' attribute

[Dan Williams]

Sysfs userspace tooling generally expects the kernel to emit a newlines
when reading sysfs attributes.

Signed-off-by: Dan Williams 
---
 tools/testing/nvdimm/test/nfit.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
index c8c88363311b..51fc41c24a77 100644
--- a/tools/testing/nvdimm/test/nfit.c
+++ b/tools/testing/nvdimm/test/nfit.c
@@ -1193,7 +1193,7 @@ static ssize_t handle_show(struct device *dev, struct 
device_attribute *attr,
if (dimm < 0)
return dimm;
 
-   return sprintf(buf, "%#x", handle[dimm]);
+   return sprintf(buf, "%#x\n", handle[dimm]);
 }
 DEVICE_ATTR_RO(handle);
 

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 3/5] tools/testing/nvdimm: support nfit_test_dimm attributes under nfit_test.1

[Dan Williams]

The nfit_test.1 bus provides a pmem topology without blk-aperture
enabling, so it presents different failure modes for label space
handling. Allow custom DSM command error injection.

Signed-off-by: Dan Williams 
---
 tools/testing/nvdimm/test/nfit.c |   43 ++
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
index dc6cf5630280..c8c88363311b 100644
--- a/tools/testing/nvdimm/test/nfit.c
+++ b/tools/testing/nvdimm/test/nfit.c
@@ -1166,12 +1166,12 @@ static int ars_state_init(struct device *dev, struct 
ars_state *ars_state)
 
 static void put_dimms(void *data)
 {
-   struct device **dimm_dev = data;
+   struct nfit_test *t = data;
int i;
 
-   for (i = 0; i < NUM_DCR; i++)
-   if (dimm_dev[i])
-   device_unregister(dimm_dev[i]);
+   for (i = 0; i < t->num_dcr; i++)
+   if (t->dimm_dev[i])
+   device_unregister(t->dimm_dev[i]);
 }
 
 static struct class *nfit_test_dimm;
@@ -1180,13 +1180,11 @@ static int dimm_name_to_id(struct device *dev)
 {
int dimm;
 
-   if (sscanf(dev_name(dev), "test_dimm%d", ) != 1
-   || dimm >= NUM_DCR || dimm < 0)
+   if (sscanf(dev_name(dev), "test_dimm%d", ) != 1)
return -ENXIO;
return dimm;
 }
 
-
 static ssize_t handle_show(struct device *dev, struct device_attribute *attr,
char *buf)
 {
@@ -1259,7 +1257,6 @@ static ssize_t fail_cmd_code_store(struct device *dev, 
struct device_attribute *
 }
 static DEVICE_ATTR_RW(fail_cmd_code);
 
-
 static struct attribute *nfit_test_dimm_attributes[] = {
_attr_fail_cmd.attr,
_attr_fail_cmd_code.attr,
@@ -1276,6 +1273,23 @@ static const struct attribute_group 
*nfit_test_dimm_attribute_groups[] = {
NULL,
 };
 
+static int nfit_test_dimm_init(struct nfit_test *t)
+{
+   int i;
+
+   if (devm_add_action_or_reset(>pdev.dev, put_dimms, t))
+   return -ENOMEM;
+   for (i = 0; i < t->num_dcr; i++) {
+   t->dimm_dev[i] = device_create_with_groups(nfit_test_dimm,
+   >pdev.dev, 0, NULL,
+   nfit_test_dimm_attribute_groups,
+   "test_dimm%d", i + t->dcr_idx);
+   if (!t->dimm_dev[i])
+   return -ENOMEM;
+   }
+   return 0;
+}
+
 static void smart_init(struct nfit_test *t)
 {
int i;
@@ -1371,17 +1385,8 @@ static int nfit_test0_alloc(struct nfit_test *t)
if (!t->_fit)
return -ENOMEM;
 
-   if (devm_add_action_or_reset(>pdev.dev, put_dimms, t->dimm_dev))
+   if (nfit_test_dimm_init(t))
return -ENOMEM;
-   for (i = 0; i < NUM_DCR; i++) {
-   t->dimm_dev[i] = device_create_with_groups(nfit_test_dimm,
-   >pdev.dev, 0, NULL,
-   nfit_test_dimm_attribute_groups,
-   "test_dimm%d", i);
-   if (!t->dimm_dev[i])
-   return -ENOMEM;
-   }
-
smart_init(t);
return ars_state_init(>pdev.dev, >ars_state);
 }
@@ -1413,6 +1418,8 @@ static int nfit_test1_alloc(struct nfit_test *t)
if (!t->spa_set[1])
return -ENOMEM;
 
+   if (nfit_test_dimm_init(t))
+   return -ENOMEM;
smart_init(t);
return ars_state_init(>pdev.dev, >ars_state);
 }

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 0/5] libnvdimm: fix locked status detection

[Dan Williams]

The new ACPI _LSx methods moved the 'dimm locked' error status from the
result of ND_CMD_GET_CONFIG_SIZE to an error status result of
ND_CMD_GET_CONFIG_DATA. Error code translation prevents the proper error
code from making it back to the 'nd_dimm' driver.

Fix the error code propagation and add some unit test infrastructure for
regression testing this case.

---

Dan Williams (5):
  libnvdimm, dimm: handle EACCES failures from label reads
  tools/testing/nvdimm: allow custom error code injection
  tools/testing/nvdimm: support nfit_test_dimm attributes under nfit_test.1
  tools/testing/nvdimm: fix missing newline in nfit_test_dimm 'handle' 
attribute
  tools/testing/nvdimm: enable labels for nfit_test.1 dimms


 drivers/nvdimm/dimm_devs.c   |   22 +-
 tools/testing/nvdimm/test/nfit.c |   84 +-
 2 files changed, 77 insertions(+), 29 deletions(-)
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 2/5] tools/testing/nvdimm: allow custom error code injection

[Dan Williams]

Given that libnvdimm driver stack takes specific actions on DIMM command
error codes like -EACCES, provide a facility to inject custom failures.

Signed-off-by: Dan Williams 
---
 tools/testing/nvdimm/test/nfit.c |   38 +-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
index cb166be4918d..dc6cf5630280 100644
--- a/tools/testing/nvdimm/test/nfit.c
+++ b/tools/testing/nvdimm/test/nfit.c
@@ -138,6 +138,7 @@ static u32 handle[] = {
 };
 
 static unsigned long dimm_fail_cmd_flags[NUM_DCR];
+static int dimm_fail_cmd_code[NUM_DCR];
 
 struct nfit_test_fw {
enum intel_fw_update_state state;
@@ -892,8 +893,11 @@ static int get_dimm(struct nfit_mem *nfit_mem, unsigned 
int func)
if (i >= ARRAY_SIZE(handle))
return -ENXIO;
 
-   if ((1 << func) & dimm_fail_cmd_flags[i])
+   if ((1 << func) & dimm_fail_cmd_flags[i]) {
+   if (dimm_fail_cmd_code[i])
+   return dimm_fail_cmd_code[i];
return -EIO;
+   }
 
return i;
 }
@@ -1225,8 +1229,40 @@ static ssize_t fail_cmd_store(struct device *dev, struct 
device_attribute *attr,
 }
 static DEVICE_ATTR_RW(fail_cmd);
 
+static ssize_t fail_cmd_code_show(struct device *dev, struct device_attribute 
*attr,
+   char *buf)
+{
+   int dimm = dimm_name_to_id(dev);
+
+   if (dimm < 0)
+   return dimm;
+
+   return sprintf(buf, "%d\n", dimm_fail_cmd_code[dimm]);
+}
+
+static ssize_t fail_cmd_code_store(struct device *dev, struct device_attribute 
*attr,
+   const char *buf, size_t size)
+{
+   int dimm = dimm_name_to_id(dev);
+   unsigned long val;
+   ssize_t rc;
+
+   if (dimm < 0)
+   return dimm;
+
+   rc = kstrtol(buf, 0, );
+   if (rc)
+   return rc;
+
+   dimm_fail_cmd_code[dimm] = val;
+   return size;
+}
+static DEVICE_ATTR_RW(fail_cmd_code);
+
+
 static struct attribute *nfit_test_dimm_attributes[] = {
_attr_fail_cmd.attr,
+   _attr_fail_cmd_code.attr,
_attr_handle.attr,
NULL,
 };

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 1/5] libnvdimm, dimm: handle EACCES failures from label reads

[Dan Williams]

The new support for the standard _LSR and _LSW methods neglected to also
update the nvdimm_init_config_data() and nvdimm_set_config_data() to
return the translated error code from failed commands. This precision is
necessary because the locked status that was previously returned on
ND_CMD_GET_CONFIG_SIZE commands is now returned on
ND_CMD_{GET,SET}_CONFIG_DATA commands.

If the kernel misses this indication it can inadvertently fall back to
label-less mode when it should otherwise avoid all access to locked
regions.

Cc: 
Fixes: 4b27db7e26cd ("acpi, nfit: add support for the _LSI, _LSR, and...")
Signed-off-by: Dan Williams 
---
 drivers/nvdimm/dimm_devs.c |   22 --
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index e00d45522b80..8d348b22ba45 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -88,9 +88,9 @@ int nvdimm_init_nsarea(struct nvdimm_drvdata *ndd)
 int nvdimm_init_config_data(struct nvdimm_drvdata *ndd)
 {
struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(ndd->dev);
+   int rc = validate_dimm(ndd), cmd_rc = 0;
struct nd_cmd_get_config_data_hdr *cmd;
struct nvdimm_bus_descriptor *nd_desc;
-   int rc = validate_dimm(ndd);
u32 max_cmd_size, config_size;
size_t offset;
 
@@ -124,9 +124,11 @@ int nvdimm_init_config_data(struct nvdimm_drvdata *ndd)
cmd->in_offset = offset;
rc = nd_desc->ndctl(nd_desc, to_nvdimm(ndd->dev),
ND_CMD_GET_CONFIG_DATA, cmd,
-   cmd->in_length + sizeof(*cmd), NULL);
-   if (rc || cmd->status) {
-   rc = -ENXIO;
+   cmd->in_length + sizeof(*cmd), _rc);
+   if (rc < 0)
+   break;
+   if (cmd_rc < 0) {
+   rc = cmd_rc;
break;
}
memcpy(ndd->data + offset, cmd->out_buf, cmd->in_length);
@@ -140,9 +142,9 @@ int nvdimm_init_config_data(struct nvdimm_drvdata *ndd)
 int nvdimm_set_config_data(struct nvdimm_drvdata *ndd, size_t offset,
void *buf, size_t len)
 {
-   int rc = validate_dimm(ndd);
size_t max_cmd_size, buf_offset;
struct nd_cmd_set_config_hdr *cmd;
+   int rc = validate_dimm(ndd), cmd_rc = 0;
struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(ndd->dev);
struct nvdimm_bus_descriptor *nd_desc = nvdimm_bus->nd_desc;
 
@@ -164,7 +166,6 @@ int nvdimm_set_config_data(struct nvdimm_drvdata *ndd, 
size_t offset,
for (buf_offset = 0; len; len -= cmd->in_length,
buf_offset += cmd->in_length) {
size_t cmd_size;
-   u32 *status;
 
cmd->in_offset = offset + buf_offset;
cmd->in_length = min(max_cmd_size, len);
@@ -172,12 +173,13 @@ int nvdimm_set_config_data(struct nvdimm_drvdata *ndd, 
size_t offset,
 
/* status is output in the last 4-bytes of the command buffer */
cmd_size = sizeof(*cmd) + cmd->in_length + sizeof(u32);
-   status = ((void *) cmd) + cmd_size - sizeof(u32);
 
rc = nd_desc->ndctl(nd_desc, to_nvdimm(ndd->dev),
-   ND_CMD_SET_CONFIG_DATA, cmd, cmd_size, NULL);
-   if (rc || *status) {
-   rc = rc ? rc : -ENXIO;
+   ND_CMD_SET_CONFIG_DATA, cmd, cmd_size, _rc);
+   if (rc < 0)
+   break;
+   if (cmd_rc < 0) {
+   rc = cmd_rc;
break;
}
}

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v2] ndctl: add support in libndctl to provide deep flush

[Dan Williams]

On Tue, Apr 10, 2018 at 3:06 PM, Dave Jiang  wrote:
> Providing an API call in libndctl to support accessing the region deep_flush
> in sysfs.
>
> Signed-off-by: Dave Jiang 
> ---
>
> v2: Cover case where deep_flush doesn't exist, i.e. memmap=nn!ss. (Dan)
>
>  ndctl/lib/libndctl.c   |   35 +++
>  ndctl/lib/libndctl.sym |1 +
>  ndctl/libndctl.h   |1 +
>  3 files changed, 37 insertions(+)
>
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index 580a450e..c34f1e09 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -181,6 +181,8 @@ struct ndctl_region {
> FILE *badblocks;
> struct badblock bb;
> enum ndctl_persistence_domain persistence_domain;
> +   /* file descriptor for deep flush sysfs entry */
> +   int flush_fd;
>  };
>
>  /**
> @@ -511,6 +513,8 @@ static void free_region(struct ndctl_region *region)
> free(region->region_path);
> if (region->badblocks)
> fclose(region->badblocks);
> +   if (region->flush_fd > 0)
> +   close(region->flush_fd);
> free(region);
>  }
>
> @@ -1049,6 +1053,14 @@ NDCTL_EXPORT unsigned long long 
> ndctl_region_get_resource(struct ndctl_region *r
> return strtoull(buf, NULL, 0);
>  }
>
> +NDCTL_EXPORT int ndctl_region_deep_flush(struct ndctl_region *region)
> +{
> +   int rc = pwrite(region->flush_fd, "1", 1, 0);

Make this write "1\n" to match standard sysfs manipulation done by
"echo" that includes a newline.

> +
> +   return (rc == -1) ? -errno : 0;
> +}
> +
> +
>  NDCTL_EXPORT const char *ndctl_bus_get_cmd_name(struct ndctl_bus *bus, int 
> cmd)
>  {
> return nvdimm_bus_cmd_name(cmd);
> @@ -1791,6 +1803,7 @@ static void *add_region(void *parent, int id, const 
> char *region_base)
> struct ndctl_bus *bus = parent;
> struct ndctl_ctx *ctx = bus->ctx;
> char *path = calloc(1, strlen(region_base) + 100);
> +   int perm;
>
> if (!path)
> return NULL;
> @@ -1866,6 +1879,28 @@ static void *add_region(void *parent, int id, const 
> char *region_base)
> else
> region->persistence_domain = region_get_pd_type(buf);
>
> +   sprintf(path, "%s/deep_flush", region_base);
> +   region->flush_fd = open(path, O_RDWR);

Make this O_CLOEXEC, so we don't leak fds on exec.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v3] ndctl: add support in libndctl to provide deep flush

[Dave Jiang]

Providing an API call in libndctl to support accessing the region deep_flush
in sysfs.

Signed-off-by: Dave Jiang 
---

v3: 
- Add "\n" to sysfs write. (Dan)
- add O_CLOEXEC to open() call for sysfs. (Dan)

v2: Cover case where deep_flush doesn't exist, i.e. memmap=nn!ss. (Dan)

 ndctl/lib/libndctl.c   |   35 +++
 ndctl/lib/libndctl.sym |1 +
 ndctl/libndctl.h   |1 +
 3 files changed, 37 insertions(+)

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index 580a450e..03a18985 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -181,6 +181,8 @@ struct ndctl_region {
FILE *badblocks;
struct badblock bb;
enum ndctl_persistence_domain persistence_domain;
+   /* file descriptor for deep flush sysfs entry */
+   int flush_fd;
 };
 
 /**
@@ -511,6 +513,8 @@ static void free_region(struct ndctl_region *region)
free(region->region_path);
if (region->badblocks)
fclose(region->badblocks);
+   if (region->flush_fd > 0)
+   close(region->flush_fd);
free(region);
 }
 
@@ -1049,6 +1053,14 @@ NDCTL_EXPORT unsigned long long 
ndctl_region_get_resource(struct ndctl_region *r
return strtoull(buf, NULL, 0);
 }
 
+NDCTL_EXPORT int ndctl_region_deep_flush(struct ndctl_region *region)
+{
+   int rc = pwrite(region->flush_fd, "1\n", 1, 0);
+
+   return (rc == -1) ? -errno : 0;
+}
+
+
 NDCTL_EXPORT const char *ndctl_bus_get_cmd_name(struct ndctl_bus *bus, int cmd)
 {
return nvdimm_bus_cmd_name(cmd);
@@ -1791,6 +1803,7 @@ static void *add_region(void *parent, int id, const char 
*region_base)
struct ndctl_bus *bus = parent;
struct ndctl_ctx *ctx = bus->ctx;
char *path = calloc(1, strlen(region_base) + 100);
+   int perm;
 
if (!path)
return NULL;
@@ -1866,6 +1879,28 @@ static void *add_region(void *parent, int id, const char 
*region_base)
else
region->persistence_domain = region_get_pd_type(buf);
 
+   sprintf(path, "%s/deep_flush", region_base);
+   region->flush_fd = open(path, O_RDWR | O_CLOEXEC);
+   if (region->flush_fd == -1) {
+   /* for those that do not export deep_flush sysfs file */
+   if (errno == ENOENT)
+   goto out;
+   else
+   goto err_read;
+   }
+
+   if (pread(region->flush_fd, buf, 1, 0) == -1) {
+   close(region->flush_fd);
+   goto err_read;
+   }
+
+   perm = strtol(buf, NULL, 0);
+   if (perm == 0) {
+   region->flush_fd = -1;
+   close(region->flush_fd);
+   }
+
+ out:
free(path);
return region;
 
diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
index 3209aefe..38cc3b9a 100644
--- a/ndctl/lib/libndctl.sym
+++ b/ndctl/lib/libndctl.sym
@@ -352,4 +352,5 @@ global:
ndctl_dimm_fw_update_supported;
ndctl_region_get_persistence_domain;
ndctl_bus_get_persistence_domain;
+   ndctl_region_deep_flush;
 } LIBNDCTL_14;
diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
index cf6a77fd..1a622ae5 100644
--- a/ndctl/libndctl.h
+++ b/ndctl/libndctl.h
@@ -365,6 +365,7 @@ int ndctl_region_enable(struct ndctl_region *region);
 int ndctl_region_disable_invalidate(struct ndctl_region *region);
 int ndctl_region_disable_preserve(struct ndctl_region *region);
 void ndctl_region_cleanup(struct ndctl_region *region);
+int ndctl_region_deep_flush(struct ndctl_region *region);
 
 struct ndctl_interleave_set;
 struct ndctl_interleave_set *ndctl_region_get_interleave_set(

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v2] libnvdimm: fix NULL ptr access in nvdimm_flush when region is disabled

[Dave Jiang]

When a region is disabled, there is no driver attached. Therefore
dev->driverdata is NULL. An attempt to write to regionN/deep_flush via sysfs
would cause a NULL pointer dereference. Bail when dev->driver is NULL to
protect this scenario.

Fix: ab630891ce0eb(libnvdimm, region: sysfs trigger for nvdimm_flush())

Signed-off-by: Dave Jiang 
---

v2: Move to deep_flush_store. (Dan)

 drivers/nvdimm/region_devs.c |5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index a612be6f019d..65cc2a5b48b8 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -290,6 +290,11 @@ static ssize_t deep_flush_store(struct device *dev, struct 
device_attribute *att
return rc;
if (!flush)
return -EINVAL;
+
+   /* protect against disabled region */
+   if (!nd_region->dev.driver)
+   return -ENXIO;
+
nvdimm_flush(nd_region);
 
return len;

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] libnvdimm: fix NULL ptr access in nvdimm_flush when region is disabled

[Dan Williams]

On Tue, Apr 10, 2018 at 1:42 PM, Dave Jiang  wrote:
> When a region is disabled, there is no driver attached. Therefore
> dev->driverdata is NULL. An attempt to write to regionN/deep_flush via sysfs
> would cause a NULL pointer dereference. Bail when dev->driver is NULL to
> protect this scenario.
>
> Fix: ab630891ce0eb(libnvdimm, region: sysfs trigger for nvdimm_flush())
>
> Signed-off-by: Dave Jiang 
> ---
>  drivers/nvdimm/region_devs.c |4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
> index a612be6f019d..d5619b7feb6a 100644
> --- a/drivers/nvdimm/region_devs.c
> +++ b/drivers/nvdimm/region_devs.c
> @@ -1074,6 +1074,10 @@ void nvdimm_flush(struct nd_region *nd_region)
> struct nd_region_data *ndrd = dev_get_drvdata(_region->dev);
> int i, idx;
>
> +   /* protect against disabled region */
> +   if (!nd_region->dev.driver)
> +   return;
> +

Move this to deep_flush_store(). That's the only caller that can
trigger nvdimm_flush() while the region might be disabled, and that
needs to return an error code.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH] ndctl: add support in libndctl to provide deep flush

[Dave Jiang]

Providing an API call in libndctl to support accessing the region deep_flush
in sysfs.

Signed-off-by: Dave Jiang 
---
 0 files changed

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index 580a450e..fb4dca73 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -181,6 +181,8 @@ struct ndctl_region {
FILE *badblocks;
struct badblock bb;
enum ndctl_persistence_domain persistence_domain;
+   /* file descriptor for deep flush sysfs entry */
+   int flush_fd;
 };
 
 /**
@@ -511,6 +513,8 @@ static void free_region(struct ndctl_region *region)
free(region->region_path);
if (region->badblocks)
fclose(region->badblocks);
+   if (region->flush_fd > 0)
+   close(region->flush_fd);
free(region);
 }
 
@@ -1049,6 +1053,12 @@ NDCTL_EXPORT unsigned long long 
ndctl_region_get_resource(struct ndctl_region *r
return strtoull(buf, NULL, 0);
 }
 
+NDCTL_EXPORT int ndctl_region_deep_flush(struct ndctl_region *region)
+{
+   return pwrite(region->flush_fd, "1", 1, 0);
+}
+
+
 NDCTL_EXPORT const char *ndctl_bus_get_cmd_name(struct ndctl_bus *bus, int cmd)
 {
return nvdimm_bus_cmd_name(cmd);
@@ -1791,6 +1801,7 @@ static void *add_region(void *parent, int id, const char 
*region_base)
struct ndctl_bus *bus = parent;
struct ndctl_ctx *ctx = bus->ctx;
char *path = calloc(1, strlen(region_base) + 100);
+   int perm;
 
if (!path)
return NULL;
@@ -1866,6 +1877,22 @@ static void *add_region(void *parent, int id, const char 
*region_base)
else
region->persistence_domain = region_get_pd_type(buf);
 
+   sprintf(path, "%s/deep_flush", region_base);
+   region->flush_fd = open(path, O_RDWR);
+   if (region->flush_fd == -1)
+   goto err_read;
+
+   if (pread(region->flush_fd, buf, 1, 0) == -1) {
+   close(region->flush_fd);
+   goto err_read;
+   }
+
+   perm = strtol(buf, NULL, 0);
+   if (perm == 0) {
+   region->flush_fd = -1;
+   close(region->flush_fd);
+   }
+
free(path);
return region;
 
diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
index 3209aefe..38cc3b9a 100644
--- a/ndctl/lib/libndctl.sym
+++ b/ndctl/lib/libndctl.sym
@@ -352,4 +352,5 @@ global:
ndctl_dimm_fw_update_supported;
ndctl_region_get_persistence_domain;
ndctl_bus_get_persistence_domain;
+   ndctl_region_deep_flush;
 } LIBNDCTL_14;
diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
index cf6a77fd..1a622ae5 100644
--- a/ndctl/libndctl.h
+++ b/ndctl/libndctl.h
@@ -365,6 +365,7 @@ int ndctl_region_enable(struct ndctl_region *region);
 int ndctl_region_disable_invalidate(struct ndctl_region *region);
 int ndctl_region_disable_preserve(struct ndctl_region *region);
 void ndctl_region_cleanup(struct ndctl_region *region);
+int ndctl_region_deep_flush(struct ndctl_region *region);
 
 struct ndctl_interleave_set;
 struct ndctl_interleave_set *ndctl_region_get_interleave_set(

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH] libnvdimm: fix NULL ptr access in nvdimm_flush when region is disabled

[Dave Jiang]

When a region is disabled, there is no driver attached. Therefore
dev->driverdata is NULL. An attempt to write to regionN/deep_flush via sysfs
would cause a NULL pointer dereference. Bail when dev->driver is NULL to
protect this scenario.

Fix: ab630891ce0eb(libnvdimm, region: sysfs trigger for nvdimm_flush())

Signed-off-by: Dave Jiang 
---
 drivers/nvdimm/region_devs.c |4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index a612be6f019d..d5619b7feb6a 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -1074,6 +1074,10 @@ void nvdimm_flush(struct nd_region *nd_region)
struct nd_region_data *ndrd = dev_get_drvdata(_region->dev);
int i, idx;
 
+   /* protect against disabled region */
+   if (!nd_region->dev.driver)
+   return;
+
/*
 * Try to encourage some diversity in flush hint addresses
 * across cpus assuming a limited number of flush hints.

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v3] ndctl: add support in libndctl to provide deep flush

[Dan Williams]

On Tue, Apr 10, 2018 at 4:35 PM, Dave Jiang  wrote:
> Providing an API call in libndctl to support accessing the region deep_flush
> in sysfs.
>
> Signed-off-by: Dave Jiang 
> ---
>
> v3:
> - Add "\n" to sysfs write. (Dan)
> - add O_CLOEXEC to open() call for sysfs. (Dan)
>
> v2: Cover case where deep_flush doesn't exist, i.e. memmap=nn!ss. (Dan)
>
>  ndctl/lib/libndctl.c   |   35 +++
>  ndctl/lib/libndctl.sym |1 +
>  ndctl/libndctl.h   |1 +
>  3 files changed, 37 insertions(+)
>
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index 580a450e..03a18985 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -181,6 +181,8 @@ struct ndctl_region {
> FILE *badblocks;
> struct badblock bb;
> enum ndctl_persistence_domain persistence_domain;
> +   /* file descriptor for deep flush sysfs entry */
> +   int flush_fd;
>  };
>
>  /**
> @@ -511,6 +513,8 @@ static void free_region(struct ndctl_region *region)
> free(region->region_path);
> if (region->badblocks)
> fclose(region->badblocks);
> +   if (region->flush_fd > 0)
> +   close(region->flush_fd);
> free(region);
>  }
>
> @@ -1049,6 +1053,14 @@ NDCTL_EXPORT unsigned long long 
> ndctl_region_get_resource(struct ndctl_region *r
> return strtoull(buf, NULL, 0);
>  }
>
> +NDCTL_EXPORT int ndctl_region_deep_flush(struct ndctl_region *region)
> +{
> +   int rc = pwrite(region->flush_fd, "1\n", 1, 0);
> +
> +   return (rc == -1) ? -errno : 0;
> +}
> +
> +
>  NDCTL_EXPORT const char *ndctl_bus_get_cmd_name(struct ndctl_bus *bus, int 
> cmd)
>  {
> return nvdimm_bus_cmd_name(cmd);
> @@ -1791,6 +1803,7 @@ static void *add_region(void *parent, int id, const 
> char *region_base)
> struct ndctl_bus *bus = parent;
> struct ndctl_ctx *ctx = bus->ctx;
> char *path = calloc(1, strlen(region_base) + 100);
> +   int perm;
>
> if (!path)
> return NULL;
> @@ -1866,6 +1879,28 @@ static void *add_region(void *parent, int id, const 
> char *region_base)
> else
> region->persistence_domain = region_get_pd_type(buf);
>
> +   sprintf(path, "%s/deep_flush", region_base);
> +   region->flush_fd = open(path, O_RDWR | O_CLOEXEC);
> +   if (region->flush_fd == -1) {
> +   /* for those that do not export deep_flush sysfs file */
> +   if (errno == ENOENT)

Hmm, what about EPERM? For example, it should be fine for non-root
users (who can't open this file as writable) can still list regions.
So, I think lets kill all "goto err_read" for this thing because
->flush_fd will be -1 and the user will discover that fact if they
actually try to trigger the flush. Otherwise pure "ndctl list" users
should not see any failures to open this file as writable.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Returned mail: Data format error

[Mail Administrator]

The original message was received at Wed, 11 Apr 2018 11:29:58 +0800
from lists.01.org [3.200.103.101]

- The following addresses had permanent fatal errors -




___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH v2] ndctl: complete move to "fsdax" and "devdax"

[Verma, Vishal L]

On Tue, 2018-04-10 at 11:09 -0600, Ross Zwisler wrote:
> Add on to the work started by:
> 
> commit ebb4fb605e68 ("ndctl, create-namespace: introduce "fsdax" and
> "devdax" modes")
> 
> and change some more user visible places to use "fsdax" and "devdax"
> modes
> instead of "memory" and "dax", respectively.  Having multiple terms for
> the
> same mode is confusing for users.
> 
> We will continue to accept "memory" and "dax" as parameters, but all
> output
> and man pages will now use the updated terms.
> 
> Note that after the above referenced commit we still printed the old
> names
> in the default 'ndctl list' output for backward compatibility with
> scripts.
> This patch intentionally breaks that backward compatibility in favor of
> avoiding confusion and using the new mode names everywhere.
> 
> Signed-off-by: Ross Zwisler 
> ---
>  Documentation/ndctl/ndctl-inject-error.txt |  2 +-
>  Documentation/ndctl/ndctl-list.txt |  6 +++---
>  ndctl/namespace.c  | 16 
>  util/json.c| 10 ++
>  4 files changed, 14 insertions(+), 20 deletions(-)

This also needs unit test updates to create.sh and clear.sh as they look
for 'memory'

> 
> diff --git a/Documentation/ndctl/ndctl-inject-error.txt
> b/Documentation/ndctl/ndctl-inject-error.txt
> index 01f6c22..94c4e69 100644
> --- a/Documentation/ndctl/ndctl-inject-error.txt
> +++ b/Documentation/ndctl/ndctl-inject-error.txt
> @@ -45,7 +45,7 @@ OPTIONS
>  
>   NOTE: The offset is interpreted in different ways based on the
> "mode"
>   of the namespace. For "raw" mode, the offset is the base
> namespace
> - offset. For "memory" mode (i.e. a "pfn" namespace), the offset
> is
> + offset. For "fsdax" mode (i.e. a "pfn" namespace), the offset is
>   relative to the user-visible part of the namespace, and the
> offset
>   introduced by the kernel's metadata will be accounted for. For a
>   "sector" mode namespace (i.e. a "BTT" namespace), the offset is
> diff --git a/Documentation/ndctl/ndctl-list.txt
> b/Documentation/ndctl/ndctl-list.txt
> index 04affc4..2abc572 100644
> --- a/Documentation/ndctl/ndctl-list.txt
> +++ b/Documentation/ndctl/ndctl-list.txt
> @@ -49,7 +49,7 @@ EXAMPLE
>"namespaces":[
>  {
>"dev":"namespace0.0",
> -  "mode":"memory",
> +  "mode":"fsdax",
>"size":8589934592,
>"blockdev":"pmem0"
>  }
> @@ -132,11 +132,11 @@ include::xable-region-options.txt[]
>  -X::
>  --device-dax::
>   Include device-dax ("daxregion") details when a namespace is in
> - "dax" mode.
> + "devdax" mode.
>  [verse]
>  {
>"dev":"namespace0.0",
> -  "mode":"dax",
> +  "mode":"devdax",
>"size":4225761280,
>"uuid":"18ae1bbb-bb62-4efc-86df-4a5caacb5dcc",
>"daxregion":{
> diff --git a/ndctl/namespace.c b/ndctl/namespace.c
> index f2c5644..fe86d82 100644
> --- a/ndctl/namespace.c
> +++ b/ndctl/namespace.c
> @@ -103,7 +103,7 @@ OPT_STRING('n', "name", , "name", \
>  OPT_STRING('s', "size", , "size", \
>   "specify the namespace size in bytes (default: available
> capacity)"), \
>  OPT_STRING('m', "mode", , "operation-mode", \
> - "specify a mode for the namespace, 'sector', 'memory', or
> 'raw'"), \
> + "specify a mode for the namespace, 'sector', 'fsdax', 'devdax'
> or 'raw'"), \
>  OPT_STRING('M', "map", , "memmap-location", \
>   "specify 'mem' or 'dev' for the location of the memmap"), \
>  OPT_STRING('l', "sector-size", _size, "lba-size", \
> @@ -533,7 +533,7 @@ static int validate_namespace_options(struct
> ndctl_region *region,
>* supported a 2M default alignment when
>* ndctl_pfn_has_align() returns false.
>*/
> - debug("%s not support 'align' for memory
> mode\n",
> + debug("%s not support 'align' for fsdax mode\n",
>   region_name);
>   return -EAGAIN;
>   } else if (p->mode == NDCTL_NS_MODE_DAX
> @@ -542,7 +542,7 @@ static int validate_namespace_options(struct
> ndctl_region *region,
>* Unlike the pfn case, we require the kernel to
>* have 'align' support for device-dax.
>*/
> - debug("%s not support 'align' for dax mode\n",
> + debug("%s not support 'align' for devdax
> mode\n",
>   region_name);
>   return -EAGAIN;
>   } else if (!param.align_default
> @@ -696,7 +696,7 @@ static int validate_namespace_options(struct
> ndctl_region *region,
>  
>   if (ndns && p->mode != NDCTL_NS_MODE_MEMORY
>   && p->mode != NDCTL_NS_MODE_DAX) {
> - debug("%s: --map= only valid for memory mode
> namespace\n",
> + debug("%s: --map= only 

[PATCH v4] ndctl: add support in libndctl to provide deep flush

[Dave Jiang]

Providing an API call in libndctl to support accessing the region deep_flush
in sysfs.

Signed-off-by: Dave Jiang 
---

v4: Make setup of deep_flush open not interfere with add_region. (Dan)

v3:
- Add "\n" to sysfs write. (Dan)
- add O_CLOEXEC to open() call for sysfs. (Dan)

v2: Cover case where deep_flush doesn't exist, i.e. memmap=nn!ss. (Dan)

 ndctl/lib/libndctl.c   |   31 +++
 ndctl/lib/libndctl.sym |1 +
 ndctl/libndctl.h   |1 +
 3 files changed, 33 insertions(+)

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index 580a450e..5d91d34e 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -181,6 +181,8 @@ struct ndctl_region {
FILE *badblocks;
struct badblock bb;
enum ndctl_persistence_domain persistence_domain;
+   /* file descriptor for deep flush sysfs entry */
+   int flush_fd;
 };
 
 /**
@@ -511,6 +513,8 @@ static void free_region(struct ndctl_region *region)
free(region->region_path);
if (region->badblocks)
fclose(region->badblocks);
+   if (region->flush_fd > 0)
+   close(region->flush_fd);
free(region);
 }
 
@@ -1049,6 +1053,14 @@ NDCTL_EXPORT unsigned long long 
ndctl_region_get_resource(struct ndctl_region *r
return strtoull(buf, NULL, 0);
 }
 
+NDCTL_EXPORT int ndctl_region_deep_flush(struct ndctl_region *region)
+{
+   int rc = pwrite(region->flush_fd, "1\n", 1, 0);
+
+   return (rc == -1) ? -errno : 0;
+}
+
+
 NDCTL_EXPORT const char *ndctl_bus_get_cmd_name(struct ndctl_bus *bus, int cmd)
 {
return nvdimm_bus_cmd_name(cmd);
@@ -1791,6 +1803,7 @@ static void *add_region(void *parent, int id, const char 
*region_base)
struct ndctl_bus *bus = parent;
struct ndctl_ctx *ctx = bus->ctx;
char *path = calloc(1, strlen(region_base) + 100);
+   int perm;
 
if (!path)
return NULL;
@@ -1866,6 +1879,24 @@ static void *add_region(void *parent, int id, const char 
*region_base)
else
region->persistence_domain = region_get_pd_type(buf);
 
+   sprintf(path, "%s/deep_flush", region_base);
+   region->flush_fd = open(path, O_RDWR | O_CLOEXEC);
+   if (region->flush_fd == -1)
+   goto out;
+
+   if (pread(region->flush_fd, buf, 1, 0) == -1) {
+   close(region->flush_fd);
+   region->flush_fd = -1;
+   goto out;
+   }
+
+   perm = strtol(buf, NULL, 0);
+   if (perm == 0) {
+   region->flush_fd = -1;
+   close(region->flush_fd);
+   }
+
+ out:
free(path);
return region;
 
diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
index 3209aefe..38cc3b9a 100644
--- a/ndctl/lib/libndctl.sym
+++ b/ndctl/lib/libndctl.sym
@@ -352,4 +352,5 @@ global:
ndctl_dimm_fw_update_supported;
ndctl_region_get_persistence_domain;
ndctl_bus_get_persistence_domain;
+   ndctl_region_deep_flush;
 } LIBNDCTL_14;
diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
index cf6a77fd..1a622ae5 100644
--- a/ndctl/libndctl.h
+++ b/ndctl/libndctl.h
@@ -365,6 +365,7 @@ int ndctl_region_enable(struct ndctl_region *region);
 int ndctl_region_disable_invalidate(struct ndctl_region *region);
 int ndctl_region_disable_preserve(struct ndctl_region *region);
 void ndctl_region_cleanup(struct ndctl_region *region);
+int ndctl_region_deep_flush(struct ndctl_region *region);
 
 struct ndctl_interleave_set;
 struct ndctl_interleave_set *ndctl_region_get_interleave_set(

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm