Hi Vishal, Thanks for reviewing this patch. My responses below:
"Verma, Vishal L" <[email protected]> writes: > 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. If calloc fails, variable 'errno' will already be set to ENOMEM. Hence didn't explicitly return -ENOMEM. But fair suggestion will update this in v6. >> >> /* >> * '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)); > Fair suggestion. Will update this in v6. >> + goto err_read; >> + } >> + >> list_add(&bus->dimms, &dimm->list); >> free(path); >> -- Cheers ~ Vaibhav _______________________________________________ Linux-nvdimm mailing list -- [email protected] To unsubscribe send an email to [email protected]
