linux-kernel - Re: [PATCH] PCI: mediatek: Change MSI interrupt processing sequence

Open Source and information security mailing list archives

Message-ID: <20220125165748.GA1458116@bhelgaas>
Date:   Tue, 25 Jan 2022 10:57:48 -0600
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     qizhong cheng <qizhong.cheng@...iatek.com>
Cc:     Ryder Lee <ryder.lee@...iatek.com>,
        Jianjun Wang <jianjun.wang@...iatek.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, chuanjia.liu@...iatek.com
Subject: Re: [PATCH] PCI: mediatek: Change MSI interrupt processing sequence

All patches change *something*.  Can you update the subject line so it
says something specific about the change?

Maybe something like "Clear MSI status before dispatching handler"?

On Sun, Jan 23, 2022 at 11:33:06AM +0800, qizhong cheng wrote:
> As an edge-triggered interrupts, its interrupt status should be cleared
> before dispatch to the handler of device.

I'm not an IRQ expert, but the reasoning that "we should clear the MSI
interrupt status before dispatching the handler because MSI is an
edge-triggered interrupt" doesn't seem completely convincing because
your code will now look like this:

  /* Clear the INTx */
  writel(1 << bit, port->base + PCIE_INT_STATUS);
  generic_handle_domain_irq(port->irq_domain, bit - INTX_SHIFT);
  ...

  /* Clear MSI interrupt status */
  writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
  generic_handle_domain_irq(port->inner_domain, bit);

You clear interrupt status before dispatching the handler for *both*
level-triggered INTx interrupts and edge-triggered MSI interrupts.

So it doesn't seem that simply being edge-triggered is the critical
factor here.

> Signed-off-by: qizhong cheng <qizhong.cheng@...iatek.com>
> ---
>  drivers/pci/controller/pcie-mediatek.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index 2f3f974977a3..705ea33758b1 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -624,12 +624,12 @@ static void mtk_pcie_intr_handler(struct irq_desc *desc)
>  		if (status & MSI_STATUS){
>  			unsigned long imsi_status;
>  
> +			/* Clear MSI interrupt status */
> +			writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
>  			while ((imsi_status = readl(port->base + PCIE_IMSI_STATUS))) {
>  				for_each_set_bit(bit, &imsi_status, MTK_MSI_IRQS_NUM)
>  					generic_handle_domain_irq(port->inner_domain, bit);
>  			}
> -			/* Clear MSI interrupt status */
> -			writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
>  		}
>  	}
>  
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

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