Re: [PATCH 07/18] cxl: Add callback to parse the DSLBIS subtable from CDAT

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

On 2/9/23 6:50 AM, Jonathan Cameron wrote:
> On Mon, 06 Feb 2023 13:50:23 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Provide a callback to parse the Device Scoped Latency and Bandwidth
>> Information Structure (DSLBIS) in the CDAT structures. The DSLBIS
>> contains the bandwidth and latency information that's tied to a DSMAS
>> handle. The driver will retrieve the read and write latency and
>> bandwidth associated with the DSMAS which is tied to a DPA range.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> A few comments inline,
> 
> Thanks,
> 
> Jonathan
> 
>> ---
>>   drivers/cxl/core/cdat.c |   34 ++++++++++++++++++++++++++++++++++
>>   drivers/cxl/cxl.h       |    2 ++
>>   drivers/cxl/port.c      |    9 ++++++++-
>>   include/acpi/actbl1.h   |    5 +++++
>>   4 files changed, 49 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
>> index f9a64a0f1ee4..3c8f3956487e 100644
>> --- a/drivers/cxl/core/cdat.c
>> +++ b/drivers/cxl/core/cdat.c
>> @@ -121,3 +121,37 @@ int cxl_dsmas_parse_entry(struct acpi_cdat_header *header, void *arg)
>>   	return 0;
>>   }
>>   EXPORT_SYMBOL_NS_GPL(cxl_dsmas_parse_entry, CXL);
>> +
>> +int cxl_dslbis_parse_entry(struct acpi_cdat_header *header, void *arg)
>> +{
>> +	struct cxl_port *port = (struct cxl_port *)arg;
>> +	struct dsmas_entry *dent;
>> +	struct acpi_cdat_dslbis *dslbis;
> 
> Perhaps reorder to maintain the pretty upside-down Christmas trees
> (I don't care :)

will fix
> 
>> +	u64 val;
>> +
>> +	if (header->type != ACPI_CDAT_TYPE_DSLBIS)
>> +		return -EINVAL;
> 
> Isn't this guaranteed by the caller?  Seems overkill do it twice
> and I don't think these will ever be called outside of that wrapper that
> loops over the entries. I could be wrong though!
>

ok will remove


>> +
>> +	dslbis = (struct acpi_cdat_dslbis *)((unsigned long)header + sizeof(*header));
> header + 1
> 
>> +	if ((dslbis->flags & ACPI_CEDT_DSLBIS_MEM_MASK) !=
> 
> This field 'must be ignored' if the DSMAS handle isn't a match
> (as it's an initiator only entry) Odd though it may seem I think we
> might see one of those on a type 3 device and we are probably going to
> have other users of this function anyway.
> 
> I think you need to do the walk below to check we have a DSMAS match, before
> running this check.

ok, will move down to where entry is matched

> 
>> +	     ACPI_CEDT_DSLBIS_MEM_MEMORY)
>> +		return 0;
>> +
>> +	if (dslbis->data_type > ACPI_HMAT_WRITE_BANDWIDTH)
>> +		return -ENXIO;
> 
> This would probably imply a new HMAT spec value, so probably just
> log it and ignore rather than error out.

ok

> 
>> +
>> +	/* Value calculation with base_unit, see ACPI Spec 6.5 5.2.28.4 */
>> +	val = dslbis->entry[0] * dslbis->entry_base_unit;
> 
> In theory this might overflow as u64 * u16.
> Doubt it will ever happen in reality, but maybe a check and debug print if it does?

ok will use check_mul_overflow()
> 
>> +
>> +	mutex_lock(&port->cdat.dsmas_lock);
>> +	list_for_each_entry(dent, &port->cdat.dsmas_list, list) {
>> +		if (dslbis->handle == dent->handle) {
>> +			dent->qos[dslbis->data_type] = val;
>> +			break;
>> +		}
>> +	}
>> +	mutex_unlock(&port->cdat.dsmas_lock);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_dslbis_parse_entry, CXL);
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index 1e5e69f08480..849b22236f1d 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -705,6 +705,7 @@ struct dsmas_entry {
>>   	struct list_head list;
>>   	struct range dpa_range;
>>   	u16 handle;
>> +	u64 qos[ACPI_HMAT_WRITE_BANDWIDTH + 1];
>>   };
>>   
>>   typedef int (*cdat_tbl_entry_handler)(struct acpi_cdat_header *header, void *arg);
>> @@ -716,6 +717,7 @@ int cdat_table_parse_dslbis(void *table, cdat_tbl_entry_handler handler,
>>   			    void *arg);
>>   
>>   int cxl_dsmas_parse_entry(struct acpi_cdat_header *header, void *arg);
>> +int cxl_dslbis_parse_entry(struct acpi_cdat_header *header, void *arg);
>>   
>>   /*
>>    * Unit test builds overrides this to __weak, find the 'strong' version
>> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
>> index b1da73e99bab..8de311208b37 100644
>> --- a/drivers/cxl/port.c
>> +++ b/drivers/cxl/port.c
>> @@ -65,8 +65,15 @@ static int cxl_port_probe(struct device *dev)
>>   			rc = cdat_table_parse_dsmas(port->cdat.table,
>>   						    cxl_dsmas_parse_entry,
>>   						    (void *)port);
>> -			if (rc < 0)
>> +			if (rc > 0) {
>> +				rc = cdat_table_parse_dslbis(port->cdat.table,
>> +							     cxl_dslbis_parse_entry,
>> +							     (void *)port);
>> +				if (rc <= 0)
>> +					dev_dbg(dev, "Failed to parse DSLBIS: %d\n", rc);
> 
> If we have entries and they won't parse, I think we should be screaming louder.
> dev_warn() would be my preference for this and the one in the previous patch.
> Sure we can carry on, but something on the device is not working as expected.

ok will fix this one and previous.

> 
>> +			} else {
>>   				dev_dbg(dev, "Failed to parse DSMAS: %d\n", rc);
>> +			}
>>   		}
>>   
>>   		rc = cxl_hdm_decode_init(cxlds, cxlhdm);
>> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
>> index e8297cefde09..ff6092e45196 100644
>> --- a/include/acpi/actbl1.h
>> +++ b/include/acpi/actbl1.h
>> @@ -369,6 +369,11 @@ struct acpi_cdat_dslbis {
>>   	u16 reserved2;
>>   };
>>   
>> +/* Flags for subtable above */
>> +
>> +#define ACPI_CEDT_DSLBIS_MEM_MASK	GENMASK(3, 0)
>> +#define ACPI_CEDT_DSLBIS_MEM_MEMORY	0
>> +
>>   /* Subtable 2: Device Scoped Memory Side Cache Information Structure (DSMSCIS) */
>>   
>>   struct acpi_cdat_dsmscis {
>>
>>
>