Re: [PATCH v2 01/15] cxl: add type2 device basic support

Alejandro Lucero Palau
2024-08-19T11:13:18Z

On 8/15/24 17:38, Jonathan Cameron wrote:
> On Tue, 13 Aug 2024 09:30:08 +0100
> Alejandro Lucero Palau <alucerop@amd.com> wrote:
>
>> On 8/12/24 12:16, Alejandro Lucero Palau wrote:
>>> On 8/4/24 18:10, Jonathan Cameron wrote:
>>>> On Mon, 15 Jul 2024 18:28:21 +0100
>>>> <alejandro.lucero-palau@amd.com> wrote:
>>>>
>>>>> From: Alejandro Lucero <alucerop@amd.com>
>>>>>
>>>>> Differientiate Type3, aka memory expanders, from Type2, aka device
>>>>> accelerators, with a new function for initializing cxl_dev_state.
>>>>>
>>>>> Create opaque struct to be used by accelerators relying on new access
>>>>> functions in following patches.
>>>>>
>>>>> Add SFC ethernet network driver as the client.
>>>>>
>>>>> Based on
>>>>> https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/T/#m52543f85d0e41ff7b3063fdb9caa7e845b446d0e
>>>>>
>>>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>>>>> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
>>>>
>>>
>>>>> +
>>>>> +void cxl_accel_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec)
>>>>> +{
>>>>> +    cxlds->cxl_dvsec = dvsec;
>>>> Nothing to do with accel. If these make sense promote to cxl
>>>> core and a linux/cxl/ header.  Also we may want the type3 driver to
>>>> switch to them long term. If nothing else, making that handle the
>>>> cxl_dev_state as more opaque will show up what is still directly
>>>> accessed and may need to be wrapped up for a future accelerator driver
>>>> to use.
>>>>
>>> I will change the function name then, but not sure I follow the
>>> comment about more opaque ...
>>>
>>>
>>>
>> I have second thoughts about this.
>>
>>
>> I consider this as an accessor  for, as you said in a previous exchange,
>> facilitating changes to the core structs without touching those accel
>> drivers using it.
>>
>> Type3 driver is part of the CXL core and easy to change for these kind
>> of updates since it will only be one driver supporting all Type3, and an
>> accessor is not required then.
>>
>> Let me know what you think.
> It's less critical, but longer term I'd expect any stuff that makes
> sense for accelerators and the type 3 driver to use the same
> approaches and code paths. Makes it easier to see where they
> are related than opencoding the accesses in the type 3 driver will
> do. In the very long term, I'd expect the type 3 driver to just be
> another CXL driver alongside many others.

It makes sense, so I will change the name.

A following patchset when this is hopefully going through will be to use
the accessors in the CXL PCI driver.

Thanks!

> Jonathan
>
>>