Received: by 2002:a25:1506:0:0:0:0:0 with SMTP id 6csp4164105ybv; Mon, 10 Feb 2020 13:41:59 -0800 (PST) X-Google-Smtp-Source: APXvYqzMt8JaTO4AZygrbc94kOfQZfckVjTkHSvmQcsWxTdmV9lS6QRnC8oQyRzm1q54vsJcimhq X-Received: by 2002:a9d:2c2a:: with SMTP id f39mr2618091otb.301.1581370919008; Mon, 10 Feb 2020 13:41:59 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1581370919; cv=none; d=google.com; s=arc-20160816; b=m4h3w1cNc7wprx5fAj9Zl3S2vJtFW3wPQ/sh+mf1DQ7cscARQ2JWRSDgw8LdWo3hkI iiWm+xZBN074iDKQtA0Wz8DYs6hrZ1SvsBHlnjzjfIbVC3uDtX0+wB+n4ODJE5K2O5UV i/hWO1gUMoo1gE5F4XzWwI0dgUos8AEf2uREo6wbfSnEorJQuWt46+l7K/ayRqV40kkF WrJj5LH3s6ytKGVYHp/3eLoCd9ZHlLihQAt497RXOnZpNkFfmlHc4r79hsSz8A0Bjw49 y00Uk1ovbfLG1cT77+w8eM1tHkm0vrLVs7w9cbnaJQRb+WAGnw51jZsbv431imrqISJW mQPw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject:dkim-signature; bh=Z/D6Fd99EbdIRz8ar+s3m0b7qLUHgWkFrp8oH1lTC6A=; b=DbBYwPnH2XAypYMmRX01r93cWVueuagAGb/dZe3/J5Hm2GrINg0Y0+8Z21SFML/1JO 7UPAGp2dXHaRm87fRdVIQ02LBGvAPe+GWxjvHxGtudNxaNQKlw9V65Q7y1U/K+Jyl3xy CdpSDhXPoAQ66eaHmt01et4uEBJ9kNfLoDs1KK4zoYioRy9yUdnHOYoQNz1i8wGxTtH9 ubw0SZFgg00mcuhCAcSOxt3+4Q2m2mg025OSYb4c0GHFXIoUNGD6m319ZOiC047acZQr PzJn3F4S7QhSrGzmMMjbtSD8kKM+bm0QkMBCtzGJxI+4R4NGsGuFymB3aBPkHEb0MLT2 znHQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=Q0RyMKKV; 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; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id c7si806347otf.87.2020.02.10.13.41.46; Mon, 10 Feb 2020 13:41:58 -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; dkim=pass header.i=@gmail.com header.s=20161025 header.b=Q0RyMKKV; 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; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727555AbgBJVkj (ORCPT + 99 others); Mon, 10 Feb 2020 16:40:39 -0500 Received: from mail-ot1-f68.google.com ([209.85.210.68]:46601 "EHLO mail-ot1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727003AbgBJVkj (ORCPT ); Mon, 10 Feb 2020 16:40:39 -0500 Received: by mail-ot1-f68.google.com with SMTP id g64so7932163otb.13; Mon, 10 Feb 2020 13:40:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=Z/D6Fd99EbdIRz8ar+s3m0b7qLUHgWkFrp8oH1lTC6A=; b=Q0RyMKKV63X2bmF78nEErgFcwotd6neuItsYU4LwBKwxVF1x1/mnHCvSZcyI/G429k /Wz/LQnmozI+fBN114EE9TGwx9a1miAj93zul4vvpZBmLyMk2twKCANBwNGH1hHjIr3L n1rocDYnP1qNQ87+7pdnn0j64kZqy6/GMPPTh9hXBvJhhS0i7wrOpAShjk/7bDqF1mcj DonHc0JulP0HxCyWatS4bwWDmOznBI2aZIk9n7S/RLT+q+5kieyc54sUrA4G346J+hLZ Bc4olaL7uMVCJt3XY5pcOPsl611BjF2J2LFH5smdo72E9GEJvMYA9+MiSN7F/u2/zj0f EzUw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=Z/D6Fd99EbdIRz8ar+s3m0b7qLUHgWkFrp8oH1lTC6A=; b=Ed5C5mLsh7LqyeaBg2vboJyCuDmHI2fd50yL8RNeOaYRZqh7ivGRlbhSGJwHA4l/ao IGZfVs/HiMU5eTHDACru7nW9giOtod+FprWAa7R3FVthBjwcNQ2wAYsSTOKPK4mez/hg SlemoFAskUM/UojWSB/iFGoikpnEACqjvg/psnRbGie9U9UNo5iU18pfrWdnUpaL+8Ep Bgi1DamOmq62SitQekXeKm72yUJX5lc4AVLxvoikEJea8Qol5l9poCRVf+i2cyQYU8fG 7fVL8kOMHc7jRNttoCtJFx8y06FBC08J/zRBIOch88kuRyaUYddCKbkVAKPcc2ww0slG a2Cw== X-Gm-Message-State: APjAAAUt3va7bM9OU5/HG0AMd5SLG5IgOVDQQzaTzsNqLDyMz1ITe5ai zvvp8PhDrwytUhTWo+WnMLs= X-Received: by 2002:a05:6830:1615:: with SMTP id g21mr2783459otr.49.1581370838369; Mon, 10 Feb 2020 13:40:38 -0800 (PST) Received: from [100.71.96.87] ([143.166.81.254]) by smtp.gmail.com with ESMTPSA id n75sm486799ota.68.2020.02.10.13.40.37 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 10 Feb 2020 13:40:37 -0800 (PST) Subject: Re: [PATCH v3] PCI: pciehp: Make sure pciehp_isr clears interrupt events To: Lukas Wunner 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 References: <20200207195450.52026-1-stuart.w.hayes@gmail.com> <20200209150328.2x2zumhqbs6fihmc@wunner.de> <20200209180722.ikuyjignnd7ddfp5@wunner.de> <20200209202512.rzaqoc7tydo2ouog@wunner.de> From: Stuart Hayes Message-ID: <0f772c73-5616-ae7c-6808-ecefac8ebf13@gmail.com> Date: Mon, 10 Feb 2020 15:40:36 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: <20200209202512.rzaqoc7tydo2ouog@wunner.de> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2/9/20 2:25 PM, Lukas Wunner wrote: > 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) > I tested this patch, and it fixes the issue on my system.