Re: [PATCH 8/9] cxl/core: Add dax_kmem_region and sysram_region drivers

["Cheatham, Benjamin" <benjamin.cheatham@amd.com>]

On 1/30/2026 4:12 PM, Gregory Price wrote:
> On Fri, Jan 30, 2026 at 03:27:12PM -0600, Cheatham, Benjamin wrote:
>> On 1/29/2026 3:04 PM, Gregory Price wrote:
>>> In the current kmem driver binding process, the only way for users
>>> to define hotplug policy is via a build-time option, or by not
>>> onlining memory by default and setting each individual memory block
>>> online after hotplug occurs.  We can solve this with a configuration
>>> step between region-probe and dax-probe.
>>>
>>> Add the infrastructure for a two-stage driver binding for kmem-mode
>>> dax regions. The cxl_dax_kmem_region driver probes cxl_sysram_region
>>> devices and creates cxl_dax_region with dax_driver=kmem.
>>>
>>> This creates an interposition step where users can configure policy.
>>>
>>> Device hierarchy:
>>>   region0 -> sysram_region0 -> dax_region0 -> dax0.0
>>
>> This technically comes up in the devdax_region driver patch first, but I noticed it here
>> so this is where I'm putting it:
>>
>> I like the idea here, but the implementation is all off. Firstly, devm_cxl_add_sysram_region()
>> is never called outside of sysram_region_driver::probe(), so I'm not sure how they ever get
>> added to the system (same with devdax regions).
>>
>> Second, there's this weird pattern of adding sub-region (sysram, devdax, etc.) devices being added
>> inside of the sub-region driver probe. I would expect the devices are added then the probe function
>> is called. 
> 
> I originally tried doing with region0/region_driver, but that design
> pattern is also confusing - and it creates differently bad patterns.
> 
>     echo region0 > decoder0.0/create_ram_region   -> creates region0
> 
>     # Current pattern
>     echo region > driver/region/probe  /* auto-region behavior */
> 
>     # region_driver attribute pattern
>     echo "sysram" > region0/region_driver
>     echo region0 > driver/region/probe   /* uses sysram region driver */
> 
> https://lore.kernel.org/linux-cxl/20260113202138.3021093-1-gourry@gourry.net/
> 
> Ira pointed out that this design makes the "implicit" design of the
> driver worse.  The user doesn't actually know what driver is being used
> under the hood - it just knows something is being used.
> 
> This at least makes it explicit which driver is being used - and splits
> the uses-case logic up into discrete drivers (dax users don't have to
> worry about sysram users breaking their stuff).
> 
> If it makes more sense, you could swap the ordering of the names
> 
>     echo region0 > region/bind
>     echo region0 > region_sysram/bind
>     echo region0 > region_daxdev/bind
>     echo region0 > region_dax_kmem/bind
>     echo region0 > region_pony/bind
> 
> --- 
> 
> The  underlying issue is that region::probe() is trying to be a
> god-function for every possible use case, and hiding the use case
> behind an attribute vs a driver is not good.
> 
> (also the default behavior for region::probe() in an otherwise
>  unconfigured region is required for backwards compatibility)

Ok, that makes sense. I think I just got lost in the sauce while looking at this last
week and this explanation helped a lot.> 
>> What I think should be going on here (and correct me if I'm wrong) is:
>> 	1. a cxl_region device is added to the system
>> 	2. cxl_region::probe() is called on said device (one in cxl/core/region.c)
>> 	3. Said probe function figures out the device is a dax_region or whatever else and creates that type of region device
>> 	(i.e. cxl_region::probe() -> device_add(&cxl_sysram_device))
>> 	4. if the device's dax driver type is DAXDRV_DEVICE_TYPE it gets sent to the daxdev_region driver
>> 	5a. if the device's dax driver type is DAXDRV_KMEM_TYPE it gets sent to the sysram_region driver which holds it until
>> 	the online_type is set
>> 	5b. Once the online_type is set, the device is forwarded to the dax_kmem_region driver? Not sure on this part
>>
>> What seems to be happening is that the cxl_region is added, all of these region drivers try
>> to bind to it since they all use the same device id (CXL_DEVICE_REGION) and the correct one is
>> figured out by magic? I'm somewhat confused at this point :/.
>>
> 
> For auto-regions:
>    region_probe() eats it and you get the default behavior.
> 
> For non-auto regions:
>    create_x_region generates an un-configured region and fails to probe
>    until the user commits it and probes it.

I think this was the source of my misunderstanding. I was trying to understand how it
works for auto regions when it's never meant to apply to them.

Sorry if this is a stupid question, but what stops auto regions from binding to the
sysram/dax region drivers? They all bind to region devices, so I assume there's something
keeping them from binding before the core region driver gets a chance.

Thanks,
Ben
> 
> auto-regions are evil and should be discouraged.
> 
> ~Gregory