Re: [ndctl PATCH v6 2/5] libncdtl: Add initial support for NVDIMM_FAMILY_PAPR nvdimm family

[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:
>> Add necessary scaffolding in libndctl for dimms that support papr_scm
>
> support /the/ papr_scm specification
>
>> specification[1]. Since there can be platforms that support
>> Open-Firmware[2] but not the papr_scm specification, hence the changes
>
> s/hence//
>
>> proposed first add support for probing if the dimm bus supports
>> Open-Firmware. This is done via querying for sysfs attribute 'of_node'
>
> This is done /by/ querying for /the/ sysfs attribute
>
>> in dimm device sysfs directory. If available newly introduced member
>> 'struct ndctl_bus.has_of_node' is set. During the probe of the dimm
>> and execution of add_dimm(), the newly introduced add_papr_dimm()
>
> During 'add_dimm()', the newly introduced..
>
>> is called if dimm bus reports supports Open-Firmware.
>> 
>> Function add_papr_dimm() queries the 'compatible' device tree
>> attribute via newly introduced ndctl_bus_is_papr_scm() and based on
>> its value assign NVDIMM_FAMILY_PAPR to the dimm command family. In
>
> based on its value, assigns NVDIM_..
>
>> future, based on the contents of 'compatible' attribute more of_pmem
>
> In /the/ future
>
>> dimm families can be queried.
>> 
>> We also add support for parsing the dimm flags for
>
> 'We' can be ambiguous. Say something like: "Additionally, add support.."
>
>> NVDIMM_FAMILY_PAPR supporting nvdimms as described at [3]. A newly
>> introduced function parse_papr_flags() reads the contents of this
>> flag file and sets appropriate flag bits in 'struct
>> ndctl_dimm.flags'.
>> 
>> Also we advertise support for monitor mode by allocating a file
>
> "Advertise support for monitor mode.."
>
>> descriptor to the dimm 'flags' file and assigning it to 'struct
>> ndctl_dimm.health_event_fd'.
>> 
>> The dimm-ops implementation for NVDIMM_FAMILY_PAPR is
>> available in global variable 'papr_dimm_ops' which points to
>> skeleton implementation in newly introduced file 'lib/papr.c'.
>
> This paragraph can just be dropped - it's a minor implementation detail,
> and doesn't add much to the commit message. The same actually goes for
> the part above that talks about setting flags.
>
>> 
>> References:
>> [1] Documentation/powerpc/papr_hcalls.rst
>> https://lore.kernel.org/linux-nvdimm/20200529214719.223344-2-vaibhav@linux.ibm.com
>> 
>> [2] https://en.wikipedia.org/wiki/Open_Firmware
>> 
>> [3] Documentation/ABI/testing/sysfs-bus-papr-pmem
>> https://lore.kernel.org/linux-nvdimm/20200529214719.223344-4-vaibhav@linux.ibm.com
>
> Not a huge deal, but the lore links can probably be updated to the
> latest posting.
>
>> 
>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>> ---
>> 
> [..]
>
>> diff --git a/ndctl/lib/papr.c b/ndctl/lib/papr.c
>> new file mode 100644
>> index 000000000000..4b6ce8beccab
>> --- /dev/null
>> +++ b/ndctl/lib/papr.c
>> @@ -0,0 +1,22 @@
>> +// SPDX-License-Identifier: LGPL-2.1
>
> I'm not sure if you intended to drop the copyright line here :)
>
>> +
>> +#include <stdint.h>
>> +#include <stdlib.h>
>> +#include <limits.h>
>> +#include <util/log.h>
>> +#include <ndctl.h>
>> +#include <ndctl/libndctl.h>
>> +#include <lib/private.h>
>> +
>> +static bool papr_cmd_is_supported(struct ndctl_dimm *dimm, int cmd)
>> +{
>> +	/* Handle this separately to support monitor mode */
>> +	if (cmd == ND_CMD_SMART)
>> +		return true;
>> +
>> +	return !!(dimm->cmd_mask & (1ULL << cmd));
>> +}
>> +
>> +struct ndctl_dimm_ops * const papr_dimm_ops = &(struct ndctl_dimm_ops) {
>> +	.cmd_is_supported = papr_cmd_is_supported,
>> +};
>> 

-- 
Cheers
~ Vaibhav
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org