* [PATCH v2] PCI: Re-enable downstream port LTR if it was previously enabled
@ 2021-01-19 13:14 Mika Westerberg
2021-01-21 22:31 ` Bjorn Helgaas
0 siblings, 1 reply; 6+ messages in thread
From: Mika Westerberg @ 2021-01-19 13:14 UTC (permalink / raw)
To: Bjorn Helgaas, Rafael J. Wysocki
Cc: Utkarsh H Patel, Mika Westerberg, linux-pci
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 <utkarsh.h.patel@intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
Previous version can be found here:
https://lore.kernel.org/linux-pci/20210114134724.79511-1-mika.westerberg@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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] PCI: Re-enable downstream port LTR if it was previously enabled
2021-01-19 13:14 [PATCH v2] PCI: Re-enable downstream port LTR if it was previously enabled Mika Westerberg
@ 2021-01-21 22:31 ` Bjorn Helgaas
2021-01-22 7:03 ` Mingchuang Qiao
0 siblings, 1 reply; 6+ messages in thread
From: Bjorn Helgaas @ 2021-01-21 22:31 UTC (permalink / raw)
To: Mika Westerberg
Cc: Bjorn Helgaas, Rafael J. Wysocki, Utkarsh H Patel, linux-pci,
mingchuang.qiao, matthias.bgg, lambert.wang, linux-mediatek,
haijun.liu, linux-arm-kernel, linux-kernel, Alex Williamson
[+cc Alex and Mingchuang et al from
https://lore.kernel.org/r/20210112072739.31624-1-mingchuang.qiao@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 <utkarsh.h.patel@intel.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
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?
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.westerberg@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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] PCI: Re-enable downstream port LTR if it was previously enabled
2021-01-21 22:31 ` Bjorn Helgaas
@ 2021-01-22 7:03 ` Mingchuang Qiao
2021-01-22 10:05 ` Mika Westerberg
2021-01-22 13:20 ` Bjorn Helgaas
0 siblings, 2 replies; 6+ messages in thread
From: Mingchuang Qiao @ 2021-01-22 7:03 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Mika Westerberg, Bjorn Helgaas, Rafael J. Wysocki,
Utkarsh H Patel, linux-pci, matthias.bgg, lambert.wang,
linux-mediatek, haijun.liu, linux-arm-kernel, linux-kernel,
Alex Williamson
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.qiao@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 <utkarsh.h.patel@intel.com>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>
> 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.westerberg@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
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] PCI: Re-enable downstream port LTR if it was previously enabled
2021-01-22 7:03 ` Mingchuang Qiao
@ 2021-01-22 10:05 ` Mika Westerberg
2021-01-22 13:20 ` Bjorn Helgaas
1 sibling, 0 replies; 6+ messages in thread
From: Mika Westerberg @ 2021-01-22 10:05 UTC (permalink / raw)
To: Mingchuang Qiao
Cc: Bjorn Helgaas, Bjorn Helgaas, Rafael J. Wysocki, Utkarsh H Patel,
linux-pci, matthias.bgg, lambert.wang, linux-mediatek,
haijun.liu, linux-arm-kernel, linux-kernel, Alex Williamson
Hi,
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.qiao@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 <utkarsh.h.patel@intel.com>
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> >
> > 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?
I looked at your patch and indeed it is essentially doing the same as
this one. So let's forget this patch and go forward with yours :)
Would you like to expand your patch to handle the reset case too that
Bjorn desribes below?
> > 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] PCI: Re-enable downstream port LTR if it was previously enabled
2021-01-22 7:03 ` Mingchuang Qiao
2021-01-22 10:05 ` Mika Westerberg
@ 2021-01-22 13:20 ` Bjorn Helgaas
2021-01-25 10:14 ` Mingchuang Qiao
1 sibling, 1 reply; 6+ messages in thread
From: Bjorn Helgaas @ 2021-01-22 13:20 UTC (permalink / raw)
To: Mingchuang Qiao
Cc: Mika Westerberg, Bjorn Helgaas, Rafael J. Wysocki,
Utkarsh H Patel, linux-pci, matthias.bgg, lambert.wang,
linux-mediatek, haijun.liu, linux-arm-kernel, linux-kernel,
Alex Williamson
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.qiao@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 <utkarsh.h.patel@intel.com>
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> >
> > 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.
> > 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.westerberg@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
> > >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] PCI: Re-enable downstream port LTR if it was previously enabled
2021-01-22 13:20 ` Bjorn Helgaas
@ 2021-01-25 10:14 ` Mingchuang Qiao
0 siblings, 0 replies; 6+ messages in thread
From: Mingchuang Qiao @ 2021-01-25 10:14 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Mika Westerberg, Bjorn Helgaas, Rafael J. Wysocki,
Utkarsh H Patel, linux-pci, matthias.bgg, lambert.wang,
linux-mediatek, haijun.liu, linux-arm-kernel, linux-kernel,
Alex Williamson
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.qiao@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 <utkarsh.h.patel@intel.com>
> > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > >
> > > 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.westerberg@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
> > > >
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-26 19:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-19 13:14 [PATCH v2] PCI: Re-enable downstream port LTR if it was previously enabled Mika Westerberg
2021-01-21 22:31 ` Bjorn Helgaas
2021-01-22 7:03 ` Mingchuang Qiao
2021-01-22 10:05 ` Mika Westerberg
2021-01-22 13:20 ` Bjorn Helgaas
2021-01-25 10:14 ` Mingchuang Qiao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).