Re: [PATCH v10 20/22] cxl: Store QTG IDs and related info to the CXL memory device context

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

On 10/11/23 06:19, Jonathan Cameron wrote:
> On Tue, 10 Oct 2023 18:06:59 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Once the QTG ID _DSM is executed successfully, the QTG ID is retrieved from
>> the return package. Create a list of entries in the cxl_memdev context and
>> store the QTG ID as qos_class token and the associated DPA range. This
>> information can be exposed to user space via sysfs in order to help region
>> setup for hot-plugged CXL memory devices.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>
>> ---
>> v10:
>> - Store single qos_class value. (Dan)
> 
> I'm fine with doing this, but I'd like a print if more than one is
> provided by the DSMAS (e.g. multiple DSMAS entries for a given region)
> Mostly so we have something to let us know if anyone is shipping such a
> device.
> 
> Otherwise a couple of minor comments inline.
> 
> Jonathan
> 
> 
>> - Rename cxl_memdev_set_qtg() to cxl_memdev_set_qos_class()
>> - Removed Jonathan's review tag due to code changes.
> 
>> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
>> index 99a619360bc5..049a16b7eb1f 100644
>> --- a/drivers/cxl/port.c
>> +++ b/drivers/cxl/port.c
>> @@ -105,6 +105,40 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
>>  	return 0;
>>  }
>>  
>> +static void cxl_memdev_set_qos_class(struct cxl_dev_state *cxlds,
>> +				     struct list_head *dsmas_list)
>> +{
>> +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
>> +	struct range pmem_range = {
>> +		.start = cxlds->pmem_res.start,
>> +		.end = cxlds->pmem_res.end,
>> +	};
>> +	struct range ram_range = {
>> +		.start = cxlds->ram_res.start,
>> +		.end = cxlds->ram_res.end,
>> +	};
>> +	struct perf_prop_entry *perf;
>> +	struct dsmas_entry *dent;
>> +
>> +	list_for_each_entry(dent, dsmas_list, list) {
>> +		perf = devm_kzalloc(cxlds->dev, sizeof(*perf), GFP_KERNEL);
>> +		if (!perf)
>> +			return;
>> +
>> +		perf->dpa_range = dent->dpa_range;
>> +		perf->coord = dent->coord;
>> +		perf->qos_class = dent->qos_class;
>> +		list_add_tail(&perf->list, &mds->perf_list);
>> +
>> +		if (resource_size(&cxlds->ram_res) &&
>> +		    range_contains(&ram_range, &dent->dpa_range))
>> +			mds->ram_qos_class = perf->qos_class;
> 
> So this assumes one DSMAS per memory type.
> Not an unreasonable starting place, but I think this should 
> print something to the log if it does see more that one.

I'll add that. Also I seem to have dropped the check from before for multiple entries so we aren't clobbering the previous set value.

> 
>> +		else if (resource_size(&cxlds->pmem_res) &&
>> +			 range_contains(&pmem_range, &dent->dpa_range))
>> +			mds->pmem_qos_class = perf->qos_class;
>> +	}
>> +}
>> +
>>  static int cxl_switch_port_probe(struct cxl_port *port)
>>  {
>>  	struct cxl_hdm *cxlhdm;
>> @@ -201,6 +235,8 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
>>  			if (rc)
>>  				dev_dbg(&port->dev,
>>  					"Failed to do perf coord calculations.\n");
>> +			else
>> +				cxl_memdev_set_qos_class(cxlds, &dsmas_list);
> 
> This is getting a bit deeply nested.  Perhaps a follow up patch to factor
> it out makes sense so we can use a goto to cleanup the dmsmas_list without
> that label being nested as well.

I'll just fix it in place. It doesn't look too bad.

> 
>>  		}
>>  
>>  		cxl_cdat_dsmas_list_destroy(&dsmas_list);
>>
>>
>