Re: [PATCH v2 6/6] cxl/mem: Introduce cxl_memdev_attach for CXL-dependent operation

[dan.j.williams]

Jonathan Cameron wrote:
> On Wed, 17 Dec 2025 08:27:00 -0800
> dan.j.williams@xxxxxxxxx wrote:
> 
> > Jonathan Cameron wrote:
> > > On Mon, 15 Dec 2025 16:56:16 -0800
> > > Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
> > >   
> > > > Unlike the cxl_pci class driver that opportunistically enables memory
> > > > expansion with no other dependent functionality, CXL accelerator drivers
> > > > have distinct PCIe-only and CXL-enhanced operation states. If CXL is
> > > > available some additional coherent memory/cache operations can be enabled,
> > > > otherwise traditional DMA+MMIO over PCIe/CXL.io is a fallback.
> > > > 
> > > > Allow for a driver to pass a routine to be called in cxl_mem_probe()
> > > > context. This ability is inspired by and mirrors the semantics of
> > > > faux_device_create(). It allows for the caller to run CXL-topology
> > > > attach-dependent logic on behalf of the caller.  
> > > 
> > > This seems confusing.   
> > 
> > Is faux_device_create() confusing?
> 
> Just to be clear this question is all about the word 'caller' being repeated
> in that sentence. Not about the code itself or anything else in the explanation
> or flow. 

Oh, sorry, I took it as the design was confusing.

> This comment that reads very oddly to me and I think means something very
> different from what is going on here.
> 
> > 
> > > The caller is running logic for the caller?  It can do that whenever
> > > it likes!  One of those is presumably callee  
> > 
> > No, it cannot execute CXL topology attach dependendent functionality in
> > the device's initial probe context synchronous with the device-attach
> > event "whenever it likes".
> 
> I'm still lost. Why 'caller to run' ... 'on behalf of the caller.'  In this case
> caller is in both cases the function calling cxl_memdev_alloc()?
> 
> Maybe something like
> 
> "This arranges for CXL-topology attach-dependent logic to be run later, on behalf of
> the caller."
> 
> Though that kind of repeats what follows, so maybe just drop the sentence.

How about this reworked changelog?

---

Unlike the cxl_pci class driver that opportunistically enables memory
expansion with no other dependent functionality, CXL accelerator drivers
have distinct PCIe-only and CXL-enhanced operation states. If CXL is
available some additional coherent memory/cache operations can be enabled,
otherwise traditional DMA+MMIO over PCIe/CXL.io is a fallback.

This constitutes a new mode of operation where the caller of
devm_cxl_add_memdev() wants to make a "go/no-go" decision about running
in CXL accelerated mode or falling back to PCIe-only operation. Part of
that decision making process likely also includes additional
CXL-acceleration-specific resource setup. Encapsulate both of those
requirements into 'struct cxl_memdev_attach' that provides a ->probe()
callback. The probe callback runs in cxl_mem_probe() context, after the
port topology is successfully attached for the given memdev. It supports
a contract where, upon successful return from devm_cxl_add_memdev(),
everything needed for CXL accelerated operation has been enabled.

Additionally the presence of @cxlmd->attach indicates that the accelerator
driver be detached when CXL operation ends. This conceptually makes a CXL
link loss event mirror a PCIe link loss event which results in triggering
the ->remove() callback of affected devices+drivers. A driver can re-attach
to recover back to PCIe-only operation. Live recovery, i.e. without a
->remove()/->probe() cycle, is left as a future consideration.

---

> > > > @@ -1081,6 +1093,18 @@ static struct cxl_memdev *cxl_memdev_autoremove(struct cxl_memdev *cxlmd)
> > > >  {
> > > >  	int rc;
> > > >  
> > > > +	/*  
> > > 
> > > The general approach is fine but is the function name appropriate for this
> > > new stuff?  Naturally I'm not suggesting the bikeshed should be any particular
> > > alternative color just maybe not the current blue.  
> > 
> > The _autoremove() verb appears multiple times in the subsystem, not sure
> > why it is raising bikeshed concerns now. Please send a new proposal if
> > "autoremove" is not jibing.
> 
> It felt like a stretch given the additional code that is not about registering
> autoremove for later, but doing it now under some circumstances.  Ah well I don't
> care that much what it's called.

It is the same semantic as devm_add_action_or_reset() in that if the
"add_action" fails then the "reset" is triggered.

Yes, there is additional code that validates that the device to be
registered for removal is attached to its driver. That organization
supports having a single handoff from scoped-based cleanup to devm based
cleanup.

If you can think of a better organization and name I am open to hearing
options, but nothing better is immediately jumping out at me.