Ensure that the return value from dev_err_probe() is consistently assigned
back to return in all error paths within rockchip_pcie_init_port()
function. This ensures the original error code are propagation for
debugging.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 drivers/pci/controller/pcie-rockchip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-rockchip.c b/drivers/pci/controller/pcie-rockchip.c
index 0f88da3788054..124ab7b9f3404 100644
--- a/drivers/pci/controller/pcie-rockchip.c
+++ b/drivers/pci/controller/pcie-rockchip.c
@@ -134,7 +134,7 @@ int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
 	err = reset_control_bulk_assert(ROCKCHIP_NUM_CORE_RSTS,
 					rockchip->core_rsts);
 	if (err) {
-		dev_err_probe(dev, err, "Couldn't assert Core resets\n");
+		err = dev_err_probe(dev, err, "Couldn't assert Core resets\n");
 		goto err_exit_phy;
 	}
 

base-commit: f406055cb18c6e299c4a783fc1effeb16be41803
-- 
2.50.1

On Sat, Oct 18, 2025 at 11:41:26AM +0530, Anand Moon wrote:
> Ensure that the return value from dev_err_probe() is consistently assigned
> back to return in all error paths within rockchip_pcie_init_port()
> function. This ensures the original error code are propagation for
> debugging.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  drivers/pci/controller/pcie-rockchip.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip.c b/drivers/pci/controller/pcie-rockchip.c
> index 0f88da3788054..124ab7b9f3404 100644
> --- a/drivers/pci/controller/pcie-rockchip.c
> +++ b/drivers/pci/controller/pcie-rockchip.c
> @@ -134,7 +134,7 @@ int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
>  	err = reset_control_bulk_assert(ROCKCHIP_NUM_CORE_RSTS,
>  					rockchip->core_rsts);
>  	if (err) {
> -		dev_err_probe(dev, err, "Couldn't assert Core resets\n");
> +		err = dev_err_probe(dev, err, "Couldn't assert Core resets\n");

This change is pointless. Is the 'err' value going to change with the
reassignment? NO. Then what this change is about?

Please do not send cleanup patches that does nothing useful.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

> Ensure that the return value from dev_err_probe() is consistently assigned
> back to return in all error paths within rockchip_pcie_init_port()
> function. This ensures the original error code are propagation for
> debugging.

I find the change description improvable.


Would an other source code variant become more desirable?
https://elixir.bootlin.com/linux/v6.17.1/source/drivers/base/core.c#L5031-L5075

	err = dev_err_probe(dev,
			    reset_control_bulk_assert(ROCKCHIP_NUM_CORE_RSTS, rockchip->core_rsts),
			    "Couldn't assert Core resets\n");
	if (err)
		goto err_exit_phy;


Regards,
Markus

Hi Markus,

On Sat, 18 Oct 2025 at 13:09, Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > Ensure that the return value from dev_err_probe() is consistently assigned
> > back to return in all error paths within rockchip_pcie_init_port()
> > function. This ensures the original error code are propagation for
> > debugging.
>
> I find the change description improvable.
>
Ok, is this ok?

When using the dev_err_probe() helper function in rockchip_pcie_init_port(),
ensure its return value is consistently assigned to the return variable. This
guarantees that the original error code, whether it's a specific error
or -EPROBE_DEFER,
is correctly propagated up the call stack for proper error handling
and debugging.

>
> Would an other source code variant become more desirable?
> https://elixir.bootlin.com/linux/v6.17.1/source/drivers/base/core.c#L5031-L5075
>
>         err = dev_err_probe(dev,
>                             reset_control_bulk_assert(ROCKCHIP_NUM_CORE_RSTS, rockchip->core_rsts),
>                             "Couldn't assert Core resets\n");
>         if (err)
>                 goto err_exit_phy;
No, the correct code ensures that dev_err_probe() is only called when
an actual error has
occurred, providing a clear and accurate log entry. For deferred probe
(-EPROBE_DEFER),
it will correctly log at a debug level, as intended for that scenario.
For other errors, it will provide
a standard error message.
>
>
> Regards,
> Markus

Thanks
-Anand

>> I find the change description improvable.
>>
> Ok, is this ok?

I present other wording preferences.


> When using the dev_err_probe() helper function in rockchip_pcie_init_port(),
> ensure its return value is consistently assigned to the return variable.

I hope that we can achieve consensus on the corresponding source code places.
https://elixir.bootlin.com/linux/v6.17.1/source/drivers/pci/controller/pcie-rockchip.c#L115-L202


> This guarantees that the original error code, whether it's a specific error
> or -EPROBE_DEFER,
> is correctly propagated up the call stack for proper error handling
> and debugging.

The mentioned programming interface should be applied correctly.


>> Would an other source code variant become more desirable?
>> https://elixir.bootlin.com/linux/v6.17.1/source/drivers/base/core.c#L5031-L5075
>>
>>         err = dev_err_probe(dev,
>>                             reset_control_bulk_assert(ROCKCHIP_NUM_CORE_RSTS, rockchip->core_rsts),
>>                             "Couldn't assert Core resets\n");
>>         if (err)
>>                 goto err_exit_phy;
> No, the correct code ensures that dev_err_probe() is only called when
> an actual error has
> occurred, providing a clear and accurate log entry. …

Do you think that anything different would happen according to my transformation suggestion?

Regards,
Markus

…
> +++ b/drivers/pci/controller/pcie-rockchip.c
> @@ -134,7 +134,7 @@ int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
>  	err = reset_control_bulk_assert(ROCKCHIP_NUM_CORE_RSTS,
>  					rockchip->core_rsts);
>  	if (err) {
> -		dev_err_probe(dev, err, "Couldn't assert Core resets\n");
> +		err = dev_err_probe(dev, err, "Couldn't assert Core resets\n");
>  		goto err_exit_phy;
>  	}

I find such a variable reassignment not helpful here.
Do you really miss an error code propagation?

Regards,
Markus