Re: [ndctl PATCH v4 08/17] libcxl: add support for the 'GET_LSA' command

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Thu, 2021-10-14 at 09:35 -0700, Dan Williams wrote:
> On Thu, Oct 7, 2021 at 1:22 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
> > 
> > Add a command allocator and accessor APIs for the 'GET_LSA' mailbox
> > command.
> > 
> > Cc: Ben Widawsky <ben.widawsky@intel.com>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  cxl/lib/private.h  |  5 +++++
> >  cxl/lib/libcxl.c   | 36 ++++++++++++++++++++++++++++++++++++
> >  cxl/libcxl.h       |  7 +++----
> >  cxl/lib/libcxl.sym |  4 ++--
> >  4 files changed, 46 insertions(+), 6 deletions(-)
> > 
> > diff --git a/cxl/lib/private.h b/cxl/lib/private.h
> > index f76b518..9c6317b 100644
> > --- a/cxl/lib/private.h
> > +++ b/cxl/lib/private.h
> > @@ -73,6 +73,11 @@ struct cxl_cmd_identify {
> >         u8 qos_telemetry_caps;
> >  } __attribute__((packed));
> > 
> > +struct cxl_cmd_get_lsa_in {
> > +       le32 offset;
> > +       le32 length;
> > +} __attribute__((packed));
> > +
> >  struct cxl_cmd_get_health_info {
> >         u8 health_status;
> >         u8 media_status;
> > diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> > index 413be9c..33cc462 100644
> > --- a/cxl/lib/libcxl.c
> > +++ b/cxl/lib/libcxl.c
> > @@ -1028,6 +1028,42 @@ CXL_EXPORT struct cxl_cmd *cxl_cmd_new_raw(struct cxl_memdev *memdev,
> >         return cmd;
> >  }
> > 
> > +CXL_EXPORT struct cxl_cmd *cxl_cmd_new_read_label(struct cxl_memdev *memdev,
> > +               unsigned int offset, unsigned int length)
> > +{
> > +       struct cxl_cmd_get_lsa_in *get_lsa;
> > +       struct cxl_cmd *cmd;
> > +
> > +       cmd = cxl_cmd_new_generic(memdev, CXL_MEM_COMMAND_ID_GET_LSA);
> > +       if (!cmd)
> > +               return NULL;
> > +
> > +       get_lsa = (void *)cmd->send_cmd->in.payload;
> 
> Any reason that @payload is not already a 'void *' to avoid this casting?

The send_cmd is part of the uapi which defined it as __u64.

> 
> Other than that this looks good to me.
> 
> You can add:
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>