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: >> 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/[email protected] >> >> [2] https://en.wikipedia.org/wiki/Open_Firmware >> >> [3] Documentation/ABI/testing/sysfs-bus-papr-pmem >> https://lore.kernel.org/linux-nvdimm/[email protected] > > Not a huge deal, but the lore links can probably be updated to the > latest posting. > >> >> Signed-off-by: Vaibhav Jain <[email protected]> >> --- >> > [..] > >> 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 -- [email protected] To unsubscribe send an email to [email protected]
