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

["Verma, Vishal L" <vishal.l.verma@intel.com>]

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,
> +};
> 
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org