From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: <ira.weiny@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>,
Alison Schofield <alison.schofield@intel.com>,
Vishal Verma <vishal.l.verma@intel.com>,
"Ben Widawsky" <ben.widawsky@intel.com>,
<linux-cxl@vger.kernel.org>
Subject: Re: [PATCH V2 2/3] cxl/mem: Account for partitionable space in ram/pmem ranges
Date: Fri, 18 Jun 2021 14:59:02 +0100 [thread overview]
Message-ID: <20210618145902.00001dbe@Huawei.com> (raw)
In-Reply-To: <20210617221620.1904031-3-ira.weiny@intel.com>
On Thu, 17 Jun 2021 15:16:19 -0700
<ira.weiny@intel.com> wrote:
> From: Ira Weiny <ira.weiny@intel.com>
>
> Memory devices may specify volatile only, persistent only, and
> partitionable space which when added together result in a total capacity.
>
> If Identify Memory Device.Partition Alignment != 0 the device supports
> partitionable space. This partitionable space can be split between
> volatile and persistent space. The total volatile and persistent sizes
> are reported in Get Partition Info. ie
>
> active volatile memory = volatile only + partitionable volatile
> active persistent memory = persistent only + partitionable persistent
>
> Define cxl_mem_get_partition(), check for partitionable support, and use
> cxl_mem_get_partition() if applicable.
>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
Trivial editorial stuff inline.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> ---
> Changes for V2:
> Jonathan
> Account for Get Partition Info being optional
> Fix documentation
> Dan
> Store the active capacities in struct cxl_mem
> Make cxl_mem_get_partition() static until there is a
> need to export it.
> ---
> drivers/cxl/mem.h | 5 +++
> drivers/cxl/pci.c | 98 ++++++++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 98 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
> index 9dc34418db36..8d7bd966a298 100644
> --- a/drivers/cxl/mem.h
> +++ b/drivers/cxl/mem.h
> @@ -79,5 +79,10 @@ struct cxl_mem {
> u64 volatile_only_bytes;
> u64 persistent_only_bytes;
> u64 partition_align_bytes;
> +
> + u64 active_volatile_bytes;
> + u64 active_persistent_bytes;
> + u64 next_volatile_bytes;
> + u64 next_persistent_bytes;
> };
> #endif /* __CXL_MEM_H__ */
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 94b7ee08ef67..341885345b53 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1455,6 +1455,55 @@ static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_mem *cxlm)
> return ret;
> }
>
> +/**
> + * cxl_mem_get_partition_info - Get partition info
> + * @cxlm: The device to act on
> + * @active_volatile_bytes: returned active volatile capacity; in bytes
Perhaps over documented? Would be very very odd if _bytes attribute wasn't
in bytes so maybe don't need to state it in the comment?
> + * @active_persistent_bytes: returned active persistent capacity; in bytes
> + * @next_volatile_bytes: return next volatile capacity; in bytes
> + * @next_persistent_bytes: return next persistent capacity; in bytes
> + *
> + * Retrieve the current partition info for the device specified. The active
> + * values are the current capacity in bytes. If not 0, the 'next' values are
Definitely don't need to say they are in bytes again.
> + * the pending values, in bytes, which take affect on next cold reset.
> + *
> + * Return: 0 if no error: or the result of the mailbox command.
> + *
> + * See CXL @8.2.9.5.2.1 Get Partition Info
> + */
> +static int cxl_mem_get_partition_info(struct cxl_mem *cxlm,
> + u64 *active_volatile_bytes,
> + u64 *active_persistent_bytes,
> + u64 *next_volatile_bytes,
> + u64 *next_persistent_bytes)
> +{
> + struct cxl_mbox_get_partition_info {
> + u64 active_volatile_cap;
> + u64 active_persistent_cap;
> + u64 next_volatile_cap;
> + u64 next_persistent_cap;
> + } __packed pi;
> + int rc;
> +
> + rc = cxl_mem_mbox_send_cmd(cxlm, CXL_MBOX_OP_GET_PARTITION_INFO,
> + NULL, 0, &pi, sizeof(pi));
> +
In my view, slightly better to not have a blank line between statement
and error handling.
> + if (rc)
> + return rc;
> +
> + *active_volatile_bytes = le64_to_cpu(pi.active_volatile_cap);
> + *active_persistent_bytes = le64_to_cpu(pi.active_persistent_cap);
> + *next_volatile_bytes = le64_to_cpu(pi.next_volatile_cap);
> + *next_persistent_bytes = le64_to_cpu(pi.next_volatile_cap);
> +
> + *active_volatile_bytes *= CXL_CAPACITY_MULTIPLIER;
> + *active_persistent_bytes *= CXL_CAPACITY_MULTIPLIER;
> + *next_volatile_bytes *= CXL_CAPACITY_MULTIPLIER;
> + *next_persistent_bytes *= CXL_CAPACITY_MULTIPLIER;
> +
> + return 0;
> +}
> +
> /**
> * cxl_mem_enumerate_cmds() - Enumerate commands for a device.
> * @cxlm: The device.
> @@ -1573,18 +1622,53 @@ static int cxl_mem_identify(struct cxl_mem *cxlm)
> cxlm->persistent_only_bytes,
> cxlm->partition_align_bytes);
>
> + cxlm->lsa_size = le32_to_cpu(id.lsa_size);
> + memcpy(cxlm->firmware_version, id.fw_revision, sizeof(id.fw_revision));
> +
> + return 0;
> +}
> +
> +static int cxl_mem_create_range_info(struct cxl_mem *cxlm)
> +{
> + int rc;
> +
> + if (cxlm->partition_align_bytes == 0) {
> + cxlm->ram_range.start = 0;
> + cxlm->ram_range.end = cxlm->volatile_only_bytes - 1;
> + cxlm->pmem_range.start = 0;
> + cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1;
> + return 0;
> + }
> +
> + rc = cxl_mem_get_partition_info(cxlm,
> + &cxlm->active_volatile_bytes,
> + &cxlm->active_persistent_bytes,
> + &cxlm->next_volatile_bytes,
> + &cxlm->next_persistent_bytes);
> + if (rc < 0) {
> + dev_err(&cxlm->pdev->dev, "Failed to query partition information\n");
> + return rc;
> + }
> +
> + dev_dbg(&cxlm->pdev->dev, "Get Partition Info\n"
> + " active_volatile_bytes = %#llx\n"
> + " active_persistent_bytes = %#llx\n"
> + " next_volatile_bytes = %#llx\n"
> + " next_persistent_bytes = %#llx\n",
> + cxlm->active_volatile_bytes,
> + cxlm->active_persistent_bytes,
> + cxlm->next_volatile_bytes,
> + cxlm->next_persistent_bytes);
> +
> /*
> * TODO: enumerate DPA map, as 'ram' and 'pmem' do not alias.
> * For now, only the capacity is exported in sysfs
> */
> cxlm->ram_range.start = 0;
> - cxlm->ram_range.end = cxlm->volatile_only_bytes - 1;
> + cxlm->ram_range.end = cxlm->active_volatile_bytes - 1;
>
> cxlm->pmem_range.start = 0;
> - cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1;
> -
> - cxlm->lsa_size = le32_to_cpu(id.lsa_size);
> - memcpy(cxlm->firmware_version, id.fw_revision, sizeof(id.fw_revision));
> + cxlm->pmem_range.end = cxlm->active_persistent_bytes - 1;
>
> return 0;
> }
> @@ -1618,6 +1702,10 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> if (rc)
> return rc;
>
> + rc = cxl_mem_create_range_info(cxlm);
> + if (rc)
> + return rc;
> +
> return cxl_mem_add_memdev(cxlm);
> }
>
next prev parent reply other threads:[~2021-06-18 13:59 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-17 22:16 [PATCH V2 0/3] Query and use Partition Info ira.weiny
2021-06-17 22:16 ` [PATCH V2 1/3] cxl/pci: Store memory capacity values ira.weiny
2021-06-18 13:40 ` Jonathan Cameron
2021-06-17 22:16 ` [PATCH V2 2/3] cxl/mem: Account for partitionable space in ram/pmem ranges ira.weiny
2021-06-18 13:59 ` Jonathan Cameron [this message]
2021-06-18 16:30 ` Ira Weiny
2021-06-18 16:31 ` [PATCH V3] " ira.weiny
2021-06-18 16:58 ` Ben Widawsky
2021-06-18 18:48 ` Ira Weiny
2021-06-18 19:32 ` Ben Widawsky
2021-08-11 1:49 ` [PATCH v4 2/3] " Dan Williams
2021-06-17 22:16 ` [PATCH V2 3/3] cxl/mem: Adjust ram/pmem range to represent DPA ranges ira.weiny
2021-06-18 14:03 ` Jonathan Cameron
2021-06-21 19:54 ` [PATCH V3] " ira.weiny
2021-10-27 22:40 ` [PATCH 0/9] CDAT/DSMAS reading and cleanups ira.weiny
2021-10-27 22:40 ` [PATCH 1/9] Documentation/auxiliary_bus: Clarify auxiliary_device creation ira.weiny
2021-10-27 22:40 ` [PATCH 2/9] Documentation/auxiliary_bus: Clarify match_name ira.weiny
2021-10-27 22:40 ` [PATCH 3/9] Documentation/auxiliary_bus: Update Auxiliary device lifespan ira.weiny
2021-10-27 22:43 ` [PATCH 0/9] CDAT/DSMAS reading and cleanups Ira Weiny
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210618145902.00001dbe@Huawei.com \
--to=jonathan.cameron@huawei.com \
--cc=alison.schofield@intel.com \
--cc=ben.widawsky@intel.com \
--cc=dan.j.williams@intel.com \
--cc=ira.weiny@intel.com \
--cc=linux-cxl@vger.kernel.org \
--cc=vishal.l.verma@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.