PCI: dwc: ep: Fix broken set_msix() callback

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 <[email protected]>

---
 drivers/pci/controller/cadence/pcie-cadence-ep.c | 5 +++--

 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/pci/controller/cadence/pcie-cadence-ep.c +1 -0

index 599ec4b1223e..112ae200b393 100644
@@ -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;

drivers/pci/controller/cadence/pcie-cadence-ep.c +1 -1

-	val |= interrupts;
+	val |= interrupts; /* 0's based value */

  cdns_pcie_ep_fn_writew(pcie, fn, reg, val);
  /* Set MSI-X BAR and offset */

drivers/pci/controller/cadence/pcie-cadence-ep.c +0 -0

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

drivers/pci/controller/cadence/pcie-cadence-ep.c +1 -1

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

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 <[email protected]>

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 <[email protected]>

---
 drivers/pci/controller/dwc/pcie-designware-ep.c | 5 +++--

 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/pci/controller/dwc/pcie-designware-ep.c +1 -0

index 1a0bf9341542..24026f3f3413 100644
@@ -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)

drivers/pci/controller/dwc/pcie-designware-ep.c +1 -1

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

drivers/pci/controller/dwc/pcie-designware-ep.c +0 -0

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

drivers/pci/controller/dwc/pcie-designware-ep.c +1 -1

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

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 <[email protected]>

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 <[email protected]>

---
 drivers/pci/controller/dwc/pcie-designware-ep.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/pci/controller/dwc/pcie-designware-ep.c +3 -2

index 1a0bf9341542..24026f3f3413 100644
@@ -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);

Signature · 1 line

2.49.0

Quoted 5 lines · 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.

Quoted 4 lines

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?

Quoted 8 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

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

This doesn't seem like the correct fixes commit.

Quoted 6 lines · Manivannan Sadhasivam

Quoted 5 lines · 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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 22 lines

Quoted 9 lines

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.

Quoted 6 lines

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.

Quoted 1 line

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.

Quoted 5 lines

Quoted 3 lines

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.

Quoted 22 lines · Niklas Cassel

Quoted 6 lines · Manivannan Sadhasivam

Quoted 5 lines · 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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 3 lines

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.

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