linux-cxl.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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);
> >  }
> >  
> 

  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).