Re: [RFC 0/2] type2 cxl initialization

[Ben Cheatham <benjamin.cheatham@amd.com>]

On 3/3/25 2:49 PM, Dan Williams wrote:
> Ben Cheatham wrote:
>> On 2/20/25 2:00 PM, alejandro.lucero-palau@amd.com wrote:
>>> From: Alejandro Lucero <alucerop@amd.com>
>>>
>>> Avoiding to send the full type2 support patchset until there is an
>>> agreement about how to do the cxl initialization from accel drivers.
>>>
>>> Using the idea from fwctl for embedding cxl_dev_state in a private
>>> accel driver struct and a macro helping for the allocation and
>>> initialization of such struct.
>>>
>>> The main problem is the amount of internal cxl structs which need to be
>>> public for accel drivers which we tried to avoid since v1 of the type2
>>> patchset history.
>>
>> I have a suggestion or two in the next patch, but in my opinion the only
>> potentially problematic exports are the cxl register structs. I'm assuming
>> allowing accelerator drivers access to those may cause stale values in the
>> CXL driver, but I haven't looked. The only suggestion I have there is to
>> use pointers to the struct instead, but I imagine achieving could be a
>> non-trivial amount of effort.
> 
> Hmm, say more about why you think those are a problem. Those are
> "write-once at init" things, why would an accelerator driver have any
> reason to play with those post init?
> 

I could see a scenario where an accelerator driver tries to do some concurrent
setup with the CXL driver and may cause an issue. For example if a type 2 driver
tried to do something with HDM during setup, but I think a driver touching
any of these registers when the CXL driver already does this stuff is pretty
unlikely.

So that leads me to think it's better to not expose this at all, but I think
that would require more work than it's worth.

>> The only other member that may be a problem is the DPA partition info,
>> but I don't know enough about that to comment.
> 
> I think it is sufficient to mark the bulk of 'struct cxl_dev_state' as
> private and then move things into the public space as needed, something
> like:
> 
> struct cxl_dev_state {
> /* public */
>         struct device *dev;
>         struct cxl_memdev *cxlmd;
> /* private */
>         struct cxl_register_map reg_map;
>         struct cxl_regs regs;
>         int cxl_dvsec;
>         bool rcd;
>         bool media_ready;
>         struct resource dpa_res;
>         struct cxl_dpa_partition part[CXL_NR_PARTITIONS_MAX];
>         unsigned int nr_partitions;
>         u64 serial;
>         enum cxl_devtype type;
>         struct cxl_mailbox cxl_mbox;
> };
> 
> ...but otherwise let accelerator drivers see the full definition of that
> struct for constainer_of() operations.
That's fine by me.