From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7720AC433E0 for ; Thu, 18 Feb 2021 16:50:32 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 17EF364E33 for ; Thu, 18 Feb 2021 16:50:32 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 17EF364E33 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Message-ID:Subject:To:From: Date:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:References:List-Owner; bh=WweHrW4O7o4U87xUnhqY7FGbzfRqOCozmGM9x29Q4KQ=; b=zICzIPPZtegEfE9blXekCXhSN SAAfYSD3U/WcCWeD7aG9/bWOa8OxTvCKhRKmfTk4ugyfbloNqbsIevorTSKKWlLLazn4/jNqExVXW NAhVYx/DiclHQSPnhRzWusR3+XDN7dZMpfIfQIKWTC74QrgUqg87Z4+mrn9Vc9y7zXxrXl6kDbJpE TGMcI9pdw84pioO39d0Z7tfTIvGntJ5syzyeRHyk6/Witans3dvlglkg/dlwMJrVL/2htvhOjOjfg j6VcX9TIcnKJ2nClp6SocvuSFfowu+acCOhr9FsT+dsg2UBCd02IdAYQ74HtPVxGFL7quz+9hhf1n F9HoxMwJA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1lCmVK-00042V-OT; Thu, 18 Feb 2021 16:50:14 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1lCmVG-00041r-9W; Thu, 18 Feb 2021 16:50:11 +0000 Received: by mail.kernel.org (Postfix) with ESMTPSA id 47B1E64EB3; Thu, 18 Feb 2021 16:50:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1613667008; bh=92qqsNKNLtQETmYq95JpSvV7z/qtYNHIl+uuhSbp41U=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=ha/QTsAIoUVp9c44+GpGrnWclaUGHIhd29nH8xrV1yz1URHEFONOiaMOpm82sNNfM 1b7fUrjk0tVZGc5lnza6WvRTAqpSysWcauDvEPeQRWS2JlELbX/40TDGMROlLKCgmw 8vOOT9C5HhwHLTMyUteJ7yytgxTcwXtk8ScIYmWg0+1pFQKRKrrtFxvexBaE3u/PY8 qSOh16p1wxThaFAdpW75VooJvGCecUcD4nwOH6iasCI1M/wWz2na8vveAnOzvx5uAJ +Q0crPP+4O4atc5UpW6XyKmJRiq14xZ9SyRCpeUuj45oAAmOkh4/fCymsKScOObW56 e1Iaipe1Q8MGA== Date: Thu, 18 Feb 2021 10:50:06 -0600 From: Bjorn Helgaas To: mingchuang.qiao@mediatek.com Subject: Re: [v4] PCI: Avoid unsync of LTR mechanism configuration Message-ID: <20210218165006.GA983767@bjorn-Precision-5520> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20210204095125.9212-1-mingchuang.qiao@mediatek.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210218_115010_505126_77B69D62 X-CRM114-Status: GOOD ( 28.57 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kerun.zhu@mediatek.com, linux-pci@vger.kernel.org, lambert.wang@mediatek.com, rjw@rjwysocki.net, linux-kernel@vger.kernel.org, matthias.bgg@gmail.com, alex.williamson@redhat.com, linux-mediatek@lists.infradead.org, utkarsh.h.patel@intel.com, haijun.liu@mediatek.com, bhelgaas@google.com, mika.westerberg@linux.intel.com, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org On Thu, Feb 04, 2021 at 05:51:25PM +0800, mingchuang.qiao@mediatek.com wrote: > From: Mingchuang Qiao > > In bus scan flow, the "LTR Mechanism Enable" bit of DEVCTL2 register is > configured in pci_configure_ltr(). If device and bridge both support LTR > mechanism, the "LTR Mechanism Enable" bit of device and bridge will be > enabled in DEVCTL2 register. And pci_dev->ltr_path will be set as 1. > > If PCIe link goes down when device resets, the "LTR Mechanism Enable" bit > of bridge will change to 0 according to PCIe r5.0, sec 7.5.3.16. However, > the pci_dev->ltr_path value of bridge is still 1. > > For following conditions, check and re-configure "LTR Mechanism Enable" bit > of bridge to make "LTR Mechanism Enable" bit match ltr_path value. > -before configuring device's LTR for hot-remove/hot-add > -before restoring device's DEVCTL2 register when restore device state There's definitely a bug here. The commit log should say a little more about what it is. I *think* if LTR is enabled and we suspend (putting the device in D3cold) and resume, LTR probably doesn't work after resume because LTR is disabled in the upstream bridge, which would be an obvious bug. Also, if a device with LTR enabled is hot-removed, and we hot-add a device, I think LTR will not work on the new device. Possibly also a bug, although I'm not convinced we know how to configure LTR on the new device anyway. So I'd *like* to merge the bug fix for v5.12, but I think I'll wait because of the issue below. > Signed-off-by: Mingchuang Qiao > --- > changes of v4 > -fix typo of commit message > -rename: pci_reconfigure_bridge_ltr()->pci_bridge_reconfigure_ltr() > changes of v3 > -call pci_reconfigure_bridge_ltr() in probe.c > changes of v2 > -modify patch description > -reconfigure bridge's LTR before restoring device DEVCTL2 register > --- > drivers/pci/pci.c | 25 +++++++++++++++++++++++++ > drivers/pci/pci.h | 1 + > drivers/pci/probe.c | 13 ++++++++++--- > 3 files changed, 36 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index b9fecc25d213..6bf65d295331 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -1437,6 +1437,24 @@ static int pci_save_pcie_state(struct pci_dev *dev) > return 0; > } > > +void pci_bridge_reconfigure_ltr(struct pci_dev *dev) > +{ > +#ifdef CONFIG_PCIEASPM > + struct pci_dev *bridge; > + u32 ctl; > + > + bridge = pci_upstream_bridge(dev); > + if (bridge && bridge->ltr_path) { > + pcie_capability_read_dword(bridge, PCI_EXP_DEVCTL2, &ctl); > + if (!(ctl & PCI_EXP_DEVCTL2_LTR_EN)) { > + pci_dbg(bridge, "re-enabling LTR\n"); > + pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2, > + PCI_EXP_DEVCTL2_LTR_EN); This pattern of updating the upstream bridge on behalf of "dev" is problematic because it's racy: CPU 1 CPU 2 ------------------- --------------------- ctl = read DEVCTL2 ctl = read(DEVCTL2) ctl |= DEVCTL2_LTR_EN ctl |= DEVCTL2_ARI write(DEVCTL2, ctl) write(DEVCTL2, ctl) Now the bridge has ARI set, but not LTR_EN. We have the same problem in the pci_enable_device() path. The most recent try at fixing it is [1]. [1] https://lore.kernel.org/linux-pci/20201218174011.340514-2-s.miroshnichenko@yadro.com/ > + } > + } > +#endif > +} > + > static void pci_restore_pcie_state(struct pci_dev *dev) > { > int i = 0; > @@ -1447,6 +1465,13 @@ static void pci_restore_pcie_state(struct pci_dev *dev) > if (!save_state) > return; > > + /* > + * Downstream ports reset the LTR enable bit when link goes down. > + * Check and re-configure the bit here before restoring device. > + * PCIe r5.0, sec 7.5.3.16. > + */ > + pci_bridge_reconfigure_ltr(dev); > + > cap = (u16 *)&save_state->cap.data[0]; > pcie_capability_write_word(dev, PCI_EXP_DEVCTL, cap[i++]); > pcie_capability_write_word(dev, PCI_EXP_LNKCTL, cap[i++]); > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index 5c59365092fa..b3a5e5287cb7 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -111,6 +111,7 @@ void pci_free_cap_save_buffers(struct pci_dev *dev); > bool pci_bridge_d3_possible(struct pci_dev *dev); > void pci_bridge_d3_update(struct pci_dev *dev); > void pci_bridge_wait_for_secondary_bus(struct pci_dev *dev); > +void pci_bridge_reconfigure_ltr(struct pci_dev *dev); > > static inline void pci_wakeup_event(struct pci_dev *dev) > { > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 953f15abc850..ade055e9fb58 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -2132,9 +2132,16 @@ static void pci_configure_ltr(struct pci_dev *dev) > * Complex and all intermediate Switches indicate support for LTR. > * PCIe r4.0, sec 6.18. > */ > - if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT || > - ((bridge = pci_upstream_bridge(dev)) && > - bridge->ltr_path)) { > + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) { > + pcie_capability_set_word(dev, PCI_EXP_DEVCTL2, > + PCI_EXP_DEVCTL2_LTR_EN); > + dev->ltr_path = 1; > + return; > + } > + > + bridge = pci_upstream_bridge(dev); > + if (bridge && bridge->ltr_path) { > + pci_bridge_reconfigure_ltr(dev); > pcie_capability_set_word(dev, PCI_EXP_DEVCTL2, > PCI_EXP_DEVCTL2_LTR_EN); > dev->ltr_path = 1; > -- > 2.18.0 _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek