Re: [PATCH v29 4/5] sfc: obtain and map cxl range using devm_cxl_probe_mem

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

Alejandro Lucero Palau wrote:
[..]
> >> +{
> > If you are going to have an explicit efx_cxl_exit() then I would also
> > add an explicit unregistration of the memdev.
> 
> 
> This is necessary for undoing the mmap. Nothing else happens there 
> because it is all relying on devm ...
> 
> 
> I could change the ioremap_wc call to devm_ioremap_wc, but
> 
> 
> > This would also fix the
> > Sashiko report about pci_disable_device() running while the cxl_memdev
> > is still registered. Unfortunately, mixing devm and explicit unwind is
> > always fraught.
> 
> 
> I do not think there is a problem here. The cxl core does not need what 
> a type2 driver can do regarding PCI BAR mappings, or at least it is not 
> the case for sfc.
> 
> Any action through sysfs cxl will go through cxl core and the only thing 
> linked to the type device is the CXL registers which are mapped inside 
> cxl_map_component_regs() and those are managed resources.
> 
> 
> So, I can not see why this change is needed. If it is really necessary, 
> please describe the problem with more detail.
> 
> 
> It looks like you need reasons for delaying this further ...

What? Help with Sashiko reports is an act of malice? I assumed you
wanted help with those so that other maintainers would proceed with
these patches. 

I did do another run through to see if there are any paths that the CXL
core can reach if someone tried to fuzz the CXL ABIs or kernel paths
while SFC is unloading. I think Sashiko is hallucinating a sysfs path to
the BAR mapping given there is no mailbox and the EDAC capabilities are
usually not present on a type-2 device. The RAS path looks valid, but
that may also get lucky that most (all?) of the RAS use cases lock the
device before accessing the registers, so devres_release_all() would
become consistent with pci_disable_device() before any access attempt.
That does not seem like a clean design, but it is also does not appear
to be immediately exploitable.

If you believe the patches are ready and the Sashiko reports are
invalid, please do say so, no more comments from me on this set from
this point forward.

> > Let me know if this passes your testing, and I can send it out as a
> > standalone patch. You could also use it to unwind if the ioremap()
> > fails.
> 
> 
> You did not read my comments on v28 ...
> 
> 
> I changed efx_cxl_init to make the driver probe to fail if cxl is 
> supported and enabled but the cxl initialization fails, including 
> ioremap_wc(). What you proposed to do, explicitly undo cxl 
> initialization bits, has the same outcome: device detached from the driver.

Right, I did read that and that motivated the devm_cxl_remove_mem()
helper to undo the memdev creation without unloading the driver. You are
free to ignore that helper.