Re: [PATCH v26 6/8] cxl: attach region to an accelerator/type2 memdev

["Dan Williams (nvidia)" <djbw@kernel.org>]

Alejandro Lucero Palau wrote:
> 
> On 5/2/26 01:46, Dan Williams (nvidia) wrote:
> > Alejandro Lucero Palau wrote:
> > [..]
> >>>> +	}
> >>> A couple problems here.
> >>>
> >>> 1/ Nothing stops a CXL class device from implementing a decoder with
> >>>      CXL_DECODER_DEVMEM (HDM-DB).
> >> Uhmmm...
> > Consider a helpful expander that does not require the host OS to use
> > cpu_cache_invalidate_memregion() whenever DPA space is changed. I
> > imagine that would be useful for DCD where there could be a higher
> > frequency of extent changes.
> 
> 
> What do you want here then? We had a param for specifically asking for 
> creating a dax device for the region, but it was considered unnecessary, 
> and I think it was you then. Should I put it back?

This comment was referring to the proposal to change the way region's
are released based on their decoder type. My point is that the way
region's are released must not depend on details like that.

Now, it took me longer than I wanted but I have a generic solution now
in hand, will post, that allows regions to be safely deleted either by
an endpoint callback or the root. It also addresses the issue Sungwoo
posted. There is still a tiny theoretical race if you are able to time
region deletion with devres_release_all(), but the practical /
repeatable devm_release_action() warning and double unregister bug is
gone.

> >>>> +	/* hold endpoint lock to setup autoremove of the region */
> >>>> +	guard(device)(&endpoint->dev);
> >>> This does not handle the case when ->endpoint is an ERR_PTR() because
> >>> the memdev never attached in the first instance.
> >> Not sure about this but, is it not the success of devm_cxl_add_memdev()
> >> ensuring this can not happen?
> > That is only ensured by using the "attach" mechanism.
> > devm_cxl_add_memdev(..., NULL) is only for the generic memory expander
> > case. Where the entire usage model is governed by memdev ABIs.
> >
> 
> Is your concern that the memdev probe could not happen synchronously so 
> a further call like the one implemented here could fail due to the 
> endpoint not there yet?

The concern is dropping and retaking locks which exposes some lifetime
details. I have a patch that introduces a devm_cxl_probe_mem() call,
that hides all the attachment details. It is functionally equivalent to
a standalone helper. I like that part of your proposal.

[..snip detach discussion, my proposal is leave out custom detach
complication for now..]

> > All drivers must already be prepared to be unloaded. So we start with
> > the simple semantic first to get this functionality landed and then
> > think about adding sophistication like live fallback to PCI operation.
> 
> 
> Ok. Let's try this one. You want to trigger device_release_driver or 
> something similar on the pci_dev->dev linked to the memdev. Right?
> 
> Do we have this support now?

Yes, cxl_test tests it for a long time with the existing type3 device,
and I tested it now again with Dave's type2 cxl_test device and the
proposed devm_cxl_probe_mem(). Thanks, Dave!

> If we do not, have you evaluated the complexity required for ensuring
> no deadlocks if this is triggered while the sfc driver is still
> probing?

The cxl_core invokes device_release_driver() from its own workqueue
context.

> Maybe I'm overthinking this option, so if you have a clear idea about 
> how to do this, please tell me, assuming it is a matter of calling such 
> a pci_dev "unbinding" from its current driver.

I do not blame you, especially with all the details getting generic
region teardown working. However, it is working and minimizes the
exported complexity outside the core.