Re: [v3] PCI: Avoid unsync of LTR mechanism configuration
Hi, On Mon, 2021-02-01 at 13:32 +0200, Mika Westerberg wrote: > Hi, > > On Fri, Jan 29, 2021 at 03:11:37PM +0800, mingchuang.q...@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 mtach ltr_path value. > > Typo mtach -> match. > > >-before configuring device's LTR for hot-remove/hot-add > >-before restoring device's DEVCTL2 register when restore device state > > > > Signed-off-by: Mingchuang Qiao > > --- > > changes of v2 > > -modify patch description > > -reconfigure bridge's LTR before restoring device DEVCTL2 register > > changes of v3 > > -call pci_reconfigure_bridge_ltr() in probe.c > > Hmm, which part of this patch takes care of the reset path? It is not > entirely clear to me at least. > When device resets and link goes down, there seems to have two methods to recover for software. -One is that trigger device removal and rescan. -The other is that restore device with pci_restore_state() after link comes back up. For above both scenarios, we need check and reconfigure "LTR Mechanism Enable" bit of bridge. It's also this patch intends to do. -For the rescan scenario, it's done in pci_configure_ltr(). -For the restore scenario, it's done in pci_restore_pcie_state(). > > --- > > 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..12b557c8f062 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_reconfigure_bridge_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); > > + } > > + } > > +#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_reconfigure_bridge_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..a660a01358c5 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_reconfigure_bridge_ltr(struct pci_dev *dev); > > Nit: calling it pci_bridge_reconfigure_ltr() would match better with the > other function names. > Thanks for the suggestion. I will update the name in next patch :) > > > > static inli
Re: [v2] PCI: Avoid unsync of LTR mechanism configuration
On Thu, 2021-01-28 at 16:27 +0200, Mika Westerberg wrote: > Hi, > > On Thu, Jan 28, 2021 at 06:05:31PM +0800, mingchuang.q...@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 mtach ltr_path value. > >-before configuring device's LTR for hot-remove/hot-add > >-before restoring device's DEVCTL2 register when restore device state > > > > Signed-off-by: Mingchuang Qiao > > --- > > changes of v2 > > -modify patch description > > -reconfigure bridge's LTR before restoring device DEVCTL2 register > > --- > > drivers/pci/pci.c | 25 + > > drivers/pci/probe.c | 19 --- > > 2 files changed, 41 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > index b9fecc25d213..88b4eb70c252 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; > > } > > > > +static void pci_reconfigure_bridge_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); > > + } > > + } > > +#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_reconfigure_bridge_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/probe.c b/drivers/pci/probe.c > > index 953f15abc850..4ad172517fd2 100644 > > --- a/drivers/pci/probe.c > > +++ b/drivers/pci/probe.c > > @@ -2132,9 +2132,22 @@ 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) { > > + 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); > > + } > > + > > Can't you use pci_reconfigure_bridge_ltr() here too? > > Otherwise looks good. Thanks for review. I have sent a new patch for this. https://lore.kernel.org/linux-arm-kernel/20210129071137.8743-1-mingchuang.q...@mediatek.com/
Re: [v4] PCI: Avoid unsync of LTR mechanism configuration
Hi Bjorn, On Thu, 2021-02-18 at 10:50 -0600, Bjorn Helgaas wrote: > On Thu, Feb 04, 2021 at 05:51:25PM +0800, mingchuang.q...@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. > Thanks for review. Any further process shall I make to get this patch merged? > > 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 DEVCTL2ctl = 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.miroshniche...@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 *)&
Re: [PATCH] pci: avoid unsync of LTR mechanism configuration
On Tue, 2021-01-12 at 15:36 -0600, Bjorn Helgaas wrote: > Note subject line tips at > https://lore.kernel.org/r/20171026223701.ga25...@bhelgaas-glaptop.roam.corp.google.com > > On Tue, Jan 12, 2021 at 03:27:39PM +0800, mingchuang.q...@mediatek.com wrote: > > From: Mingchuang Qiao > > > > In pci bus scan flow, the LTR mechanism enable bit of DEVCTL2 register > > is configured in pci_configure_ltr(). If device and it's bridge both > > support LTR mechanism, LTR mechanism of device and it's bridge will > > be enabled in DEVCTL2 register. And the flag pci_dev->ltr_path will > > be set as 1. > > s/it's/its/ twice above. > It's == It is. > Its == belonging to 'it'. > Weird, I know, but that's English for you :) > > > For some pcie products, pcie link becomes down when device reset. And then > > the LTR mechanism enable bit of bridge will become 0 based on description > > in PCIE r4.0, sec 7.8.16. However, the pci_dev->ltr_path value of bridge > > is still 1. Remove and rescan flow could be triggered to recover after > > device reset. In the bus rescan flow, the LTR mechanism of device will be > > enabled in pci_configure_ltr() due to ltr_path of its bridge is still 1. > > s/pcie/PCIe/ twice above. > s/PCIE/PCIe/; also reference r5.0 instead of r4.0 if possible. > Sorry for the misspelling and inconvenience. Thanks for the correction and I will follow the suggestion in later patch. > This sounds like a general problem of most device config bits being > cleared by reset. Usually these are restored by pci_restore_state(). > Is that function missing a restore for PCI_EXP_DEVCTL2? I'd rather > have a general-purpose way of restoring all the config state than > little pieces scattered all over. > Actually it’s not PCI_EXP_DEVCTL2 restore missing issue. The PCI_EXP_DEVCTL2 is correctly saved/restored during bridge runtime suspend/resume. It’s the issue that ltr_path value of the bridge mismatches the actual value in bridge’s PCI_EXP_DEVCTL2. Here is the scenario for the issue: 1.PCI bus scan done -For both PCIe Device and bridge: -"LTR Mechanism Enable" bit in PCI_EXP_DEVCTL2 is 1 - ltr_path value is 1 2.Device resets and PCIe link is down, then "LTR Mechanism Enable" bit of bridge changes to 0 according to PCIe r5.0, sec 7.5.3.16. -The ltr_path value of bridge is still 1 but the "LTR Mechanism Enable" bit within PCI_EXP_DEVCTL2 changed to 0. 3.Trigger device removal and bridge enters runtime suspend flow. -"LTR Mechanism Enable" bit of bridge is 0 and saved by pci_save_state(). 4.Trigger bus rescan and bridge enters runtime resume flow. -"LTR Mechanism Enable" bit of bridge is restored by pci_restore_state() and the value is 0. 5.Scan device and configure device's LTR in pci_configure_ltr(). -"LTR Mechanism Enable" bit of device is configured as 1 due to bridge's ltr_path value is 1. 6.The "LTR Mechanism Enable" bit of device and bridge is different now. -When device sends LTR Message, bridge will treat the Message as Unsupported Request according to PCIe r5.0, sec 6.18. This patch is used to make bridge's ltr_path value match "LTR Mechanism Enable" bit within DEVCTL2 register and avoid the Unsupported Request. > > When device's LTR mechanism is enabled, device will send LTR message to > > bridge. Bridge receives the device's LTR message and found bridge's LTR > > mechanism is disabled. Then the bridge will generate unsupported request > > and other error handling flow will be triggered such as AER Non-Fatal > > error handling. > > > > This patch is used to avoid this unsupported request and make the bridge's > > ltr_path value is aligned with DEVCTL2 register value. Check bridge > > register value if aligned with ltr_path in pci_configure_ltr(). If > > register value is disable and the ltr_path is 1, we need configure > > the register value as enable. > > > > Signed-off-by: Mingchuang Qiao > > --- > > drivers/pci/probe.c | 18 +++--- > > 1 file changed, 15 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > index 953f15abc850..49355cf4af54 100644 > > --- a/drivers/pci/probe.c > > +++ b/drivers/pci/probe.c > > @@ -2132,9 +2132,21 @@ 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)) && > > -
Re: [PATCH v2] PCI: Re-enable downstream port LTR if it was previously enabled
On Thu, 2021-01-21 at 16:31 -0600, Bjorn Helgaas wrote: > [+cc Alex and Mingchuang et al from > https://lore.kernel.org/r/20210112072739.31624-1-mingchuang.q...@mediatek.com] > > On Tue, Jan 19, 2021 at 04:14:10PM +0300, Mika Westerberg wrote: > > PCIe r5.0, sec 7.5.3.16 says that the downstream ports must reset the > > LTR enable bit if the link goes down (port goes DL_Down status). Now, if > > we had LTR previously enabled and the PCIe endpoint gets hot-removed and > > then hot-added back the ->ltr_path of the downstream port is still set > > but the port now does not have the LTR enable bit set anymore. > > > > For this reason check if the bridge upstream had LTR enabled previously > > and re-enable it before enabling LTR for the endpoint. > > > > Reported-by: Utkarsh H Patel > > Signed-off-by: Mika Westerberg > > I think this and Mingchuang's patch, which is essentially identical, > are right and solves the problem for hot-remove/hot-add. In that > scenario we call pci_configure_ltr() on the hot-added device, and > with this patch, we'll re-enable LTR on the bridge leading to the new > device before enabling LTR on the new device itself. > > But don't we have a similar problem if we simply do a Fundamental > Reset on a device? I think the reset path will restore the device's > state, including PCI_EXP_DEVCTL2, but it doesn't do anything with the > upstream bridge, does it? > Yes. I think the same problem exists under such scenario, and that’s the issue my patch intends to resolve. I also prepared a v2 patch for review(update the patch description). Shall I submit the v2 patch for review? > So if a bridge and a device below it both have LTR enabled, can't we > have the following: > > - bridge LTR enabled > - device LTR enabled > - reset device, e.g., via Secondary Bus Reset > - link goes down, bridge disables LTR > - link comes back up, LTR disabled in both bridge and device > - restore device state, including LTR enable > - device sends LTR message > - bridge reports Unsupported Request > > > --- > > Previous version can be found here: > > > > > > https://lore.kernel.org/linux-pci/20210114134724.79511-1-mika.westerb...@linux.intel.com/ > > > > Changes from the previous version: > > > > * Corrected typos in the commit message > > * No need to call pcie_downstream_port() > > > > drivers/pci/probe.c | 17 - > > 1 file changed, 16 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > index 0eb68b47354f..a4a8c0305fb9 100644 > > --- a/drivers/pci/probe.c > > +++ b/drivers/pci/probe.c > > @@ -2153,7 +2153,7 @@ static void pci_configure_ltr(struct pci_dev *dev) > > { > > #ifdef CONFIG_PCIEASPM > > struct pci_host_bridge *host = pci_find_host_bridge(dev->bus); > > - struct pci_dev *bridge; > > + struct pci_dev *bridge = NULL; > > u32 cap, ctl; > > > > if (!pci_is_pcie(dev)) > > @@ -2191,6 +2191,21 @@ static void pci_configure_ltr(struct pci_dev *dev) > > if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT || > > ((bridge = pci_upstream_bridge(dev)) && > > bridge->ltr_path)) { > > + /* > > +* Downstream ports reset the LTR enable bit when the > > +* link goes down (e.g on hot-remove) so re-enable the > > +* bit here if not set anymore. > > +* PCIe r5.0, sec 7.5.3.16. > > +*/ > > + if (bridge) { > > + 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); > > + } > > + } > > + pci_dbg(dev, "enabling LTR\n"); > > pcie_capability_set_word(dev, PCI_EXP_DEVCTL2, > > PCI_EXP_DEVCTL2_LTR_EN); > > dev->ltr_path = 1; > > -- > > 2.29.2 > >
Re: [PATCH v2] PCI: Re-enable downstream port LTR if it was previously enabled
On Fri, 2021-01-22 at 07:20 -0600, Bjorn Helgaas wrote: > On Fri, Jan 22, 2021 at 03:03:11PM +0800, Mingchuang Qiao wrote: > > On Thu, 2021-01-21 at 16:31 -0600, Bjorn Helgaas wrote: > > > [+cc Alex and Mingchuang et al from > > > https://lore.kernel.org/r/20210112072739.31624-1-mingchuang.q...@mediatek.com] > > > > > > On Tue, Jan 19, 2021 at 04:14:10PM +0300, Mika Westerberg wrote: > > > > PCIe r5.0, sec 7.5.3.16 says that the downstream ports must reset the > > > > LTR enable bit if the link goes down (port goes DL_Down status). Now, if > > > > we had LTR previously enabled and the PCIe endpoint gets hot-removed and > > > > then hot-added back the ->ltr_path of the downstream port is still set > > > > but the port now does not have the LTR enable bit set anymore. > > > > > > > > For this reason check if the bridge upstream had LTR enabled previously > > > > and re-enable it before enabling LTR for the endpoint. > > > > > > > > Reported-by: Utkarsh H Patel > > > > Signed-off-by: Mika Westerberg > > > > > > I think this and Mingchuang's patch, which is essentially identical, > > > are right and solves the problem for hot-remove/hot-add. In that > > > scenario we call pci_configure_ltr() on the hot-added device, and > > > with this patch, we'll re-enable LTR on the bridge leading to the new > > > device before enabling LTR on the new device itself. > > > > > > But don't we have a similar problem if we simply do a Fundamental > > > Reset on a device? I think the reset path will restore the device's > > > state, including PCI_EXP_DEVCTL2, but it doesn't do anything with the > > > upstream bridge, does it? > > > > Yes. I think the same problem exists under such scenario, and that’s the > > issue my patch intends to resolve. > > I also prepared a v2 patch for review(update the patch description). > > Shall I submit the v2 patch for review? > > How does your patch solve this for the reset path? I don't think we > call pci_configure_ltr() when we reset a device. > Sorry, I misunderstand the reset path. When we do a Fundamental Reset on a device, we can trigger device removal and rescan flow to restore the device. In device rescan flow, pci_configure_ltr() will be invoked. I regard the "remove and rescan flow" as a part of reset path for this case :) If we restore device just with pci_restore_state() in device driver after device resets, the LTR problem also exists due to pci_restore_state() does nothing with upstream bridge. In next patch, I would like to re-enable LTR for upstream bridge before restoring device's PCI_EXP_DEVCTL2 if it is needed. > > > So if a bridge and a device below it both have LTR enabled, can't we > > > have the following: > > > > > > - bridge LTR enabled > > > - device LTR enabled > > > - reset device, e.g., via Secondary Bus Reset > > > - link goes down, bridge disables LTR > > > - link comes back up, LTR disabled in both bridge and device > > > - restore device state, including LTR enable > > > - device sends LTR message > > > - bridge reports Unsupported Request > > > > > > > --- > > > > Previous version can be found here: > > > > > > > > > > > > https://lore.kernel.org/linux-pci/20210114134724.79511-1-mika.westerb...@linux.intel.com/ > > > > > > > > Changes from the previous version: > > > > > > > > * Corrected typos in the commit message > > > > * No need to call pcie_downstream_port() > > > > > > > > drivers/pci/probe.c | 17 - > > > > 1 file changed, 16 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > > > index 0eb68b47354f..a4a8c0305fb9 100644 > > > > --- a/drivers/pci/probe.c > > > > +++ b/drivers/pci/probe.c > > > > @@ -2153,7 +2153,7 @@ static void pci_configure_ltr(struct pci_dev *dev) > > > > { > > > > #ifdef CONFIG_PCIEASPM > > > > struct pci_host_bridge *host = pci_find_host_bridge(dev->bus); > > > > - struct pci_dev *bridge; > > > > + struct pci_dev *bridge = NULL; > > > > u32 cap, ctl; > > > > > > > > if (!pci_is_pcie(dev)) > > > > @@ -2191,6 +2191,21 @@ static void pci_