Re: [ndctl PATCH v6 1/5] libndctl: Refactor out add_dimm() to handle NFIT specific init

[Vaibhav Jain <vaibhav@linux.ibm.com>]

Hi Vishal,

Thanks for reviewing this patch. I will be addressing your review
comments in v7 of this patch series.

~ Vaibhav

"Verma, Vishal L" <vishal.l.verma@intel.com> 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 <vaibhav@linux.ibm.com>
>> ---
>> 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 -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org