linux-i2c.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Wang Zhang <silver_code@hust.edu.cn>
To: Peter Korsgaard <peter@korsgaard.com>, Andrew Lunn <andrew@lunn.ch>
Cc: hust-os-kernel-patches@googlegroups.com,
	Wang Zhang <silver_code@hust.edu.cn>,
	linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: [PATCH v2] i2c: ocores: use devm_ managed clks
Date: Mon, 17 Apr 2023 21:27:14 +0800	[thread overview]
Message-ID: <20230417132714.81627-1-silver_code@hust.edu.cn> (raw)
In-Reply-To: <843fab4d-0fdd-4610-91ed-1d8e9accbd25@lunn.ch>

Smatch Warns:
drivers/i2c/busses/i2c-ocores.c:701 ocores_i2c_probe() warn:
missing unwind goto?

If any wrong occurs in ocores_i2c_of_probe, the i2c->clk needs to be
released. But the function returns directly in line 701 without freeing
the clock. Even though we can fix it by freeing the clock manually if
platform_get_irq_optional fails, it may not be following the best practice.
The original code for this driver contains if (IS_ERR()) checks 
throughout, explicitly allowing the driver to continue loading even if 
devm_clk_get() fails.

While it is not entirely clear why the original author implemented this
behavior, there may have been certain circumstances or issues that were not
apparent to us. It's possible that they were trying to work around a bug by
employing an unconventional solution.Using `devm_clk_get_enabled()` rather
than devm_clk_get() can automatically track the usage of clocks and free
them when they are no longer needed or an error occurs.

