Re: [PATCH 03/26] cxl/mem: Read dynamic capacity configuration from the device

[Jørgen Hansen]

On 4/5/24 20:09, Ira Weiny wrote:
> Jørgen Hansen wrote:
>> On 3/25/24 00:18, ira.weiny@xxxxxxxxx wrote:
>>
>>> From: Navneet Singh <navneet.singh@xxxxxxxxx>
>>>
> 
> [snip]
> 
>>>    /**
>>>     * struct cxl_memdev_state - Generic Type-3 Memory Device Class driver data
>>>     *
>>> @@ -467,6 +482,8 @@ struct cxl_dev_state {
>>>     * @enabled_cmds: Hardware commands found enabled in CEL.
>>>     * @exclusive_cmds: Commands that are kernel-internal only
>>>     * @total_bytes: sum of all possible capacities
>>> + * @static_cap: Sum of static RAM and PMEM capacities
>>> + * @dynamic_cap: Complete DPA range occupied by DC regions
>>
>> How about naming these total_range, static_cap and dynamic_range to make
>> it clear that the DPA range occupied by DC regions isn't necessarily
>> usable capacity (as opposed to the static_cap where the spec defines it
>> as usable capacity).
> 
> I thought this was a good idea but on second thought these are not range
> variables at all.  They really represent the various lengths of the
> resources.
> 
> For total_bytes the documentation already says 'sum of all __possible__
> capacities >
> I think you have a point for the new fields though.  They should all be
> named in some consistent manner and documented as such.
> 
> So I propose:
> 
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 94531af018f8..9c18b229f69a 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -481,9 +481,9 @@ struct cxl_dc_region_info {
>    * @dcd_cmds: List of DCD commands implemented by memory device
>    * @enabled_cmds: Hardware commands found enabled in CEL.
>    * @exclusive_cmds: Commands that are kernel-internal only
> - * @total_bytes: sum of all possible capacities
> - * @static_cap: Sum of static RAM and PMEM capacities
> - * @dynamic_cap: Complete DPA range occupied by DC regions
> + * @total_bytes: length of all possible capacities
> + * @static_bytes: length of possible static RAM and PMEM partitions
> + * @dynamic_bytes: length of possible DC partitions (DC Regions)
>    * @volatile_only_bytes: hard volatile capacity
>    * @persistent_only_bytes: hard persistent capacity
>    * @partition_align_bytes: alignment size for partition-able capacity
> @@ -515,8 +515,8 @@ struct cxl_memdev_state {
>          DECLARE_BITMAP(exclusive_cmds, CXL_MEM_COMMAND_ID_MAX);
> 
>          u64 total_bytes;
> -       u64 static_cap;
> -       u64 dynamic_cap;
> +       u64 static_bytes;
> +       u64 dynamic_bytes;
>          u64 volatile_only_bytes;
>          u64 persistent_only_bytes;
>          u64 partition_align_bytes;

That looks good. My main concern was that the DC regions may be 
separated by gaps that take up part of the DPA range but isn't part of 
the usable capacity. Pre-DCD, total_bytes was in fact all the usable 
capacity of the device as reported by the device itself, but now it 
includes the potential gaps between DC regions as well as the potential 
gap between static and dynamic regions.

Thanks,
Jørgen