On Thu, Jan 27, 2022 at 12:46 PM Alison Schofield <[email protected]> wrote: > > Hi Dan, > Thanks for the review. I'm still working thru this, but a clarifying > question below... > > On Wed, Jan 26, 2022 at 03:41:14PM -0800, Dan Williams wrote: > > On Tue, Jan 18, 2022 at 12:20 PM <[email protected]> wrote: > > > > > > From: Alison Schofield <[email protected]> > > > > > > Users may want the ability to change the partition layout of a CXL > > > memory device. > > > > > > Add interfaces to libcxl to allocate and send a SET_PARTITION_INFO > > > mailbox as defined in the CXL 2.0 specification. > > > > > > Signed-off-by: Alison Schofield <[email protected]> > > > --- > > > cxl/lib/libcxl.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++ > > > cxl/lib/libcxl.sym | 5 +++++ > > > cxl/lib/private.h | 8 ++++++++ > > > cxl/libcxl.h | 5 +++++ > > > 4 files changed, 68 insertions(+) > > > > snip > > > > > > I don't understand what this is for? > > > > Let's back up. In order to future proof against spec changes, and > > endianness, struct packing and all other weird things that make struct > > ABIs hard to maintain compatibility the ndctl project adopts the > > libabc template of just not letting library consumers see any raw data > > structures or bit fields by default [1]. For a situation like this > > since the command only has one flag that affects the mode of operation > > I would just go ahead and define an enum for that explicitly. > > > > enum cxl_setpartition_mode { > > CXL_SETPART_NONE, > > CXL_SETPART_NEXTBOOT, > > CXL_SETPART_IMMEDIATE, > > }; > > > > Then the main function prototype becomes: > > > > int cxl_cmd_new_setpartition(struct cxl_memdev *memdev, unsigned long > > long volatile_capacity); > > > > ...with a new: > > > > int cxl_cmd_setpartition_set_mode(struct cxl_cmd *cmd, enum > > cxl_setpartition_mode mode); > > > > I don't understand setting of the mode separately. Can it be: > > int cxl_cmd_new_setpartition(struct cxl_memdev *memdev, > unsigned long long volatile_capacity, > enum cxl_setpartition_mode mode);
It could be, but what happens when the specification defines a new flag for this command? Then we would have cxl_cmd_new_setpartition() and cxl_cmd_new_setpartition2() to add the new parameters. A helper function after establishing the cxl_cmd context lets you have flexibility to extend the base command by as many new flags and modes that come along... hopefully none, but you never know.
