Received: by 2002:a25:1506:0:0:0:0:0 with SMTP id 6csp2992594ybv; Sun, 9 Feb 2020 12:26:32 -0800 (PST) X-Google-Smtp-Source: APXvYqwreo4pgMMQxi70FQhYvOJY6x0USMngdBP9gZVWVYIJG40bw6vzckqAotHD+JcToaDv0b4k X-Received: by 2002:a9d:22:: with SMTP id 31mr7332739ota.173.1581279992682; Sun, 09 Feb 2020 12:26:32 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1581279992; cv=none; d=google.com; s=arc-20160816; b=yRmfaHsKzo9Tqc49gPEXR/KMDVJ7epNZvgOuQLAUUDu1Se74gk+xndoCIbMRxl9c1m 2GB/jgg8r+Yqb/sIreox5hty459XfnNch4vpzPaW6JYGhWnrnE/p4KmECHd1P7UxgCv8 1Srt3JuJcewKNXl3M75ztK0LueRArwMuLSv6m9v0PYc8Zl9xl/nktc2NtjBeq5TaIltL 1HaqJGChxeBUZqCyjNfvllUp9iEO1DiC2vUPnysMyGQIWw5ml2kkPvM9yVqMCZ8Lq5aR TiYTImlkZ/zqAukJvRA5U+ZFe2vfc8f84XcaJJcT7TarrKHbgyeD2TJU5W1weXp/Fjwt uqHg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date; bh=58xNWDImkMLGVaRj+XlCaJZ73JU3IIa4+e7dvBQgxDs=; b=vHQrd0to4Amws2rYTxxQcDpyLxQNe9lXJir+od5pkjP+nNXP/SWbLeif/FVdb7nSZj Vt/Gc9nYjzk0zIhmcXjBbb96dvXtXrx384SmBXK+w0kPKlZtlLcfEt+eZjGWupk6rYnX erv5Y59xP0CkFAiuPILk3qCujvbNf2OSEw4ASWHpUS/mxykugwEc3j3vKKssi+fiml+K cPLrREHsAYCruNPWrQqFzBUSAWJ8BwsvXtW29R0A5XaXK3bTF8x8D1FAPqVR4kgSKR7p sjgjc4XynjqhfuKDPrlPDmOSC4rKgY7mocq0y1IHT/oq4Sni9Y6tWLj9rodlR27z52+W 0OyA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id i3si3538278otc.272.2020.02.09.12.26.09; Sun, 09 Feb 2020 12:26:32 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727723AbgBIUZQ (ORCPT + 99 others); Sun, 9 Feb 2020 15:25:16 -0500 Received: from bmailout2.hostsharing.net ([83.223.78.240]:54433 "EHLO bmailout2.hostsharing.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727416AbgBIUZP (ORCPT ); Sun, 9 Feb 2020 15:25:15 -0500 Received: from h08.hostsharing.net (h08.hostsharing.net [83.223.95.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "*.hostsharing.net", Issuer "COMODO RSA Domain Validation Secure Server CA" (not verified)) by bmailout2.hostsharing.net (Postfix) with ESMTPS id 064102800BB92; Sun, 9 Feb 2020 21:25:13 +0100 (CET) Received: by h08.hostsharing.net (Postfix, from userid 100393) id BF09A14BAA7; Sun, 9 Feb 2020 21:25:12 +0100 (CET) Date: Sun, 9 Feb 2020 21:25:12 +0100 From: Lukas Wunner To: Stuart Hayes Cc: Bjorn Helgaas , Austin Bolen , Keith Busch , Alexandru Gagniuc , "Rafael J . Wysocki" , Mika Westerberg , Andy Shevchenko , "Gustavo A . R . Silva" , Sinan Kaya , Oza Pawandeep , linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, narendra_k@dell.com Subject: Re: [PATCH v3] PCI: pciehp: Make sure pciehp_isr clears interrupt events Message-ID: <20200209202512.rzaqoc7tydo2ouog@wunner.de> References: <20200207195450.52026-1-stuart.w.hayes@gmail.com> <20200209150328.2x2zumhqbs6fihmc@wunner.de> <20200209180722.ikuyjignnd7ddfp5@wunner.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200209180722.ikuyjignnd7ddfp5@wunner.de> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Feb 09, 2020 at 07:07:22PM +0100, Lukas Wunner wrote: > 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? Sorry, I missed a few things: * pm_runtime_put() is called too often in the MSI case. * If only the CC bit is set or if ignore_hotplug is set, the function may return prematurely without re-reading the Slot Status register. * Returning IRQ_NONE in the MSI case even though the IRQ thread was woken may incorrectly signal a spurious interrupt to the genirq code. It's better to return IRQ_HANDLED instead. Below is another attempt. I'll have to take a look at this with a fresh pair of eyeballs though to verify I haven't overlooked anything else and also to determine if this is actually simpler than Stuart's approach. Again, the advantage here is that processing of the events by the IRQ thread is sped up by not delaying it until the Slot Status register has settled. Thanks. -- >8 -- diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c index c3e3f53..db5baa5 100644 --- a/drivers/pci/hotplug/pciehp_hpc.c +++ b/drivers/pci/hotplug/pciehp_hpc.c @@ -530,6 +530,7 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id) struct controller *ctrl = (struct controller *)dev_id; struct pci_dev *pdev = ctrl_dev(ctrl); struct device *parent = pdev->dev.parent; + irqreturn_t ret = IRQ_NONE; u16 status, events; /* @@ -553,6 +554,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__); @@ -579,13 +581,11 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id) if (!events) { if (parent) pm_runtime_put(parent); - return IRQ_NONE; + return ret; } pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, events); ctrl_dbg(ctrl, "pending interrupts %#06x from Slot Status\n", events); - if (parent) - pm_runtime_put(parent); /* * Command Completed notifications are not deferred to the @@ -595,21 +595,33 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id) ctrl->cmd_busy = 0; smp_mb(); wake_up(&ctrl->queue); - - if (events == PCI_EXP_SLTSTA_CC) - return IRQ_HANDLED; - events &= ~PCI_EXP_SLTSTA_CC; } if (pdev->ignore_hotplug) { ctrl_dbg(ctrl, "ignoring hotplug event %#06x\n", events); - return IRQ_HANDLED; + events = 0; } /* Save pending events for consumption by IRQ thread. */ atomic_or(events, &ctrl->pending_events); - return IRQ_WAKE_THREAD; + + /* + * 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); + ret = IRQ_HANDLED; + goto read_status; + } + + if (parent) + pm_runtime_put(parent); + if (events) + return IRQ_WAKE_THREAD; + return IRQ_HANDLED; } static irqreturn_t pciehp_ist(int irq, void *dev_id)