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

["Lucero Palau, Alejandro" <alejandro.lucero-palau@amd.com>]

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.


>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>   drivers/net/ethernet/sfc/efx_cxl.c | 75 ++++++++++++++++++++++++++----
>>   1 file changed, 66 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
>> index 348d7404cd7a..bed8d9c59185 100644
>> --- a/drivers/net/ethernet/sfc/efx_cxl.c
>> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
>> @@ -13,6 +13,31 @@
>>   #include "efx_cxl.h"
>>   
>>   #define EFX_CTPIO_BUFFER_SIZE	SZ_256M
>> +#define EFX_CTPIO_BUFFER_PER_PF_SIZE	SZ_8M
>> +
>> +static int cxl_map(struct efx_probe_data *probe_data, struct efx_cxl *cxl,
>> +		   u64 devfn, struct range cxl_pio_range)
>> +{
>> +	struct efx_nic *efx = &probe_data->efx;
>> +	struct pci_dev *pci_dev = efx->pci_dev;
>> +	u64 cxl_pio_pf_start;
>> +
>> +	cxl_pio_pf_start = cxl_pio_range.start + devfn *
>> +			   EFX_CTPIO_BUFFER_PER_PF_SIZE;
> Wrap as per operator precedence as easier to read.
>
> 			   cxl_pio_range.start +
> 			   devfn * EFX_CTPIO_BUFFER_PER_SIZE;


OK


>> +
>> +	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?


>> +
>> +	return 0;
>> +}
>>   
>>   int efx_cxl_init(struct efx_probe_data *probe_data)
>>   {
>> @@ -21,8 +46,48 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
>>   	struct range cxl_pio_range;
>>   	struct efx_cxl *cxl;
>>   	u16 dvsec;
>> +	u8 devfn;
>>   	int rc;
>>   
>> +	if (efx->type->is_vf)
>> +		return 0;
>> +
>> +	/* are we PF0? */
> First things we ask seems to be Are we not PF0?


Yeah. I will change that.


>> +	devfn = PCI_FUNC(pci_dev->devfn);
>> +	if (devfn != 0) {
> I'd factor this lot out as a helper to slightly improve readability.
> Perhaps factor out both paths and then have an if else.


Yes, I think this makes sense.


>
>> +		struct pci_dev *pf0_pci_dev;
>> +		struct cxl_memdev *cxlmd;
>> +
>> +		pf0_pci_dev = pci_get_slot(pci_dev->bus,
>> +					   PCI_DEVFN(PCI_SLOT(pci_dev->devfn),
>> +						     0));
>> +		/* This should not happen! */
>> +		if (!pf0_pci_dev)
>> +			return 0;
>> +
>> +		/* Is the PF0 device configured with and using CXL? */
>> +		if (!pcie_is_cxl(pf0_pci_dev))
>> +			return 0;
>> +
>> +		cxlmd = cxl_get_pf0_memdev(&pf0_pci_dev->dev, &pci_dev->dev,
>> +					   &cxl_pio_range);
>> +
>> +		if (IS_ERR(cxlmd))
>> +			return -EPROBE_DEFER;
>> +
>> +		cxl = kzalloc_obj(*cxl, GFP_KERNEL);
>> +		if (!cxl)
>> +			return -ENOMEM;
>> +
>> +		cxl->cxlmd = cxlmd;
>> +
>> +		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.


Thanks,

Alejandro


>> +		}
>> +		return 0;
>> +	}
>> +
>>   	/* Is the device configured with and using CXL? */
>>   	if (!pcie_is_cxl(pci_dev))
>>   		return 0;
>> @@ -80,16 +145,8 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
>>   		return PTR_ERR(cxl->cxlmd);
>>   	}
>>   
>> -	cxl->ctpio_cxl = ioremap_wc(cxl_pio_range.start,
>> -				    range_len(&cxl_pio_range));
>> -	if (!cxl->ctpio_cxl) {
>> -		pci_err(pci_dev, "CXL ioremap region (%pra) failed\n",
>> -			&cxl_pio_range);
>> +	if (!cxl_map(probe_data, cxl, 0, cxl_pio_range))
>>   		return -ENOMEM;
>> -	}
>> -
>> -	probe_data->cxl_pio_initialised = true;
>> -	probe_data->cxl = cxl;
>>   
>>   	return 0;
>>   }