Re: [PATCH v14 19/19] cxl: Check qos_class validity on memdev probe

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

On 12/19/23 09:42, Jonathan Cameron wrote:
> On Wed, 13 Dec 2023 09:43:16 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Add a check to make sure the qos_class for the device will match one of
>> the root decoders qos_class. If no match is found, then the qos_class for
>> the device is set to invalid. Also add a check to ensure that the device's
>> host bridge matches to one of the root decoder's downstream targets.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> A few comments on places I think this can be simplified.
> 
> Jonathan
> 
>> ---
>> v14:
>> - Move unmatched entries to discard list (Dan)
>> - Update perf_prop_entry to cxl_dpa_perf (Dan)
>> - Use DEFINE_FREE() in cxl_qos_class_verify to clean up code (Dan)
>> - Move qos verify to core/cdat.c in order to be called before region setup
>>   (Dan)
>> ---
>>  drivers/cxl/core/cdat.c |  137 +++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 137 insertions(+)
>>
>> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
>> index 8c561f1deec6..5fe57fe5e2ee 100644
>> --- a/drivers/cxl/core/cdat.c
>> +++ b/drivers/cxl/core/cdat.c
>> @@ -270,6 +270,142 @@ static void cxl_memdev_set_qos_class(struct cxl_dev_state *cxlds,
>>  	devm_add_action_or_reset(&cxlds->cxlmd->dev, free_perf_ents, mds);
>>  }
>>  
>> +struct qos_class_ctx {
>> +	bool matched;
>> +	int dev_qos_class;
>> +};
>> +
>> +static int match_cxlrd_qos_class(struct device *dev, void *data)
>> +{
>> +	struct qos_class_ctx *ctx = data;
>> +	struct cxl_root_decoder *cxlrd;
>> +
>> +	if (!is_root_decoder(dev))
>> +		return 0;
>> +
>> +	cxlrd = to_cxl_root_decoder(dev);
>> +	if (cxlrd->qos_class == CXL_QOS_CLASS_INVALID ||
>> +	    ctx->dev_qos_class == CXL_QOS_CLASS_INVALID)
> Why check this one in here?  Caller should take that out first.

Ok I'll move to caller.

> 
>> +		return 0;
>> +
>> +	if (cxlrd->qos_class == ctx->dev_qos_class) {
>> +		ctx->matched = 1;
>> +		return 1;
> 
> As noted below, this is the return value for the device_for_each_child()
> so can use that instead of needing ctx->matched.

Yes. Will simplify.

> 
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int cxl_qos_match(struct cxl_port *root_port,
>> +			 struct list_head *work_list,
>> +			 struct list_head *discard_list)
>> +{
>> +	struct cxl_dpa_perf *dpa_perf, *n;
>> +	struct qos_class_ctx ctx;
>> +	int rc;
>> +
>> +	if (list_empty(work_list))
>> +		return 0;
>> +
>> +	list_for_each_entry_safe(dpa_perf, n, work_list, list) {
>> +		ctx = (struct qos_class_ctx) {
>> +			.matched = false,
>> +			.dev_qos_class = dpa_perf->qos_class,
>> +		};
> 
> 		as above the dev_qos_class doesn't change so can we not
> 		reject it early here? 
> 		if (!dpa_perf->qos_class)
> 			continue; /* I think? */

Not sure what you mean here. qos_class can be 0.

I ended up just changing it to this:

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, *n;
	int rc;

	list_for_each_entry_safe(dpa_perf, n, work_list, list) {
		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, &ctx, match_cxlrd_qos_class);
>> +		if (rc < 0)
>> +			return -ENOENT;
> 
> Can only return 0 or 1  and 1 is returned only on a match, so can use that
> in place of ctx.matched if you like.

Yes I will remove ctx.matched completely.

> 
>> +
>> +		if (!ctx.matched)
>> +			list_move_tail(&dpa_perf->list, discard_list);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +struct qos_hb_ctx {
>> +	bool matched;
>> +	struct device *host_bridge;
>> +};
>> +
>> +static int match_cxlrd_hb(struct device *dev, void *data)
>> +{
>> +	struct cxl_switch_decoder *cxlsd;
>> +	struct qos_hb_ctx *ctx = data;
>> +	struct cxl_root_decoder *cxlrd;
>> +	unsigned int seq;
>> +
>> +	if (!is_root_decoder(dev))
>> +		return 0;
>> +
>> +	cxlrd = to_cxl_root_decoder(dev);
>> +	cxlsd = &cxlrd->cxlsd;
>> +
>> +	do {
>> +		seq = read_seqbegin(&cxlsd->target_lock);
>> +		for (int i = 0; i < cxlsd->nr_targets; i++) {
>> +			if (ctx->host_bridge ==
>> +			    cxlsd->target[i]->dport_dev) {
>> +				ctx->matched = true;
>> +				return 1;
>> +			}
>> +		}
>> +	} while (read_seqretry(&cxlsd->target_lock, seq));
>> +
>> +	return 0;
>> +}
>> +
>> +static void discard_dpa_perf(struct list_head *list)
>> +{
>> +	struct cxl_dpa_perf *dpa_perf, *n;
>> +
>> +	list_for_each_entry_safe(dpa_perf, n, list, list) {
>> +		list_del(&dpa_perf->list);
>> +		kfree(dpa_perf);
>> +	}
>> +}
>> +DEFINE_FREE(dpa_perf, struct list_head *, if (_T) discard_dpa_perf(_T))
> 
> Can if (_T) ever fail?
> The list can be empty but I think the pointer is always valid.

Right. I'll change it to if (!list_empty(_T)) given here we are pointing to a list_head on stack.

> 
>> +
>> +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);
>> +	struct cxl_port *root_port __free(put_device) = NULL;
>> +	LIST_HEAD(__discard);
>> +	struct list_head *discard __free(dpa_perf) = &__discard;
>> +	struct qos_hb_ctx hbctx;
>> +	int rc;
>> +
>> +	root_port = find_cxl_root(cxlmd->endpoint);
>> +	if (!root_port)
>> +		return -ENODEV;
>> +
>> +	/* Check that the QTG IDs are all sane between end device and root decoders */
>> +	rc = cxl_qos_match(root_port, &mds->ram_perf_list, discard);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	rc = cxl_qos_match(root_port, &mds->pmem_perf_list, discard);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	/* Check to make sure that the device's host bridge is under a root decoder */
>> +	hbctx = (struct qos_hb_ctx) {
>> +		.matched = false,
>> +		.host_bridge = cxlmd->endpoint->host_bridge,
>> +	};
>> +	rc = device_for_each_child(&root_port->dev, &hbctx, match_cxlrd_hb);
>> +	if (rc < 0)
>> +		return rc;
> As above failre sure this can't return an error, and I think you can use the return
> value == 1 in place of the matched variable allowing simpler context.

Will simplify

>> +
>> +	if (!hbctx.matched) {
>> +		list_splice_tail_init(&mds->ram_perf_list, discard);
>> +		list_splice_tail_init(&mds->pmem_perf_list, discard);
>> +	}
>> +
>> +	return rc;
>> +}
>> +
> 
>