[PATCH 0/2] PCI: dwc: ep: MSI/MSI-X checks

[PATCH 0/2] PCI: dwc: ep: MSI/MSI-X checks

[Manivannan Sadhasivam]

Hi,

This series adds checks mandated by the PCIe spec before raising MSI and MSI-X
from the DWC EP driver.

Note: I've compiled tested this series only. It'd be good if someone with EP
hardware can test it and report back.

- Mani

Manivannan Sadhasivam (2):
  PCI: dwc: ep: Add MSI Enable checks before raising MSI to the host
  PCI: dwc: ep: Add MSI-X Enable checks before raising MSI-X to the host

 .../pci/controller/dwc/pcie-designware-ep.c   | 52 ++++++++++++++++---
 1 file changed, 46 insertions(+), 6 deletions(-)

-- 
2.51.0

[PATCH 1/2] PCI: dwc: ep: Add MSI Enable checks before raising MSI to the host

[Manivannan Sadhasivam]

PCIe spec r7, sec 7.7.1.2 mandates that a Function should raise MSI only if
the MSI Enable bit is set and MSI-X enable bit is clear.

Hence, add those checks to be spec compliant with relevant helpers to avoid
code duplication.

Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 .../pci/controller/dwc/pcie-designware-ep.c   | 43 ++++++++++++++++---
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 295076cf70de..fcbd648e049e 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -672,6 +672,17 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 	return 0;
 }
 
+static bool dw_pcie_ep_msi_enabled(struct dw_pcie_ep *ep,
+				   struct dw_pcie_ep_func *ep_func, u8 func_no)
+{
+	u32 val, reg;
+
+	reg = ep_func->msi_cap + PCI_MSI_FLAGS;
+	val = dw_pcie_ep_readw_dbi(ep, func_no, reg);
+
+	return FIELD_GET(PCI_MSI_FLAGS_ENABLE, val);
+}
+
 static int dw_pcie_ep_get_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
 {
 	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
@@ -682,11 +693,11 @@ static int dw_pcie_ep_get_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
 	if (!ep_func || !ep_func->msi_cap)
 		return -EINVAL;
 
-	reg = ep_func->msi_cap + PCI_MSI_FLAGS;
-	val = dw_pcie_ep_readw_dbi(ep, func_no, reg);
-	if (!(val & PCI_MSI_FLAGS_ENABLE))
+	if (!dw_pcie_ep_msi_enabled(ep, ep_func, func_no))
 		return -EINVAL;
 
+	reg = ep_func->msi_cap + PCI_MSI_FLAGS;
+	val = dw_pcie_ep_readw_dbi(ep, func_no, reg);
 	val = FIELD_GET(PCI_MSI_FLAGS_QSIZE, val);
 
 	return 1 << val;
@@ -716,6 +727,17 @@ static int dw_pcie_ep_set_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 	return 0;
 }
 
