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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <682f7ddf1f0b9_3e70100fb@dwillia2-xfh.jf.intel.com.notmuch>
Date: Thu, 22 May 2025 12:41:19 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: Alejandro Lucero Palau <alucerop@....com>, Dan Williams
	<dan.j.williams@...el.com>, <alejandro.lucero-palau@....com>,
	<linux-cxl@...r.kernel.org>, <netdev@...r.kernel.org>, <edward.cree@....com>,
	<davem@...emloft.net>, <kuba@...nel.org>, <pabeni@...hat.com>,
	<edumazet@...gle.com>, <dave.jiang@...el.com>
CC: Jonathan Cameron <Jonathan.Cameron@...wei.com>, Edward Cree
	<ecree.xilinx@...il.com>
Subject: Re: [PATCH v16 02/22] sfc: add cxl support

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".