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=-16.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable 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 CB66CC433DB for ; Thu, 18 Feb 2021 18:53:23 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 7C52664EBB for ; Thu, 18 Feb 2021 18:53:23 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233383AbhBRSwW (ORCPT ); Thu, 18 Feb 2021 13:52:22 -0500 Received: from mail.kernel.org ([198.145.29.99]:60388 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233913AbhBRQwD (ORCPT ); Thu, 18 Feb 2021 11:52:03 -0500 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 Cc: bhelgaas@google.com, matthias.bgg@gmail.com, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, haijun.liu@mediatek.com, lambert.wang@mediatek.com, kerun.zhu@mediatek.com, mika.westerberg@linux.intel.com, alex.williamson@redhat.com, rjw@rjwysocki.net, utkarsh.h.patel@intel.com Subject: Re: [v4] PCI: Avoid unsync of LTR mechanism configuration Message-ID: <20210218165006.GA983767@bjorn-Precision-5520> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210204095125.9212-1-mingchuang.qiao@mediatek.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.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 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 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=unavailable 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 E2C6BC433E6 for ; Thu, 18 Feb 2021 16:52:19 +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 836CA64EAF for ; Thu, 18 Feb 2021 16:52:19 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 836CA64EAF 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-arm-kernel-bounces+linux-arm-kernel=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=8SRC1YMyo/XEhcbb3zDNAATGww0zEVX5UrJJFzn09NA=; b=MLyrK//+QCUX+cvSYqgpgmJ0H URc/8eu/V1+ZtOEgNHB9tSInHS+FETD+JVFMOifZPGa2Ma3akixPFz2E4pYbFWoIWLG2hJoKzDafh qyI3KFb8ovc1+lZNnRcIEt5ovP+xm9/8wOVJ1eLK+2Ln3Y9YiXNckdYXH1UHf/gncb4LLtHYx59HY 1eVi/qP9T8MsnwP3UrapbnMcQUsVpFsYQKd5Naig35KadWx3Aq9v3SH4KZBpY6rI8aAor7enrxayQ ns6HlwW+QQEcj6X581rhO5Qorh+7jmFWt1Z3ifHgp5WldhV/zh3hmOqRnszH2WUE0VtWmGBI8/wzz NGFzXAptg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1lCmVJ-00042J-KF; Thu, 18 Feb 2021 16:50:13 +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-arm-kernel@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-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=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-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel