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

[Dan Williams <djbw@kernel.org>]

Alejandro Lucero Palau wrote:
> 
> On 4/3/26 22:00, Dan Williams wrote:
> > To date, platform firmware maps accelerator memory and accelerator drivers
> > simply want an address range that they can map themselves. This typically
> > results in a single region being auto-assembled upon registration of a
> > memory device.  Use the @attach mechanism of devm_cxl_add_memdev()
> > parameter to retrieve that region while also adhering to CXL subsystem
> > locking and lifetime rules.
> 
> 
> I doubt this approach is safer than the type2 v25 case. 

I believe it is safer, see evidence below...

> Apparently you are reducing the potential concurrency issue if someone
> removes cxl_acpi, but I think it is as strong as the v25 approach.
> With the rwsem lock, the region will exist or it will not. If the
> removal happens after the driver gets the region, the ioremap can
> race, with your approach and with the v25 one.

Yes, ioremap can race and in the attach case when the ioremap is
invalidated the sfc driver gets unloaded. In v25 it leads to
use-after-free issues.

> Also, you are keen to use your attach function for doing the same thing 
> I have, which needs to be justified.

No, you need to justify usage and export of cxl_core objects without
taking care of the current lifetime rules. Did you see these reports
from sashiko about v25? These issues are the motivation to pull
responsibility away from client drivers.

https://sashiko.dev/#/patchset/20260330143827.1278677-1-alejandro.lucero-palau%40amd.com?part=6
https://sashiko.dev/#/patchset/20260330143827.1278677-1-alejandro.lucero-palau%40amd.com?part=7

Now, I do not think the "attach" appraoch is the final end state. This
needs to get simpler still, but it can not get simpler with an
increasing attack surface.

> I can not see a reason for that attach functionality with the sfc
> case. You did not reply to that comment:
> 
> 
> https://lore.kernel.org/linux-cxl/20260330143827.1278677-1-alejandro.lucero-palau@amd.com/T/#m21968fd0ddf02df3641194d1450cb2bd392def26

Yes, did not have bandwidth to reply to that beyond sending out the
attach approach to better contain CXL core complexity.

> And although I have not tested it, I think this is buggy. Removal of 
> cxl_acpi will trigger unregister_region and a subsequent driver exit 
> will invoke, inside the autoremove call linked to the endpoint device, 
> devm_release_action on something not existing anymore.

If there is a bug here I would like to see that test case because as far
as I can see it would be shared with the generic expander case.

> It is the same problem with v25 type2 and potential cxl_acpi removal.
> Maybe we should avoid linking a type2-related region to the cxl root
> port ...
> 
> 
> And IMO, hiding what we are doing to the user/driver is not convenient. 
> Although current type2 is going to rely on a committed decoder, that 
> will not be the case if we look beyond impending needs.

Yes, start with the simple committed decoder case. Later, follow-on with
auto-create support. That would have no need to go back and teach
accelerator drivers how to create regions in the simple case the same
helper would "just work".

> A function with a name describing this situation can add clarity to
> the different possibilities to support as a follow up of this initial
> work. About naming, attaching a region to a memdev is confusing ... as
> it is already "attached" or linked, and the reason you give for having
> that explicit attachment not being clear from a safety point of view.

I would be happy if the only detail left to discuss was naming.

In this case "attach" means "attach to CXL the topology". A PCI driver
can create a cxl_memdev, but if that cxl_memdev finds no active CXL
topology then that "attachment" fails.