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

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

[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: stable@vger.kernel.org	# v5.15+
Signed-off-by: Diederik de Haas <didi.debian@cknow.org>
---
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index c624b7ebd118..4f92639650e3 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -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 = {
-- 
2.49.0

Re: [PATCH] PCI: dw-rockchip: Fix function call sequence in rockchip_pcie_phy_deinit

[Dragan Simic]

Hello Diederik,

On 2025-04-17 16:21, Diederik de Haas wrote:
> 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: stable@vger.kernel.org	# v5.15+
> Signed-off-by: Diederik de Haas <didi.debian@cknow.org>
> ---
>  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index c624b7ebd118..4f92639650e3 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -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 = {

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 <dsimic@manjaro.org>

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

Re: [PATCH] PCI: dw-rockchip: Fix function call sequence in rockchip_pcie_phy_deinit

[Diederik de Haas]

[-- Attachment #1: Type: text/plain, Size: 2859 bytes --]

Hi Dragan,

On Thu Apr 17, 2025 at 6:20 PM CEST, Dragan Simic wrote:
> On 2025-04-17 16:21, Diederik de Haas wrote:
>> 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: stable@vger.kernel.org	# v5.15+
>> Signed-off-by: Diederik de Haas <didi.debian@cknow.org>
>> ---
>>  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>> b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>> index c624b7ebd118..4f92639650e3 100644
>> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>> @@ -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 = {
>
> 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.

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/1744352048-178994-1-git-send-email-shawn.lin@rock-chips.com/

> Thus, please feel free to include
>
> Reviewed-by: Dragan Simic <dsimic@manjaro.org>

Thanks :-)

Cheers,
  Diederik

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] PCI: dw-rockchip: Fix function call sequence in rockchip_pcie_phy_deinit

[Dragan Simic]

On 2025-04-17 19:09, Diederik de Haas wrote:
> On Thu Apr 17, 2025 at 6:20 PM CEST, Dragan Simic wrote:
>> On 2025-04-17 16:21, Diederik de Haas wrote:
>>> 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: stable@vger.kernel.org	# v5.15+
>>> Signed-off-by: Diederik de Haas <didi.debian@cknow.org>
>>> ---
>>>  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>> b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>> index c624b7ebd118..4f92639650e3 100644
>>> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>> @@ -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 = {
>> 
>> 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.
> 
> 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/1744352048-178994-1-git-send-email-shawn.lin@rock-chips.com/

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.

Re: [PATCH] PCI: dw-rockchip: Fix function call sequence in rockchip_pcie_phy_deinit

[Niklas Cassel]

On Thu, Apr 17, 2025 at 04:21:18PM +0200, Diederik de Haas wrote:
> 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: stable@vger.kernel.org	# v5.15+
> Signed-off-by: Diederik de Haas <didi.debian@cknow.org>
> ---
>  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index c624b7ebd118..4f92639650e3 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -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 = {
> -- 
> 2.49.0
> 

Reviewed-by: Niklas Cassel <cassel@kernel.org>

Re: [PATCH] PCI: dw-rockchip: Fix function call sequence in rockchip_pcie_phy_deinit

[Shawn Lin]

在 2025/04/17 星期四 22:21, 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 <shawn.lin@rock-chips.com>

> 
> Fixes: 0e898eb8df4e ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
> Cc: stable@vger.kernel.org	# v5.15+
> Signed-off-by: Diederik de Haas <didi.debian@cknow.org>
> ---
>   drivers/pci/controller/dwc/pcie-dw-rockchip.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index c624b7ebd118..4f92639650e3 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -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 = {

Re: [PATCH] PCI: dw-rockchip: Fix function call sequence in rockchip_pcie_phy_deinit

[Manivannan Sadhasivam]

On Thu, 17 Apr 2025 16:21:18 +0200, Diederik de Haas wrote:
> 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,
-- 
Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>