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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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.

> +		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.

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