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

From: Dan Williams <dan.j.williams@intel.com>
To: "Schofield, Alison" <alison.schofield@intel.com>
Cc: Ben Widawsky <ben.widawsky@intel.com>,
	Ira Weiny <ira.weiny@intel.com>,
	 Vishal Verma <vishal.l.verma@intel.com>,
	Linux NVDIMM <nvdimm@lists.linux.dev>,
	 linux-cxl@vger.kernel.org
Subject: Re: [ndctl PATCH v4 1/6] libcxl: add GET_PARTITION_INFO mailbox command and accessors
Date: Tue, 8 Feb 2022 12:20:56 -0800	[thread overview]
Message-ID: <CAPcyv4gYbLGkd99fvKixAmLAhpkfQU8gNJ0e0BHrmVUZ3wWA_A@mail.gmail.com> (raw)
In-Reply-To: <396ccc39525b3eb829acd4e06f704f6fb57a94a8.1644271559.git.alison.schofield@intel.com>

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>