[PATCH] firmware: tegra: bpmp: Revert "firmware: tegra: bpmp: Use scoped device node handling to simplify error paths"

Krzysztof Kozlowski posted 1 patch 1 month, 3 weeks ago
drivers/firmware/tegra/bpmp.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
[PATCH] firmware: tegra: bpmp: Revert "firmware: tegra: bpmp: Use scoped device node handling to simplify error paths"
Posted by Krzysztof Kozlowski 1 month, 3 weeks ago
This reverts commit 8812b8689ee6 ("firmware: tegra: bpmp: Use scoped
device node handling to simplify error paths") because it was silently
modified by committer during commit process, by moving declaration of
'struct device_node *np' above the initializer/constructor.  Such code
was not intention of the author, is not conforming to cleanup.h code
style and decreases the code readability.

I did not write such code and I did not agree to put my name with such
commit.

Original patch:
https://lore.kernel.org/all/20240816135722.105945-2-krzysztof.kozlowski@linaro.org/

Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

It's very strange to see own patches silently modified without any
explanation in Signed-off-by area.
---
 drivers/firmware/tegra/bpmp.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c
index 2bee6e918f81..c3a1dc344961 100644
--- a/drivers/firmware/tegra/bpmp.c
+++ b/drivers/firmware/tegra/bpmp.c
@@ -3,7 +3,6 @@
  * Copyright (c) 2016, NVIDIA CORPORATION.  All rights reserved.
  */
 
-#include <linux/cleanup.h>
 #include <linux/clk/tegra.h>
 #include <linux/genalloc.h>
 #include <linux/mailbox_client.h>
@@ -35,24 +34,29 @@ channel_to_ops(struct tegra_bpmp_channel *channel)
 
 struct tegra_bpmp *tegra_bpmp_get(struct device *dev)
 {
-	struct device_node *np __free(device_node);
 	struct platform_device *pdev;
 	struct tegra_bpmp *bpmp;
+	struct device_node *np;
 
 	np = of_parse_phandle(dev->of_node, "nvidia,bpmp", 0);
 	if (!np)
 		return ERR_PTR(-ENOENT);
 
 	pdev = of_find_device_by_node(np);
-	if (!pdev)
-		return ERR_PTR(-ENODEV);
+	if (!pdev) {
+		bpmp = ERR_PTR(-ENODEV);
+		goto put;
+	}
 
 	bpmp = platform_get_drvdata(pdev);
 	if (!bpmp) {
+		bpmp = ERR_PTR(-EPROBE_DEFER);
 		put_device(&pdev->dev);
-		return ERR_PTR(-EPROBE_DEFER);
+		goto put;
 	}
 
+put:
+	of_node_put(np);
 	return bpmp;
 }
 EXPORT_SYMBOL_GPL(tegra_bpmp_get);
-- 
2.43.0
Re: [PATCH] firmware: tegra: bpmp: Revert "firmware: tegra: bpmp: Use scoped device node handling to simplify error paths"
Posted by Thierry Reding 1 month ago
On Tue, Oct 01, 2024 at 10:40:25PM +0200, Krzysztof Kozlowski wrote:
> This reverts commit 8812b8689ee6 ("firmware: tegra: bpmp: Use scoped
> device node handling to simplify error paths") because it was silently
> modified by committer during commit process, by moving declaration of
> 'struct device_node *np' above the initializer/constructor.  Such code
> was not intention of the author, is not conforming to cleanup.h code
> style and decreases the code readability.
> 
> I did not write such code and I did not agree to put my name with such
> commit.
> 
> Original patch:
> https://lore.kernel.org/all/20240816135722.105945-2-krzysztof.kozlowski@linaro.org/
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> ---
> 
> It's very strange to see own patches silently modified without any
> explanation in Signed-off-by area.
> ---
>  drivers/firmware/tegra/bpmp.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)

Sorry this got burried. I will sometimes do cosmetic cleanups like this
instead of having submitters go through an extra round of review. I
suppose I could've mentioned it specifically, but I thought this was
minor enough that it didn't matter.

The reason why I changed it was because the original didn't conform to
any discernible kind of coding style and I thought it was difficult to
read. I also did go through the cleanup.h documentation to check that
doing it this way was fine. I am aware of the LIFO behavior of these
helpers, but since there's exactly one occurrence of these it's
perfectly fine to do so in this case.

Anyway, I'll revert this as you requested.

Thierry