PCI: rockchip: 5.0 GT/s speed may be dangerous
v3 cover PCI: rockchip: 5.0 GT/s speed may be dangerous
Dragan Simic already had warned me of potential issues with 5.0 GT/s speed operation in Rockchip PCIe. However, in recent interactions with Shawn Lin from Rockchip it came to my attention there's grave danger in the unknown errata regarding 5.0 GT/s operational speed of their PCIe core. Even if the odds are low, to contain any damage, let's prevent 5.0 GT/s operation as well as add a comment to Root Complex driver core, documenting this danger of data loss or worse.
---
Changes in v3:
- Clarify warning message even though Rockchip won't disclose details
- Drop DT changes as they were applied as subset by Heiko
- Link to v2: https://lore.kernel.org/all/[email protected]/
Changes in v2:
- hard limit to 2.5 GT/s, not just warn
- add Reported-by: and Reviewed-by: Dragan Simic
- remove redundant declaration of max-link-speed from helios64 dts
- fix Link: of helios64 patch
- simplify RC mode comment
- Link to v1: https://lore.kernel.org/all/aRhR79u5BPtRRFw3@geday/
---
Geraldo Nascimento (2):
PCI: rockchip: limit RK3399 to 2.5 GT/s to avoid data loss
PCI: rockchip-host: comment danger of 5.0 GT/s speed
drivers/pci/controller/pcie-rockchip-host.c | 4 ++++ drivers/pci/controller/pcie-rockchip.c | 10 ++++++++-- 2 files changed, 12 insertions(+), 2 deletions(-)
Signature · 1 line
2.52.0
PATCH v3 1/2 PCI: rockchip: limit RK3399 to 2.5 GT/s to avoid data loss
Shawn Lin from Rockchip has reiterated that there may be danger of "catastrophic failure" in using their PCIe with 5.0 GT/s speeds.
This may cause data loss or worse. Warn the user if they make a DT change from the default and drive at 2.5 GT/s only, even if the DT max-link-speed property is invalid or inexistent.
This change is corroborated by RK3399 official datasheet [1], which states maximum link speed for this platform is 2.5 GT/s.
[1] https://opensource.rock-chips.com/images/d/d7/Rockchip_RK3399_Datasheet_V2.1-20200323.pdf
--- drivers/pci/controller/pcie-rockchip.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/pci/controller/pcie-rockchip.c +8 -2
@@ -66,8 +66,14 @@ int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip) } rockchip->link_gen =of_pci_get_max_link_speed (node); -if (rockchip->link_gen <0 || rockchip->link_gen >2 ) - rockchip->link_gen =2 ; +if (rockchip->link_gen <0 || rockchip->link_gen >2 ) { + rockchip->link_gen =1 ; +dev_warn (dev,"invalid max-link-speed, set to 2.5 GT/s\n" ); + } +else if (rockchip->link_gen ==2 ) { + rockchip->link_gen =1 ; +dev_warn (dev,"5.0 GT/s may cause data loss, force 2.5 GT/s\n" ); + }for (i =0 ; i <ROCKCHIP_NUM_PM_RSTS ; i++) rockchip->pm_rsts[i].id = rockchip_pci_pm_rsts[i];
Signature · 1 line
2.52.0
PATCH v3 2/2 PCI: rockchip-host: comment danger of 5.0 GT/s speed
In reply to Geraldo NascimentoAccording to Rockchip sources, there is grave danger in enabling 5.0 GT/s speed for this core. Add a comment documenting this risk of "catastrophic failure" and discouraging end-users from forcing higher speed.
--- drivers/pci/controller/pcie-rockchip-host.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/pci/controller/pcie-rockchip-host.c +4 -0
@@ -332,6 +332,10 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)/* * Enable retrain for gen2. This should be configured only after * gen1 finished. +* +* Dangerous and may lead to "catastrophic failure", with data loss +* or worse! +* */ status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_CR +PCI_EXP_LNKCTL2 ); status &= ~PCI_EXP_LNKCTL2_TLS ;
Signature · 1 line
2.52.0
Re: [PATCH v3 2/2] PCI: rockchip-host: comment danger of 5.0 GT/s speed
Quoted 4 lines · Geraldo Nascimento
According to Rockchip sources, there is grave danger in enabling 5.0 GT/s speed for this core. Add a comment documenting this risk of "catastrophic failure" and discouraging end-users from forcing higher speed.
A comment in the code might be useful but doesn't discourage end users.
Quoted 15 lines
Reported-by Dragan Simic <[email protected]>Reported-by Shawn Lin <[email protected]>Reviewed-by Dragan Simic <[email protected]>Signed-off-by Geraldo Nascimento <[email protected]>--- drivers/pci/controller/pcie-rockchip-host.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/pci/controller/pcie-rockchip-host.c +0
-0
drivers/pci/controller/pcie-rockchip-host.c static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip) · line 338
Quoted 7 lines
drivers/pci/controller/pcie-rockchip-host.c +4
-0@@ -332,6 +332,10 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)/* * Enable retrain for gen2. This should be configured only after * gen1 finished. +* +* Dangerous and may lead to "catastrophic failure", with data loss +* or worse! +*
Wrap comment to fit in 80 columns like the rest of the file.
Remove spurious blank line at end.
This should probably be squashed with the first patch; I don't see the benefit of having them separate.
Why don't we just remove this whole "link_gen == 2" block? If we don't want to use it, there's no point in keeping the code.
Quoted 11 lines
drivers/pci/controller/pcie-rockchip-host.c +0
-0*/ status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_CR +PCI_EXP_LNKCTL2 ); status &= ~PCI_EXP_LNKCTL2_TLS ;Signature · 7 lines
2.52.0 _______________________________________________ Linux-rockchip mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-rockchip
Re: [PATCH v3 2/2] PCI: rockchip-host: comment danger of 5.0 GT/s speed
Quoted 8 lines · Bjorn Helgaas
Quoted 4 lines · Geraldo Nascimento
According to Rockchip sources, there is grave danger in enabling 5.0 GT/s speed for this core. Add a comment documenting this risk of "catastrophic failure" and discouraging end-users from forcing higher speed.
A comment in the code might be useful but doesn't discourage end users.
Hi Bjorn, thanks for the quick review.
Quoted 26 lines
Quoted 16 lines
Reported-by Dragan Simic <[email protected]>Reported-by Shawn Lin <[email protected]>Reviewed-by Dragan Simic <[email protected]>Signed-off-by Geraldo Nascimento <[email protected]>--- drivers/pci/controller/pcie-rockchip-host.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/pci/controller/pcie-rockchip-host.c +0
-0drivers/pci/controller/pcie-rockchip-host.c static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip) · line 338
Quoted 7 lines
drivers/pci/controller/pcie-rockchip-host.c +4
-0@@ -332,6 +332,10 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)/* * Enable retrain for gen2. This should be configured only after * gen1 finished. +* +* Dangerous and may lead to "catastrophic failure", with data loss +* or worse! +* Wrap comment to fit in 80 columns like the rest of the file.
Quoted 7 lines
Remove spurious blank line at end.
This should probably be squashed with the first patch; I don't see the benefit of having them separate.
Why don't we just remove this whole "
link_gen== 2" block? If we don't want to use it, there's no point in keeping the code.
Yes, I think I'll go with this second option and remove the code block outright.
Thanks,
Geraldo Nascimento
Quoted 12 lines
Quoted 12 lines
drivers/pci/controller/pcie-rockchip-host.c +0
-0*/ status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_CR +PCI_EXP_LNKCTL2 ); status &= ~PCI_EXP_LNKCTL2_TLS ;Signature · 7 lines
2.52.0 _______________________________________________ Linux-rockchip mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-rockchip
Re: [PATCH v3 2/2] PCI: rockchip-host: comment danger of 5.0 GT/s speed
Hello Geraldo and Bjorn,
Quoted 39 lines · Geraldo Nascimento
Quoted 8 lines · Bjorn Helgaas
Quoted 4 lines · Geraldo Nascimento
According to Rockchip sources, there is grave danger in enabling 5.0 GT/s speed for this core. Add a comment documenting this risk of "catastrophic failure" and discouraging end-users from forcing higher speed.
A comment in the code might be useful but doesn't discourage end users.
Hi Bjorn, thanks for the quick review.
Quoted 27 lines
Quoted 15 lines
Reported-by Dragan Simic <[email protected]>Reported-by Shawn Lin <[email protected]>Reviewed-by Dragan Simic <[email protected]>Signed-off-by Geraldo Nascimento <[email protected]>--- drivers/pci/controller/pcie-rockchip-host.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/pci/controller/pcie-rockchip-host.c +0
-0drivers/pci/controller/pcie-rockchip-host.c static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip) · line 338
Quoted 7 lines
drivers/pci/controller/pcie-rockchip-host.c +4
-0@@ -332,6 +332,10 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)/* * Enable retrain for gen2. This should be configured only after * gen1 finished. +* +* Dangerous and may lead to "catastrophic failure", with data loss +* or worse! +* Wrap comment to fit in 80 columns like the rest of the file.
Remove spurious blank line at end.
Quoted 8 lines
Quoted 5 lines
This should probably be squashed with the first patch; I don't see the benefit of having them separate.
Why don't we just remove this whole "
link_gen== 2" block? If we don't want to use it, there's no point in keeping the code.Yes, I think I'll go with this second option and remove the code block outright.
I agree about removing the effectively unused code block that handles the "link_gen == 2" case from pcie-rockchip-host.c, but then please remove the PCIE_CLIENT_GEN_SEL_2 define from pcie-rockchip.h and its single "link_gen == 2" use from pcie-rockchip.c as well.
As part of that additional removal, an early check for "link_gen == 1" should be added to function rockchip_pcie_init_port() in pcie-rockchip.c, because that will become the only "link_gen" value it supports.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/pci/controller/pcie-rockchip-host.c +0
-0*/ status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_CR +PCI_EXP_LNKCTL2 ); status &= ~PCI_EXP_LNKCTL2_TLS ;
Re: [PATCH v3 2/2] PCI: rockchip-host: comment danger of 5.0 GT/s speed
Hi Dragan, thank you for the review!
Quoted 4 lines · Dragan Simic
I agree about removing the effectively unused code block that handles the "
link_gen== 2" case frompcie-rockchip-host.c, but then please remove thePCIE_CLIENT_GEN_SEL_2define frompcie-rockchip.hand its single "link_gen== 2" use frompcie-rockchip.cas well.
Good.
There's also some code that needs to be dropped from the endpoint driver.
Quoted 4 lines
As part of that additional removal, an early check for "
link_gen== 1" should be added to functionrockchip_pcie_init_port()inpcie-rockchip.c, because that will become the only "link_gen" value it supports.
What do you think, should the driver bail out completely if link_gen != 1 or just let it force 2.5 GT/s speed with a warning?
I tend to go with the latter.
Thanks,
Geraldo Nascimento
Re: [PATCH v3 2/2] PCI: rockchip-host: comment danger of 5.0 GT/s speed
Quoted 10 lines · Geraldo Nascimento
Quoted 4 lines · Dragan Simic
I agree about removing the effectively unused code block that handles the "
link_gen== 2" case frompcie-rockchip-host.c, but then please remove thePCIE_CLIENT_GEN_SEL_2define frompcie-rockchip.hand its single "link_gen== 2" use frompcie-rockchip.cas well.Good.
There's also some code that needs to be dropped from the endpoint driver.
Oh indeed, there's a redundant code block that handles a "link_gen == 2" case in function rockchip_pcie_ep_link_training(). As a note, a similar early check against "link_gen == 2" should be added there as well.
Quoted 8 lines
Quoted 3 lines
As part of that additional removal, an early check for "
link_gen== 1" should be added to functionrockchip_pcie_init_port()inpcie-rockchip.c, because that will become the only "link_gen" value it supports.What do you think, should the driver bail out completely if
link_gen!= 1 or just let it force 2.5 GT/s speed with a warning?I tend to go with the latter.
Actually, I'd go with the first option, i.e. bailing out early in function rockchip_pcie_host_init_port() with an error message emitted, because that function isn't supposed to define the link parameters. Thus, if it ends up receiving wrong link parameters, the issue lies somewhere else and should be fixed at its origin.