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

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Thu, 18 Dec 2025 11:50:10 -0800
<dan.j.williams@intel.com> wrote:

> Jonathan Cameron wrote:
> > On Wed, 17 Dec 2025 08:27:00 -0800
> > dan.j.williams@intel.com wrote:
> >   
> > > Jonathan Cameron wrote:  
> > > > On Mon, 15 Dec 2025 16:56:16 -0800
> > > > Dan Williams <dan.j.williams@intel.com> 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.

I should have been clearer!

> 
> > 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.  
Nice.  Thanks for the rewrite.

> 
> ---
> 
> > > > > @@ -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.
> 
Let's go with current naming.  I don't like the only options I can come up
with either.  If inspiration strikes we can always change it later.

Jonathan