Re: [PATCH RFC 2/2] cxl/memdev: Remove temporary variables from cxl_memdev_state

Alejandro Lucero Palau
2025-02-05T09:01:08Z

<snip>
>>> I was hoping this would get further away from new in/out arguments and
>>> look at centralizing all partition enumeration into one routine.
>> I don't understand? get partition info is only required if the partition align
>> bytes is not 0. IOW if the device allows for partitions to be changed.
>> cxl_mem_get_partition_info() is only called IFF that extra query is required.
>> So this does centralize byte information queries into one routine. It leaves
>> creating partitions to the device driver which moves us toward these being
>> mailbox only calls...
> The crux of the concern for me is less about the role of
> cxl_mem_get_partition_info() and more about the introduction of a new 'struct
> cxl_mem_dev_info' in/out parameter which is similar in function to
> 'struct cxl_dpa_info'. If you can find a way to avoid another level of
> indirection or otherwise consolidate all these steps into a straight
> line routine that does "all the DPA enumeration" things.
>

All this discussion after Dan's patches makes me feel miserable.

I have already used those patches for the Type2 support, and I'm using a
struct to be used by accel drivers without a mbox for setting up the
current mds information, required for building up the DPA partitions
used the new functions. In this case, a struct is needed for sure,
because there are two alternatives which are more painful than using
such a struct:

- one alternative is to allow accel drivers to manipulate internal cxl
structs what would require, to start with, to export them fully for
accel drivers. Then those drivers doing non trivial work for something
the current cxl core is already doing and, IMO, which could be reused
hidden the complexity.

- Another option, if not such new struct is used, is to pass the
required data one by one, and although it could be fine by now, I think
it is not as clean and it does not take into account potential changes
hardcoded by accel drivers which would require further arguments.

What I have now is quite similar to current pci driver, but using a new
function for accel drivers (where that new struct is used) instead of
cxl_dev_state_identify, although some accel drivers will just call that
function if there exists an mbox. Then cxl_mem_dpa_fetch and
cxl_dpa_setup will make all the work as they do for the pci driver.

The change is simple and the code now cleaner than the previous versions
where the DPA mess was still there. So, I'm happy with the DPA mess
being solved, but ...

... what you seem to suggest now, and I mean both, Ira and Dan, is to
optimize this which will make life harder for an accel driver. Or
further helper functions will be needed, or the accel driver will need
to do a lot of the work now performed by the core. I guess it is not a
surprise if I speak up against any of these two possibilities.

Maybe it is worth if you have a look at v10 that will be sent later
today, early in the morning for you, where all this can hopefully be
seen clearly.