Re: [PATCH 06/13] PCI: aardvark: Do not clear status bits of masked interrupts

From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
To: "Marek Behún" <kabel@kernel.org>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	linux-pci@vger.kernel.org, pali@kernel.org,
	stable@vger.kernel.org, maz@kernel.org
Subject: Re: [PATCH 06/13] PCI: aardvark: Do not clear status bits of masked interrupts
Date: Mon, 4 Oct 2021 15:06:53 +0100	[thread overview]
Message-ID: <20211004140653.GB24914@lpieralisi> (raw)
In-Reply-To: <20211001195856.10081-7-kabel@kernel.org>

[+Marc - always better to have his eyes on IRQ handling code]

On Fri, Oct 01, 2021 at 09:58:49PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> It is incorrect to clear status bits of masked interrupts.
> 
> The aardvark driver clears all status interrupt bits if no unmasked
> status bit is set. Masked bits should never be cleared.
> 
> Fixes: 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host controller driver")
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <kabel@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> Cc: stable@vger.kernel.org
> ---
>  drivers/pci/controller/pci-aardvark.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index d5d6f92e5143..e4986806a189 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -1295,11 +1295,8 @@ static void advk_pcie_handle_int(struct advk_pcie *pcie)
>  	isr1_mask = advk_readl(pcie, PCIE_ISR1_MASK_REG);
>  	isr1_status = isr1_val & ((~isr1_mask) & PCIE_ISR1_ALL_MASK);
>  
> -	if (!isr0_status && !isr1_status) {
> -		advk_writel(pcie, isr0_val, PCIE_ISR0_REG);
> -		advk_writel(pcie, isr1_val, PCIE_ISR1_REG);

This looks fine - on the other hand if no interrupt is set in the status
registers (that are filtered with the masks) we are dealing with a
spurious IRQ right ? Just gauging how severe this is.

Lorenzo

> +	if (!isr0_status && !isr1_status)
>  		return;
> -	}
>  
>  	/* Process MSI interrupts */
>  	if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
> -- 
> 2.32.0
> 