Re: [PATCH v1 4/4] sfc: add multipf support

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 25 Sep 2026 12:16:56 +0100
"Lucero Palau, Alejandro" <alejandro.lucero-palau@amd.com> wrote:

> On 24/09/2026 02:15, Jonathan Cameron wrote:
> > On Mon, 21 Sep 2026 20:12:39 +0100
> > <alucerop@amd.com> wrote:
> >  
> >> From: Alejandro Lucero <alucerop@amd.com>
> >>
> >> Use CXL core accelerator API for registering non-PF0 PFs to the memdev
> >> linked to the PF0, along with its complementary unregister.
> >>
> >> Adapt the ioremap call per PF to be an offset based on the PF function
> >> index and a hardcoded per PF CXL.mem slot size.  
> > I was wondering how you'd know what memory belonged to which one!
> > Simple solutions work best I suppose :)  
> 
> 
> Hi Jonathan,
> 
> 
> Yes, I think nowadays it is simple. I'm afraid if CXL usage increases 
> this will require some request to the firmware ... which could depend on 
> previous setting requests to that same firmware through fwctl.

Ultimately I'd kind of expect either an allocation mechanism where
we tell the device which portion of memory it has (nice if that was
shared architecture rather than a per device thing), or a way
to discover if in practice it is fixed (like here).

> 
> 
> >> +
> >> +	cxl->ctpio_cxl = ioremap_wc(cxl_pio_pf_start,
> >> +				    EFX_CTPIO_BUFFER_PER_PF_SIZE);
> >> +	if (!cxl->ctpio_cxl) {
> >> +		pci_err(pci_dev, "CXL ioremap region (%pra) failed\n",
> >> +			&cxl_pio_range);
> >> +		return -ENOMEM;
> >> +	}
> >> +
> >> +	probe_data->cxl = cxl;
> >> +	probe_data->cxl_pio_initialised = true;  
> > 'map' is carry quite a lot here that isn't really about mapping anything.
> > Maybe think a bit more on the naming?  
> 
> 
> Not sure I understand your complain as ioremap is being invoked here. 
> Maybe cxl_iomap or sfc_cxl_iomap as this is a static/local function 
> would address your concern?

The cxl_pio_initialized doesn't have anything to do with mapping as such.

> 
> 
> >> +
> >> +	return 0;
> >> +}
>
> >> +
> >> +		if (!cxl_map(probe_data, cxl, (u64)devfn, cxl_pio_range)) {
> >> +			kfree(cxl);
> >> +			return -ENOMEM;  
> > ENOMEM for a map failure? Seems a little odd but if there is precedence
> > fair enough.  
> 
> 
> Confused here. I can see ENOMEM being a common error if ioremap fails 
> through the kernel. Maybe this related to your previous concern about 
> the function naming, but cxl_map can only fail in one way and that being 
> not different to an ioremap failure.

Ok. If it's common choice than fine to stick with that.

> 
> 
> Thanks,
> 
> Alejandro