[PATCH 1/2] PCI: dwc: ep: Fix broken set_msix() callback

[PATCH 1/2] PCI: dwc: ep: Fix broken set_msix() callback

[Niklas Cassel]

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-designware-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: 83153d9f36e2 ("PCI: endpoint: Fix ->set_msix() to take BIR and offset as arguments")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/controller/dwc/pcie-designware-ep.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 1a0bf9341542..24026f3f3413 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -585,6 +585,7 @@ static int dw_pcie_ep_set_msix(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
 	struct dw_pcie_ep_func *ep_func;
 	u32 val, reg;
+	u16 actual_interrupts = interrupts + 1;
 
 	ep_func = dw_pcie_ep_get_func_from_ep(ep, func_no);
 	if (!ep_func || !ep_func->msix_cap)
@@ -595,7 +596,7 @@ static int dw_pcie_ep_set_msix(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 	reg = ep_func->msix_cap + PCI_MSIX_FLAGS;
 	val = dw_pcie_ep_readw_dbi(ep, func_no, reg);
 	val &= ~PCI_MSIX_FLAGS_QSIZE;
-	val |= interrupts;
+	val |= interrupts; /* 0's based value */
 	dw_pcie_writew_dbi(pci, reg, val);
 
 	reg = ep_func->msix_cap + PCI_MSIX_TABLE;
@@ -603,7 +604,7 @@ static int dw_pcie_ep_set_msix(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 	dw_pcie_ep_writel_dbi(ep, func_no, reg, val);
 
 	reg = ep_func->msix_cap + PCI_MSIX_PBA;
-	val = (offset + (interrupts * PCI_MSIX_ENTRY_SIZE)) | bir;
+	val = (offset + (actual_interrupts * PCI_MSIX_ENTRY_SIZE)) | bir;
 	dw_pcie_ep_writel_dbi(ep, func_no, reg, val);
 
 	dw_pcie_dbi_ro_wr_dis(pci);
-- 
2.49.0

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

[Niklas Cassel]

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;
-- 
2.49.0

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

[Wilfred Mallawa]

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

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

[Damien Le Moal]

On 4/30/25 21:32, 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>

Looks good to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/2] PCI: dwc: ep: Fix broken set_msix() callback

[Wilfred Mallawa]

On Wed, 2025-04-30 at 14:31 +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-designware-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: 83153d9f36e2 ("PCI: endpoint: Fix ->set_msix() to take BIR and
> offset as arguments")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/pci/controller/dwc/pcie-designware-ep.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c
> b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 1a0bf9341542..24026f3f3413 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -585,6 +585,7 @@ static int dw_pcie_ep_set_msix(struct pci_epc
> *epc, u8 func_no, u8 vfunc_no,
>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>  	struct dw_pcie_ep_func *ep_func;
>  	u32 val, reg;
> +	u16 actual_interrupts = interrupts + 1;
>  
>  	ep_func = dw_pcie_ep_get_func_from_ep(ep, func_no);
>  	if (!ep_func || !ep_func->msix_cap)
> @@ -595,7 +596,7 @@ static int dw_pcie_ep_set_msix(struct pci_epc
> *epc, u8 func_no, u8 vfunc_no,
>  	reg = ep_func->msix_cap + PCI_MSIX_FLAGS;
>  	val = dw_pcie_ep_readw_dbi(ep, func_no, reg);
>  	val &= ~PCI_MSIX_FLAGS_QSIZE;
> -	val |= interrupts;
> +	val |= interrupts; /* 0's based value */
>  	dw_pcie_writew_dbi(pci, reg, val);
>  
>  	reg = ep_func->msix_cap + PCI_MSIX_TABLE;
> @@ -603,7 +604,7 @@ static int dw_pcie_ep_set_msix(struct pci_epc
> *epc, u8 func_no, u8 vfunc_no,
>  	dw_pcie_ep_writel_dbi(ep, func_no, reg, val);
>  
>  	reg = ep_func->msix_cap + PCI_MSIX_PBA;
> -	val = (offset + (interrupts * PCI_MSIX_ENTRY_SIZE)) | bir;
> +	val = (offset + (actual_interrupts * PCI_MSIX_ENTRY_SIZE)) |
> bir;
>  	dw_pcie_ep_writel_dbi(ep, func_no, reg, val);
>  
>  	dw_pcie_dbi_ro_wr_dis(pci);

Reviewed-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>

Wilfred

Re: [PATCH 1/2] PCI: dwc: ep: Fix broken set_msix() callback

[Damien Le Moal]

On 4/30/25 21:31, 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'.

It would be nice to fix this discrepancy between the high level api and the
driver operatrion as it is awfully confusing... But that is separate from this fix.

> 
> While the set_msix() callback function in pcie-designware-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: 83153d9f36e2 ("PCI: endpoint: Fix ->set_msix() to take BIR and offset as arguments")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/2] PCI: dwc: ep: Fix broken set_msix() callback

[Manivannan Sadhasivam]

On Wed, Apr 30, 2025 at 02:31:59PM +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(),

Only {get/set}_msix() callbacks were having this behavior, right?

> even though the
> parameter name for the callback function is also named 'interrupts'.
> 
> While the set_msix() callback function in pcie-designware-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.
> 

Thanks for the fix! We should also fix the API discrepancy w.r.t interrupts as
it is causing much of a headache. One more example is the interrupt vector. API
expects the vectors to be 1-based, but in reality, vectors start from 0. So the
callers of raise_irq() has to increment the vector and the implementation has to
decrement it.

If you want to fix it up too, let me know. Otherwise, I may do it at some point.

> Fixes: 83153d9f36e2 ("PCI: endpoint: Fix ->set_msix() to take BIR and offset as arguments")

This doesn't seem like the correct fixes commit.

- Mani

> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/pci/controller/dwc/pcie-designware-ep.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 1a0bf9341542..24026f3f3413 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -585,6 +585,7 @@ static int dw_pcie_ep_set_msix(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>  	struct dw_pcie_ep_func *ep_func;
>  	u32 val, reg;
> +	u16 actual_interrupts = interrupts + 1;
>  
>  	ep_func = dw_pcie_ep_get_func_from_ep(ep, func_no);
>  	if (!ep_func || !ep_func->msix_cap)
> @@ -595,7 +596,7 @@ static int dw_pcie_ep_set_msix(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  	reg = ep_func->msix_cap + PCI_MSIX_FLAGS;
>  	val = dw_pcie_ep_readw_dbi(ep, func_no, reg);
>  	val &= ~PCI_MSIX_FLAGS_QSIZE;
> -	val |= interrupts;
> +	val |= interrupts; /* 0's based value */
>  	dw_pcie_writew_dbi(pci, reg, val);
>  
>  	reg = ep_func->msix_cap + PCI_MSIX_TABLE;
> @@ -603,7 +604,7 @@ static int dw_pcie_ep_set_msix(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  	dw_pcie_ep_writel_dbi(ep, func_no, reg, val);
>  
>  	reg = ep_func->msix_cap + PCI_MSIX_PBA;
> -	val = (offset + (interrupts * PCI_MSIX_ENTRY_SIZE)) | bir;
> +	val = (offset + (actual_interrupts * PCI_MSIX_ENTRY_SIZE)) | bir;
>  	dw_pcie_ep_writel_dbi(ep, func_no, reg, val);
>  
>  	dw_pcie_dbi_ro_wr_dis(pci);
> -- 
> 2.49.0
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 1/2] PCI: dwc: ep: Fix broken set_msix() callback

[Niklas Cassel]

On Sat, May 10, 2025 at 11:27:55AM +0530, Manivannan Sadhasivam wrote:
> On Wed, Apr 30, 2025 at 02:31:59PM +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(),
> 
> Only {get/set}_msix() callbacks were having this behavior, right?

pci_epc_get_msix() adds 1 to the return of epc->ops->get_msix().
pci_epc_set_msix() subtracts 1 to the parameter sent to epc->ops->set_msix().

pci_epc_get_msi() does 1 << interrupt from the return of epc->ops->get_msi().
So a return of 0 from epc->ops->get_msi() will result in pci_epc_get_msi()
returning 1. A return of 1 from epc->ops->get_msi() will result in
pci_epc_get_msi() returning 2.

Similar for pci_epc_set_msi(). It will call order_base_2() on the parameter
before sending it to epc->ops->set_msi().

So pci_epc_get_msi() / pci_epc_set_msi() takes a interrupts parameter
that actually represents the number of interrupts.

epc->ops->set_msi() / epc->ops->get_msi() takes an interrupts parameter,
but that value does NOT represent the actual number of interrupts.
It is instead the value encoded per the Multiple Message Enable (MME)
field in the "7.7.1.2 Message Control Register for MSI". So it is quite
confusing that these the parameter for epc->ops->set_msi() /
epc->ops->get_msi() is also called interrupts. A better parameter name
would have been "mme".

It is however called 'interrupts' in the pci-epc.h header:
https://github.com/torvalds/linux/blob/v6.15-rc5/include/linux/pci-epc.h#L102-L103

and in the DWC driver and RCar driver:
static int dw_pcie_ep_set_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no, u8 interrupts)
static int rcar_pcie_ep_set_msi(struct pci_epc *epc, u8 fn, u8 vfn, u8 interrupts)

However, some drivers have seen this weirdness and actually named the parameter
differently:
static int cdns_pcie_ep_set_msi(struct pci_epc *epc, u8 fn, u8 vfn, u8 mmc)
static int rockchip_pcie_ep_set_msi(struct pci_epc *epc, u8 fn, u8 vfn, u8 multi_msg_cap)

TL;DR: all of these callbacks are a mess IMO, not only {get/set}_msix().

I did the smallest fix possible, because doing a cleanup of this will
require changing all drivers that implement these callbacks, which seems
different from a fix.


> 
> > even though the
> > parameter name for the callback function is also named 'interrupts'.
> > 
> > While the set_msix() callback function in pcie-designware-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.
> > 
> 
> Thanks for the fix! We should also fix the API discrepancy w.r.t interrupts as
> it is causing much of a headache. One more example is the interrupt vector. API
> expects the vectors to be 1-based, but in reality, vectors start from 0. So the
> callers of raise_irq() has to increment the vector and the implementation has to
> decrement it.
> 
> If you want to fix it up too, let me know. Otherwise, I may do it at some point.

As you know, I'm working on adding SRNS/SRIS support for dw-rockchip,
I might send a cleanup for the Tegra driver too.

I do not intend to clean up all the drivers.
I am a bit worried about patches after the cleanup getting backported, which
will need to be different before and after this cleanup. Perhaps renaming the
callbacks at the same as the cleanup might be a good idea?

It should be a simple cleanup though, just do the order_base_2() etc in the
driver callbacks themselves.

We really should rename the parameter of .set_msi() though, as it is totally
misleading as of now.


> 
> > Fixes: 83153d9f36e2 ("PCI: endpoint: Fix ->set_msix() to take BIR and offset as arguments")
> 
> This doesn't seem like the correct fixes commit.

It is correct.

Commit 83153d9f36e2 ("PCI: endpoint: Fix ->set_msix() to take BIR and offset as arguments")
added the PBA offset to dw_pcie_ep_set_msix().

dw_pcie_ep_set_msix() already wrote the interrupts value (zeroes based) to the
QSIZE register. Commit 83153d9f36e2 added this code:
+       val = (offset + (interrupts * PCI_MSIX_ENTRY_SIZE)) | bir;

So it used a zeroes based value to calculate the size,
which is obviously wrong.


Kind regards,
Niklas

Re: [PATCH 1/2] PCI: dwc: ep: Fix broken set_msix() callback

[Manivannan Sadhasivam]

On Sat, May 10, 2025 at 01:43:39PM +0200, Niklas Cassel wrote:
> On Sat, May 10, 2025 at 11:27:55AM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Apr 30, 2025 at 02:31:59PM +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(),
> > 
> > Only {get/set}_msix() callbacks were having this behavior, right?
> 
> pci_epc_get_msix() adds 1 to the return of epc->ops->get_msix().
> pci_epc_set_msix() subtracts 1 to the parameter sent to epc->ops->set_msix().
> 
> pci_epc_get_msi() does 1 << interrupt from the return of epc->ops->get_msi().
> So a return of 0 from epc->ops->get_msi() will result in pci_epc_get_msi()
> returning 1. A return of 1 from epc->ops->get_msi() will result in
> pci_epc_get_msi() returning 2.
> 
> Similar for pci_epc_set_msi(). It will call order_base_2() on the parameter
> before sending it to epc->ops->set_msi().
> 

Yeah. I was pointing out the fact that bitshifting != incrementing/decrementing
1. And you just mentioned the latter for both msi/msix callbacks. I can ammend
it while applying.

> So pci_epc_get_msi() / pci_epc_set_msi() takes a interrupts parameter
> that actually represents the number of interrupts.
> 
> epc->ops->set_msi() / epc->ops->get_msi() takes an interrupts parameter,
> but that value does NOT represent the actual number of interrupts.
> It is instead the value encoded per the Multiple Message Enable (MME)
> field in the "7.7.1.2 Message Control Register for MSI". So it is quite
> confusing that these the parameter for epc->ops->set_msi() /
> epc->ops->get_msi() is also called interrupts. A better parameter name
> would have been "mme".
> 

Yeah. But I would try not to name it as 'mme' and rather keep the semantics for
'interrupt'.

> It is however called 'interrupts' in the pci-epc.h header:
> https://github.com/torvalds/linux/blob/v6.15-rc5/include/linux/pci-epc.h#L102-L103
> 
> and in the DWC driver and RCar driver:
> static int dw_pcie_ep_set_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no, u8 interrupts)
> static int rcar_pcie_ep_set_msi(struct pci_epc *epc, u8 fn, u8 vfn, u8 interrupts)
> 
> However, some drivers have seen this weirdness and actually named the parameter
> differently:
> static int cdns_pcie_ep_set_msi(struct pci_epc *epc, u8 fn, u8 vfn, u8 mmc)
> static int rockchip_pcie_ep_set_msi(struct pci_epc *epc, u8 fn, u8 vfn, u8 multi_msg_cap)
> 
> TL;DR: all of these callbacks are a mess IMO, not only {get/set}_msix().
> 
> I did the smallest fix possible, because doing a cleanup of this will
> require changing all drivers that implement these callbacks, which seems
> different from a fix.
> 

Agree.

> 
> > 
> > > even though the
> > > parameter name for the callback function is also named 'interrupts'.
> > > 
> > > While the set_msix() callback function in pcie-designware-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.
> > > 
> > 
> > Thanks for the fix! We should also fix the API discrepancy w.r.t interrupts as
> > it is causing much of a headache. One more example is the interrupt vector. API
> > expects the vectors to be 1-based, but in reality, vectors start from 0. So the
> > callers of raise_irq() has to increment the vector and the implementation has to
> > decrement it.
> > 
> > If you want to fix it up too, let me know. Otherwise, I may do it at some point.
> 
> As you know, I'm working on adding SRNS/SRIS support for dw-rockchip,
> I might send a cleanup for the Tegra driver too.
> 
> I do not intend to clean up all the drivers.

Okay, that's totally fine.

> I am a bit worried about patches after the cleanup getting backported, which
> will need to be different before and after this cleanup.

We can add stable+noautosel to mark the patches to not backport.

> Perhaps renaming the
> callbacks at the same as the cleanup might be a good idea?
> 
> It should be a simple cleanup though, just do the order_base_2() etc in the
> driver callbacks themselves.
> 
> We really should rename the parameter of .set_msi() though, as it is totally
> misleading as of now.
> 

As I said above, we should keep the semantics for 'interrupt' and make changes
accordingly i.e., by doing order_base_2() inside the callbacks as you suggested.

> 
> > 
> > > Fixes: 83153d9f36e2 ("PCI: endpoint: Fix ->set_msix() to take BIR and offset as arguments")
> > 
> > This doesn't seem like the correct fixes commit.
> 
> It is correct.
> 
> Commit 83153d9f36e2 ("PCI: endpoint: Fix ->set_msix() to take BIR and offset as arguments")
> added the PBA offset to dw_pcie_ep_set_msix().
> 
> dw_pcie_ep_set_msix() already wrote the interrupts value (zeroes based) to the
> QSIZE register. Commit 83153d9f36e2 added this code:
> +       val = (offset + (interrupts * PCI_MSIX_ENTRY_SIZE)) | bir;
> 
> So it used a zeroes based value to calculate the size,
> which is obviously wrong.
> 

Ah okay. I only looked into the 'interrupt' argument and missed the PBA change.
Thanks for clarifying.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 1/2] PCI: dwc: ep: Fix broken set_msix() callback

[Niklas Cassel]

On Mon, May 12, 2025 at 01:00:02PM +0530, Manivannan Sadhasivam wrote:
> On Sat, May 10, 2025 at 01:43:39PM +0200, Niklas Cassel wrote:
> > On Sat, May 10, 2025 at 11:27:55AM +0530, Manivannan Sadhasivam wrote:
> > > On Wed, Apr 30, 2025 at 02:31:59PM +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(),
> > > 
> > > Only {get/set}_msix() callbacks were having this behavior, right?
> > 
> > pci_epc_get_msix() adds 1 to the return of epc->ops->get_msix().
> > pci_epc_set_msix() subtracts 1 to the parameter sent to epc->ops->set_msix().
> > 
> > pci_epc_get_msi() does 1 << interrupt from the return of epc->ops->get_msi().
> > So a return of 0 from epc->ops->get_msi() will result in pci_epc_get_msi()
> > returning 1. A return of 1 from epc->ops->get_msi() will result in
> > pci_epc_get_msi() returning 2.
> > 
> > Similar for pci_epc_set_msi(). It will call order_base_2() on the parameter
> > before sending it to epc->ops->set_msi().
> > 
> 
> Yeah. I was pointing out the fact that bitshifting != incrementing/decrementing
> 1. And you just mentioned the latter for both msi/msix callbacks. I can ammend
> it while applying.

Thank you!



> > I am a bit worried about patches after the cleanup getting backported, which
> > will need to be different before and after this cleanup.
> 
> We can add stable+noautosel to mark the patches to not backport.
> 
> > Perhaps renaming the
> > callbacks at the same as the cleanup might be a good idea?
> > 
> > It should be a simple cleanup though, just do the order_base_2() etc in the
> > driver callbacks themselves.
> > 
> > We really should rename the parameter of .set_msi() though, as it is totally
> > misleading as of now.
> > 
> 
> As I said above, we should keep the semantics for 'interrupt' and make changes
> accordingly i.e., by doing order_base_2() inside the callbacks as you suggested.

Yeah, I agree, better to rename the parameter of the existing callbacks that use
mmc/mme to interrupts, and actually perform the cleanup so that the take interrupts
instead of mmc/mme.


Considering that it is only 4 drivers, with a maximum of 4 callbacks in each driver
that needs to be cleaned up, it's not so bad.

Since we seem to have identified all the weirdness with the existing APIs,
let me try to cleanup this mess this week.


(Would be nice to get this fixed queued first though, so cleanup patches can
come on top.)


Kind regards,
Niklas

Re: [PATCH 1/2] PCI: dwc: ep: Fix broken set_msix() callback

[Niklas Cassel]

On Mon, May 12, 2025 at 11:28:33AM +0200, Niklas Cassel wrote:
> 
> (Would be nice to get this fixed queued first though, so cleanup patches can
> come on top.)

I'm done with the cleanups,
but since you wanted me to rewrite the commit message,
let me send out a V2 which has the strict fix as the first two patches
(i.e. the two patches in this series),
and then put the cleanups on top, so that it will be a single series.


Kind regards,
Niklas