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

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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 25 Jan 2022 17:21:21 +0000
From:   Marc Zyngier <maz@...nel.org>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     qizhong cheng <qizhong.cheng@...iatek.com>,
        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

On 2022-01-25 16:57, Bjorn Helgaas wrote:
> 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.

This is the usual problem with these half-baked implementations.
The signalling to the primary interrupt controller is level, as
they take a multitude of input and (crucially) latch the MSI
edges. Effectively, this is an edge-to-level converter, with
all the problems that this creates.

By clearing the status *after* the handling, you lose edges that
have been received and coalesced after the read of the status
register. By clearing it *before*, you are acknowledging the
interrupts early, and allowing them to be coalesced independently
of the ones that have been received earlier.

This is however mostly an educated guess. Someone with access
to the TRM should verify this.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...