Ensure that the return value from dev_err_probe() is consistently assigned
back to ret in all error paths within j721e_pcie_probe(). This ensures
the original error code are propagation for debugging.
Cc: Siddharth Vadapalli <s-vadapalli@ti.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v1: new patch in this series
---
drivers/pci/controller/cadence/pci-j721e.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index 5bc5ab20aa6d..9c7bfa77a66e 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -569,20 +569,20 @@ static int j721e_pcie_probe(struct platform_device *pdev)
pm_runtime_enable(dev);
ret = pm_runtime_get_sync(dev);
if (ret < 0) {
- dev_err_probe(dev, ret, "pm_runtime_get_sync failed\n");
+ ret = dev_err_probe(dev, ret, "pm_runtime_get_sync failed\n");
goto err_get_sync;
}
ret = j721e_pcie_ctrl_init(pcie);
if (ret < 0) {
- dev_err_probe(dev, ret, "j721e_pcie_ctrl_init failed\n");
+ ret = dev_err_probe(dev, ret, "j721e_pcie_ctrl_init failed\n");
goto err_get_sync;
}
ret = devm_request_irq(dev, irq, j721e_pcie_link_irq_handler, 0,
"j721e-pcie-link-down-irq", pcie);
if (ret < 0) {
- dev_err_probe(dev, ret, "failed to request link state IRQ %d\n", irq);
+ ret = dev_err_probe(dev, ret, "failed to request link state IRQ %d\n", irq);
goto err_get_sync;
}
@@ -599,7 +599,7 @@ static int j721e_pcie_probe(struct platform_device *pdev)
ret = cdns_pcie_init_phy(dev, cdns_pcie);
if (ret) {
- dev_err_probe(dev, ret, "Failed to init phy\n");
+ ret = dev_err_probe(dev, ret, "Failed to init phy\n");
goto err_get_sync;
}
@@ -611,7 +611,7 @@ static int j721e_pcie_probe(struct platform_device *pdev)
ret = clk_prepare_enable(clk);
if (ret) {
- dev_err_probe(dev, ret, "failed to enable pcie_refclk\n");
+ ret = dev_err_probe(dev, ret, "failed to enable pcie_refclk\n");
goto err_pcie_setup;
}
pcie->refclk = clk;
@@ -638,7 +638,7 @@ static int j721e_pcie_probe(struct platform_device *pdev)
case PCI_MODE_EP:
ret = cdns_pcie_init_phy(dev, cdns_pcie);
if (ret) {
- dev_err_probe(dev, ret, "Failed to init phy\n");
+ ret = dev_err_probe(dev, ret, "Failed to init phy\n");
goto err_get_sync;
}
--
2.50.1On Tue, Oct 14, 2025 at 05:02:27PM +0530, Anand Moon wrote:
> Ensure that the return value from dev_err_probe() is consistently assigned
> back to ret in all error paths within j721e_pcie_probe(). This ensures
> the original error code are propagation for debugging.
>
> Cc: Siddharth Vadapalli <s-vadapalli@ti.com>
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> v1: new patch in this series
> ---
> drivers/pci/controller/cadence/pci-j721e.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> index 5bc5ab20aa6d..9c7bfa77a66e 100644
> --- a/drivers/pci/controller/cadence/pci-j721e.c
> +++ b/drivers/pci/controller/cadence/pci-j721e.c
> @@ -569,20 +569,20 @@ static int j721e_pcie_probe(struct platform_device *pdev)
> pm_runtime_enable(dev);
> ret = pm_runtime_get_sync(dev);
> if (ret < 0) {
> - dev_err_probe(dev, ret, "pm_runtime_get_sync failed\n");
> + ret = dev_err_probe(dev, ret, "pm_runtime_get_sync failed\n");
> goto err_get_sync;
Wait, no, this doesn't make sense. It's just assigning ret to itself.
That's just confusing and pointless.
regards,
dan carpenter
Hi Dan
On Sat, 18 Oct 2025 at 15:18, Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Tue, Oct 14, 2025 at 05:02:27PM +0530, Anand Moon wrote:
> > Ensure that the return value from dev_err_probe() is consistently assigned
> > back to ret in all error paths within j721e_pcie_probe(). This ensures
> > the original error code are propagation for debugging.
> >
> > Cc: Siddharth Vadapalli <s-vadapalli@ti.com>
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> > v1: new patch in this series
> > ---
> > drivers/pci/controller/cadence/pci-j721e.c | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> > index 5bc5ab20aa6d..9c7bfa77a66e 100644
> > --- a/drivers/pci/controller/cadence/pci-j721e.c
> > +++ b/drivers/pci/controller/cadence/pci-j721e.c
> > @@ -569,20 +569,20 @@ static int j721e_pcie_probe(struct platform_device *pdev)
> > pm_runtime_enable(dev);
> > ret = pm_runtime_get_sync(dev);
> > if (ret < 0) {
> > - dev_err_probe(dev, ret, "pm_runtime_get_sync failed\n");
> > + ret = dev_err_probe(dev, ret, "pm_runtime_get_sync failed\n");
> > goto err_get_sync;
>
> Wait, no, this doesn't make sense. It's just assigning ret to itself.
> That's just confusing and pointless.
>
Ok, Thanks for clarifying.
> regards,
> dan carpenter
>
Thanks
-Anand
> Ensure that the return value from dev_err_probe() is consistently assigned > back to ret in all error paths within j721e_pcie_probe(). This ensures > the original error code are propagation for debugging. I find the change description improvable. I propose to take another source code transformation approach better into account. https://elixir.bootlin.com/linux/v6.17.1/source/drivers/base/core.c#L5031-L5075 Example: https://elixir.bootlin.com/linux/v6.17.1/source/drivers/pci/controller/cadence/pci-j721e.c#L444-L636 ret = dev_err_probe(dev, cdns_pcie_init_phy(dev, cdns_pcie), "Failed to init phy\n"); if (ret) goto err_get_sync; How do you think about to achieve such a source code variant also with the help of the semantic patch language (Coccinelle software)? Regards, Markus
Hi Markus, Thanks for the review comments. On Sat, 18 Oct 2025 at 14:26, Markus Elfring <Markus.Elfring@web.de> wrote: > > > Ensure that the return value from dev_err_probe() is consistently assigned > > back to ret in all error paths within j721e_pcie_probe(). This ensures > > the original error code are propagation for debugging. > > I find the change description improvable. > Ok, I will update this once I get some more feedback on the changes. > > I propose to take another source code transformation approach better into account. > https://elixir.bootlin.com/linux/v6.17.1/source/drivers/base/core.c#L5031-L5075 > > Example: > https://elixir.bootlin.com/linux/v6.17.1/source/drivers/pci/controller/cadence/pci-j721e.c#L444-L636 > > ret = dev_err_probe(dev, cdns_pcie_init_phy(dev, cdns_pcie), "Failed to init phy\n"); > if (ret) > goto err_get_sync; > 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. > > How do you think about to achieve such a source code variant also with the help of > the semantic patch language (Coccinelle software)? I do not have any idea about this. > > Regards, > Markus Thanks -Anand
>> How do you think about to achieve such a source code variant also with the help of >> the semantic patch language (Coccinelle software)? > I do not have any idea about this. Your software understanding evolved into another direction. Can the following source code search pattern represent discussed development concerns in a succinct way? @display@ expression dev, e; @@ *e = dev_err_probe(dev, e, ...) The evaluation of such a tiny SmPL script can point out that 14 source files of the software “Linux next-20251017” contain corresponding update candidates. Will any adjustments become helpful also at these places? Regards, Markus
>> How do you think about to achieve such a source code variant also with the help of
>> the semantic patch language (Coccinelle software)?
> I do not have any idea about this.
Can another source code search pattern (like the following) trigger further
development considerations?
@display@
expression dev, e, x;
@@
if (e)
{
... when != e = x
* dev_err_probe(dev, e, ...);
...
}
Regards,
Markus
>> I propose to take another source code transformation approach better into account. >> https://elixir.bootlin.com/linux/v6.17.1/source/drivers/base/core.c#L5031-L5075 >> >> Example: >> https://elixir.bootlin.com/linux/v6.17.1/source/drivers/pci/controller/cadence/pci-j721e.c#L444-L636 >> >> ret = dev_err_probe(dev, cdns_pcie_init_phy(dev, cdns_pcie), "Failed to init phy\n"); >> if (ret) >> goto err_get_sync; >> > 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. … Where do you see undesirable technical differences? Regards, Markus
Hi Markus, Vignesh, On Sat, 18 Oct 2025 at 16:12, Markus Elfring <Markus.Elfring@web.de> wrote: > > >> I propose to take another source code transformation approach better into account. > >> https://elixir.bootlin.com/linux/v6.17.1/source/drivers/base/core.c#L5031-L5075 > >> > >> Example: > >> https://elixir.bootlin.com/linux/v6.17.1/source/drivers/pci/controller/cadence/pci-j721e.c#L444-L636 > >> > >> ret = dev_err_probe(dev, cdns_pcie_init_phy(dev, cdns_pcie), "Failed to init phy\n"); > >> if (ret) > >> goto err_get_sync; > >> > > 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. … > > Where do you see undesirable technical differences? The primary issue I wanted to confirm was the function execution order. since cdns_pcie_init_phy within dev_err_probe function If other developers agree with the approach, I will modify this in a separate patch As Dan Carpenter pointed out - " Wait, no, this doesn't make sense. It's just assigning ret to itself." This patch seems irrelevant to me as the return value gets propagated to the error path. Sorry for the noise. Let's drop these changes. Since I don't have this hardware for testing, I will verify it on another available device. > > Regards, > Markus Thanks -Anand
Le 19/10/2025 à 12:15, Anand Moon a écrit : > Hi Markus, Vignesh, > > On Sat, 18 Oct 2025 at 16:12, Markus Elfring <Markus.Elfring@web.de> wrote: >> >>>> I propose to take another source code transformation approach better into account. >>>> https://elixir.bootlin.com/linux/v6.17.1/source/drivers/base/core.c#L5031-L5075 >>>> >>>> Example: >>>> https://elixir.bootlin.com/linux/v6.17.1/source/drivers/pci/controller/cadence/pci-j721e.c#L444-L636 >>>> >>>> ret = dev_err_probe(dev, cdns_pcie_init_phy(dev, cdns_pcie), "Failed to init phy\n"); >>>> if (ret) >>>> goto err_get_sync; >>>> >>> 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. … >> >> Where do you see undesirable technical differences? > > The primary issue I wanted to confirm was the function execution order. > since cdns_pcie_init_phy within dev_err_probe function > > If other developers agree with the approach, I will modify this in a > separate patch This other approach is just broken. Using: ret = dev_err_probe(dev, cdns_pcie_init_phy(dev, cdns_pcie), "Failed to init phy\n"); 1) is hard to read and understand. 2) would log an error message even if 0 is returned. This is just wrong. 2 good reasons not to do such things. You should ignore people that are already ignored by most people on these lists. CJ > > As Dan Carpenter pointed out - " Wait, no, this doesn't make sense. > It's just assigning ret to itself." Yes, Dan is right. > > This patch seems irrelevant to me as the return value gets propagated > to the error path. > Sorry for the noise. Let's drop these changes. > > Since I don't have this hardware for testing, I will verify it on > another available device. >> >> Regards, >> Markus > > Thanks > -Anand > >
… > Using: > ret = dev_err_probe(dev, cdns_pcie_init_phy(dev, cdns_pcie), "Failed to > init phy\n"); > > 1) is hard to read and understand. … I would like to inform you that it can be determined with the help of another small SmPL script that variations of such assignment expressions are used in 108 source files of the software “Linux next-20251017”. How will development imaginations evolve further then? Regards, Markus
> > As Dan Carpenter pointed out - " Wait, no, this doesn't make sense. > > It's just assigning ret to itself." > > Yes, Dan is right. Would you become more interested in an other source code transformation? https://lore.kernel.org/cocci/fe93e337-7d8e-4b82-b187-b5a67b627544@web.de/ https://sympa.inria.fr/sympa/arc/cocci/2025-10/msg00020.html Regards, Markus
>>>> I propose to take another source code transformation approach better into account. >>>> https://elixir.bootlin.com/linux/v6.17.1/source/drivers/base/core.c#L5031-L5075 >>>> >>>> Example: >>>> https://elixir.bootlin.com/linux/v6.17.1/source/drivers/pci/controller/cadence/pci-j721e.c#L444-L636 >>>> >>>> ret = dev_err_probe(dev, cdns_pcie_init_phy(dev, cdns_pcie), "Failed to init phy\n"); >>>> if (ret) >>>> goto err_get_sync; >>>> >>> 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. … >> >> Where do you see undesirable technical differences? > > The primary issue I wanted to confirm was the function execution order. The desired control flow can be clarified in some ways. > since cdns_pcie_init_phy within dev_err_probe function One function should be executed before its return value will be directly passed to a subsequent function call, shouldn't it? > If other developers agree with the approach, I will modify this in a > separate patch There might be special coding style preferences involved (for a while). Regards, Markus
© 2016 - 2025 Red Hat, Inc.