| Message ID | 20241105161820.32512-3-jiashengjiangcool@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] i2c: lpc2k: Add check for clk_enable() | expand |
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
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;
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(-)