Re: [ndctl PATCH v5 2/6] libncdtl: Add initial support for NVDIMM_FAMILY_PAPR nvdimm family

["Verma, Vishal L" <vishal.l.verma@intel.com>]

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-leave@lists.01.org