Re: [ndctl PATCH v4 1/6] libcxl: add GET_PARTITION_INFO mailbox command and accessors

[Alison Schofield <alison.schofield@intel.com>]

Thanks Dan! Got it all.
<eom>

On Tue, Feb 08, 2022 at 12:20:56PM -0800, Dan Williams wrote:
> On Mon, Feb 7, 2022 at 3:06 PM <alison.schofield@intel.com> wrote:
> >
> > From: Alison Schofield <alison.schofield@intel.com>
> >
> > Users need access to the CXL GET_PARTITION_INFO mailbox command
> > to inspect and confirm changes to the partition layout of a memory
> > device.
> >
> > Add libcxl APIs to create a new GET_PARTITION_INFO mailbox command,
> > the command output data structure (privately), and accessor APIs to
> > return the different fields in the partition info output.
> >
> > Per the CXL 2.0 specification, devices report partition capacities
> > as multiples of 256MB. Define and use a capacity multiplier to
> > convert the raw data into bytes for user consumption. Use byte
> > format as the norm for all capacity values produced or consumed
> > using CXL Mailbox commands.
> >
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  Documentation/cxl/lib/libcxl.txt |  1 +
> >  cxl/lib/libcxl.c                 | 57 ++++++++++++++++++++++++++++++++
> >  cxl/lib/libcxl.sym               |  5 +++
> >  cxl/lib/private.h                | 10 ++++++
> >  cxl/libcxl.h                     |  5 +++
> >  util/size.h                      |  1 +
> >  6 files changed, 79 insertions(+)
> >
> > diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt
> > index 4392b47..a6986ab 100644
> > --- a/Documentation/cxl/lib/libcxl.txt
> > +++ b/Documentation/cxl/lib/libcxl.txt
> > @@ -131,6 +131,7 @@ int cxl_memdev_read_label(struct cxl_memdev *memdev, void *buf, size_t length,
> >                           size_t offset);
> >  int cxl_memdev_write_label(struct cxl_memdev *memdev, void *buf, size_t length,
> >                            size_t offset);
> > +struct cxl_cmd *cxl_cmd_new_get_partition(struct cxl_memdev *memdev);
> >
> >  ----
> >
> > diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> > index e0b443f..33cf06b 100644
> > --- a/cxl/lib/libcxl.c
> > +++ b/cxl/lib/libcxl.c
> > @@ -1985,6 +1985,12 @@ static int cxl_cmd_validate_status(struct cxl_cmd *cmd, u32 id)
> >         return 0;
> >  }
> >
> > +static unsigned long long
> > +capacity_to_bytes(unsigned long long size)
> 
> If this helper converts an encoded le64 to bytes then the function
> signature should reflect that:
> 
> static uint64_t cxl_capacity_to_bytes(leint64_t size)
> 
> > +{
> > +       return le64_to_cpu(size) * CXL_CAPACITY_MULTIPLIER;
> > +}
> > +
> >  /* Helpers for health_info fields (no endian conversion) */
> >  #define cmd_get_field_u8(cmd, n, N, field)                             \
> >  do {                                                                   \
> > @@ -2371,6 +2377,57 @@ CXL_EXPORT ssize_t cxl_cmd_read_label_get_payload(struct cxl_cmd *cmd,
> >         return length;
> >  }
> >
> > +CXL_EXPORT struct cxl_cmd *cxl_cmd_new_get_partition(struct cxl_memdev *memdev)
> > +{
> > +       return cxl_cmd_new_generic(memdev,
> > +                                  CXL_MEM_COMMAND_ID_GET_PARTITION_INFO);
> > +}
> > +
> > +static struct cxl_cmd_get_partition *
> > +cmd_to_get_partition(struct cxl_cmd *cmd)
> > +{
> 
> This could also check for cmd == NULL just to be complete.
> 
> > +       if (cxl_cmd_validate_status(cmd, CXL_MEM_COMMAND_ID_GET_PARTITION_INFO))
> > +               return NULL;
> > +
> > +       return cmd->output_payload;
> > +}
> > +
> > +CXL_EXPORT unsigned long long
> > +cxl_cmd_partition_get_active_volatile_size(struct cxl_cmd *cmd)
> > +{
> > +       struct cxl_cmd_get_partition *c;
> > +
> > +       c = cmd_to_get_partition(cmd);
> > +       return c ? capacity_to_bytes(c->active_volatile) : ULLONG_MAX;
> 
> I'd prefer kernel coding style which wants:
> 
> if (!c)
>     return ULLONG_MAX;
> return cxl_capacity_to_bytes(c->active_volatile);
> 
> Otherwise, looks good to me:
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>