| Message ID | 20210114134724.79511-1-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Re-enable downstream port LTR if it was previously enabled | expand |
[+cc Puranjay] On Thu, Jan 14, 2021 at 04:47:24PM +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. IIRC LTR is only needed for L1.2, and of course the LTR Capability (Max Snoop/No-Snoop Latency registers) and the L1 PM Substates Capability (LTR_L1.2_THRESHOLD) must be programmed before enabling LTR. For the bridge, I guess we're assuming those were programmed before the hot-remove, and they remain valid after the hot-add. But what about the endpoint that we hot-added? How do we program its LTR and L1 PM Substates Capabilities? I know we have aspm_calc_l1ss_info() for L1 PM Substates, but I really don't trust it, and I don't think we do anything at all to program the LTR Capability. I used to think the LTR _DSM was a way to help us program the LTR Capability, and Puranjay did a nice job implementing support for it [1]. But I now think that _DSM doesn't give us enough information (and of course it doesn't help at all for non-ACPI systems or for hierarchies not integrated on the system board), so I didn't merge Puranjay's work. I tried to have some discussion in the PCI SIG about this, but it never really went anywhere. Here's my basic question, just for the archives: I think the LTR capability Max Snoop registers could also use some clarification. The base spec says "Software should set this to the platform's maximum supported latency or less." I assume this platform data is supposed to come from the ACPI LTR _DSM. The firmware spec says software should sum the latencies along the path between each downstream port (I wonder if this should say "Root Port"?) and an endpoint that supports LTR. Switches not embedded in the platform will not have this _DSM, but I assume they contribute to this sum. But I don't know *what* they contribute. > For this reason check if the bridge upstrea had LTR enabled set > previously and re-enable it before enabling LTR for the endpoint. s/upstrea/upstream/ s/enabled set/enabled/ Seems like there could be more things in the upstream bridge that need to be reprogrammed when the link comes back up (MPS, Common Clock Configuration, etc?). I don't see anything in the spec about link status affecting MPS, but if we hot-removed a device that supported 4KB MPS and hot-added one that only support 128B, we might need more extensive reconfiguration. I haven't checked; maybe that's already covered? I think Common Clock Config also depends on characteristics of the hot-added device, so we might need to take a look at that, too. If it turns out that we need to do more to the upstream bridge than just this LTR thing, I wonder if we should pull it out to some kind of "reconfig bridge" function so it's not buried in several random places. [1] https://lore.kernel.org/r/20201015080311.7811-1-puranjay12@gmail.com > Reported-by: Utkarsh H Patel <utkarsh.h.patel@intel.com> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > 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..cd174e06f46f 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_downstream_port(bridge)) { Why test for pcie_downstream_port(bridge) here? "dev" is a PCIe device, and "bridge" is a PCI device leading to "dev". I think the only possibilities are that "bridge" is a root port, a switch downstream port, or a PCI-to-PCIe bridge, i.e., exactly what pcie_downstream_port() tests for. > + 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 >
Hi Bjorn, On Thu, Jan 14, 2021 at 06:10:07PM -0600, Bjorn Helgaas wrote: > [+cc Puranjay] > > On Thu, Jan 14, 2021 at 04:47:24PM +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. > > IIRC LTR is only needed for L1.2, and of course the LTR Capability > (Max Snoop/No-Snoop Latency registers) and the L1 PM Substates > Capability (LTR_L1.2_THRESHOLD) must be programmed before enabling > LTR. For the bridge, I guess we're assuming those were programmed > before the hot-remove, and they remain valid after the hot-add. > > But what about the endpoint that we hot-added? How do we program its > LTR and L1 PM Substates Capabilities? I know we have > aspm_calc_l1ss_info() for L1 PM Substates, but I really don't trust > it, and I don't think we do anything at all to program the LTR > Capability. True - we don't. However, enabling the LTR bit here for the endpoint (and for the bridges all the way up to the root port) makes the endpoint to report that there is no LTR requirement and that allows the SoC to do some PM optimizations or so. We actually see that if this is not re-programmed like this on a Tiger Lake based ChromeBook S0ix fails (S0ix is Intel low power idle state). > I used to think the LTR _DSM was a way to help us program the LTR > Capability, and Puranjay did a nice job implementing support for it > [1]. But I now think that _DSM doesn't give us enough information > (and of course it doesn't help at all for non-ACPI systems or for > hierarchies not integrated on the system board), so I didn't merge > Puranjay's work. > > I tried to have some discussion in the PCI SIG about this, but it > never really went anywhere. Here's my basic question, just for the > archives: > > I think the LTR capability Max Snoop registers could also use some > clarification. The base spec says "Software should set this to the > platform's maximum supported latency or less." I assume this > platform data is supposed to come from the ACPI LTR _DSM. The > firmware spec says software should sum the latencies along the path > between each downstream port (I wonder if this should say "Root > Port"?) and an endpoint that supports LTR. Switches not embedded in > the platform will not have this _DSM, but I assume they contribute > to this sum. But I don't know *what* they contribute. > That's a fair question :) I personally think that the driver for the specific hardware should know what is the latency tolerance for the device when it is doing different "tasks". > > For this reason check if the bridge upstrea had LTR enabled set > > previously and re-enable it before enabling LTR for the endpoint. > > s/upstrea/upstream/ > s/enabled set/enabled/ Thanks, I'll fix those. > Seems like there could be more things in the upstream bridge that need > to be reprogrammed when the link comes back up (MPS, Common Clock > Configuration, etc?). > > I don't see anything in the spec about link status affecting MPS, but > if we hot-removed a device that supported 4KB MPS and hot-added one > that only support 128B, we might need more extensive reconfiguration. > I haven't checked; maybe that's already covered? It looks like that's covered in pcie_find_smpss(). It limits the MPS to 128 if there are hotplug bridges in the topology. Assuming I read it correctly. > I think Common Clock Config also depends on characteristics of the > hot-added device, so we might need to take a look at that, too. I think pcie_aspm_configure_common_clock() takes care of that already. It programs both ends of the link when a device is being added. > If it turns out that we need to do more to the upstream bridge than > just this LTR thing, I wonder if we should pull it out to some kind of > "reconfig bridge" function so it's not buried in several random > places. It seems that at the moment it is only the LTR thing that needs to be reconfigured as others looks like they are covered by their corresponding "enable" functions. Not sure if it is better to move those to "reconfig bridge" function because it might be harder to follow if it is not close to the code that enables the feature in the first place. But I can do that if you still think it is better. > [1] https://lore.kernel.org/r/20201015080311.7811-1-puranjay12@gmail.com > > > Reported-by: Utkarsh H Patel <utkarsh.h.patel@intel.com> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > --- > > 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..cd174e06f46f 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_downstream_port(bridge)) { > > Why test for pcie_downstream_port(bridge) here? "dev" is a PCIe > device, and "bridge" is a PCI device leading to "dev". I think the > only possibilities are that "bridge" is a root port, a switch > downstream port, or a PCI-to-PCIe bridge, i.e., exactly what > pcie_downstream_port() tests for. Good point that check is not needed. I'll remove it. > > + 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 > >
On Fri, Jan 15, 2021 at 11:00:21AM +0200, Mika Westerberg wrote: > On Thu, Jan 14, 2021 at 06:10:07PM -0600, Bjorn Helgaas wrote: > > On Thu, Jan 14, 2021 at 04:47:24PM +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. > > > > IIRC LTR is only needed for L1.2, and of course the LTR Capability > > (Max Snoop/No-Snoop Latency registers) and the L1 PM Substates > > Capability (LTR_L1.2_THRESHOLD) must be programmed before enabling > > LTR. For the bridge, I guess we're assuming those were programmed > > before the hot-remove, and they remain valid after the hot-add. > > > > But what about the endpoint that we hot-added? How do we program its > > LTR and L1 PM Substates Capabilities? I know we have > > aspm_calc_l1ss_info() for L1 PM Substates, but I really don't trust > > it, and I don't think we do anything at all to program the LTR > > Capability. > > True - we don't. However, enabling the LTR bit here for the endpoint > (and for the bridges all the way up to the root port) makes the endpoint > to report that there is no LTR requirement and that allows the SoC to do > some PM optimizations or so. > > We actually see that if this is not re-programmed like this on a Tiger > Lake based ChromeBook S0ix fails (S0ix is Intel low power idle state). So if we set PCI_EXP_DEVCTL2_LTR_EN for the bridge and also for the hot-added endpoint, S0ix works. But we never get to the S0ix state if we don't set those bits? And we never actually set the Max Snoop/No-Snoop Latency registers in the LTR Capability, so they should power up as zeroes. IIRC, that is the most conservative setting (PCIe r5.0, sec 6.18 says "all 0's indicates that the device will be impacted by any delay and that the best possible service is requested"). So I *guess* it makes some sort of sense to enable LTR in that situation, although I wish we could set the content of the messages before enabling them. > > I used to think the LTR _DSM was a way to help us program the LTR > > Capability, and Puranjay did a nice job implementing support for it > > [1]. But I now think that _DSM doesn't give us enough information > > (and of course it doesn't help at all for non-ACPI systems or for > > hierarchies not integrated on the system board), so I didn't merge > > Puranjay's work. > > > > I tried to have some discussion in the PCI SIG about this, but it > > never really went anywhere. Here's my basic question, just for the > > archives: > > > > I think the LTR capability Max Snoop registers could also use some > > clarification. The base spec says "Software should set this to the > > platform's maximum supported latency or less." I assume this > > platform data is supposed to come from the ACPI LTR _DSM. The > > firmware spec says software should sum the latencies along the path > > between each downstream port (I wonder if this should say "Root > > Port"?) and an endpoint that supports LTR. Switches not embedded in > > the platform will not have this _DSM, but I assume they contribute > > to this sum. But I don't know *what* they contribute. > > > > That's a fair question :) > > I personally think that the driver for the specific hardware should know > what is the latency tolerance for the device when it is doing different > "tasks". > > > > For this reason check if the bridge upstrea had LTR enabled set > > > previously and re-enable it before enabling LTR for the endpoint. > > > > s/upstrea/upstream/ > > s/enabled set/enabled/ > > Thanks, I'll fix those. > > > Seems like there could be more things in the upstream bridge that need > > to be reprogrammed when the link comes back up (MPS, Common Clock > > Configuration, etc?). > > > > I don't see anything in the spec about link status affecting MPS, but > > if we hot-removed a device that supported 4KB MPS and hot-added one > > that only support 128B, we might need more extensive reconfiguration. > > I haven't checked; maybe that's already covered? > > It looks like that's covered in pcie_find_smpss(). It limits the MPS to > 128 if there are hotplug bridges in the topology. Assuming I read it > correctly. OK, and it looks like that's called in the hot-add path: pciehp_configure_device pcie_bus_configure_settings pcie_find_smpss so that's good. I forgot that we had that path. > > I think Common Clock Config also depends on characteristics of the > > hot-added device, so we might need to take a look at that, too. > > I think pcie_aspm_configure_common_clock() takes care of that already. > It programs both ends of the link when a device is being added. Yep, right. > > If it turns out that we need to do more to the upstream bridge than > > just this LTR thing, I wonder if we should pull it out to some kind of > > "reconfig bridge" function so it's not buried in several random > > places. > > It seems that at the moment it is only the LTR thing that needs to be > reconfigured as others looks like they are covered by their > corresponding "enable" functions. Not sure if it is better to move those > to "reconfig bridge" function because it might be harder to follow if it > is not close to the code that enables the feature in the first place. > > But I can do that if you still think it is better. No, I don't think it's worth doing that. Bjorn
On Fri, Jan 15, 2021 at 06:36:06PM -0600, Bjorn Helgaas wrote: > On Fri, Jan 15, 2021 at 11:00:21AM +0200, Mika Westerberg wrote: > > On Thu, Jan 14, 2021 at 06:10:07PM -0600, Bjorn Helgaas wrote: > > > On Thu, Jan 14, 2021 at 04:47:24PM +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. > > > > > > IIRC LTR is only needed for L1.2, and of course the LTR Capability > > > (Max Snoop/No-Snoop Latency registers) and the L1 PM Substates > > > Capability (LTR_L1.2_THRESHOLD) must be programmed before enabling > > > LTR. For the bridge, I guess we're assuming those were programmed > > > before the hot-remove, and they remain valid after the hot-add. > > > > > > But what about the endpoint that we hot-added? How do we program its > > > LTR and L1 PM Substates Capabilities? I know we have > > > aspm_calc_l1ss_info() for L1 PM Substates, but I really don't trust > > > it, and I don't think we do anything at all to program the LTR > > > Capability. > > > > True - we don't. However, enabling the LTR bit here for the endpoint > > (and for the bridges all the way up to the root port) makes the endpoint > > to report that there is no LTR requirement and that allows the SoC to do > > some PM optimizations or so. > > > > We actually see that if this is not re-programmed like this on a Tiger > > Lake based ChromeBook S0ix fails (S0ix is Intel low power idle state). > > So if we set PCI_EXP_DEVCTL2_LTR_EN for the bridge and also for the > hot-added endpoint, S0ix works. But we never get to the S0ix state if > we don't set those bits? Yes, correct. > And we never actually set the Max Snoop/No-Snoop Latency registers in > the LTR Capability, so they should power up as zeroes. IIRC, that is > the most conservative setting (PCIe r5.0, sec 6.18 says "all 0's > indicates that the device will be impacted by any delay and that the > best possible service is requested"). > > So I *guess* it makes some sort of sense to enable LTR in that > situation, although I wish we could set the content of the messages > before enabling them. Right. We actually have a bus agnostic API for that in PM QoS that requires the PCI core to implement dev->power.set_latency_tolerance() and then drivers can take advantage of this. However, it does not make much sense to implement it without an actual user (driver). > > > I used to think the LTR _DSM was a way to help us program the LTR > > > Capability, and Puranjay did a nice job implementing support for it > > > [1]. But I now think that _DSM doesn't give us enough information > > > (and of course it doesn't help at all for non-ACPI systems or for > > > hierarchies not integrated on the system board), so I didn't merge > > > Puranjay's work. > > > > > > I tried to have some discussion in the PCI SIG about this, but it > > > never really went anywhere. Here's my basic question, just for the > > > archives: > > > > > > I think the LTR capability Max Snoop registers could also use some > > > clarification. The base spec says "Software should set this to the > > > platform's maximum supported latency or less." I assume this > > > platform data is supposed to come from the ACPI LTR _DSM. The > > > firmware spec says software should sum the latencies along the path > > > between each downstream port (I wonder if this should say "Root > > > Port"?) and an endpoint that supports LTR. Switches not embedded in > > > the platform will not have this _DSM, but I assume they contribute > > > to this sum. But I don't know *what* they contribute. > > > > > > > That's a fair question :) > > > > I personally think that the driver for the specific hardware should know > > what is the latency tolerance for the device when it is doing different > > "tasks". > > > > > > For this reason check if the bridge upstrea had LTR enabled set > > > > previously and re-enable it before enabling LTR for the endpoint. > > > > > > s/upstrea/upstream/ > > > s/enabled set/enabled/ > > > > Thanks, I'll fix those. > > > > > Seems like there could be more things in the upstream bridge that need > > > to be reprogrammed when the link comes back up (MPS, Common Clock > > > Configuration, etc?). > > > > > > I don't see anything in the spec about link status affecting MPS, but > > > if we hot-removed a device that supported 4KB MPS and hot-added one > > > that only support 128B, we might need more extensive reconfiguration. > > > I haven't checked; maybe that's already covered? > > > > It looks like that's covered in pcie_find_smpss(). It limits the MPS to > > 128 if there are hotplug bridges in the topology. Assuming I read it > > correctly. > > OK, and it looks like that's called in the hot-add path: > > pciehp_configure_device > pcie_bus_configure_settings > pcie_find_smpss > > so that's good. I forgot that we had that path. > > > > I think Common Clock Config also depends on characteristics of the > > > hot-added device, so we might need to take a look at that, too. > > > > I think pcie_aspm_configure_common_clock() takes care of that already. > > It programs both ends of the link when a device is being added. > > Yep, right. > > > > If it turns out that we need to do more to the upstream bridge than > > > just this LTR thing, I wonder if we should pull it out to some kind of > > > "reconfig bridge" function so it's not buried in several random > > > places. > > > > It seems that at the moment it is only the LTR thing that needs to be > > reconfigured as others looks like they are covered by their > > corresponding "enable" functions. Not sure if it is better to move those > > to "reconfig bridge" function because it might be harder to follow if it > > is not close to the code that enables the feature in the first place. > > > > But I can do that if you still think it is better. > > No, I don't think it's worth doing that. OK, thanks.
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 0eb68b47354f..cd174e06f46f 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_downstream_port(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;
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 upstrea had LTR enabled set 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> --- drivers/pci/probe.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)