Re: [PATCH v16 02/22] sfc: add cxl support

[Dan Williams <dan.j.williams@intel.com>]

Alejandro Lucero Palau wrote:
> 
> On 5/21/25 18:12, Dan Williams wrote:
> > Alejandro Lucero Palau wrote:
> > [..]
> >>>> +void efx_cxl_exit(struct efx_probe_data *probe_data)
> >>>> +{
> >>> So this is empty which means it leaks the cxl_dev_state_create()
> >>> allocation, right?
> >>
> >> Yes, because I was wrongly relying on devres ...
> >>
> >>
> >> Previous patchsets were doing the explicit release here.
> >>
> >>
> >> Your suggestion below relies on adding more awareness of cxl into
> >> generic efx code, what we want to avoid using the specific efx_cxl.* files.
> >>
> >> As I mentioned in patch 1, I think the right thing to do is to add
> >> devres for cxl_dev_state_create.
> > ...but I thought netdev is anti-devres? I am ok having a
> > devm_cxl_dev_state_create() alongside a "manual" cxl_dev_state_create()
> > if that is the case.
> 
> 
> But a netdev is using the CXL API where devres is being used already. 
> AFAIK, netdev maintainers prefer to not use devres by netdev drivers, 
> but I do not think they can impose their view to external API, mainly 
> when other driver types could likely also make use of it in the future.

From the CXL perspective I am neutral. As long as the parallel manual
interfaces arrange to undo everything it "should just work (TM)". You
would need to create the manual version of devm_cxl_add_memdev(), and
audit that the paired cxl_del_memdev() does not result in any cxl_core
internal devres events to leak past the ->remove() event for the
accelerator driver.

> >> Before sending v17 with this change, are you ok with the rest of the
> >> patches or you want to go through them as well?
> > So I did start taking a look and then turned away upon finding a
> > memory-leak on the first 2 patches in the series. I will continue going
> > through it, but in general the lifetime and locking rules of the CXL
> > subsystem continue to be a source of trouble in new enabling. At a
> > minimum that indicates a need/opportunity to review the rules at a
> > future CXL collab meeting.
> 
> Great. And I agree about potential improvements mostly required after 
> all this new code (hopefully) ends up being merged, which I'll be happy 
> to contribute. Also, note this patchset original RFC and  cover letters 
> since then states "basic Type2 support".

It would help to define "basic" in terms of impact. How much end-user
benefit arrives at this stage, and what is driving motivation to go
beyond basic. E.g. "PIO buffer in CXL == X amount of goodness, and Y
amount of goodness comes with additional changes".