Re: [PATCH v4 04/23] cxl: Add common helpers for cdat parsing

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

On 4/20/23 2:41 AM, Jonathan Cameron wrote:
> On Wed, 19 Apr 2023 13:21:25 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Add helper functions to parse the CDAT table and provide a callback to
>> parse the sub-table. Helpers are provided for DSMAS and DSLBIS sub-table
>> parsing. The code is patterned after the ACPI table parsing helpers.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>
> A few minor things inline.   More than possible you addressed them
> in earlier versions though.
> 
> Jonathan
> 
>> ---
>> v2:
>> - Use local headers to handle LE instead of ACPI header
>> - Reduce complexity of parser function. (Jonathan)
>> - Directly access header type. (Jonathan)
>> - Simplify header ptr math. (Jonathan)
>> - Move parsed counter to the correct location. (Jonathan)
>> - Add LE to host conversion for entry length
>> ---
>>   drivers/cxl/core/Makefile |    1
>>   drivers/cxl/core/cdat.c   |  100 +++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/cxl/cxlpci.h      |   29 +++++++++++++
>>   3 files changed, 130 insertions(+)
>>   create mode 100644 drivers/cxl/core/cdat.c
>>
>> diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
>> index ca4ae31d8f57..867a8014b462 100644
>> --- a/drivers/cxl/core/Makefile
>> +++ b/drivers/cxl/core/Makefile
>> @@ -12,5 +12,6 @@ cxl_core-y += memdev.o
>>   cxl_core-y += mbox.o
>>   cxl_core-y += pci.o
>>   cxl_core-y += hdm.o
>> +cxl_core-y += cdat.o
>>   cxl_core-$(CONFIG_TRACING) += trace.o
>>   cxl_core-$(CONFIG_CXL_REGION) += region.o
>> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
>> new file mode 100644
>> index 000000000000..210f4499bddb
>> --- /dev/null
>> +++ b/drivers/cxl/core/cdat.c
>> @@ -0,0 +1,100 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/* Copyright(c) 2023 Intel Corporation. All rights reserved. */
>> +#include "cxlpci.h"
>> +#include "cxl.h"
>> +
>> +static bool has_handler(struct cdat_subtable_proc *proc)
> 
> Even though they are static, I'd add a cxl_ or cdat_ prefix
> to these to make it clear they are local.

Ok I'll change to cdat_*

> 
>> +{
>> +	return proc->handler;
>> +}
>> +
>> +static int call_handler(struct cdat_subtable_proc *proc,
>> +			struct cdat_subtable_entry *ent)
>> +{
>> +	if (has_handler(proc))
> 
> Do we need to check this again? It's checked in the parse_entries code
> well before this point.
> 
> Also, if moving to checking it once, then is it worth the
> little wrapper functions?

Ok I'll call it directly and remove the wrapper.

> 
> 
>> +		return proc->handler(ent->hdr, proc->arg);
>> +	return -EINVAL;
>> +}
>> +
>> +static bool cdat_is_subtable_match(struct cdat_subtable_entry *ent)
>> +{
>> +	return ent->hdr->type == ent->type;
>> +}
>> +
>> +static int cdat_table_parse_entries(enum cdat_type type,
>> +				    struct cdat_header *table_header,
>> +				    struct cdat_subtable_proc *proc)
>> +{
>> +	unsigned long table_end, entry_len;
>> +	struct cdat_subtable_entry entry;
>> +	int count = 0;
>> +	int rc;
>> +
>> +	if (!has_handler(proc))
>> +		return -EINVAL;
>> +
>> +	table_end = (unsigned long)table_header + table_header->length;
>> +
>> +	if (type >= CDAT_TYPE_RESERVED)
>> +		return -EINVAL;
>> +
>> +	entry.type = type;
>> +	entry.hdr = (struct cdat_entry_header *)(table_header + 1);
>> +
>> +	while ((unsigned long)entry.hdr < table_end) {
>> +		entry_len = le16_to_cpu(entry.hdr->length);
>> +
>> +		if ((unsigned long)entry.hdr + entry_len > table_end)
>> +			return -EINVAL;
>> +
>> +		if (entry_len == 0)
>> +			return -EINVAL;
>> +
>> +		if (cdat_is_subtable_match(&entry)) {
>> +			rc = call_handler(proc, &entry);
>> +			if (rc)
>> +				return rc;
>> +			count++;
>> +		}
>> +
>> +		entry.hdr = (struct cdat_entry_header *)((unsigned long)entry.hdr + entry_len);
>> +	}
>> +
>> +	return count;
>> +}
> 
> ...
> 
>> +int cdat_table_parse_sslbis(struct cdat_header *table,
>> +			    cdat_tbl_entry_handler handler, void *arg)
> 
> Feels like these ones should take a typed arg.  Sure you'll loose
> that again to use the generic handling code, but at this level we can
> do it I think.

while DSMAS and DSLBIS takes a list_head, SSLBIS takes an xarray. I can 
create a union.

> 
>> +{
>> +	struct cdat_subtable_proc proc = {
>> +		.handler	= handler,
>> +		.arg		= arg,
>> +	};
>> +
>> +	return cdat_table_parse_entries(CDAT_TYPE_SSLBIS, table, &proc);
>> +}
>