[PATCH v1] PCI: rockchip: Propagate dev_err_probe return value

[PATCH v1] PCI: rockchip: Propagate dev_err_probe return value

[Anand Moon]

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

Re: [PATCH] PCI: rockchip: Propagate dev_err_probe return value

[Markus Elfring]

…
> +++ 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

Re: [PATCH] PCI: rockchip: Propagate dev_err_probe return value

[Markus Elfring]

> 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

Re: [PATCH] PCI: rockchip: Propagate dev_err_probe return value

[Anand Moon]

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

Re: [PATCH] PCI: rockchip: Propagate dev_err_probe return value

[Markus Elfring]

>> 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

Re: [PATCH v1] PCI: rockchip: Propagate dev_err_probe return value

[Manivannan Sadhasivam]

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

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