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

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

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. 

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.

> 
> > > The probe callback runs after the port topology is successfully attached
> > > for the given memdev.
> > > 
> > > 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.

Jonathan