linux-kernel - Re: [PATCH v3] PCI: pciehp: Make sure pciehp

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] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 9 Feb 2020 19:07:22 +0100
From:   Lukas Wunner <lukas@...ner.de>
To:     Stuart Hayes <stuart.w.hayes@...il.com>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>,
        Austin Bolen <austin_bolen@...l.com>,
        Keith Busch <kbusch@...nel.org>,
        Alexandru Gagniuc <mr.nuke.me@...il.com>,
        "Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        "Gustavo A . R . Silva" <gustavo@...eddedor.com>,
        Sinan Kaya <okaya@...nel.org>,
        Oza Pawandeep <poza@...eaurora.org>, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org, narendra_k@...l.com
Subject: Re: [PATCH v3] PCI: pciehp: Make sure pciehp_isr clears interrupt
 events

On Sun, Feb 09, 2020 at 04:03:28PM +0100, Lukas Wunner wrote:
> Using a for (;;) or do/while loop that you jump out of if
> (!status || !pci_dev_msi_enabled(pdev)) might be more readable
> than a goto, but I'm not sure.

Actually, scratch that.  After thinking about this problem for a day
I've come up with a much simpler and more elegant solution.  Could you
test if the below works for you?

This solution has the added benefit that the IRQ thread is started up
once the first event bits have been collected.  If more event bits are
found in the additional loop iterations, they're added to the collected
event bits and the IRQ thread will pick them up asynchronously.  Once no
more bits are found, the hardirq handler exits with IRQ_NONE.  This means
that the genirq code won't wake the IRQ thread but that doesn't matter
because the ISR has already done that itself.  Should also work correctly
in poll mode and the behavior in INTx mode should be as before.

-- >8 --
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index c3e3f53..707324d 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -553,6 +553,7 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
 		}
 	}
 
+read_status:
 	pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &status);
 	if (status == (u16) ~0) {
 		ctrl_info(ctrl, "%s: no response from device\n", __func__);
@@ -609,6 +610,17 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
 
 	/* Save pending events for consumption by IRQ thread. */
 	atomic_or(events, &ctrl->pending_events);
+
+	/*
+	 * In MSI mode, all event bits must be zero before the port will send
+	 * a new interrupt (PCIe Base Spec r5.0 sec 6.7.3.4).  So re-read the
+	 * Slot Status register in case a bit was set between read and write.
+	 */
+	if (pci_dev_msi_enabled(pdev) && !pciehp_poll_mode) {
+		irq_wake_thread(irq, ctrl);
+		goto read_status;
+	}
+
 	return IRQ_WAKE_THREAD;
 }