[PATCH v2 1/3] i2c: lpc2k: Add check for clk_enable()

[PATCH v2 1/3] i2c: lpc2k: Add check for clk_enable()

[Jiasheng Jiang]

Add check for the return value of clk_enable() in order to catch the
potential exception.

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

v1 -> v2:

1. Remove the Fixes tag.
2. Use dev_err_probe to simplify error handling.
---
 drivers/i2c/busses/i2c-lpc2k.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-lpc2k.c b/drivers/i2c/busses/i2c-lpc2k.c
index 9fb33cbf7419..0fe7d4421a31 100644
--- a/drivers/i2c/busses/i2c-lpc2k.c
+++ b/drivers/i2c/busses/i2c-lpc2k.c
@@ -442,8 +442,12 @@ static int i2c_lpc2k_suspend(struct device *dev)
 static int i2c_lpc2k_resume(struct device *dev)
 {
 	struct lpc2k_i2c *i2c = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_enable(i2c->clk);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to enable clock: %d\n", ret);
 
-	clk_enable(i2c->clk);
 	i2c_lpc2k_reset(i2c);
 
 	return 0;
-- 
2.25.1

[PATCH v2 2/3] i2c: pxa: Add check for clk_enable() and clk_prepare_enable()

[Jiasheng Jiang]

Add check for the return values of clk_enable() and clk_prepare_enable()
in order to catch the potential exceptions.

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

v1 -> v2:

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

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 4d76e71cdd4b..0ee6fe83de40 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1503,7 +1503,9 @@ static int i2c_pxa_probe(struct platform_device *dev)
 				i2c->adap.name);
 	}
 
-	clk_prepare_enable(i2c->clk);
+	ret = clk_prepare_enable(i2c->clk);
+	if (ret)
+		return dev_err_probe(&dev->dev, ret, "failed to enable clock: %d\n", ret);
 
 	if (i2c->use_pio) {
 		i2c->adap.algo = &i2c_pxa_pio_algorithm;
@@ -1560,8 +1562,12 @@ static int i2c_pxa_suspend_noirq(struct device *dev)
 static int i2c_pxa_resume_noirq(struct device *dev)
 {
 	struct pxa_i2c *i2c = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_enable(i2c->clk);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to enable clock: %d\n", ret);
 
-	clk_enable(i2c->clk);
 	i2c_pxa_reset(i2c);
 
 	return 0;
-- 
2.25.1

[PATCH v2 3/3] i2c: rk3x: Add check for clk_enable()

[Jiasheng Jiang]

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:

v1 -> v2:

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

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 4ef9bad77b85..57c2d37fc7c3 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,9 @@ 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)
+		return dev_err_probe(i2c->dev, ret, "Can't enable bus clk for rk3399: %d\n", ret);
 
 	spin_lock_irqsave(&i2c->lock, flags);
 	val = i2c_readl(i2c, REG_CON);
@@ -904,6 +906,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 +946,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 +1080,18 @@ 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);
+		return dev_err_probe(i2c->dev, ret, "Can't enable bus clk: %d\n", ret);
+	}
+
+	ret = clk_enable(i2c->pclk);
+	if (ret) {
+		clk_disable(i2c->clk);
+		spin_unlock_irqrestore(&i2c->lock, flags);
+		return dev_err_probe(i2c->dev, ret, "Can't enable bus clk for rk3399: %d\n", ret);
+	}
 
 	i2c->is_last_msg = false;
 
@@ -1149,9 +1171,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 +1385,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;
-- 
2.25.1

Re: [PATCH v2 3/3] i2c: rk3x: Add check for clk_enable()

[Doug Anderson]

Hi,

On Tue, Nov 5, 2024 at 8:18 AM Jiasheng Jiang
<jiashengjiangcool@gmail.com> wrote:
>
> 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:
>
> v1 -> v2:
>
> 1. Remove the Fixes tag.
> 2. Use dev_err_probe to simplify error handling.
> ---
>  drivers/i2c/busses/i2c-rk3x.c | 51 +++++++++++++++++++++++++----------
>  1 file changed, 37 insertions(+), 14 deletions(-)

Wow, this is a whole lot of code to add to check for an error that
can't really happen as far as I'm aware. Turning on a clock is just
some MMIO writes and can't fail, right? Is this really worth it? Maybe
just wrap clk_enable() and spam an error to the logs if it fails? If
we ever see that error we can figure out what's going on and if
there's a sensible reason it could fail we could add the more complex
code.


