| Message ID | 20241105161820.32512-3-jiashengjiangcool@gmail.com |
|---|---|
| State | Superseded |
| 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
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
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
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(-)