diff mbox

[v3,2/6] PCI: xilinx: Unify INTx & MSI interrupt FIFO decode

Message ID 1454602213-967-3-git-send-email-paul.burton@imgtec.com
State Changes Requested
Headers show

Commit Message

Paul Burton Feb. 4, 2016, 4:10 p.m. UTC 
When decoding either an INTx or MSI interrupt, the driver has no way to
know which it will pull out of the interrupt FIFO. If both were pending
then this would lead to either the interrupt being handled incorrectly
(MSI interrupt treated as INTx) or not at all (INTx interrupt dropped by
MSI path). Unify the reading of the interrupt FIFO & act according to
the type of interrupt actually read.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Fixes: 8961def56845 ("PCI: xilinx: Add Xilinx AXI PCIe Host Bridge IP driver")

---

Changes in v3:
- Split out from Boston patchset.

Changes in v2:
- Add Fixes tag.

 drivers/pci/host/pcie-xilinx.c | 47 +++++++++++++-----------------------------
 1 file changed, 14 insertions(+), 33 deletions(-)

Comments

Bharat Kumar Gogada Feb. 11, 2016, 5:42 a.m. UTC | #1 
> Subject: [PATCH v3 2/6] PCI: xilinx: Unify INTx & MSI interrupt FIFO decode
> 
> When decoding either an INTx or MSI interrupt, the driver has no way to
> know which it will pull out of the interrupt FIFO. If both were pending then
> this would lead to either the interrupt being handled incorrectly (MSI
> interrupt treated as INTx) or not at all (INTx interrupt dropped by MSI path).
> Unify the reading of the interrupt FIFO & act according to the type of
> interrupt actually read.
> 
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> Fixes: 8961def56845 ("PCI: xilinx: Add Xilinx AXI PCIe Host Bridge IP driver")
> 
> ---
> 
> Changes in v3:
> - Split out from Boston patchset.
> 
> Changes in v2:
> - Add Fixes tag.
> 
>  drivers/pci/host/pcie-xilinx.c | 47 +++++++++++++-----------------------------
>  1 file changed, 14 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-xilinx.c b/drivers/pci/host/pcie-xilinx.c index
> 1490bd1..afdfb09 100644
> --- a/drivers/pci/host/pcie-xilinx.c
> +++ b/drivers/pci/host/pcie-xilinx.c
> @@ -397,7 +397,7 @@ static const struct irq_domain_ops intx_domain_ops
> = {  static irqreturn_t xilinx_pcie_intr_handler(int irq, void *data)  {
>  	struct xilinx_pcie_port *port = (struct xilinx_pcie_port *)data;
> -	u32 val, mask, status, msi_data;
> +	u32 val, mask, status;
> 
>  	/* Read interrupt decode and mask registers */
>  	val = pcie_read(port, XILINX_PCIE_REG_IDR); @@ -437,8 +437,8 @@
> static irqreturn_t xilinx_pcie_intr_handler(int irq, void *data)
>  		xilinx_pcie_clear_err_interrupts(port);
>  	}
> 
> -	if (status & XILINX_PCIE_INTR_INTX) {
> -		/* INTx interrupt received */
> +	if (status & (XILINX_PCIE_INTR_INTX | XILINX_PCIE_INTR_MSI)) {
> +		/* Interrupt received */
>  		val = pcie_read(port, XILINX_PCIE_REG_RPIFR1);
> 
>  		/* Check whether interrupt valid */
> @@ -447,41 +447,22 @@ static irqreturn_t xilinx_pcie_intr_handler(int irq,
> void *data)
>  			return IRQ_HANDLED;
>  		}
> 
> -		if (!(val & XILINX_PCIE_RPIFR1_MSI_INTR)) {
> -			/* Clear interrupt FIFO register 1 */
> -			pcie_write(port, XILINX_PCIE_RPIFR1_ALL_MASK,
> -				   XILINX_PCIE_REG_RPIFR1);
> -
> -			/* Handle INTx Interrupt */
> +		if (val & XILINX_PCIE_RPIFR1_MSI_INTR) {
> +			irq = pcie_read(port, XILINX_PCIE_REG_RPIFR2) &
> +				XILINX_PCIE_RPIFR2_MSG_DATA;
> +		} else {
>  			val = ((val & XILINX_PCIE_RPIFR1_INTR_MASK) >>
>  				XILINX_PCIE_RPIFR1_INTR_SHIFT) + 1;
> -			generic_handle_irq(irq_find_mapping(port-
> >irq_domain,
> -							    val));
> +			irq = irq_find_mapping(port->irq_domain, val);
>  		}
> -	}
> 
> -	if (status & XILINX_PCIE_INTR_MSI) {
> -		/* MSI Interrupt */
> -		val = pcie_read(port, XILINX_PCIE_REG_RPIFR1);
> +		/* Clear interrupt FIFO register 1 */
> +		pcie_write(port, XILINX_PCIE_RPIFR1_ALL_MASK,
> +			   XILINX_PCIE_REG_RPIFR1);
> 
> -		if (!(val & XILINX_PCIE_RPIFR1_INTR_VALID)) {
> -			dev_warn(port->dev, "RP Intr FIFO1 read error\n");
> -			return IRQ_HANDLED;
> -		}
> -
> -		if (val & XILINX_PCIE_RPIFR1_MSI_INTR) {
> -			msi_data = pcie_read(port,
> XILINX_PCIE_REG_RPIFR2) &
> -				   XILINX_PCIE_RPIFR2_MSG_DATA;
> -
> -			/* Clear interrupt FIFO register 1 */
> -			pcie_write(port, XILINX_PCIE_RPIFR1_ALL_MASK,
> -				   XILINX_PCIE_REG_RPIFR1);
> -
> -			if (IS_ENABLED(CONFIG_PCI_MSI)) {
> -				/* Handle MSI Interrupt */
> -				generic_handle_irq(msi_data);
> -			}
> -		}
> +		if (IS_ENABLED(CONFIG_PCI_MSI) ||
> +			!(val & XILINX_PCIE_RPIFR1_MSI_INTR))
> +			generic_handle_irq(irq);
>  	}
> 
>  	if (status & XILINX_PCIE_INTR_SLV_UNSUPP)
> --

Hi Paul,

Even with above condition you are still missing either MSI or legacy interrupt handling, when both MSI and legacy interrupts occurred.

Bharat
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bharat Kumar Gogada Feb. 11, 2016, 5:50 a.m. UTC | #2 
> > Subject: [PATCH v3 2/6] PCI: xilinx: Unify INTx & MSI interrupt FIFO
> > decode
> >
> > When decoding either an INTx or MSI interrupt, the driver has no way
> > to know which it will pull out of the interrupt FIFO. If both were
> > pending then this would lead to either the interrupt being handled
> > incorrectly (MSI interrupt treated as INTx) or not at all (INTx interrupt
> dropped by MSI path).
> > Unify the reading of the interrupt FIFO & act according to the type of
> > interrupt actually read.
> >
> > Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> > Fixes: 8961def56845 ("PCI: xilinx: Add Xilinx AXI PCIe Host Bridge IP
> > driver")
> >
> > ---
> >
> > Changes in v3:
> > - Split out from Boston patchset.
> >
> > Changes in v2:
> > - Add Fixes tag.
> >
> >  drivers/pci/host/pcie-xilinx.c | 47
> > +++++++++++++-----------------------------
> >  1 file changed, 14 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/pci/host/pcie-xilinx.c
> > b/drivers/pci/host/pcie-xilinx.c index
> > 1490bd1..afdfb09 100644
> > --- a/drivers/pci/host/pcie-xilinx.c
> > +++ b/drivers/pci/host/pcie-xilinx.c
> > @@ -397,7 +397,7 @@ static const struct irq_domain_ops
> intx_domain_ops
> > = {  static irqreturn_t xilinx_pcie_intr_handler(int irq, void *data)  {
> >  	struct xilinx_pcie_port *port = (struct xilinx_pcie_port *)data;
> > -	u32 val, mask, status, msi_data;
> > +	u32 val, mask, status;
> >
> >  	/* Read interrupt decode and mask registers */
> >  	val = pcie_read(port, XILINX_PCIE_REG_IDR); @@ -437,8 +437,8 @@
> > static irqreturn_t xilinx_pcie_intr_handler(int irq, void *data)
> >  		xilinx_pcie_clear_err_interrupts(port);
> >  	}
> >
> > -	if (status & XILINX_PCIE_INTR_INTX) {
> > -		/* INTx interrupt received */
> > +	if (status & (XILINX_PCIE_INTR_INTX | XILINX_PCIE_INTR_MSI)) {
> > +		/* Interrupt received */
> >  		val = pcie_read(port, XILINX_PCIE_REG_RPIFR1);
> >
> >  		/* Check whether interrupt valid */ @@ -447,41 +447,22 @@
> static
> > irqreturn_t xilinx_pcie_intr_handler(int irq, void *data)
> >  			return IRQ_HANDLED;
> >  		}
> >
> > -		if (!(val & XILINX_PCIE_RPIFR1_MSI_INTR)) {
> > -			/* Clear interrupt FIFO register 1 */
> > -			pcie_write(port, XILINX_PCIE_RPIFR1_ALL_MASK,
> > -				   XILINX_PCIE_REG_RPIFR1);
> > -
> > -			/* Handle INTx Interrupt */
> > +		if (val & XILINX_PCIE_RPIFR1_MSI_INTR) {
> > +			irq = pcie_read(port, XILINX_PCIE_REG_RPIFR2) &
> > +				XILINX_PCIE_RPIFR2_MSG_DATA;
> > +		} else {
> >  			val = ((val & XILINX_PCIE_RPIFR1_INTR_MASK) >>
> >  				XILINX_PCIE_RPIFR1_INTR_SHIFT) + 1;
> > -			generic_handle_irq(irq_find_mapping(port-
> > >irq_domain,
> > -							    val));
> > +			irq = irq_find_mapping(port->irq_domain, val);
> >  		}
> > -	}
> >
> > -	if (status & XILINX_PCIE_INTR_MSI) {
> > -		/* MSI Interrupt */
> > -		val = pcie_read(port, XILINX_PCIE_REG_RPIFR1);
> > +		/* Clear interrupt FIFO register 1 */
> > +		pcie_write(port, XILINX_PCIE_RPIFR1_ALL_MASK,
> > +			   XILINX_PCIE_REG_RPIFR1);
> >
> > -		if (!(val & XILINX_PCIE_RPIFR1_INTR_VALID)) {
> > -			dev_warn(port->dev, "RP Intr FIFO1 read error\n");
> > -			return IRQ_HANDLED;
> > -		}
> > -
> > -		if (val & XILINX_PCIE_RPIFR1_MSI_INTR) {
> > -			msi_data = pcie_read(port,
> > XILINX_PCIE_REG_RPIFR2) &
> > -				   XILINX_PCIE_RPIFR2_MSG_DATA;
> > -
> > -			/* Clear interrupt FIFO register 1 */
> > -			pcie_write(port, XILINX_PCIE_RPIFR1_ALL_MASK,
> > -				   XILINX_PCIE_REG_RPIFR1);
> > -
> > -			if (IS_ENABLED(CONFIG_PCI_MSI)) {
> > -				/* Handle MSI Interrupt */
> > -				generic_handle_irq(msi_data);
> > -			}
> > -		}
> > +		if (IS_ENABLED(CONFIG_PCI_MSI) ||
> > +			!(val & XILINX_PCIE_RPIFR1_MSI_INTR))
> > +			generic_handle_irq(irq);
> >  	}
> >
> >  	if (status & XILINX_PCIE_INTR_SLV_UNSUPP)
> > --
> 
> Hi Paul,
> 
> Even with above condition you are still missing either MSI or legacy interrupt
> handling, when both MSI and legacy interrupts occurred.

It would be better if the condition when both legacy and MSI interrupts occurred can be handled separately, leaving the current individual interrupt cases as they are.

Bharat
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pci/host/pcie-xilinx.c b/drivers/pci/host/pcie-xilinx.c
index 1490bd1..afdfb09 100644
--- a/drivers/pci/host/pcie-xilinx.c
+++ b/drivers/pci/host/pcie-xilinx.c
@@ -397,7 +397,7 @@  static const struct irq_domain_ops intx_domain_ops = {
 static irqreturn_t xilinx_pcie_intr_handler(int irq, void *data)
 {
 	struct xilinx_pcie_port *port = (struct xilinx_pcie_port *)data;
-	u32 val, mask, status, msi_data;
+	u32 val, mask, status;
 
 	/* Read interrupt decode and mask registers */
 	val = pcie_read(port, XILINX_PCIE_REG_IDR);
@@ -437,8 +437,8 @@  static irqreturn_t xilinx_pcie_intr_handler(int irq, void *data)
 		xilinx_pcie_clear_err_interrupts(port);
 	}
 
-	if (status & XILINX_PCIE_INTR_INTX) {
-		/* INTx interrupt received */
+	if (status & (XILINX_PCIE_INTR_INTX | XILINX_PCIE_INTR_MSI)) {
+		/* Interrupt received */
 		val = pcie_read(port, XILINX_PCIE_REG_RPIFR1);
 
 		/* Check whether interrupt valid */
@@ -447,41 +447,22 @@  static irqreturn_t xilinx_pcie_intr_handler(int irq, void *data)
 			return IRQ_HANDLED;
 		}
 
-		if (!(val & XILINX_PCIE_RPIFR1_MSI_INTR)) {
-			/* Clear interrupt FIFO register 1 */
-			pcie_write(port, XILINX_PCIE_RPIFR1_ALL_MASK,
-				   XILINX_PCIE_REG_RPIFR1);
-
-			/* Handle INTx Interrupt */
+		if (val & XILINX_PCIE_RPIFR1_MSI_INTR) {
+			irq = pcie_read(port, XILINX_PCIE_REG_RPIFR2) &
+				XILINX_PCIE_RPIFR2_MSG_DATA;
+		} else {
 			val = ((val & XILINX_PCIE_RPIFR1_INTR_MASK) >>
 				XILINX_PCIE_RPIFR1_INTR_SHIFT) + 1;
-			generic_handle_irq(irq_find_mapping(port->irq_domain,
-							    val));
+			irq = irq_find_mapping(port->irq_domain, val);
 		}
-	}
 
-	if (status & XILINX_PCIE_INTR_MSI) {
-		/* MSI Interrupt */
-		val = pcie_read(port, XILINX_PCIE_REG_RPIFR1);
+		/* Clear interrupt FIFO register 1 */
+		pcie_write(port, XILINX_PCIE_RPIFR1_ALL_MASK,
+			   XILINX_PCIE_REG_RPIFR1);
 
-		if (!(val & XILINX_PCIE_RPIFR1_INTR_VALID)) {
-			dev_warn(port->dev, "RP Intr FIFO1 read error\n");
-			return IRQ_HANDLED;
-		}
-
-		if (val & XILINX_PCIE_RPIFR1_MSI_INTR) {
-			msi_data = pcie_read(port, XILINX_PCIE_REG_RPIFR2) &
-				   XILINX_PCIE_RPIFR2_MSG_DATA;
-
-			/* Clear interrupt FIFO register 1 */
-			pcie_write(port, XILINX_PCIE_RPIFR1_ALL_MASK,
-				   XILINX_PCIE_REG_RPIFR1);
-
-			if (IS_ENABLED(CONFIG_PCI_MSI)) {
-				/* Handle MSI Interrupt */
-				generic_handle_irq(msi_data);
-			}
-		}
+		if (IS_ENABLED(CONFIG_PCI_MSI) ||
+			!(val & XILINX_PCIE_RPIFR1_MSI_INTR))
+			generic_handle_irq(irq);
 	}
 
 	if (status & XILINX_PCIE_INTR_SLV_UNSUPP)