diff mbox series

[6/7] PCI: aardvark: Correctly clear and unmask all MSI interrupts

Message ID 20210625090319.10220-7-pali@kernel.org (mailing list archive)
State Superseded
Delegated to: Lorenzo Pieralisi
Headers show
Series PCI: aardvark: Interrupt fixes | expand

Commit Message

Pali Rohár June 25, 2021, 9:03 a.m. UTC 
Define a new macro PCIE_MSI_ALL_MASK and use it for masking, unmasking and
clearing all MSI interrupts.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
Cc: stable@vger.kernel.org
---
 drivers/pci/controller/pci-aardvark.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Comments

Marc Zyngier Aug. 6, 2021, 8:32 a.m. UTC | #1 
On 2021-06-25 10:03, Pali Rohár wrote:
> Define a new macro PCIE_MSI_ALL_MASK and use it for masking, unmasking 
> and
> clearing all MSI interrupts.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <kabel@kernel.org>
> Cc: stable@vger.kernel.org
> ---
>  drivers/pci/controller/pci-aardvark.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c
> b/drivers/pci/controller/pci-aardvark.c
> index 0e81d89f307d..7cad6d989f6c 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -117,6 +117,7 @@
>  #define PCIE_MSI_ADDR_HIGH_REG			(CONTROL_BASE_ADDR + 0x54)
>  #define PCIE_MSI_STATUS_REG			(CONTROL_BASE_ADDR + 0x58)
>  #define PCIE_MSI_MASK_REG			(CONTROL_BASE_ADDR + 0x5C)
> +#define     PCIE_MSI_ALL_MASK			GENMASK(31, 0)
>  #define PCIE_MSI_PAYLOAD_REG			(CONTROL_BASE_ADDR + 0x9C)
>  #define     PCIE_MSI_DATA_MASK			GENMASK(15, 0)
> 
> @@ -470,19 +471,22 @@ static void advk_pcie_setup_hw(struct advk_pcie 
> *pcie)
>  	advk_writel(pcie, reg, PCIE_CORE_CTRL2_REG);
> 
>  	/* Clear all interrupts */
> +	advk_writel(pcie, PCIE_MSI_ALL_MASK, PCIE_MSI_STATUS_REG);
>  	advk_writel(pcie, PCIE_ISR0_ALL_MASK, PCIE_ISR0_REG);
>  	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_REG);
>  	advk_writel(pcie, PCIE_IRQ_ALL_MASK, HOST_CTRL_INT_STATUS_REG);
> 
>  	/* Disable All ISR0/1 Sources */
> -	reg = PCIE_ISR0_ALL_MASK;
> -	reg &= ~PCIE_ISR0_MSI_INT_PENDING;
> -	advk_writel(pcie, reg, PCIE_ISR0_MASK_REG);
> -
> +	advk_writel(pcie, PCIE_ISR0_ALL_MASK, PCIE_ISR0_MASK_REG);
>  	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_MASK_REG);
> 
>  	/* Unmask all MSIs */
> -	advk_writel(pcie, 0, PCIE_MSI_MASK_REG);
> +	advk_writel(pcie, ~(u32)PCIE_MSI_ALL_MASK, PCIE_MSI_MASK_REG);
> +
> +	/* Unmask summary MSI interrupt */
> +	reg = advk_readl(pcie, PCIE_ISR0_MASK_REG);
> +	reg &= ~PCIE_ISR0_MSI_INT_PENDING;
> +	advk_writel(pcie, reg, PCIE_ISR0_MASK_REG);
> 
>  	/* Enable summary interrupt for GIC SPI source */
>  	reg = PCIE_IRQ_ALL_MASK & (~PCIE_IRQ_ENABLE_INTS_MASK);
> @@ -1177,7 +1181,7 @@ static void advk_pcie_handle_msi(struct advk_pcie 
> *pcie)
> 
>  	msi_mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
>  	msi_val = advk_readl(pcie, PCIE_MSI_STATUS_REG);
> -	msi_status = msi_val & ~msi_mask;
> +	msi_status = msi_val & ((~msi_mask) & PCIE_MSI_ALL_MASK);
> 
>  	for (msi_idx = 0; msi_idx < MSI_IRQ_NUM; msi_idx++) {
>  		if (!(BIT(msi_idx) & msi_status))

This still begs the question: why would you ever enable all
MSIs the first place? They should be enabled on request only.

         M.
Pali Rohár Aug. 6, 2021, 8:35 a.m. UTC | #2 
On Friday 06 August 2021 09:32:24 Marc Zyngier wrote:
> On 2021-06-25 10:03, Pali Rohár wrote:
> > Define a new macro PCIE_MSI_ALL_MASK and use it for masking, unmasking
> > and
> > clearing all MSI interrupts.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Reviewed-by: Marek Behún <kabel@kernel.org>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/pci/controller/pci-aardvark.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pci-aardvark.c
> > b/drivers/pci/controller/pci-aardvark.c
> > index 0e81d89f307d..7cad6d989f6c 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -117,6 +117,7 @@
> >  #define PCIE_MSI_ADDR_HIGH_REG			(CONTROL_BASE_ADDR + 0x54)
> >  #define PCIE_MSI_STATUS_REG			(CONTROL_BASE_ADDR + 0x58)
> >  #define PCIE_MSI_MASK_REG			(CONTROL_BASE_ADDR + 0x5C)
> > +#define     PCIE_MSI_ALL_MASK			GENMASK(31, 0)
> >  #define PCIE_MSI_PAYLOAD_REG			(CONTROL_BASE_ADDR + 0x9C)
> >  #define     PCIE_MSI_DATA_MASK			GENMASK(15, 0)
> > 
> > @@ -470,19 +471,22 @@ static void advk_pcie_setup_hw(struct advk_pcie
> > *pcie)
> >  	advk_writel(pcie, reg, PCIE_CORE_CTRL2_REG);
> > 
> >  	/* Clear all interrupts */
> > +	advk_writel(pcie, PCIE_MSI_ALL_MASK, PCIE_MSI_STATUS_REG);
> >  	advk_writel(pcie, PCIE_ISR0_ALL_MASK, PCIE_ISR0_REG);
> >  	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_REG);
> >  	advk_writel(pcie, PCIE_IRQ_ALL_MASK, HOST_CTRL_INT_STATUS_REG);
> > 
> >  	/* Disable All ISR0/1 Sources */
> > -	reg = PCIE_ISR0_ALL_MASK;
> > -	reg &= ~PCIE_ISR0_MSI_INT_PENDING;
> > -	advk_writel(pcie, reg, PCIE_ISR0_MASK_REG);
> > -
> > +	advk_writel(pcie, PCIE_ISR0_ALL_MASK, PCIE_ISR0_MASK_REG);
> >  	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_MASK_REG);
> > 
> >  	/* Unmask all MSIs */
> > -	advk_writel(pcie, 0, PCIE_MSI_MASK_REG);
> > +	advk_writel(pcie, ~(u32)PCIE_MSI_ALL_MASK, PCIE_MSI_MASK_REG);
> > +
> > +	/* Unmask summary MSI interrupt */
> > +	reg = advk_readl(pcie, PCIE_ISR0_MASK_REG);
> > +	reg &= ~PCIE_ISR0_MSI_INT_PENDING;
> > +	advk_writel(pcie, reg, PCIE_ISR0_MASK_REG);
> > 
> >  	/* Enable summary interrupt for GIC SPI source */
> >  	reg = PCIE_IRQ_ALL_MASK & (~PCIE_IRQ_ENABLE_INTS_MASK);
> > @@ -1177,7 +1181,7 @@ static void advk_pcie_handle_msi(struct advk_pcie
> > *pcie)
> > 
> >  	msi_mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
> >  	msi_val = advk_readl(pcie, PCIE_MSI_STATUS_REG);
> > -	msi_status = msi_val & ~msi_mask;
> > +	msi_status = msi_val & ((~msi_mask) & PCIE_MSI_ALL_MASK);
> > 
> >  	for (msi_idx = 0; msi_idx < MSI_IRQ_NUM; msi_idx++) {
> >  		if (!(BIT(msi_idx) & msi_status))
> 
> This still begs the question: why would you ever enable all
> MSIs the first place? They should be enabled on request only.

This is going to be fixed in followup patches. Just Lorenzo asked to
send smaller patch series, so I have to split that followup fix into
the next round.

>         M.
> -- 
> Jazz is not dead. It just smells funny...
Pali Rohár Aug. 23, 2021, 4:42 p.m. UTC | #3 
On Friday 06 August 2021 10:35:22 Pali Rohár wrote:
> On Friday 06 August 2021 09:32:24 Marc Zyngier wrote:
> > On 2021-06-25 10:03, Pali Rohár wrote:
> > > Define a new macro PCIE_MSI_ALL_MASK and use it for masking, unmasking
> > > and
> > > clearing all MSI interrupts.
> > > 
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > Reviewed-by: Marek Behún <kabel@kernel.org>
> > > Cc: stable@vger.kernel.org
> > > ---
> > >  drivers/pci/controller/pci-aardvark.c | 16 ++++++++++------
> > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/pci-aardvark.c
> > > b/drivers/pci/controller/pci-aardvark.c
> > > index 0e81d89f307d..7cad6d989f6c 100644
> > > --- a/drivers/pci/controller/pci-aardvark.c
> > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > @@ -117,6 +117,7 @@
> > >  #define PCIE_MSI_ADDR_HIGH_REG			(CONTROL_BASE_ADDR + 0x54)
> > >  #define PCIE_MSI_STATUS_REG			(CONTROL_BASE_ADDR + 0x58)
> > >  #define PCIE_MSI_MASK_REG			(CONTROL_BASE_ADDR + 0x5C)
> > > +#define     PCIE_MSI_ALL_MASK			GENMASK(31, 0)
> > >  #define PCIE_MSI_PAYLOAD_REG			(CONTROL_BASE_ADDR + 0x9C)
> > >  #define     PCIE_MSI_DATA_MASK			GENMASK(15, 0)
> > > 
> > > @@ -470,19 +471,22 @@ static void advk_pcie_setup_hw(struct advk_pcie
> > > *pcie)
> > >  	advk_writel(pcie, reg, PCIE_CORE_CTRL2_REG);
> > > 
> > >  	/* Clear all interrupts */
> > > +	advk_writel(pcie, PCIE_MSI_ALL_MASK, PCIE_MSI_STATUS_REG);
> > >  	advk_writel(pcie, PCIE_ISR0_ALL_MASK, PCIE_ISR0_REG);
> > >  	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_REG);
> > >  	advk_writel(pcie, PCIE_IRQ_ALL_MASK, HOST_CTRL_INT_STATUS_REG);
> > > 
> > >  	/* Disable All ISR0/1 Sources */
> > > -	reg = PCIE_ISR0_ALL_MASK;
> > > -	reg &= ~PCIE_ISR0_MSI_INT_PENDING;
> > > -	advk_writel(pcie, reg, PCIE_ISR0_MASK_REG);
> > > -
> > > +	advk_writel(pcie, PCIE_ISR0_ALL_MASK, PCIE_ISR0_MASK_REG);
> > >  	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_MASK_REG);
> > > 
> > >  	/* Unmask all MSIs */
> > > -	advk_writel(pcie, 0, PCIE_MSI_MASK_REG);
> > > +	advk_writel(pcie, ~(u32)PCIE_MSI_ALL_MASK, PCIE_MSI_MASK_REG);
> > > +
> > > +	/* Unmask summary MSI interrupt */
> > > +	reg = advk_readl(pcie, PCIE_ISR0_MASK_REG);
> > > +	reg &= ~PCIE_ISR0_MSI_INT_PENDING;
> > > +	advk_writel(pcie, reg, PCIE_ISR0_MASK_REG);
> > > 
> > >  	/* Enable summary interrupt for GIC SPI source */
> > >  	reg = PCIE_IRQ_ALL_MASK & (~PCIE_IRQ_ENABLE_INTS_MASK);
> > > @@ -1177,7 +1181,7 @@ static void advk_pcie_handle_msi(struct advk_pcie
> > > *pcie)
> > > 
> > >  	msi_mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
> > >  	msi_val = advk_readl(pcie, PCIE_MSI_STATUS_REG);
> > > -	msi_status = msi_val & ~msi_mask;
> > > +	msi_status = msi_val & ((~msi_mask) & PCIE_MSI_ALL_MASK);
> > > 
> > >  	for (msi_idx = 0; msi_idx < MSI_IRQ_NUM; msi_idx++) {
> > >  		if (!(BIT(msi_idx) & msi_status))
> > 
> > This still begs the question: why would you ever enable all
> > MSIs the first place? They should be enabled on request only.
> 
> This is going to be fixed in followup patches. Just Lorenzo asked to
> send smaller patch series, so I have to split that followup fix into
> the next round.

Just for reference, patch series which is handling it:
https://lore.kernel.org/linux-pci/20210815103624.19528-1-pali@kernel.org/t/#u

> >         M.
> > -- 
> > Jazz is not dead. It just smells funny...
diff mbox series

Patch

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 0e81d89f307d..7cad6d989f6c 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -117,6 +117,7 @@ 
 #define PCIE_MSI_ADDR_HIGH_REG			(CONTROL_BASE_ADDR + 0x54)
 #define PCIE_MSI_STATUS_REG			(CONTROL_BASE_ADDR + 0x58)
 #define PCIE_MSI_MASK_REG			(CONTROL_BASE_ADDR + 0x5C)
+#define     PCIE_MSI_ALL_MASK			GENMASK(31, 0)
 #define PCIE_MSI_PAYLOAD_REG			(CONTROL_BASE_ADDR + 0x9C)
 #define     PCIE_MSI_DATA_MASK			GENMASK(15, 0)
 
@@ -470,19 +471,22 @@  static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	advk_writel(pcie, reg, PCIE_CORE_CTRL2_REG);
 
 	/* Clear all interrupts */
+	advk_writel(pcie, PCIE_MSI_ALL_MASK, PCIE_MSI_STATUS_REG);
 	advk_writel(pcie, PCIE_ISR0_ALL_MASK, PCIE_ISR0_REG);
 	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_REG);
 	advk_writel(pcie, PCIE_IRQ_ALL_MASK, HOST_CTRL_INT_STATUS_REG);
 
 	/* Disable All ISR0/1 Sources */
