Re: [PATCH v3 1/3] cxl: Change 'struct cxl_memdev_state' *_perf_list to single 'struct cxl_dpa_perf'

[Dave Jiang <dave.jiang@intel.com>]

On 2/5/24 4:32 AM, Jonathan Cameron wrote:
> On Thu, 1 Feb 2024 14:47:29 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> In order to address the issue with being able to expose qos_class sysfs
>> attributes under 'ram' and 'pmem' sub-directories, the attributes must
>> be defined as static attributes rather than under driver->dev_groups.
>> To avoid implementing locking for accessing the 'struct cxl_dpa_perf`
>> lists, convert the list to a single 'struct cxl_dpa_perf' entry in
>> preparation to move the attributes to statically defined.
>>
>> While theoretically a partition may have multiple qos_class via CDAT, this
>> has not been encountered with testing on available hardware. The code is
>> simplified for now to not support the complex case until a use case is
>> needed to support that.
>>
>> Link: https://lore.kernel.org/linux-cxl/65b200ba228f_2d43c29468@dwillia2-mobl3.amr.corp.intel.com.notmuch/
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> A few comments inline.
> 
> Jonathan
> 
>> ---
>> v3:
>> - Add to commit log about simplification (Dan)
>> - Remove check for dev->driver (Dan)
>> - Remove check for invalid qos_class (Dan)
>> ---
>>  drivers/cxl/core/cdat.c | 81 ++++++++++++-----------------------------
>>  drivers/cxl/core/mbox.c |  4 +-
>>  drivers/cxl/cxlmem.h    | 10 ++---
>>  drivers/cxl/mem.c       | 28 ++------------
>>  4 files changed, 33 insertions(+), 90 deletions(-)
>>
>> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
>> index 6fe11546889f..55b82dfd794b 100644
>> --- a/drivers/cxl/core/cdat.c
> 
>>  static void cxl_qos_match(struct cxl_port *root_port,
>> -			  struct list_head *work_list,
>> -			  struct list_head *discard_list)
>> +			  struct cxl_dpa_perf *dpa_perf)
>>  {
>> -	struct cxl_dpa_perf *dpa_perf, *n;
>> +	int rc;
>>  
>> -	list_for_each_entry_safe(dpa_perf, n, work_list, list) {
>> -		int rc;
>> +	if (dpa_perf->qos_class == CXL_QOS_CLASS_INVALID)
>> +		return;
>>  
>> -		if (dpa_perf->qos_class == CXL_QOS_CLASS_INVALID)
>> -			return;
>> -
>> -		rc = device_for_each_child(&root_port->dev,
>> -					   (void *)&dpa_perf->qos_class,
>> -					   match_cxlrd_qos_class);
>> -		if (!rc)
>> -			list_move_tail(&dpa_perf->list, discard_list);
>> -	}
>> +	rc = device_for_each_child(&root_port->dev,
> 
> Over aggressive wrap.

Will fix

> 
>> +				   &dpa_perf->qos_class,
>> +				   match_cxlrd_qos_class);
>> +	if (!rc)
>> +		reset_dpa_perf(dpa_perf);
> 
> I'm not particularly keen on a function that on failure to match resets
> some internal state in one of it's inputs.
> Would prefer to see this return a bool then the caller decide to reset it.

Ok I'll change.
> 
>>  }
>>  
>>  static int match_cxlrd_hb(struct device *dev, void *data)
>> @@ -334,23 +312,10 @@ static int match_cxlrd_hb(struct device *dev, void *data)
>>  	return 0;
>>  }
>>  
> 
>>  static int cxl_qos_class_verify(struct cxl_memdev *cxlmd)
>>  {
>>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
>> -	LIST_HEAD(__discard);
>> -	struct list_head *discard __free(dpa_perf) = &__discard;
>>  	struct cxl_port *root_port;
>>  	int rc;
>>  
>> @@ -363,16 +328,16 @@ static int cxl_qos_class_verify(struct cxl_memdev *cxlmd)
>>  	root_port = &cxl_root->port;
>>  
>>  	/* Check that the QTG IDs are all sane between end device and root decoders */
>> -	cxl_qos_match(root_port, &mds->ram_perf_list, discard);
>> -	cxl_qos_match(root_port, &mds->pmem_perf_list, discard);
>> +	cxl_qos_match(root_port, &mds->ram_perf);
>> +	cxl_qos_match(root_port, &mds->pmem_perf);
>>  
>>  	/* Check to make sure that the device's host bridge is under a root decoder */
>>  	rc = device_for_each_child(&root_port->dev,
>>  				   (void *)cxlmd->endpoint->host_bridge,
> 
> Why is the explicit void * cast needed?  It's not removing const or anything like
> that so usual c rules on it being fine to implicitly cast to void * should apply.

I'll fix that in a different patch.

> 
> 
>>  				   match_cxlrd_hb);
>>  	if (!rc) {
>> -		list_splice_tail_init(&mds->ram_perf_list, discard);
>> -		list_splice_tail_init(&mds->pmem_perf_list, discard);
>> +		reset_dpa_perf(&mds->ram_perf);
>> +		reset_dpa_perf(&mds->pmem_perf);
>>  	}
>>  
>>  	return rc;