Re: [PATCH v5 06/14] cxl: Store the access coordinates for the generic ports

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

On 5/12/23 7:59 AM, Jonathan Cameron wrote:
> On Mon, 08 May 2023 13:47:29 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Each CXL host bridge is represented by an ACPI0016 device. A generic port
>> device handle that is an ACPI device is represented by a string of
>> ACPI0016 device HID and UID. Create a device handle from the ACPI device
>> and retrieve the access coordinates from the stored memory targets. The
>> access coordinates are stored under the cxl_dport that is associated with
>> the CXL host bridge.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/cxl/acpi.c |   28 ++++++++++++++++++++++++++++
>>   drivers/cxl/cxl.h  |    2 ++
>>   2 files changed, 30 insertions(+)
>>
>> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
>> index f9b35e8fe810..675a4f423f4b 100644
>> --- a/drivers/cxl/acpi.c
>> +++ b/drivers/cxl/acpi.c
>> @@ -537,8 +537,26 @@ static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
>>   	return 0;
>>   }
>>   
>> +static int get_genport_coordinates(struct device *dev, struct cxl_dport *dport)
>> +{
>> +	struct acpi_device *hb = to_cxl_host_bridge(NULL, dev);
>> +	u8 handle[ACPI_SRAT_DEVICE_HANDLE_SIZE] = { 0 };
>> +	int rc;
>> +
>> +	/* ACPI spec 6.5 tABLE 5.65 */
> 
> tABLE?

ooops caps lock

> 
>> +	memcpy(handle, acpi_device_hid(hb), 8);
>> +	memcpy(&handle[8], acpi_device_uid(hb), 4);
>> +
>> +	rc = acpi_get_genport_coordinates(handle, dport->genport_coord);
>> +	if (rc)
>> +		return rc;
>> +
>> +	return 0;
>> +}
>> +
>>   static int add_host_bridge_dport(struct device *match, void *arg)
>>   {
>> +	int ret;
>>   	acpi_status rc;
>>   	struct device *bridge;
>>   	unsigned long long uid;
>> @@ -594,6 +612,16 @@ static int add_host_bridge_dport(struct device *match, void *arg)
>>   	if (IS_ERR(dport))
>>   		return PTR_ERR(dport);
>>   
>> +	dport->genport_coord = devm_kzalloc(dport->dport,
>> +					    sizeof(*dport->genport_coord),
>> +					    GFP_KERNEL);
> 
> It's pretty small - worth allocating separately?
> 
> Maybe add something on why to the patch description if there is another reason
> for this dance.

My intention was to allow detection of whether the data exists or not 
based on if the ptr is NULL. I'll add explanation in patch description.

> 
>> +	if (!dport->genport_coord)
>> +		return -ENOMEM;
>> +
>> +	ret = get_genport_coordinates(match, dport);
>> +	if (ret)
>> +		dev_dbg(match, "Failed to get generic port perf coordinates.\n");
>> +
>>   	return 0;
>>   }
>>   
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index c1e2c3703a63..033b822a20f2 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -626,6 +626,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
>> + * @genport_coord: access coordinates (performance) from ACPI generic port
>>    * @coord: access coordinates (performance) for switch from CDAT
>>    * @link_latency: calculated PCIe downstream latency
>>    */
>> @@ -636,6 +637,7 @@ struct cxl_dport {
>>   	resource_size_t rcrb;
>>   	bool rch;
>>   	struct cxl_port *port;
>> +	struct access_coordinate *genport_coord;
>>   	struct access_coordinate coord;
>>   	long link_latency;
>>   };
>>
>>
>