+static bool dw_pcie_ep_msix_enabled(struct dw_pcie_ep *ep,
+				    struct dw_pcie_ep_func *ep_func, u8 func_no)
+{
+	u32 val, reg;
+
+	reg = ep_func->msix_cap + PCI_MSIX_FLAGS;
+	val = dw_pcie_ep_readw_dbi(ep, func_no, reg);
+
+	return FIELD_GET(PCI_MSIX_FLAGS_ENABLE, val);
+}
+
 static int dw_pcie_ep_get_msix(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
 {
 	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
@@ -726,11 +748,11 @@ static int dw_pcie_ep_get_msix(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
 	if (!ep_func || !ep_func->msix_cap)
 		return -EINVAL;
 
-	reg = ep_func->msix_cap + PCI_MSIX_FLAGS;
-	val = dw_pcie_ep_readw_dbi(ep, func_no, reg);
-	if (!(val & PCI_MSIX_FLAGS_ENABLE))
+	if (!dw_pcie_ep_msix_enabled(ep, ep_func, func_no))
 		return -EINVAL;
 
+	reg = ep_func->msix_cap + PCI_MSIX_FLAGS;
+	val = dw_pcie_ep_readw_dbi(ep, func_no, reg);
 	val &= PCI_MSIX_FLAGS_QSIZE;
 
 	return val + 1;
@@ -877,6 +899,15 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
 	if (!ep_func || !ep_func->msi_cap)
 		return -EINVAL;
 
+	/*
+	 * PCIe spec r7, sec 7.7.1.2 mandates that a Function should raise MSI
+	 * only if the MSI Enable bit is set and MSI-X Enable bit is clear.
+	 */
+	if (!dw_pcie_ep_msi_enabled(ep, ep_func, func_no) ||
+	    (dw_pcie_ep_msi_enabled(ep, ep_func, func_no) &&
+	     dw_pcie_ep_msix_enabled(ep, ep_func, func_no)))
+		return -EOPNOTSUPP;
+
 	/* Raise MSI per the PCI Local Bus Specification Revision 3.0, 6.8.1. */
 	reg = ep_func->msi_cap + PCI_MSI_FLAGS;
 	msg_ctrl = dw_pcie_ep_readw_dbi(ep, func_no, reg);
-- 
2.51.0

[PATCH 2/2] PCI: dwc: ep: Add MSI-X Enable checks before raising MSI-X to the host

[Manivannan Sadhasivam]

PCIe spec r7, sec 7.7.2.2 mandates that a Function should raise MSI-X only
if the MSI-X Enable bit is set and MSI enable bit is clear.

Hence, add those checks to be spec compliant.

Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-designware-ep.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index fcbd648e049e..47bc33346342 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -1018,6 +1018,15 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
 	if (!ep_func || !ep_func->msix_cap)
 		return -EINVAL;
 
+	/*
+	 * PCIe spec r7, sec 7.7.2.2 mandates that a Function should raise MSI-X
+	 * only if the MSI-X Enable bit is set and MSI Enable bit is clear.
+	 */
+	if (!dw_pcie_ep_msix_enabled(ep, ep_func, func_no) ||
+	    (dw_pcie_ep_msix_enabled(ep, ep_func, func_no) &&
+	     dw_pcie_ep_msi_enabled(ep, ep_func, func_no)))
+		return -EOPNOTSUPP;
+
 	reg = ep_func->msix_cap + PCI_MSIX_TABLE;
 	tbl_offset = dw_pcie_ep_readl_dbi(ep, func_no, reg);
 	bir = FIELD_GET(PCI_MSIX_TABLE_BIR, tbl_offset);
-- 
2.51.0

Re: [PATCH 1/2] PCI: dwc: ep: Add MSI Enable checks before raising MSI to the host

[Niklas Cassel]

On Wed, Feb 25, 2026 at 09:53:58PM +0530, Manivannan Sadhasivam wrote:
> PCIe spec r7, sec 7.7.1.2 mandates that a Function should raise MSI only if
> the MSI Enable bit is set and MSI-X enable bit is clear.
> 
> Hence, add those checks to be spec compliant with relevant helpers to avoid
> code duplication.
> 
> Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   | 43 ++++++++++++++++---
>  1 file changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 295076cf70de..fcbd648e049e 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -672,6 +672,17 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  	return 0;
>  }
>  
> +static bool dw_pcie_ep_msi_enabled(struct dw_pcie_ep *ep,
> +				   struct dw_pcie_ep_func *ep_func, u8 func_no)
> +{
> +	u32 val, reg;
> +
> +	reg = ep_func->msi_cap + PCI_MSI_FLAGS;
> +	val = dw_pcie_ep_readw_dbi(ep, func_no, reg);

Since you are doing a readw, I think it looks a bit weird that val
is u32 instead of u16.


> +
> +	return FIELD_GET(PCI_MSI_FLAGS_ENABLE, val);
> +}
> +
>  static int dw_pcie_ep_get_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
>  {
>  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> @@ -682,11 +693,11 @@ static int dw_pcie_ep_get_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
>  	if (!ep_func || !ep_func->msi_cap)
>  		return -EINVAL;
>  
> -	reg = ep_func->msi_cap + PCI_MSI_FLAGS;
> -	val = dw_pcie_ep_readw_dbi(ep, func_no, reg);
> -	if (!(val & PCI_MSI_FLAGS_ENABLE))
> +	if (!dw_pcie_ep_msi_enabled(ep, ep_func, func_no))
>  		return -EINVAL;
>  
> +	reg = ep_func->msi_cap + PCI_MSI_FLAGS;
> +	val = dw_pcie_ep_readw_dbi(ep, func_no, reg);
>  	val = FIELD_GET(PCI_MSI_FLAGS_QSIZE, val);
>  
>  	return 1 << val;
> @@ -716,6 +727,17 @@ static int dw_pcie_ep_set_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  	return 0;
>  }
>  
> +static bool dw_pcie_ep_msix_enabled(struct dw_pcie_ep *ep,
> +				    struct dw_pcie_ep_func *ep_func, u8 func_no)
> +{
> +	u32 val, reg;
> +
> +	reg = ep_func->msix_cap + PCI_MSIX_FLAGS;
> +	val = dw_pcie_ep_readw_dbi(ep, func_no, reg);

Since you are doing a readw, I think it looks a bit weird that val
is u32 instead of u16.


> +
> +	return FIELD_GET(PCI_MSIX_FLAGS_ENABLE, val);
> +}
> +
>  static int dw_pcie_ep_get_msix(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
>  {
>  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> @@ -726,11 +748,11 @@ static int dw_pcie_ep_get_msix(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
>  	if (!ep_func || !ep_func->msix_cap)
>  		return -EINVAL;
>  
> -	reg = ep_func->msix_cap + PCI_MSIX_FLAGS;
> -	val = dw_pcie_ep_readw_dbi(ep, func_no, reg);
> -	if (!(val & PCI_MSIX_FLAGS_ENABLE))
> +	if (!dw_pcie_ep_msix_enabled(ep, ep_func, func_no))
>  		return -EINVAL;
>  
> +	reg = ep_func->msix_cap + PCI_MSIX_FLAGS;
> +	val = dw_pcie_ep_readw_dbi(ep, func_no, reg);
>  	val &= PCI_MSIX_FLAGS_QSIZE;

Since you are touching these lines, I think you should change this to
use FIELD_GET() just like all other functions in this file.
If you don't want to do it in the same patch, then do it in a separate
patch in the same series.


>  
>  	return val + 1;
> @@ -877,6 +899,15 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
>  	if (!ep_func || !ep_func->msi_cap)
>  		return -EINVAL;
>  
> +	/*
> +	 * PCIe spec r7, sec 7.7.1.2 mandates that a Function should raise MSI
> +	 * only if the MSI Enable bit is set and MSI-X Enable bit is clear.
> +	 */
> +	if (!dw_pcie_ep_msi_enabled(ep, ep_func, func_no) ||
> +	    (dw_pcie_ep_msi_enabled(ep, ep_func, func_no) &&
> +	     dw_pcie_ep_msix_enabled(ep, ep_func, func_no)))

Here we will call dw_pcie_ep_msi_enabled() twice per dw_pcie_ep_raise_msi_irq().
I would call dw_pcie_ep_msi_enabled() once and store the result in a local
variable, to avoid the latency of doing two reads instead of one, for each
dw_pcie_ep_raise_msi_irq().


Kind regards,
Niklas

Re: [PATCH 2/2] PCI: dwc: ep: Add MSI-X Enable checks before raising MSI-X to the host

[Niklas Cassel]

On Wed, Feb 25, 2026 at 09:53:59PM +0530, Manivannan Sadhasivam wrote:
> PCIe spec r7, sec 7.7.2.2 mandates that a Function should raise MSI-X only
> if the MSI-X Enable bit is set and MSI enable bit is clear.
> 
> Hence, add those checks to be spec compliant.
> 
> Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-ep.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index fcbd648e049e..47bc33346342 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -1018,6 +1018,15 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
>  	if (!ep_func || !ep_func->msix_cap)
>  		return -EINVAL;
>  
> +	/*
> +	 * PCIe spec r7, sec 7.7.2.2 mandates that a Function should raise MSI-X
> +	 * only if the MSI-X Enable bit is set and MSI Enable bit is clear.
> +	 */
> +	if (!dw_pcie_ep_msix_enabled(ep, ep_func, func_no) ||
> +	    (dw_pcie_ep_msix_enabled(ep, ep_func, func_no) &&
> +	     dw_pcie_ep_msi_enabled(ep, ep_func, func_no)))
> +		return -EOPNOTSUPP;

Same comment as patch 1/2.


Kind regards,
Niklas