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

[Niklas Cassel <cassel@kernel.org>]

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