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 000000000000..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 and/or modify it
> + * under the terms and conditions of the GNU Lesser General Public License,
> + * version 2.1, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT ANY
> + * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
> + * FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for
> + * more details.
> + */
Prefer SPDX license header instead fo the long form text for new files.
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
