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

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

On 5/16/23 2:13 PM, Dan Williams wrote:
> Dave Jiang wrote:
>>
>>
>> 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,
> 
> This needs to be the same device as was used to allocate @dport, and
> that's not @dport->dport.

ok

> 
>>>> +					    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.
> 
> A couple reactions, is a "zero" access coordinate not sufficient for
> that?

Probably. I can change. I was being paranoid.

> 
> Also, "genport" is an ACPI-ism and I was aiming for cxl-core objects
> like cxl_dport to remain platform implementation independent. The
> concept of a dport having a host-bridge access_coordinate is generic
> though, so I would just rename this to "hb_access" or some other
> host-bridge generic naming.

Ok