diff mbox series

[v3,3/3] i2c: rk3x: Add check for clk_enable()

Message ID 20241107211428.32273-3-jiashengjiangcool@gmail.com
State Under Review
Delegated to: Andi Shyti
Headers show
Series [v3,1/3] i2c: lpc2k: Add check for clk_enable() | expand

Commit Message

Jiasheng Jiang Nov. 7, 2024, 9:14 p.m. UTC
Add check for the return value of clk_enable() in order to catch the
potential exception. Moreover, convert the return type of
rk3x_i2c_adapt_div() into int and add the check.

Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com>
---
Changelog:

v2 -> v3:

1. Roll back unsuitable dev_err_probe() to dev_err()

v1 -> v2:

1. Remove the Fixes tag.
2. Use dev_err_probe to simplify error handling.
---
 drivers/i2c/busses/i2c-rk3x.c | 55 ++++++++++++++++++++++++++---------
 1 file changed, 41 insertions(+), 14 deletions(-)

Comments

Andi Shyti Nov. 13, 2024, 10:24 p.m. UTC | #1
Hi Jiasheng,

On Thu, Nov 07, 2024 at 09:14:28PM +0000, Jiasheng Jiang wrote:
> Add check for the return value of clk_enable() in order to catch the
> potential exception. Moreover, convert the return type of

It's more an "unlikely exception" rather than a "potential
exeption".

> rk3x_i2c_adapt_div() into int and add the check.

...

>  static u32 rk3x_i2c_func(struct i2c_adapter *adap)
> @@ -1365,9 +1389,12 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
>  	}
>  
>  	clk_rate = clk_get_rate(i2c->clk);
> -	rk3x_i2c_adapt_div(i2c, clk_rate);
> +	ret = rk3x_i2c_adapt_div(i2c, clk_rate);
>  	clk_disable(i2c->clk);

you can't disable a clock that has failed to enable, right?

BTW, although I like this patch (or at least I don't dislike), I
still want to check whether it's wanted or not.

Andi

>  
> +	if (ret)
> +		goto err_clk_notifier;
> +
>  	ret = i2c_add_adapter(&i2c->adap);
>  	if (ret < 0)
>  		goto err_clk_notifier;
> -- 
> 2.25.1
>
Jiasheng Jiang Nov. 14, 2024, 8:56 p.m. UTC | #2
On Wed, Nov 13, 2024 at 5:24 PM Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi Jiasheng,
>
> On Thu, Nov 07, 2024 at 09:14:28PM +0000, Jiasheng Jiang wrote:
> > Add check for the return value of clk_enable() in order to catch the
> > potential exception. Moreover, convert the return type of
>
> It's more an "unlikely exception" rather than a "potential
> exeption".
>
> > rk3x_i2c_adapt_div() into int and add the check.
>
> ...
>
> >  static u32 rk3x_i2c_func(struct i2c_adapter *adap)
> > @@ -1365,9 +1389,12 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
> >       }
> >
> >       clk_rate = clk_get_rate(i2c->clk);
> > -     rk3x_i2c_adapt_div(i2c, clk_rate);
> > +     ret = rk3x_i2c_adapt_div(i2c, clk_rate);
> >       clk_disable(i2c->clk);
>
> you can't disable a clock that has failed to enable, right?
>
> BTW, although I like this patch (or at least I don't dislike), I
> still want to check whether it's wanted or not.
>
> Andi
>

Thank you for your advice. I have carefully reviewed the patch.
There are two clocks: "i2c->clk" and "i2c->pclk".
The "i2c->clk" is enabled and disabled in rk3x_i2c_probe(),
while the "i2c->pclk" is managed within rk3x_i2c_adapt_div().
Thus, the "i2c->clk" has already been enabled at this point.

-Jiasheng


> >
> > +     if (ret)
> > +             goto err_clk_notifier;
> > +
> >       ret = i2c_add_adapter(&i2c->adap);
> >       if (ret < 0)
> >               goto err_clk_notifier;
> > --
> > 2.25.1
> >
Andi Shyti Nov. 19, 2024, 10:03 p.m. UTC | #3
Hi Jiasheng,

> > >  static u32 rk3x_i2c_func(struct i2c_adapter *adap)
> > > @@ -1365,9 +1389,12 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
> > >       }
> > >
> > >       clk_rate = clk_get_rate(i2c->clk);
> > > -     rk3x_i2c_adapt_div(i2c, clk_rate);
> > > +     ret = rk3x_i2c_adapt_div(i2c, clk_rate);
> > >       clk_disable(i2c->clk);
> >
> > you can't disable a clock that has failed to enable, right?
> >
> > BTW, although I like this patch (or at least I don't dislike), I
> > still want to check whether it's wanted or not.
> >
> > Andi
> >
> 
> Thank you for your advice. I have carefully reviewed the patch.
> There are two clocks: "i2c->clk" and "i2c->pclk".
> The "i2c->clk" is enabled and disabled in rk3x_i2c_probe(),
> while the "i2c->pclk" is managed within rk3x_i2c_adapt_div().
> Thus, the "i2c->clk" has already been enabled at this point.

yes, that's correct, that's a fast review and anyway your patch
doesn't have anything to do with this.

BTW, did you have real failure experience here or is it just
speculation?

Thanks,
Andi
Jiasheng Jiang Nov. 19, 2024, 10:46 p.m. UTC | #4
Hi Andi,

On Tue, Nov 19, 2024 at 5:03 PM Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi Jiasheng,
>
> > > >  static u32 rk3x_i2c_func(struct i2c_adapter *adap)
> > > > @@ -1365,9 +1389,12 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
> > > >       }
> > > >
> > > >       clk_rate = clk_get_rate(i2c->clk);
> > > > -     rk3x_i2c_adapt_div(i2c, clk_rate);
> > > > +     ret = rk3x_i2c_adapt_div(i2c, clk_rate);
> > > >       clk_disable(i2c->clk);
> > >
> > > you can't disable a clock that has failed to enable, right?
> > >
> > > BTW, although I like this patch (or at least I don't dislike), I
> > > still want to check whether it's wanted or not.
> > >
> > > Andi
> > >
> >
> > Thank you for your advice. I have carefully reviewed the patch.
> > There are two clocks: "i2c->clk" and "i2c->pclk".
> > The "i2c->clk" is enabled and disabled in rk3x_i2c_probe(),
> > while the "i2c->pclk" is managed within rk3x_i2c_adapt_div().
> > Thus, the "i2c->clk" has already been enabled at this point.
>
> yes, that's correct, that's a fast review and anyway your patch
> doesn't have anything to do with this.
>
> BTW, did you have real failure experience here or is it just
> speculation?

Currently, I have no idea how to trigger this failure.
In fact, I only identified it through static analysis.
However, I believe adding a check here will enhance the
robustness of the code.

-Jiasheng
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 4ef9bad77b85..0263ee0e0470 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -871,7 +871,7 @@  static int rk3x_i2c_v1_calc_timings(unsigned long clk_rate,
 	return ret;
 }
 
-static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate)
+static int rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate)
 {
 	struct i2c_timings *t = &i2c->t;
 	struct rk3x_i2c_calced_timings calc;
@@ -883,7 +883,11 @@  static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate)
 	ret = i2c->soc_data->calc_timings(clk_rate, t, &calc);
 	WARN_ONCE(ret != 0, "Could not reach SCL freq %u", t->bus_freq_hz);
 
