Hi Mpe, Thanks for looking into this patch.
Michael Ellerman <m...@ellerman.id.au> writes: > Shivaprasad G Bhat <sb...@linux.ibm.com> writes: >> papr_scm and ndtest share common PDSM payload structs like >> nd_papr_pdsm_health. Presently these structs are duplicated across >> papr_pdsm.h >> and ndtest.h header files. Since 'ndtest' is essentially arch independent >> and can >> run on platforms other than PPC64, a way needs to be deviced to avoid >> redundancy >> and duplication of PDSM structs in future. >> >> So the patch proposes moving the PDSM header from arch/powerpc/include/uapi/ >> to >> the generic include/uapi/linux directory. Also, there are some #defines >> common >> between papr_scm and ndtest which are not exported to the user space. So, >> move >> them to a header file which can be shared across ndtest and papr_scm via >> newly >> introduced include/linux/papr_scm.h. >> >> Signed-off-by: Shivaprasad G Bhat <sb...@linux.ibm.com> >> Signed-off-by: Vaibhav Jain <vaib...@linux.ibm.com> >> Suggested-by: "Aneesh Kumar K.V" <aneesh.ku...@linux.ibm.com> >> --- >> Changelog: >> >> Since v1: >> Link: >> https://patchwork.kernel.org/project/linux-nvdimm/patch/162505488483.72147.12741153746322191381.stgit@56e104a48989/ >> * Removed dependency on this patch for the other patches >> >> MAINTAINERS | 2 >> arch/powerpc/include/uapi/asm/papr_pdsm.h | 165 >> ----------------------------- >> arch/powerpc/platforms/pseries/papr_scm.c | 43 -------- >> include/linux/papr_scm.h | 48 ++++++++ >> include/uapi/linux/papr_pdsm.h | 165 >> +++++++++++++++++++++++++++++ > > This doesn't make sense to me. > > Anything with papr (or PAPR) in the name is fundamentally powerpc > specific, it doesn't belong in a generic header, or in a generic > location. > > What's the actual problem you're trying to solve? > The ndtest module (tools/testing/nvdimm/test/ndtest.c) is implemented in an arch independed way to enable testing of PAPR PDSMs on non ppc64 platforms like x86_64. It uses the same PDSM structs as used by papr_scm to communicate with libndctl userspace. Since papr_scm is ppc64 arch specific we were so far duplicating the PDSM structures between ndtest and papr_scm. The patch tries to solve this duplication by moving the shared structs to arch independent common include dirs. Secondly, PDSMs describes how userspace can use NVDIMM_FAMILY_PAPR to interact with NVDIMMs. So potentially a new NVDIMM beyond powerpc arch can add its support and would need access to the same structs used by papr_scm and ndtest. In that context it would make sense to move PDSM headers to generic include dirs. > If it's just including papr_scm bits into ndtest.c then that should be > as simple as: > > #ifdef __powerpc__ > #include <asm/papr_scm.h> > #endif > > Shouldn't it? > No, as ndtest implements support for NVDIMM_FAMILY_PAPR and would need access to PDSM related structs which presently are only available for powerpc. > cheers > -- Cheers ~ Vaibhav
