Hi Vishal, Thanks for reviewing this patch. I will be addressing your review comments in v7 of this patch series.
~ Vaibhav "Verma, Vishal L" <[email protected]> writes: > On Tue, 2020-06-16 at 11:00 +0530, Vaibhav Jain wrote: >> Presently add_dimm() only probes dimms that support NFIT/ACPI. Hence > > "...probes NVDIMMs for platforms that support the ACPI NFIT" > > The NFIT is a platform firmware thing, not directly related to the DIMMs > themselves. > >> this patch refactors this functionality into two functions namely > > s/Hence this patch refactors/Refactor/ > >> 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. > > No need to hyphenate 'thereby' > >> >> This patch shouldn't introduce any behavioral change. >> >> Signed-off-by: Vaibhav Jain <[email protected]> >> --- >> Changelog: >> >> v5..v6: >> * Changed a return code for add_nfit_dimm() in case a allocation >> failed. [ Vishal ] >> * Updated an error message in error path of add_dimm() [ Vishal ] >> >> v4..v5: >> * None >> >> v3..v4: >> * None >> >> v2..v3: >> * None >> >> v1..v2: >> * None >> --- >> ndctl/lib/libndctl.c | 196 +++++++++++++++++++++++++------------------ >> 1 file changed, 115 insertions(+), 81 deletions(-) >> > [..] > >> + >> + /* Assign dimm-ops based on command family */ >> + 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; >> out: >> + if (rc) { >> + /* Ensure dimm_uninit() is not called during free_dimm() */ >> + dimm->ops = NULL; > > I think the above assignment and comment are now stale.. > >> + 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); >> -- Cheers ~ Vaibhav _______________________________________________ Linux-nvdimm mailing list -- [email protected] To unsubscribe send an email to [email protected]
