"Verma, Vishal L" <[email protected]> writes:
> 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"
Sure will get this updated in v6. This function definition was derived
from parse_dimm_flags() hence looks this way.
>
>> +}
>> +
>> 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).
Sure and thanks for pointing this out. Will add this symbol to the library
version script in v6
>
>> /**
>> * 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.
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.
>
>> +
>> +
>
> Nit: double newline here
Sure. Will fix this in v6
>
>> + 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
>
Sure will address this in v6.
> (In this case, what does the 'compatible' sysfs attrubute contain? Is it
> useful to print here? - I havent't looked, just asking).
For papr-scm compatible nvdimms:
# cat /sys/class/nd/ndctl0/device/of_node/compatible
ibm,pmemory
This devicetree property is usually in format "<manufacturer>,<model>" that
tells the which kernel module to bind to the device. The papr_scm
kernel module indicates support for such device value in its device
match table ( https://git.io/JfPzF ) so that kernel can call its
probe-function.
Dumping this value here will be useful in debugging probe failures of nvdimms
that are exposed via open-firmware devicetree nodes but are not
necessarily papr_scm specification compliant.
>
>> 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.
>
Sure will address this in v6.
--
Cheers
~ Vaibhav
_______________________________________________
Linux-nvdimm mailing list -- [email protected]
To unsubscribe send an email to [email protected]
