Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 07 May 2021 11:19:48 +0100
Message-ID: <87bl9mq20r.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Pali =?UTF-8?B?Um9ow6Fy?= <pali@kernel.org>
Cc:     Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
        Rob Herring <robh@kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Russell King <rmk+kernel@armlinux.org.uk>,
        Marek =?UTF-8?B?QmVow7pu?= <kabel@kernel.org>,
        Remi Pommarel <repk@triplefau.lt>, Xogium <contact@xogium.me>,
        Tomasz Maciej Nowak <tmn505@gmail.com>,
        linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 18/42] PCI: aardvark: Correctly clear and unmask all MSI interrupts
In-Reply-To: <20210506153153.30454-19-pali@kernel.org>
References: <20210506153153.30454-1-pali@kernel.org>
        <20210506153153.30454-19-pali@kernel.org>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Thu, 06 May 2021 16:31:29 +0100,
Pali Rohár <pali@kernel.org> 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 498810c00b6d..5e0243b2c473 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)
>  
> @@ -386,19 +387,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);

I really wonder why you'd unmask all MSIs. Yes, the current code does
that already, but I'd expect MSIs to be individually unmasked as they
get enabled by the core code.

Thanks,

	M.
	
> +
> +	/* 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);
> @@ -1049,7 +1053,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))
> -- 
> 2.20.1
> 
> 

-- 
Without deviation from the norm, progress is not possible.