| Message ID | 1a07f35cdfda64ca1d5154cc85ca1dd5f01137d3.1734257330.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | Fix bwctrl boot hang | expand |
On Sun, 2024-12-15 at 11:20 +0100, Lukas Wunner wrote: > Broken PCIe devices may not set any of the bits in the Link Capabilities > Register's "Max Link Speed" field. Assume 2.5 GT/s in such a case, > which is the lowest possible PCIe speed. It must be supported by every > device per PCIe r6.2 sec 8.2.1. > > Emit a message informing about the malformed field. Use KERN_INFO > severity to minimize annoyance. This will help silicon validation > engineers take note of the issue so that regular users hopefully never > see it. > > There is currently no known affected product, but a subsequent commit > will honor the Max Link Speed field when determining supported speeds > and depends on the field being well-formed. (It uses the Max Link Speed > as highest bit in a GENMASK(highest, lowest) macro and if the field is > zero, that would result in GENMASK(0, lowest).) > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > --- > drivers/pci/pci.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 35dc9f249b86..ab0ef7b6c798 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -6233,6 +6233,13 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev) > u32 lnkcap2, lnkcap; > u8 speeds; > > + /* A device must support 2.5 GT/s (PCIe r6.2 sec 8.2.1) */ > + pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap); > + if (!(lnkcap & PCI_EXP_LNKCAP_SLS)) { > + pci_info(dev, "Undefined Max Link Speed; assume 2.5 GT/s\n"); > + return PCI_EXP_LNKCAP2_SLS_2_5GB; > + } > + > /* > * Speeds retain the reserved 0 at LSB before PCIe Supported Link > * Speeds Vector to allow using SLS Vector bit defines directly. > @@ -6244,8 +6251,6 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev) > if (speeds) > return speeds; > > - pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap); > - > /* Synthesize from the Max Link Speed field */ > if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_5_0GB) > speeds = PCI_EXP_LNKCAP2_SLS_5_0GB | PCI_EXP_LNKCAP2_SLS_2_5GB; I feel like this patch goes a bit against the idea of this being more future proof. Personally, I kind of expect that any future devices which may skip support for lower speeds would start with skipping 2.5 GT/s and a future PCIe spec might allow this. In that case with the above code we end up assuming 2.5 GT/s which won't work while the Supported Link Speeds Vector could contain supported speeds with the assumption that when in doubt software relies on that (PCIe r6.2 sec 7.5.3.18) and it might even be future spec conformant. So I think instead of assuming 2.5 GT/s I was thinking of something like the diff below (on top of this series). Thanks Niklas ---- diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index ef5c48bda012..cfb34fa96f81 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -6223,14 +6223,11 @@ EXPORT_SYMBOL(pcie_bandwidth_available); u8 pcie_get_supported_speeds(struct pci_dev *dev) { u32 lnkcap2, lnkcap; - u8 speeds; + u8 speeds, max_bits; /* A device must support 2.5 GT/s (PCIe r6.2 sec 8.2.1) */ pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap); - if (!(lnkcap & PCI_EXP_LNKCAP_SLS)) { - pci_info(dev, "Undefined Max Link Speed; assume 2.5 GT/s\n"); - return PCI_EXP_LNKCAP2_SLS_2_5GB; - } + max_bits = lnkcap & PCI_EXP_LNKCAP_SLS; /* * Speeds retain the reserved 0 at LSB before PCIe Supported Link @@ -6238,10 +6235,11 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev) */ pcie_capability_read_dword(dev, PCI_EXP_LNKCAP2, &lnkcap2); speeds = lnkcap2 & PCI_EXP_LNKCAP2_SLS; - /* Ignore speeds higher than Max Link Speed */ - speeds &= GENMASK(lnkcap & PCI_EXP_LNKCAP_SLS, - PCI_EXP_LNKCAP2_SLS_2_5GB); + if (max_bits) + speeds &= GENMASK(max_bits, PCI_EXP_LNKCAP2_SLS_2_5GB); + else + pci_info(dev, "Undefined Max Link Speed; relying on LnkCap2\n"); /* PCIe r3.0-compliant */ if (speeds)
On Sun, Dec 15, 2024 at 10:17:46PM +0100, Niklas Schnelle wrote: > On Sun, 2024-12-15 at 11:20 +0100, Lukas Wunner wrote: > > Broken PCIe devices may not set any of the bits in the Link Capabilities > > Register's "Max Link Speed" field. Assume 2.5 GT/s in such a case, > > which is the lowest possible PCIe speed. It must be supported by every > > device per PCIe r6.2 sec 8.2.1. [...] > > --- a/drivers/pci/pci.c > > +++ b/drivers/pci/pci.c > > @@ -6233,6 +6233,13 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev) > > u32 lnkcap2, lnkcap; > > u8 speeds; > > > > + /* A device must support 2.5 GT/s (PCIe r6.2 sec 8.2.1) */ > > + pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap); > > + if (!(lnkcap & PCI_EXP_LNKCAP_SLS)) { > > + pci_info(dev, "Undefined Max Link Speed; assume 2.5 GT/s\n"); > > + return PCI_EXP_LNKCAP2_SLS_2_5GB; > > + } > > + > > /* > > * Speeds retain the reserved 0 at LSB before PCIe Supported Link > > * Speeds Vector to allow using SLS Vector bit defines directly. > > I feel like this patch goes a bit against the idea of this being more > future proof. Personally, I kind of expect that any future devices > which may skip support for lower speeds would start with skipping 2.5 > GT/s and a future PCIe spec might allow this. > > In that case with the above code we end up assuming 2.5 GT/s which > won't work while the Supported Link Speeds Vector could contain > supported speeds with the assumption that when in doubt software relies > on that (PCIe r6.2 sec 7.5.3.18) and it might even be future spec > conformant. > > So I think instead of assuming 2.5 GT/s I was thinking of something > like the diff below (on top of this series). [...] > @@ -6238,10 +6235,11 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev) > */ > pcie_capability_read_dword(dev, PCI_EXP_LNKCAP2, &lnkcap2); > speeds = lnkcap2 & PCI_EXP_LNKCAP2_SLS; > - > /* Ignore speeds higher than Max Link Speed */ > - speeds &= GENMASK(lnkcap & PCI_EXP_LNKCAP_SLS, > - PCI_EXP_LNKCAP2_SLS_2_5GB); > + if (max_bits) > + speeds &= GENMASK(max_bits, PCI_EXP_LNKCAP2_SLS_2_5GB); > + else > + pci_info(dev, "Undefined Max Link Speed; relying on LnkCap2\n"); I see. Right now assuming 2.5 GT/s is the most conservative approach. We may have to revisit this once the PCIe spec does allow gaps in the Supported Link Speeds. Then again, I'm not aware of any broken devices that actually *have* an undefined Max Link Speed, so this patch is a safety measure to avoid the GENMASK() inversion in patch [2/3]. Thanks, Lukas
On Sun, 15 Dec 2024 11:20:51 +0100 Lukas Wunner <lukas@wunner.de> wrote: > Broken PCIe devices may not set any of the bits in the Link Capabilities > Register's "Max Link Speed" field. Assume 2.5 GT/s in such a case, > which is the lowest possible PCIe speed. It must be supported by every > device per PCIe r6.2 sec 8.2.1. > > Emit a message informing about the malformed field. Use KERN_INFO > severity to minimize annoyance. This will help silicon validation > engineers take note of the issue so that regular users hopefully never > see it. > > There is currently no known affected product, but a subsequent commit > will honor the Max Link Speed field when determining supported speeds > and depends on the field being well-formed. (It uses the Max Link Speed > as highest bit in a GENMASK(highest, lowest) macro and if the field is > zero, that would result in GENMASK(0, lowest).) > > Signed-off-by: Lukas Wunner <lukas@wunner.de> Seems like this is the best we can do for this (hopefully) theoretical hardware bug. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/pci/pci.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 35dc9f249b86..ab0ef7b6c798 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -6233,6 +6233,13 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev) > u32 lnkcap2, lnkcap; > u8 speeds; > > + /* A device must support 2.5 GT/s (PCIe r6.2 sec 8.2.1) */ > + pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap); > + if (!(lnkcap & PCI_EXP_LNKCAP_SLS)) { > + pci_info(dev, "Undefined Max Link Speed; assume 2.5 GT/s\n"); > + return PCI_EXP_LNKCAP2_SLS_2_5GB; > + } > + > /* > * Speeds retain the reserved 0 at LSB before PCIe Supported Link > * Speeds Vector to allow using SLS Vector bit defines directly. > @@ -6244,8 +6251,6 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev) > if (speeds) > return speeds; > > - pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap); > - > /* Synthesize from the Max Link Speed field */ > if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_5_0GB) > speeds = PCI_EXP_LNKCAP2_SLS_5_0GB | PCI_EXP_LNKCAP2_SLS_2_5GB;
On Sun, 15 Dec 2024, Lukas Wunner wrote: > Broken PCIe devices may not set any of the bits in the Link Capabilities > Register's "Max Link Speed" field. Assume 2.5 GT/s in such a case, > which is the lowest possible PCIe speed. It must be supported by every > device per PCIe r6.2 sec 8.2.1. > > Emit a message informing about the malformed field. Use KERN_INFO > severity to minimize annoyance. This will help silicon validation > engineers take note of the issue so that regular users hopefully never > see it. > > There is currently no known affected product, but a subsequent commit > will honor the Max Link Speed field when determining supported speeds > and depends on the field being well-formed. (It uses the Max Link Speed > as highest bit in a GENMASK(highest, lowest) macro and if the field is > zero, that would result in GENMASK(0, lowest).) > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > --- > drivers/pci/pci.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 35dc9f249b86..ab0ef7b6c798 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -6233,6 +6233,13 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev) > u32 lnkcap2, lnkcap; > u8 speeds; > > + /* A device must support 2.5 GT/s (PCIe r6.2 sec 8.2.1) */ > + pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap); > + if (!(lnkcap & PCI_EXP_LNKCAP_SLS)) { > + pci_info(dev, "Undefined Max Link Speed; assume 2.5 GT/s\n"); > + return PCI_EXP_LNKCAP2_SLS_2_5GB; > + } After some more thinking, I realized this is probably not a good idea (I know you got it from me :-(). IIRC, this function is also called for RCiEPs and they do not have to implement these registers. I saw supported_speeds were 0 for many devices, for most to be more precise, while developing bwctrl (none of those impacted bwctrl). If I'm correct, that print out should trigger many times on a simple boot test so it should be easy to confirm.
On 12/15/2024 04:20, Lukas Wunner wrote: > Broken PCIe devices may not set any of the bits in the Link Capabilities > Register's "Max Link Speed" field. Assume 2.5 GT/s in such a case, > which is the lowest possible PCIe speed. It must be supported by every > device per PCIe r6.2 sec 8.2.1. > > Emit a message informing about the malformed field. Use KERN_INFO > severity to minimize annoyance. This will help silicon validation > engineers take note of the issue so that regular users hopefully never > see it. > > There is currently no known affected product, but a subsequent commit > will honor the Max Link Speed field when determining supported speeds > and depends on the field being well-formed. (It uses the Max Link Speed > as highest bit in a GENMASK(highest, lowest) macro and if the field is > zero, that would result in GENMASK(0, lowest).) > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > --- > drivers/pci/pci.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 35dc9f249b86..ab0ef7b6c798 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -6233,6 +6233,13 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev) > u32 lnkcap2, lnkcap; > u8 speeds; > > + /* A device must support 2.5 GT/s (PCIe r6.2 sec 8.2.1) */ > + pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap); > + if (!(lnkcap & PCI_EXP_LNKCAP_SLS)) { > + pci_info(dev, "Undefined Max Link Speed; assume 2.5 GT/s\n"); As it's just theoretical, shouldn't it be noisier? I'm thinking at least pci_warn(). Otherwise if this goes in and stays at pci_info() it's going to be a lot easier to miss. Whereas at least messages that are warn or err get a more thorough look at during hardware bring up. > + return PCI_EXP_LNKCAP2_SLS_2_5GB; > + } > + > /* > * Speeds retain the reserved 0 at LSB before PCIe Supported Link > * Speeds Vector to allow using SLS Vector bit defines directly. > @@ -6244,8 +6251,6 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev) > if (speeds) > return speeds; > > - pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap); > - > /* Synthesize from the Max Link Speed field */ > if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_5_0GB) > speeds = PCI_EXP_LNKCAP2_SLS_5_0GB | PCI_EXP_LNKCAP2_SLS_2_5GB;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 35dc9f249b86..ab0ef7b6c798 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -6233,6 +6233,13 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev) u32 lnkcap2, lnkcap; u8 speeds; + /* A device must support 2.5 GT/s (PCIe r6.2 sec 8.2.1) */ + pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap); + if (!(lnkcap & PCI_EXP_LNKCAP_SLS)) { + pci_info(dev, "Undefined Max Link Speed; assume 2.5 GT/s\n"); + return PCI_EXP_LNKCAP2_SLS_2_5GB; + } + /* * Speeds retain the reserved 0 at LSB before PCIe Supported Link * Speeds Vector to allow using SLS Vector bit defines directly. @@ -6244,8 +6251,6 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev) if (speeds) return speeds; - pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap); - /* Synthesize from the Max Link Speed field */ if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_5_0GB) speeds = PCI_EXP_LNKCAP2_SLS_5_0GB | PCI_EXP_LNKCAP2_SLS_2_5GB;
Broken PCIe devices may not set any of the bits in the Link Capabilities Register's "Max Link Speed" field. Assume 2.5 GT/s in such a case, which is the lowest possible PCIe speed. It must be supported by every device per PCIe r6.2 sec 8.2.1. Emit a message informing about the malformed field. Use KERN_INFO severity to minimize annoyance. This will help silicon validation engineers take note of the issue so that regular users hopefully never see it. There is currently no known affected product, but a subsequent commit will honor the Max Link Speed field when determining supported speeds and depends on the field being well-formed. (It uses the Max Link Speed as highest bit in a GENMASK(highest, lowest) macro and if the field is zero, that would result in GENMASK(0, lowest).) Signed-off-by: Lukas Wunner <lukas@wunner.de> --- drivers/pci/pci.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)