Re: [PATCH v5 03/14] cxl: Add callback to parse the SSLBIS subtable from CDAT

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

On 5/12/23 7:41 AM, Jonathan Cameron wrote:
> On Mon, 08 May 2023 13:47:11 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Provide a callback to parse the Switched Scoped Latency and Bandwidth
>> Information Structure (SSLBIS) in the CDAT structures. The SSLBIS
>> contains the bandwidth and latency information that's tied to the
>> CXL switch that the data table has been read from. The extracted
>> values are stored to the cxl_dport correlated by the port_id
>> depending on the SSLBIS entry.
>>
>> Coherent Device Attribute Table 1.03 2.1 Switched Scoped Latency
>> and Bandwidth Information Structure (DSLBIS)
>>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> I'm disagreeing with myself again. Some comments inline based
> on a fresh read.
> 
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>
>> ---
>> v5:
>> - Store data to cxl_dport directly instead. (Dan)
>> - Use acpi_table_parse_cdat().
>> v3:
>> - Add spec section in commit header (Alison)
>> - Move CDAT parse to cxl_switch_port_probe()
>> - Use 'struct node_hmem_attrs'
>> ---
>>   drivers/cxl/core/cdat.c |   88 +++++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/cxl/cxl.h       |    8 ++++
>>   drivers/cxl/port.c      |   12 ++++++
>>   include/acpi/actbl1.h   |    3 ++
>>   4 files changed, 110 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
>> index 6e14d04c0453..37b135c900d5 100644
>> --- a/drivers/cxl/core/cdat.c
>> +++ b/drivers/cxl/core/cdat.c
>> @@ -124,3 +124,91 @@ int cxl_cdat_endpoint_process(struct cxl_port *port, struct list_head *list)
>>   	return rc;
>>   }
>>   EXPORT_SYMBOL_NS_GPL(cxl_cdat_endpoint_process, CXL);
>> +
>> +static int cdat_sslbis_handler(union acpi_subtable_headers *header, void *arg,
>> +			       const unsigned long end)
>> +{
>> +	struct acpi_cdat_sslbis *sslbis = (struct acpi_cdat_sslbis *)header;
>> +	struct acpi_cdat_sslbe *entry;
>> +	struct cxl_port *port = arg;
>> +	struct device *dev = &port->dev;
>> +	int remain, entries, i;
>> +	u16 len;
>> +
>> +	len = le16_to_cpu((__force __le16)sslbis->header.length);
>> +	remain = len - sizeof(*sslbis);
>> +	if (!remain || remain % sizeof(*entry) ||
>> +	    (unsigned long)header + len > end) {
>> +		dev_warn(dev, "Malformed SSLBIS table length: (%u)\n", len);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Unrecognized data type, we can skip */
>> +	if (sslbis->data_type > ACPI_HMAT_WRITE_BANDWIDTH)
>> +		return 0;
>> +
>> +	entries = remain / sizeof(*entry);
>> +	entry = (struct acpi_cdat_sslbe *)((unsigned long)header + sizeof(*sslbis));
> 
> That's a little nasty to follow.  Perhaps.
> 	
> 	entry = (struct acpi_cdat_sslbe *)(sslbis + 1);

Ok. I'll also update previous 2 patches with similar usages.

> 
> Bonus points if we can just use a variable length entry on the end of
> struct acpi_cdat_sslbis. Might break some other locations but would make
> this one nicer.

I had that when I was using local data structures for CDAT tables. But 
changing the ACPI version results in touching a lot of ACPICA code 
unfortunately.

> 
>> +
>> +	for (i = 0; i < entries; i++) {
>> +		u16 x = le16_to_cpu(entry->portx_id);
>> +		u16 y = le16_to_cpu(entry->porty_id);
>> +		struct cxl_dport *dport;
>> +		unsigned long index;
>> +		u16 dsp_id;
>> +		u64 val;
>> +
>> +		switch (x) {
>> +		case ACPI_CDAT_SSLBIS_US_PORT:
>> +			dsp_id = y;
>> +			break;
>> +		case ACPI_CDAT_SSLBIS_ANY_PORT:
>> +			switch (y) {
>> +			case ACPI_CDAT_SSLBIS_US_PORT:
>> +				dsp_id = x;
>> +				break;
>> +			case ACPI_CDAT_SSLBIS_ANY_PORT:
>> +				dsp_id = ACPI_CDAT_SSLBIS_ANY_PORT;
>> +				break;
>> +			default:
>> +				dsp_id = y;
>> +				break;
>> +			}
>> +			break;
>> +		default:
>> +			dsp_id = x;
>> +			break;
>> +		}
>> +
>> +		if (check_mul_overflow(le64_to_cpu(sslbis->entry_base_unit),
>> +				       le16_to_cpu(entry->latency_or_bandwidth),
>> +				       &val))
>> +			dev_warn(dev, "SSLBIS value overflowed!\n");
>> +
>> +		xa_for_each(&port->dports, index, dport) {
>> +			if (dsp_id == ACPI_CDAT_SSLBIS_ANY_PORT ||
>> +			    dsp_id == dport->port_id)
>> +				cxl_access_coordinate_set(&dport->coord,
>> +							  sslbis->data_type,
>> +							  val);
>> +		}
>> +
>> +		entry++;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +int cxl_cdat_switch_process(struct cxl_port *port)
>> +{
>> +	int rc;
>> +
>> +	rc = acpi_table_parse_cdat(ACPI_CDAT_TYPE_SSLBIS,
>> +				   cdat_sslbis_handler,
>> +				   port, port->cdat.table);
>> +	if (rc == 0)
>> +		rc = -ENOENT;
> 
> I'm bored of this pattern now so hide it...
> 
> Howabout
> 
> 	acpi_table_parse_cdat_fail_emtpy() that handles the rc
> rewrite internally. (with a better name).
> 
> Gets rid of all this boilerplate.

ok

> 
> 
> 
>> +
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_cdat_switch_process, CXL);
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index ca3d0d74f2e5..3e8020e0a132 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -600,6 +600,7 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev)
>>    * @rcrb: base address for the Root Complex Register Block
>>    * @rch: Indicate whether this dport was enumerated in RCH or VH mode
>>    * @port: reference to cxl_port that contains this downstream port
>> + * @coord: access coordinates (performance) for switch from CDAT
>>    */
>>   struct cxl_dport {
>>   	struct device *dport;
>> @@ -608,6 +609,7 @@ struct cxl_dport {
>>   	resource_size_t rcrb;
>>   	bool rch;
>>   	struct cxl_port *port;
>> +	struct access_coordinate coord;
>>   };
>>   
>>   /**
>> @@ -803,12 +805,18 @@ struct dsmas_entry {
>>   
>>   #ifdef CONFIG_ACPI
>>   int cxl_cdat_endpoint_process(struct cxl_port *port, struct list_head *list);
>> +int cxl_cdat_switch_process(struct cxl_port *port);
>>   #else
>>   static inline int cxl_cdat_endpoint_process(struct cxl_port *port,
>>   					    struct list_head *list)
>>   {
>>   	return -EOPNOTSUPP;
>>   }
>> +
>> +static inline int cxl_cdat_switch_process(struct cxl_port *port)
>> +{
>> +	return -EOPNOTSUPP;
>> +}
>>   #endif
>>   
>>   /*
>> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
>> index da023feaa6e2..c5a24b75bf03 100644
>> --- a/drivers/cxl/port.c
>> +++ b/drivers/cxl/port.c
>> @@ -86,7 +86,17 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>>   	if (IS_ERR(cxlhdm))
>>   		return PTR_ERR(cxlhdm);
>>   
>> -	return devm_cxl_enumerate_decoders(cxlhdm, NULL);
>> +	rc = devm_cxl_enumerate_decoders(cxlhdm, NULL);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	if (port->cdat.table) {
>> +		rc = cxl_cdat_switch_process(port);
>> +		if (rc < 0)
>> +			dev_warn(&port->dev, "Failed to parse SSLBIS: %d\n", rc);
>> +	}
>> +
>> +	return 0;
>>   }
>>   
>>   static int cxl_endpoint_port_probe(struct cxl_port *port)
>> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
>> index 8ea7e5d64bc1..82def138a7e4 100644
>> --- a/include/acpi/actbl1.h
>> +++ b/include/acpi/actbl1.h
>> @@ -419,6 +419,9 @@ struct acpi_cdat_sslbis {
>>   	u64 entry_base_unit;
>>   };
>>   
>> +#define ACPI_CDAT_SSLBIS_US_PORT	0x0100
>> +#define ACPI_CDAT_SSLBIS_ANY_PORT	0xffff
>> +
>>   /* Sub-subtable for above, sslbe_entries field */
>>   
>>   struct acpi_cdat_sslbe {
>>
>>
>