> @@ -883,7 +883,9 @@ 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)
> +               return dev_err_probe(i2c->dev, ret, "Can't enable bus clk for rk3399: %d\n", ret);

Officially you're only supposed to use "dev_err_probe()" from probe or
calls indirectly called from probe. You're now using it in a whole lot
of other places.

...also note that dev_err_probe() already prints the error so you
don't need to include it in your error message.


> @@ -942,19 +946,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;

I'm not convinced you can actually return NODIFY_STOP from the
POST_RATE_CHANGE or ABORT_RATE_CHANGE. Have you confirmed that is
actually sensible?


> @@ -1365,9 +1385,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;

This one seems especially comical to add since the only way
rk3x_i2c_adapt_div() could fail would be if a nested clk_enable()
failed. ...and I'm 99% sure that's not possible.

-Doug

Re: [PATCH v2 3/3] i2c: rk3x: Add check for clk_enable()

[Andi Shyti]

Hi Doug,

On Tue, Nov 05, 2024 at 01:29:40PM -0800, Doug Anderson wrote:
> On Tue, Nov 5, 2024 at 8:18 AM Jiasheng Jiang
> > 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:
> >
> > v1 -> v2:
> >
> > 1. Remove the Fixes tag.
> > 2. Use dev_err_probe to simplify error handling.
> > ---
> >  drivers/i2c/busses/i2c-rk3x.c | 51 +++++++++++++++++++++++++----------
> >  1 file changed, 37 insertions(+), 14 deletions(-)
> 
> Wow, this is a whole lot of code to add to check for an error that
> can't really happen as far as I'm aware. Turning on a clock is just
> some MMIO writes and can't fail, right? Is this really worth it? Maybe
> just wrap clk_enable() and spam an error to the logs if it fails? If
> we ever see that error we can figure out what's going on and if
> there's a sensible reason it could fail we could add the more complex
> code.

We've had this discussion several times. I'm of the school that
if a function can return an error, that error should be checked.
It's not spam, we do it everywhere.

On the other hand, there is another school, bigger than mine,
that doesn't want such a check.

I decided not to bother. If someone adds the check, I'm going to
accept it. If someone doesn't add the check, I won't bother
asking for it.

Said that, MMIO reads and writes can fail: in other drivers I'm
seeing bogus reads due to some firmware failures during device
reset.

I agree with you with the rest of the comments and thanks for
checking this out.

Andi

Re: [PATCH v2 3/3] i2c: rk3x: Add check for clk_enable()

[Jiasheng Jiang]

On Wed, Nov 6, 2024 at 6:01 PM Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi Doug,
>
> On Tue, Nov 05, 2024 at 01:29:40PM -0800, Doug Anderson wrote:
> > On Tue, Nov 5, 2024 at 8:18 AM Jiasheng Jiang
> > > 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:
> > >
> > > v1 -> v2:
> > >
> > > 1. Remove the Fixes tag.
> > > 2. Use dev_err_probe to simplify error handling.
> > > ---
> > >  drivers/i2c/busses/i2c-rk3x.c | 51 +++++++++++++++++++++++++----------
> > >  1 file changed, 37 insertions(+), 14 deletions(-)
> >
> > Wow, this is a whole lot of code to add to check for an error that
> > can't really happen as far as I'm aware. Turning on a clock is just
> > some MMIO writes and can't fail, right? Is this really worth it? Maybe
> > just wrap clk_enable() and spam an error to the logs if it fails? If
> > we ever see that error we can figure out what's going on and if
> > there's a sensible reason it could fail we could add the more complex
> > code.
>
> We've had this discussion several times. I'm of the school that
> if a function can return an error, that error should be checked.
> It's not spam, we do it everywhere.
>
> On the other hand, there is another school, bigger than mine,
> that doesn't want such a check.
>
> I decided not to bother. If someone adds the check, I'm going to
> accept it. If someone doesn't add the check, I won't bother
> asking for it.
>
> Said that, MMIO reads and writes can fail: in other drivers I'm
> seeing bogus reads due to some firmware failures during device
> reset.
>
> I agree with you with the rest of the comments and thanks for
> checking this out.
>
> Andi

Thanks, I have submitted a v3 series to fix these problems.

-Jiasheng