Re: [PATCH 1/5] cxl/mem : Read Dynamic capacity configuration from the device

[Ira Weiny <ira.weiny@intel.com>]

Alison Schofield wrote:
> On Wed, Jun 14, 2023 at 12:16:28PM -0700, Ira Weiny wrote:
> > From: Navneet Singh <navneet.singh@intel.com>
> > 
> > Read the Dynamic capacity configuration and store dynamic capacity region
> > information in the device state which driver will use to map into the HDM
> > ranges.
> > 
> > Implement Get Dynamic Capacity Configuration (opcode 4800h) mailbox
> > command as specified in CXL 3.0 spec section 8.2.9.8.9.1.
> > 
> > Signed-off-by: Navneet Singh <navneet.singh@intel.com>
> > 
> > ---
> > [iweiny: ensure all mds->dc_region's are named]
> > ---

[snip]

> > @@ -666,6 +697,7 @@ static int cxl_xfer_log(struct cxl_memdev_state *mds, uuid_t *uuid,
> >  static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
> >  {
> >  	struct cxl_cel_entry *cel_entry;
> > +	struct cxl_mem_command *cmd;
> >  	const int cel_entries = size / sizeof(*cel_entry);
> >  	struct device *dev = mds->cxlds.dev;
> >  	int i;
> > @@ -674,11 +706,12 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
> >  
> >  	for (i = 0; i < cel_entries; i++) {
> >  		u16 opcode = le16_to_cpu(cel_entry[i].opcode);
> > -		struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
> > +		cmd = cxl_mem_find_command(opcode);
> 
> Is the move of the 'cmd' define related to this patch?
> Checkpatch warns on it: WARNING: Missing a blank line after declarations

That seems unneeded.  Perhaps left over from a previous version.  I've
moved it back.

> 
> >  
> > -		if (!cmd && !cxl_is_poison_command(opcode)) {
> > -			dev_dbg(dev,
> > -				"Opcode 0x%04x unsupported by driver\n", opcode);
> > +		if (!cmd && !cxl_is_poison_command(opcode) &&
> > +		    !cxl_is_dcd_command(opcode)) {
> > +			dev_dbg(dev, "Opcode 0x%04x unsupported by driver\n",
> > +				opcode);
> >  			continue;
> >  		}
> >  

[snip]

> > +
> > +	/*
> > +	 * Calculate entire DPA range of all configured regions which will be mapped by
> > +	 * one or more HDM decoders
> > +	 */
> 
> Comment is needlessly going >80 chars.

My checkpatch script is set to 100 lines due to the recent change.  But
this line length is unneeded here.  Thanks for noticing.

Fixed.

> 
> 
> > +	mds->total_dynamic_capacity =
> > +		mds->dc_region[mds->nr_dc_region - 1].base +
> > +		mds->dc_region[mds->nr_dc_region - 1].decode_len -
> > +		mds->dc_region[0].base;
> > +	dev_dbg(dev, "Total dynamic capacity: %#llx\n",
> > +		mds->total_dynamic_capacity);
> > +
> > +dc_error:
> > +	kvfree(dc);
> > +	return rc;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(cxl_dev_dynamic_capacity_identify, CXL);
> > +
> >  static int add_dpa_res(struct device *dev, struct resource *parent,
> >  		       struct resource *res, resource_size_t start,
> >  		       resource_size_t size, const char *type)
> > @@ -1112,6 +1275,11 @@ int cxl_mem_create_range_info(struct cxl_memdev_state *mds)
> >  	struct cxl_dev_state *cxlds = &mds->cxlds;
> >  	struct device *dev = cxlds->dev;
> >  	int rc;
> > +	size_t untenanted_mem =
> > +		mds->dc_region[0].base - mds->total_static_capacity;
> 
> Perhaps:
> 	size_t untenanted_mem;  (and put that in reverse x-tree order)
> 
> 	untenanted_mem = mds->dc_region[0].base - mds->total_static_capacity;

That looks good, fixed.

> 
> > +
> > +	mds->total_capacity = mds->total_static_capacity +
> > +			untenanted_mem + mds->total_dynamic_capacity;
> >  
> 
> Also, looking at this first patch with the long names, wondering if
> there is an opportunity to (re-)define these fields in fewers chars.
> Do we have to describe with 'total'? Is there a partial?
> 
> I guess I'll get to the defines further down...
> 
> 

[snip]

> > @@ -334,9 +349,12 @@ struct cxl_dev_state {
> >   *                (CXL 2.0 8.2.9.5.1.1 Identify Memory Device)
> >   * @mbox_mutex: Mutex to synchronize mailbox access.
> >   * @firmware_version: Firmware version for the memory device.
> > + * @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
> > + * @total_capacity: Sum of static and dynamic capacities
> > + * @total_static_capacity: Sum of RAM and PMEM capacities
> > + * @total_dynamic_capacity: Complete DPA range occupied by DC regions
> >   * @volatile_only_bytes: hard volatile capacity
> >   * @persistent_only_bytes: hard persistent capacity
> >   * @partition_align_bytes: alignment size for partition-able capacity
> > @@ -344,6 +362,10 @@ struct cxl_dev_state {
> >   * @active_persistent_bytes: sum of hard + soft persistent
> >   * @next_volatile_bytes: volatile capacity change pending device reset
> >   * @next_persistent_bytes: persistent capacity change pending device reset
> > + * @nr_dc_region: number of DC regions implemented in the memory device
> > + * @dc_region: array containing info about the DC regions
> > + * @dc_event_log_size: The number of events the device can store in the
> > + * Dynamic Capacity Event Log before it overflows
> >   * @event: event log driver state
> >   * @poison: poison driver state info
> >   * @mbox_send: @dev specific transport for transmitting mailbox commands
> > @@ -357,9 +379,13 @@ struct cxl_memdev_state {
> >  	size_t lsa_size;
> >  	struct mutex mbox_mutex; /* Protects device mailbox and firmware */
> >  	char firmware_version[0x10];
> > +	DECLARE_BITMAP(dcd_cmds, CXL_DCD_ENABLED_MAX);
> >  	DECLARE_BITMAP(enabled_cmds, CXL_MEM_COMMAND_ID_MAX);
> >  	DECLARE_BITMAP(exclusive_cmds, CXL_MEM_COMMAND_ID_MAX);
> > -	u64 total_bytes;
> > +
> > +	u64 total_capacity;
> > +	u64 total_static_capacity;
> > +	u64 total_dynamic_capacity;
> 
> maybe cap, static_cap, dynamic_cap

Since these are new it is probably better to use shorter names.  Also we
have good kdocs for each above.

> 
> (because I think I had a hand in defining the long names that
> follow and deeply regret it ;))

Well no one made a comment to correct them.  So remember:  "There is no
crying in CXL!"

[snip]

> > +/* See CXL 3.0 Table 125 get dynamic capacity config Output Payload */
> > +struct cxl_mbox_dynamic_capacity {
> > +	u8 avail_region_count;
> > +	u8 rsvd[7];
> > +	struct cxl_dc_region_config {
> > +		__le64 region_base;
> > +		__le64 region_decode_length;
> > +		__le64 region_length;
> > +		__le64 region_block_size;
> > +		__le32 region_dsmad_handle;
> > +		u8 flags;
> > +		u8 rsvd[3];
> > +	} __packed region[];
> > +} __packed;
> >  #define  CXL_SET_PARTITION_IMMEDIATE_FLAG	BIT(0)
> 
> This ^ goes with the cxl_mbox_set_partition_info above.
> Please don't split.

Oh yea that is bad.  Fixed.

Thanks for looking,
Ira