-	clk_enable(i2c->pclk);
+	ret = clk_enable(i2c->pclk);
+	if (ret) {
+		dev_err(i2c->dev, "Can't enable bus clk for rk3399: %d\n", ret);
+		return ret;
+	}
 
 	spin_lock_irqsave(&i2c->lock, flags);
 	val = i2c_readl(i2c, REG_CON);
@@ -904,6 +908,8 @@  static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate)
 		clk_rate / 1000,
 		1000000000 / t->bus_freq_hz,
 		t_low_ns, t_high_ns);
+
+	return 0;
 }
 
 /**
@@ -942,19 +948,27 @@  static int rk3x_i2c_clk_notifier_cb(struct notifier_block *nb, unsigned long
 			return NOTIFY_STOP;
 
 		/* scale up */
-		if (ndata->new_rate > ndata->old_rate)
-			rk3x_i2c_adapt_div(i2c, ndata->new_rate);
+		if (ndata->new_rate > ndata->old_rate) {
+			if (rk3x_i2c_adapt_div(i2c, ndata->new_rate))
+				return NOTIFY_STOP;
+		}
 
 		return NOTIFY_OK;
 	case POST_RATE_CHANGE:
 		/* scale down */
-		if (ndata->new_rate < ndata->old_rate)
-			rk3x_i2c_adapt_div(i2c, ndata->new_rate);
+		if (ndata->new_rate < ndata->old_rate) {
+			if (rk3x_i2c_adapt_div(i2c, ndata->new_rate))
+				return NOTIFY_STOP;
+		}
+
 		return NOTIFY_OK;
 	case ABORT_RATE_CHANGE:
 		/* scale up */
-		if (ndata->new_rate > ndata->old_rate)
-			rk3x_i2c_adapt_div(i2c, ndata->old_rate);
+		if (ndata->new_rate > ndata->old_rate) {
+			if (rk3x_i2c_adapt_div(i2c, ndata->old_rate))
+				return NOTIFY_STOP;
+		}
+
 		return NOTIFY_OK;
 	default:
 		return NOTIFY_DONE;
@@ -1068,8 +1082,20 @@  static int rk3x_i2c_xfer_common(struct i2c_adapter *adap,
 
 	spin_lock_irqsave(&i2c->lock, flags);
 
-	clk_enable(i2c->clk);
-	clk_enable(i2c->pclk);
+	ret = clk_enable(i2c->clk);
+	if (ret) {
+		spin_unlock_irqrestore(&i2c->lock, flags);
+		dev_err(i2c->dev, "Can't enable bus clk: %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_enable(i2c->pclk);
+	if (ret) {
+		clk_disable(i2c->clk);
+		spin_unlock_irqrestore(&i2c->lock, flags);
+		dev_err(i2c->dev, "Can't enable bus clk for rk3399: %d\n", ret);
+		return ret;
+	}
 
 	i2c->is_last_msg = false;
 
@@ -1149,9 +1175,7 @@  static __maybe_unused int rk3x_i2c_resume(struct device *dev)
 {
 	struct rk3x_i2c *i2c = dev_get_drvdata(dev);
 
-	rk3x_i2c_adapt_div(i2c, clk_get_rate(i2c->clk));
-
-	return 0;
+	return rk3x_i2c_adapt_div(i2c, clk_get_rate(i2c->clk));
 }
 
 static u32 rk3x_i2c_func(struct i2c_adapter *adap)
@@ -1365,9 +1389,12 @@  static int rk3x_i2c_probe(struct platform_device *pdev)
 	}
 
 	clk_rate = clk_get_rate(i2c->clk);
-	rk3x_i2c_adapt_div(i2c, clk_rate);
+	ret = rk3x_i2c_adapt_div(i2c, clk_rate);
 	clk_disable(i2c->clk);
 
+	if (ret)
+		goto err_clk_notifier;
+
 	ret = i2c_add_adapter(&i2c->adap);
 	if (ret < 0)
 		goto err_clk_notifier;