Re: [RFC PATCH 4/4] cxl/region: Introduce cxl_memdev_attach_region

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

On 4/11/2026 6:02 PM, Dan Williams wrote:

[snip]

>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>> index 11bc0b88b05f..090f52392b20 100644
>>> --- a/drivers/cxl/core/region.c
>>> +++ b/drivers/cxl/core/region.c
>>> @@ -1123,6 +1123,19 @@ static int cxl_rr_assign_decoder(struct cxl_port *port, struct cxl_region *cxlr,
>>>  static void cxl_region_setup_flags(struct cxl_region *cxlr,
>>>                                struct cxl_decoder *cxld)
>>>  {
>>> +   if (is_endpoint_decoder(&cxld->dev)) {
>>> +           struct cxl_endpoint_decoder *cxled = to_cxl_endpoint_decoder(&cxld->dev);
>>> +           struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
>>> +
>>> +           /*
>>> +            * When a region's memdevs specify an @attach method the attach
>>> +            * provider is responsible for dispositioning the region for
>>> +            * both probe and userspace management
>>> +            */
>>> +           if (cxlmd->attach)
>>> +                   set_bit(CXL_REGION_F_LOCK, &cxlr->flags);
>>> +   }
>>> +
>>
>> It seems unfortunate that you set the region lock bit here and then
>> immediately do a check to set the bit again.
> 
> There are 2 different cases to lock the region here, presence of attach, or
> presence of hardware locked decoder.
> 
>> I can't think of a way to leverage cxld->flags for type 2 in an
>> ergonomic way though, so I guess it's fine.
> 
> The flags are just caching hardware state, I do not want to have drivers
> play games with the flags that dilute that "cached hardware value"
> meaning.
> 

Didn't realize that, makes sense.

> [..]
>>> +/*
>>> + * The presence of an attach method indicates that the region is designated for
>>> + * a purpose outside of CXL core memory expansion defaults.
>>> + */
>>> +static bool cxl_region_has_memdev_attach(struct cxl_region *cxlr)
>>> +{
>>
>> I think this should be renamed to something like "cxl_region_is_private()"; it
>> better matches the intended use of the function while lessening the burden on
>> a non-type 2 educated reader. Also has the added benefit of being one less
>> change site if the mechanism for determining if a region belongs to an accelerator
>> changes in the future.
> 
> Why does the CXL core naming need to cater to accelerator vs expansion
> use case naming?  CXL core developer needs to understand all the use
> cases and wants to make them unified as much as possible.

It doesn't. I agree that developers that are going to touch this code should
understand all the use cases but the naming here doesn't accurately convey
the intended use case. memdev_attach is only used by accelerator drivers, so
this function is checking the device type/driver.

That does somewhat go against unifying the use cases but the use cases
definitely do diverge here. It's really more about clearly delineating where
the cases diverge to someone who is less knowledgeable about accelerators
(which is most people in my experience).

> "cxl_region_is_private()" says nothing about what "private" means and
> where to look next. It also muddies the namespace for when CXL regions
> start becoming the backing store for "private" nodes. Is that a
> "private" region?

Bad naming suggestion on my part, I forgot about the private nodes set.
I was about to suggest changing it to "cxl_region_is_reserved()" but that's
an even more overloaded term :/. Going along the lines of what I said above,
maybe something like "cxl_region_reserved_by_acclerator()"? Kind of long, but
gets to the point.

Thanks,
Ben