From: Ira Weiny <ira.weiny@intel.com>
To: Jonathan Cameron <Jonathan.Cameron@Huawei.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 09:30:35 -0700 [thread overview]
Message-ID: <20210618163035.GB1905674@iweiny-DESK2.sc.intel.com> (raw)
In-Reply-To: <20210618145902.00001dbe@Huawei.com>
On Fri, Jun 18, 2021 at 02:59:02PM +0100, Jonathan Cameron wrote:
> 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>
Thanks!
>
> >
> > ---
> > 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?
Ok... I keep saying this to myself because all the table values are in 256MB
increments so I just kept typing it!!! :-D
I'll remove the redundancies...
>
> > + * @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.
Sure...
>
> > + * 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.
Fair enough. I go back and forth depending on how the code looks.
All changed, v3 comming,
Ira
>
> > + 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 16:30 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
2021-06-18 16:30 ` Ira Weiny [this message]
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=20210618163035.GB1905674@iweiny-DESK2.sc.intel.com \
--to=ira.weiny@intel.com \
--cc=Jonathan.Cameron@Huawei.com \
--cc=alison.schofield@intel.com \
--cc=ben.widawsky@intel.com \
--cc=dan.j.williams@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).