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

Alejandro Lucero Palau
2024-08-19T11:11:20Z

On 8/15/24 17:35, Jonathan Cameron wrote:
> On Mon, 12 Aug 2024 12:16:02 +0100
> Alejandro Lucero Palau <alucerop@amd.com> 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 ...
> If most code can't see the internals of cxl_dev_state because it
> doesn't include the header that defines it, then we will generally
> spot data that may not belong in that state structure in the first place
> or where it is appropriate to have an accessor function mediating that
> access.

I follow that but I do not know if you are suggesting here to make it
opaque which conflicts with a previous comment stating it does not need
to be.

> Jonathan
>
>