Skip to contentSkip to conversation

PCI: dw-rockchip: Fix function call sequence in rockchip_pcie_phy_deinit

PATCH Applied Diederik de Haas 5 participants 7 messages Apr 17, 2025 – Apr 27, 2025

Subsystems (4)
Stable branch PCI Arm Rockchip PCI native host bridge and endpoint drivers
lore
Share & tools
lore

Diederik de Haas

author
#0 lore raw

PATCH 1/1 PCI: dw-rockchip: Fix function call sequence in rockchip_pcie_phy_deinit

The documentation for the phy_power_off() function explicitly says

Must be called before phy_exit().

So let's follow that instruction.

Fixes 0e898eb8df4e ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
Cc [email protected] # v5.15+
Signed-off-by Diederik de Haas <[email protected]>
---
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/controller/dwc/pcie-dw-rockchip.c +1 -1
index c624b7ebd118..4f92639650e3 100644
@@ -410,8 +410,8 @@ static int rockchip_pcie_phy_init(struct rockchip_pcie *rockchip)
 
 static void rockchip_pcie_phy_deinit(struct rockchip_pcie *rockchip)
 {
-	phy_exit(rockchip->phy);
 	phy_power_off(rockchip->phy);
+	phy_exit(rockchip->phy);
 }
 
 static const struct dw_pcie_ops dw_pcie_ops = {
Signature · 1 line
2.49.0
Reviewed-by on v1 1/1
#1 lore raw

Hello Diederik,

Quoted 29 lines · Diederik de Haas

The documentation for the phy_power_off() function explicitly says

Must be called before phy_exit().

So let's follow that instruction.

Fixes 0e898eb8df4e ("PCI: rockchip-dwc: Add Rockchip RK356X host

controller driver")

Cc [email protected] # v5.15+
Signed-off-by Diederik de Haas <[email protected]>
---
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/controller/dwc/pcie-dw-rockchip.c +1 -1
index c624b7ebd118..4f92639650e3 100644
@@ -410,8 +410,8 @@ static int rockchip_pcie_phy_init(struct
 rockchip_pcie *rockchip)
 
 static void rockchip_pcie_phy_deinit(struct rockchip_pcie *rockchip)
 {
-	phy_exit(rockchip->phy);
 	phy_power_off(rockchip->phy);
+	phy_exit(rockchip->phy);
 }
Quoted 1 line

static const struct dw_pcie_ops dw_pcie_ops = {

Thanks for the patch, it's looking good to me. The current state of the rockchip_pcie_phy_deinit() function might actually not cause issues because the rockchip_pcie_phy_deinit() function is used only in the error-handling path in the rockchip_pcie_probe() function, so having no runtime errors leads to no possible issues.

However, it doesn't mean it shouldn't be fixed, and it would actually be good to dissolve the rockchip_pcie_phy_deinit() function into the above-mentioned error-handling path. It's a short, two-line function local to the compile unit, used in a single place only, so dissolving it is safe and would actually improve the readability of the code.

Thus, please feel free to include

Reviewed-by Dragan Simic <[email protected]>

and please consider dissolving the rockchip_pcie_phy_deinit() function in the possible v2 of this patch, as suggested above.

author
#2 lore raw

Hi Dragan,

Quoted 38 lines · Dragan Simic
Quoted 29 lines · Diederik de Haas

The documentation for the phy_power_off() function explicitly says

Must be called before phy_exit().

So let's follow that instruction.

Fixes 0e898eb8df4e ("PCI: rockchip-dwc: Add Rockchip RK356X host

controller driver")

Cc [email protected] # v5.15+
Signed-off-by Diederik de Haas <[email protected]>
---
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/controller/dwc/pcie-dw-rockchip.c +1 -1
index c624b7ebd118..4f92639650e3 100644
@@ -410,8 +410,8 @@ static int rockchip_pcie_phy_init(struct
 rockchip_pcie *rockchip)
 
 static void rockchip_pcie_phy_deinit(struct rockchip_pcie *rockchip)
 {
-	phy_exit(rockchip->phy);
 	phy_power_off(rockchip->phy);
+	phy_exit(rockchip->phy);
 }
Quoted 1 line

static const struct dw_pcie_ops dw_pcie_ops = {

Thanks for the patch, it's looking good to me. The current state of the rockchip_pcie_phy_deinit() function might actually not cause issues because the rockchip_pcie_phy_deinit() function is used only in the error-handling path in the rockchip_pcie_probe() function, so having no runtime errors leads to no possible issues.

Quoted 5 lines

However, it doesn't mean it shouldn't be fixed, and it would actually be good to dissolve the rockchip_pcie_phy_deinit() function into the above-mentioned error-handling path. It's a short, two-line function local to the compile unit, used in a single place only, so dissolving it is safe and would actually improve the readability of the code.

This patch came about while looking at [1] "PCI: dw-rockchip: Add system PM support", which would be the 2nd consumer of the rockchip_pcie_phy_deinit() function. That patch's commit message has the
following: "tries to reuse possible exist(ing) code"

Being a fan of the DRY principle, that sounds like an excellent idea :-)

So while you're right if there would only be 1 consumer, which is the case *right now*, given that a 2nd consumer is in the works, I think it's better to keep it as I've done it now.
Let me know if you disagree (including why).

[1] https://lore.kernel.org/linux-rockchip/[email protected]/

Quoted 3 lines

Thus, please feel free to include

Reviewed-by Dragan Simic <[email protected]>

Thanks :-)

Cheers,
Diederik

Quoted 2 lines

and please consider dissolving the rockchip_pcie_phy_deinit() function in the possible v2 of this patch, as suggested above.

#3 lore raw
Quoted 50 lines · Diederik de Haas
Quoted 38 lines · Dragan Simic
Quoted 29 lines · Diederik de Haas

The documentation for the phy_power_off() function explicitly says

Must be called before phy_exit().

So let's follow that instruction.

Fixes 0e898eb8df4e ("PCI: rockchip-dwc: Add Rockchip RK356X host

controller driver")

Cc [email protected] # v5.15+
Signed-off-by Diederik de Haas <[email protected]>
---
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/controller/dwc/pcie-dw-rockchip.c +1 -1
index c624b7ebd118..4f92639650e3 100644
@@ -410,8 +410,8 @@ static int rockchip_pcie_phy_init(struct
 rockchip_pcie *rockchip)
 
 static void rockchip_pcie_phy_deinit(struct rockchip_pcie *rockchip)
 {
-	phy_exit(rockchip->phy);
 	phy_power_off(rockchip->phy);
+	phy_exit(rockchip->phy);
 }
Quoted 1 line

static const struct dw_pcie_ops dw_pcie_ops = {

Thanks for the patch, it's looking good to me. The current state of the rockchip_pcie_phy_deinit() function might actually not cause issues because the rockchip_pcie_phy_deinit() function is used only in the error-handling path in the rockchip_pcie_probe() function, so having no runtime errors leads to no possible issues.

Quoted 5 lines

However, it doesn't mean it shouldn't be fixed, and it would actually be good to dissolve the rockchip_pcie_phy_deinit() function into the above-mentioned error-handling path. It's a short, two-line function local to the compile unit, used in a single place only, so dissolving it is safe and would actually improve the readability of the code.

This patch came about while looking at [1] "PCI: dw-rockchip: Add system PM support", which would be the 2nd consumer of the rockchip_pcie_phy_deinit() function. That patch's commit message has the
following: "tries to reuse possible exist(ing) code"

Quoted 8 lines

Being a fan of the DRY principle, that sounds like an excellent idea :-)

So while you're right if there would only be 1 consumer, which is the case *right now*, given that a 2nd consumer is in the works, I think it's better to keep it as I've done it now.
Let me know if you disagree (including why).

[1] https://lore.kernel.org/linux-rockchip/[email protected]/

Ah yes, you're right, thanks for reminding me about that patch. I saw it before, but I totally forgot about it for a moment.

I agree that keeping the rockchip_pcie_phy_deinit() function is the way to go. Yes, it's a short function, but maybe we'll need to do something more in it at some point, which would then be propagated to all of its consumers, instead of having to change all of the "dissolved instances" individually.

Reviewed-by on v1 1/1
#4 lore raw
In reply to Diederik de Haas
Quoted 30 lines · Diederik de Haas

The documentation for the phy_power_off() function explicitly says

Must be called before phy_exit().

So let's follow that instruction.

Fixes 0e898eb8df4e ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
Cc [email protected] # v5.15+
Signed-off-by Diederik de Haas <[email protected]>
---
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/controller/dwc/pcie-dw-rockchip.c +1 -1
index c624b7ebd118..4f92639650e3 100644
@@ -410,8 +410,8 @@ static int rockchip_pcie_phy_init(struct rockchip_pcie *rockchip)
 
 static void rockchip_pcie_phy_deinit(struct rockchip_pcie *rockchip)
 {
-	phy_exit(rockchip->phy);
 	phy_power_off(rockchip->phy);
+	phy_exit(rockchip->phy);
 }
 
 static const struct dw_pcie_ops dw_pcie_ops = {
Signature · 1 line
2.49.0
Reviewed-by Niklas Cassel <[email protected]>
Acked-by on v1 1/1
#5 lore raw
In reply to Diederik de Haas
Quoted 5 lines · Diederik de Haas

The documentation for the phy_power_off() function explicitly says

Must be called before phy_exit().

So let's follow that instruction.

Thanks for this fixing.

Acked-by Shawn Lin <[email protected]>
Quoted 22 lines
Fixes 0e898eb8df4e ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
Cc [email protected] # v5.15+
Signed-off-by Diederik de Haas <[email protected]>
---
  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/controller/dwc/pcie-dw-rockchip.c +1 -1
index c624b7ebd118..4f92639650e3 100644
@@ -410,8 +410,8 @@ static int rockchip_pcie_phy_init(struct rockchip_pcie *rockchip)
 
  static void rockchip_pcie_phy_deinit(struct rockchip_pcie *rockchip)
  {
-	phy_exit(rockchip->phy);
  	phy_power_off(rockchip->phy);
+	phy_exit(rockchip->phy);
  }
 
  static const struct dw_pcie_ops dw_pcie_ops = {
Applied
#6 lore raw
In reply to Diederik de Haas
Quoted 8 lines · Diederik de Haas

The documentation for the phy_power_off() function explicitly says

Must be called before phy_exit().

So let's follow that instruction.

[...]

Applied, thanks!

[1/1] PCI: dw-rockchip: Fix function call sequence in rockchip_pcie_phy_deinit

      commit: 286ed198b899739862456f451eda884558526a9d

Best regards,

Signature · 1 line
Manivannan Sadhasivam <[email protected]>