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