fixing it by changing `ocores_i2c_of_probe` to use `devm_clk_get_enabled()`
rather than `devm_clk_get()`, changing `goto err_clk' to direct return and
removing `err_clk`.

Signed-off-by: Wang Zhang <silver_code@hust.edu.cn>
---
v1->v2: change `ocores_i2c_of_probe` to use `devm_clk_get_enabled()`
 drivers/i2c/busses/i2c-ocores.c | 62 +++++++++++++--------------------
 1 file changed, 24 insertions(+), 38 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index a0af027db04c..1dcb1af1ad13 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -549,28 +549,24 @@ static int ocores_i2c_of_probe(struct platform_device *pdev,
 							&clock_frequency);
 	i2c->bus_clock_khz = 100;
 
-	i2c->clk = devm_clk_get(&pdev->dev, NULL);
+	i2c->clk = devm_clk_get_enabled(&pdev->dev, NULL);
 
-	if (!IS_ERR(i2c->clk)) {
-		int ret = clk_prepare_enable(i2c->clk);
-
-		if (ret) {
-			dev_err(&pdev->dev,
-				"clk_prepare_enable failed: %d\n", ret);
-			return ret;
-		}
-		i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000;
-		if (clock_frequency_present)
-			i2c->bus_clock_khz = clock_frequency / 1000;
+	if (IS_ERR(i2c->clk)) {
+		dev_err(&pdev->dev,
+			"devm_clk_get_enabled failed\n");
+		return PTR_ERR(i2c->clk);
 	}
 
+	i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000;
+	if (clock_frequency_present)
+		i2c->bus_clock_khz = clock_frequency / 1000;
+
 	if (i2c->ip_clock_khz == 0) {
 		if (of_property_read_u32(np, "opencores,ip-clock-frequency",
 						&val)) {
 			if (!clock_frequency_present) {
 				dev_err(&pdev->dev,
 					"Missing required parameter 'opencores,ip-clock-frequency'\n");
-				clk_disable_unprepare(i2c->clk);
 				return -ENODEV;
 			}
 			i2c->ip_clock_khz = clock_frequency / 1000;
@@ -675,8 +671,7 @@ static int ocores_i2c_probe(struct platform_device *pdev)
 		default:
 			dev_err(&pdev->dev, "Unsupported I/O width (%d)\n",
 				i2c->reg_io_width);
-			ret = -EINVAL;
-			goto err_clk;
+			return -EINVAL;
 		}
 	}
 
@@ -707,13 +702,13 @@ static int ocores_i2c_probe(struct platform_device *pdev)
 						   pdev->name, i2c);
 		if (ret) {
 			dev_err(&pdev->dev, "Cannot claim IRQ\n");
-			goto err_clk;
+			return ret;
 		}
 	}
 
 	ret = ocores_init(&pdev->dev, i2c);
 	if (ret)
-		goto err_clk;
+		return ret;
 
 	/* hook up driver to tree */
 	platform_set_drvdata(pdev, i2c);
@@ -725,7 +720,7 @@ static int ocores_i2c_probe(struct platform_device *pdev)
 	/* add i2c adapter to i2c tree */
 	ret = i2c_add_adapter(&i2c->adap);
 	if (ret)
-		goto err_clk;
+		return ret;
 
 	/* add in known devices to the bus */
 	if (pdata) {
@@ -734,10 +729,6 @@ static int ocores_i2c_probe(struct platform_device *pdev)
 	}
 
 	return 0;
-
-err_clk:
-	clk_disable_unprepare(i2c->clk);
-	return ret;
 }
 
 static int ocores_i2c_remove(struct platform_device *pdev)
@@ -752,9 +743,6 @@ static int ocores_i2c_remove(struct platform_device *pdev)
 	/* remove adapter & data */
 	i2c_del_adapter(&i2c->adap);
 
-	if (!IS_ERR(i2c->clk))
-		clk_disable_unprepare(i2c->clk);
-
 	return 0;
 }
 
@@ -768,8 +756,7 @@ static int ocores_i2c_suspend(struct device *dev)
 	ctrl &= ~(OCI2C_CTRL_EN | OCI2C_CTRL_IEN);
 	oc_setreg(i2c, OCI2C_CONTROL, ctrl);
 
-	if (!IS_ERR(i2c->clk))
-		clk_disable_unprepare(i2c->clk);
+	clk_disable_unprepare(i2c->clk);
 	return 0;
 }
 
@@ -777,19 +764,18 @@ static int ocores_i2c_resume(struct device *dev)
 {
 	struct ocores_i2c *i2c = dev_get_drvdata(dev);
 
-	if (!IS_ERR(i2c->clk)) {
-		unsigned long rate;
-		int ret = clk_prepare_enable(i2c->clk);
+	unsigned long rate;
+	int ret = clk_prepare_enable(i2c->clk);
 
-		if (ret) {
-			dev_err(dev,
-				"clk_prepare_enable failed: %d\n", ret);
-			return ret;
-		}
-		rate = clk_get_rate(i2c->clk) / 1000;
-		if (rate)
-			i2c->ip_clock_khz = rate;
+	if (ret) {
+		dev_err(dev,
+			"clk_prepare_enable failed: %d\n", ret);
+		return ret;
 	}
+	rate = clk_get_rate(i2c->clk) / 1000;
+	if (rate)
+		i2c->ip_clock_khz = rate;
+
 	return ocores_init(dev, i2c);
 }
 
-- 
2.34.1

  reply	other threads:[~2023-04-17 14:13 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-04-16  7:20 [PATCH] i2c: ocores: add missing unwind goto in `ocores_i2c_probe` Wang Zhang
2023-04-16 15:33 ` Andrew Lunn
2023-04-17 13:27   ` Wang Zhang [this message]
2023-04-17 14:05   ` [PATCH v2] i2c: ocores: use devm_ managed clks Wang Zhang
2023-04-17 14:29     ` Peter Rosin
2023-04-17 15:50     ` Andrew Lunn
2023-04-22 12:32       ` [PATCH v3] " Wang Zhang
2023-04-25  9:58         ` 张网
2023-04-25 11:57         ` Andrew Lunn
2023-04-25 14:47           ` 张网
2023-04-25 15:06             ` Andrew Lunn
2023-05-26  7:05               ` [PATCH v4] " Wang Zhang
2023-06-13 23:54                 ` Andi Shyti
2023-06-23  8:22                 ` Wolfram Sang
2023-06-23  8:26                   ` Wolfram Sang
2023-05-23  4:49           ` Re: [PATCH v3] " 张网
2023-05-23 12:10             ` Andrew Lunn
2023-05-23 14:33             ` Wang Zhang
2023-05-24 15:35             ` Wang Zhang
2023-05-24 15:43             ` Wang Zhang
2023-05-24 18:45               ` Dan Carpenter
2023-05-24 19:21               ` Christophe JAILLET
2023-05-25  3:33                 ` 张网
2023-05-25 19:16                   ` Christophe JAILLET
2023-05-25  5:05                 ` Dan Carpenter
2023-05-25  5:02               ` Dan Carpenter
2023-05-25  9:28                 ` Dan Carpenter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230417132714.81627-1-silver_code@hust.edu.cn \
    --to=silver_code@hust.edu.cn \
    --cc=andrew@lunn.ch \
    --cc=hust-os-kernel-patches@googlegroups.com \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peter@korsgaard.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).