Re: [PATCH 2/2] PCI: cadence-ep: Fix broken set_msix() callback

[Wilfred Mallawa <wilfred.mallawa@wdc.com>]

On Wed, 2025-04-30 at 14:32 +0200, Niklas Cassel wrote:
> While the parameter 'interrupts' to the functions pci_epc_set_msi()
> and
> pci_epc_set_msix() represent the actual number of interrupts, and
> pci_epc_get_msi() and pci_epc_get_msix() return the actual number of
> interrupts.
> 
> These endpoint library functions just mentioned will however supply
> "interrupts - 1" to the EPC callback functions pci_epc_ops->set_msi()
> and
> pci_epc_ops->set_msix(), and likewise add 1 to return value from
> pci_epc_ops->get_msi() and pci_epc_ops->get_msix(), even though the
> parameter name for the callback function is also named 'interrupts'.
> 
> While the set_msix() callback function in pcie-cadence-ep writes the
> Table Size field correctly (N-1), the calculation of the PBA offset
> is wrong because it calculates space for (N-1) entries instead of N.
> 
> This results in e.g. the following error when using QEMU with PCI
> passthrough on a device which relies on the PCI endpoint subsystem:
> failed to add PCI capability 0x11[0x50]@0xb0: table & pba overlap, or
> they don't fit in BARs, or don't align
> 
> Fix the calculation of PBA offset in the MSI-X capability.
> 
> Fixes: 3ef5d16f50f8 ("PCI: cadence: Add MSI-X support to Endpoint
> driver")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/pci/controller/cadence/pcie-cadence-ep.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c
> b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> index 599ec4b1223e..112ae200b393 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> @@ -292,13 +292,14 @@ static int cdns_pcie_ep_set_msix(struct pci_epc
> *epc, u8 fn, u8 vfn,
>  	struct cdns_pcie *pcie = &ep->pcie;
>  	u32 cap = CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET;
>  	u32 val, reg;
> +	u16 actual_interrupts = interrupts + 1;
>  
>  	fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
>  
>  	reg = cap + PCI_MSIX_FLAGS;
>  	val = cdns_pcie_ep_fn_readw(pcie, fn, reg);
>  	val &= ~PCI_MSIX_FLAGS_QSIZE;
> -	val |= interrupts;
> +	val |= interrupts; /* 0's based value */
>  	cdns_pcie_ep_fn_writew(pcie, fn, reg, val);
>  
>  	/* Set MSI-X BAR and offset */
> @@ -308,7 +309,7 @@ static int cdns_pcie_ep_set_msix(struct pci_epc
> *epc, u8 fn, u8 vfn,
>  
>  	/* Set PBA BAR and offset.  BAR must match MSI-X BAR */
>  	reg = cap + PCI_MSIX_PBA;
> -	val = (offset + (interrupts * PCI_MSIX_ENTRY_SIZE)) | bir;
> +	val = (offset + (actual_interrupts * PCI_MSIX_ENTRY_SIZE)) |
> bir;
>  	cdns_pcie_ep_fn_writel(pcie, fn, reg, val);
>  
>  	return 0;
Reviewed-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>

Wilfred