Messages in this thread | | | From | Vaibhav Jain <> | Subject | Re: [PATCH v11 5/6] ndctl/papr_scm,uapi: Add support for PAPR nvdimm specific methods | Date | Tue, 09 Jun 2020 23:23:14 +0530 |
| |
Thanks Dan for the consideration and taking time to look into this.
My responses below:
Dan Williams <dan.j.williams@intel.com> writes:
> On Mon, Jun 8, 2020 at 5:16 PM kernel test robot <lkp@intel.com> wrote: >> >> Hi Vaibhav, >> >> Thank you for the patch! Perhaps something to improve: >> >> [auto build test WARNING on powerpc/next] >> [also build test WARNING on linus/master v5.7 next-20200605] >> [cannot apply to linux-nvdimm/libnvdimm-for-next scottwood/next] >> [if your patch is applied to the wrong git tree, please drop us a note to help >> improve the system. BTW, we also suggest to use '--base' option to specify the >> base tree in git format-patch, please see https://stackoverflow.com/a/37406982] >> >> url: https://github.com/0day-ci/linux/commits/Vaibhav-Jain/powerpc-papr_scm-Add-support-for-reporting-nvdimm-health/20200607-211653 >> base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next >> config: powerpc-randconfig-r016-20200607 (attached as .config) >> compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project e429cffd4f228f70c1d9df0e5d77c08590dd9766) >> reproduce (this is a W=1 build): >> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross >> chmod +x ~/bin/make.cross >> # install powerpc cross compiling tool for clang build >> # apt-get install binutils-powerpc-linux-gnu >> # save the attached .config to linux build tree >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc >> >> If you fix the issue, kindly add following tag as appropriate >> Reported-by: kernel test robot <lkp@intel.com> >> >> All warnings (new ones prefixed by >>, old ones prefixed by <<): >> >> In file included from <built-in>:1: >> >> ./usr/include/asm/papr_pdsm.h:69:20: warning: field 'hdr' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end] >> struct nd_cmd_pkg hdr; /* Package header containing sub-cmd */ > > Hi Vaibhav, > [.] > This looks like it's going to need another round to get this fixed. I > don't think 'struct nd_pdsm_cmd_pkg' should embed a definition of > 'struct nd_cmd_pkg'. An instance of 'struct nd_cmd_pkg' carries a > payload that is the 'pdsm' specifics. As the code has it now it's > defined as a superset of 'struct nd_cmd_pkg' and the compiler warning > is pointing out a real 'struct' organization problem. > > Given the soak time needed in -next after the code is finalized this > there's no time to do another round of updates and still make the v5.8 > merge window.
Agreed that this looks bad, a solution will probably need some more review cycles resulting in this series missing the merge window.
I am investigating into the possible solutions for this reported issue and made few observations:
I see command pkg for Intel, Hpe, Msft and Hyperv families using a similar layout of embedding nd_cmd_pkg at the head of the command-pkg. struct nd_pdsm_cmd_pkg is following the same pattern.
struct nd_pdsm_cmd_pkg { struct nd_cmd_pkg hdr; /* other members */ };
struct ndn_pkg_msft { struct nd_cmd_pkg gen; /* other members */ }; struct nd_pkg_intel { struct nd_cmd_pkg gen; /* other members */ }; struct ndn_pkg_hpe1 { struct nd_cmd_pkg gen; /* other members */ };
Even though other command families implement similar command-package layout they were not flagged (yet) as they are (I am guessing) serviced in vendor acpi drivers rather than in kernel like in case of papr-scm command family.
So, I think this issue is not just specific to papr-scm command family introduced in this patch series but rather across all other command families. Every other command family assumes 'struct nd_cmd_pkg_hdr' to be embeddable and puts it at the beginning of their corresponding command-packages. And its only a matter of time when someone tries filtering/handling of vendor specific commands in nfit module when they hit similar issue.
Possible Solutions:
* One way would be to redefine 'struct nd_cmd_pkg' to mark field 'nd_payload[]' from a flexible array to zero sized array as 'nd_payload[0]'. This should make 'struct nd_cmd_pkg' embeddable and clang shouldn't report 'gnu-variable-sized-type-not-at-end' warning. Also I think this change shouldn't introduce any ABI change. * Another way to solve this issue might be to redefine 'struct nd_pdsm_cmd_pkg' to below removing the 'struct nd_cmd_pkg' member. This struct should immediately follow the 'struct nd_cmd_pkg' command package when sent to libnvdimm:
struct nd_pdsm_cmd_pkg { __s32 cmd_status; /* Out: Sub-cmd status returned back */ __u16 reserved[2]; /* Ignored and to be used in future */ __u8 payload[]; };
This should remove the flexible member nc_cmd_pkg.nd_payload from the struct with just one remaining at the end. However this would make accessing the [in|out|fw]_size members of 'struct nd_cmd_pkg' difficult for the pdsm servicing functions.
Any other solution that you think, may solve this issue ?
Thanks, -- ~ Vaibhav
| |