-	reg = PCIE_ISR0_ALL_MASK;
-	reg &= ~PCIE_ISR0_MSI_INT_PENDING;
-	advk_writel(pcie, reg, PCIE_ISR0_MASK_REG);
-
+	advk_writel(pcie, PCIE_ISR0_ALL_MASK, PCIE_ISR0_MASK_REG);
 	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_MASK_REG);
 
 	/* Unmask all MSIs */
-	advk_writel(pcie, 0, PCIE_MSI_MASK_REG);
+	advk_writel(pcie, ~(u32)PCIE_MSI_ALL_MASK, PCIE_MSI_MASK_REG);
+
+	/* Unmask summary MSI interrupt */
+	reg = advk_readl(pcie, PCIE_ISR0_MASK_REG);
+	reg &= ~PCIE_ISR0_MSI_INT_PENDING;
+	advk_writel(pcie, reg, PCIE_ISR0_MASK_REG);
 
 	/* Enable summary interrupt for GIC SPI source */
 	reg = PCIE_IRQ_ALL_MASK & (~PCIE_IRQ_ENABLE_INTS_MASK);
@@ -1177,7 +1181,7 @@  static void advk_pcie_handle_msi(struct advk_pcie *pcie)
 
 	msi_mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
 	msi_val = advk_readl(pcie, PCIE_MSI_STATUS_REG);
-	msi_status = msi_val & ~msi_mask;
+	msi_status = msi_val & ((~msi_mask) & PCIE_MSI_ALL_MASK);
 
 	for (msi_idx = 0; msi_idx < MSI_IRQ_NUM; msi_idx++) {
 		if (!(BIT(msi_idx) & msi_status))