Re: [PATCH V2 2/3] cxl/mem: Account for partitionable space in ram/pmem ranges

[Jonathan Cameron <Jonathan.Cameron@Huawei.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);
>  }
>