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

Alejandro Lucero Palau
2024-08-09T07:27:54Z

On 8/4/24 17:44, Jonathan Cameron wrote:
>>>> diff --git a/include/linux/cxl_accel_mem.h b/include/linux/cxl_accel_mem.h
>>>> new file mode 100644
>>>> index 000000000000..daf46d41f59c
>>>> --- /dev/null
>>>> +++ b/include/linux/cxl_accel_mem.h
>>>> @@ -0,0 +1,22 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +/* Copyright(c) 2024 Advanced Micro Devices, Inc. */
>>>> +
>>>> +#include <linux/cdev.h>
>>> Don't think this header is needed?
>>>
>>>> +
>>>> +#ifndef __CXL_ACCEL_MEM_H
>>>> +#define __CXL_ACCEL_MEM_H
>>>> +
>>>> +enum accel_resource{
>>>> + CXL_ACCEL_RES_DPA,
>>>> + CXL_ACCEL_RES_RAM,
>>>> + CXL_ACCEL_RES_PMEM,
>>>> +};
>>>> +
>>>> +typedef struct cxl_dev_state cxl_accel_state;
>>> Please use 'struct cxl_dev_state' directly. There's no good reason to hide the type.
>>
>> That is what I think I was told to do although not explicitly. There
>> were concerns in the RFC about accel drivers too loose for doing things
>> regarding CXL and somehow CXL core should keep control as much as
>> possible.  I was even thought I was being asked to implement auxbus with
>> the CXL part of an accel as an auxiliar device which should be bound to
>> a CXL core driver. Then Jonathan Cameron the only one explicitly giving
>> the possibility of the opaque approach and disadvising the auxbus idea.
> I wasn't thinking a typedef to hide it.
> More making all state accesses that are needed through accessor functions so
> that from the 'internals' become opaque to the accelerator code and
> we can radically change how things are structured internally with
> no impact to the (hopefully large number of) CXL accelerator drivers.
>
> So here, I'd just expect a
> struct cxl_device_state; forwards declaration.
>
> Or potentially one to a a different structure after refactors etc.

OK. It makes sense. I thought the concern was about external driver
modules using the internal cxl structs.

This is the main point in this second patchset version, so if none else
says the opposite during the next days, I will take it as the right move
forward and send a new version 3 soon.

Thank you

>>
>> Maybe I need an explicit action here.
> J