linux-kernel - Re: [PATCH 18/42] PCI: aardvark: Correctly clear and unmask all MSI interrupts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87bl9mq20r.wl-maz@kernel.org>
Date:   Fri, 07 May 2021 11:19:48 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Pali Rohár <pali@...nel.org>
Cc:     Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Rob Herring <robh@...nel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Russell King <rmk+kernel@...linux.org.uk>,
        Marek Behún <kabel@...nel.org>,
        Remi Pommarel <repk@...plefau.lt>, Xogium <contact@...ium.me>,
        Tomasz Maciej Nowak <tmn505@...il.com>,
        linux-pci@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 18/42] PCI: aardvark: Correctly clear and unmask all MSI interrupts

On Thu, 06 May 2021 16:31:29 +0100,
Pali Rohár <pali@...nel.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@...nel.org>
> Reviewed-by: Marek Behún <kabel@...nel.org>
> Cc: stable@...r.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.