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 <[email protected]>
> ---
> 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)
<snip>
> + 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)
<snip>
> 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:<number>" 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(&bus->dimms, &dimm->list);
> free(path);
>
_______________________________________________
Linux-nvdimm mailing list -- [email protected]
To unsubscribe send an email to [email protected]
