Re: [PATCH v19 18/22] cxl: Allow region creation by type2 drivers

[Dave Jiang <dave.jiang@intel.com>]

On 10/20/25 6:24 AM, Alejandro Lucero Palau wrote:
> 
> On 10/16/25 14:23, Cheatham, Benjamin wrote:
>> On 10/15/2025 4:42 PM, Dave Jiang wrote:
>>>
>>> On 10/9/25 1:56 PM, Cheatham, Benjamin wrote:
>>>> On 10/6/2025 5:01 AM, alejandro.lucero-palau@amd.com wrote:
>>>>> From: Alejandro Lucero <alucerop@amd.com>
>>>>>
>>>>> Creating a CXL region requires userspace intervention through the cxl
>>>>> sysfs files. Type2 support should allow accelerator drivers to create
>>>>> such cxl region from kernel code.
>>>>>
>>>>> Adding that functionality and integrating it with current support for
>>>>> memory expanders.
>>>>>
>>>>> Support an action by the type2 driver to be linked to the created region
>>>>> for unwinding the resources allocated properly.
>>>>>
>>>>> Based on https://lore.kernel.org/linux-cxl/168592159835.1948938.1647215579839222774.stgit@dwillia2-xfh.jf.intel.com/
>>>>>
>>>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>>>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>>>> ---
>>>> Fix for this one should be split between 13/22 and this patch, but the majority of it is in this one. The idea is
>>>> if we don't find a free decoder we check for pre-programmed decoders and use that instead. Unfortunately, this
>>>> invalidates some of the assumptions made by __construct_new_region().
>>> Wouldn't you look for a pre-programmed decoder first and construct the auto region before you try to manually create one? Also for a type 2 device, would the driver know what it wants and what the region configuration should look like? Would it be a single region either it's auto or manual, or would there be a configuration of multiple regions possible? To me a type 2 region is more intentional where the driver would know exactly what it needs and thus trying to get that from the cxl core.
>>>
>> Since this is a fix I didn't want to supersede the current behavior. A better solution would've been to add a flag to allow the type 2 driver
>> to set up an expected region type.
>>
>> As for multiple regions, I have no clue. I haven't heard of any reason why a type 2 device would need multiple regions, but it's still very
>> early days. I don't think there's anything in this set that keeps you from using multiple regions though.
> 
> 
> What Dave says is correct, so Type2 should  support these two possibilities, an HDM decoder already programmed by the BIOS and the BIOS doing nothing, at least with the Type2 HDM decoders. This patchset supports the latter, but the former is more than possible, even if the logic and what we have discussed since the RFC points to type2 driver having the full control.
> 
> 
> However, I would prefer to do that other support as a follow-up as the functionality added is enough for the initial client, the sfc driver, requiring this new Type2 support. The reason is clear: I do not want to delay this "basic Type2 support" more than necessary, and as I stated in another comment, I bet we will see other things to support soon, so better to increasingly add those after a first work set the base. Of course, the base needs to be right.

I'm fine with the follow on approach. We probably should make a note somewhere in a commit log somewhere that only manual assemble mode is currently supported. And need to reject the BIOS created region and exit type2 CXL setup if present?

DJ

> 
> 
> Thanks,
> 
> Alejandro
> 
> 
>> Thanks,
>> Ben
>>
>>> DJ
>>>