| Message ID | 20230524154318.2259-1-silver_code@hust.edu.cn |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] i2c: ocores: use devm_ managed clks | expand |
On Wed, May 24, 2023 at 11:43:18PM +0800, Wang Zhang wrote: > While it is not entirely clear why the original author implemented this > behavior, there may have been certain circumstances or issues that were not > apparent to us. This is an easy to answer question. :P Those are fancy new functions which weren't available at the time. regards, dan carpenter
Le 24/05/2023 à 17:43, Wang Zhang a écrit : > While it is not entirely clear why the original author implemented this > behavior, there may have been certain circumstances or issues that were not > apparent to us. It's possible that they were trying to work around a bug by > employing an unconventional solution.Using `devm_clk_get_enabled()` rather > than devm_clk_get() can automatically track the usage of clocks and free > them when they are no longer needed or an error occurs. > > fixing it by changing `ocores_i2c_of_probe` to use > `devm_clk_get_optional_enabled()` rather than `devm_clk_get()`, changing > `goto err_clk' to direct return and removing `err_clk`. > > Signed-off-by: Wang Zhang <silver_code@hust.edu.cn> > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > --- > v2->v3: use `devm_clk_get_optional_enabled()` to manage clks > v1->v2: change `ocores_i2c_of_probe` to use `devm_clk_get_enabled()` > --- > drivers/i2c/busses/i2c-ocores.c | 57 +++++++++++++-------------------- > 1 file changed, 22 insertions(+), 35 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c > index 2e575856c5cd..316d72cde3b9 100644 > --- a/drivers/i2c/busses/i2c-ocores.c > +++ b/drivers/i2c/busses/i2c-ocores.c > @@ -552,16 +552,14 @@ static int ocores_i2c_of_probe(struct platform_device *pdev, > &clock_frequency); > i2c->bus_clock_khz = 100; > > - i2c->clk = devm_clk_get(&pdev->dev, NULL); > + i2c->clk = devm_clk_get_optional_enabled(&pdev->dev, NULL); > > - if (!IS_ERR(i2c->clk)) { > - int ret = clk_prepare_enable(i2c->clk); > - > - if (ret) { > - dev_err(&pdev->dev, > - "clk_prepare_enable failed: %d\n", ret); > - return ret; > - } > + if (IS_ERR(i2c->clk)) { > + dev_err(&pdev->dev, > + "devm_clk_get_optional_enabled failed\n"); Maybe dev_err_probe() would be nice here. This would log the error code, and filter -EPROBE_DEFER, should it happen. > + return PTR_ERR(i2c->clk); > + } > + if (i2c->clk) { > i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000; > if (clock_frequency_present) > i2c->bus_clock_khz = clock_frequency / 1000; > @@ -573,7 +571,6 @@ static int ocores_i2c_of_probe(struct platform_device *pdev, > if (!clock_frequency_present) { > dev_err(&pdev->dev, > "Missing required parameter 'opencores,ip-clock-frequency'\n"); > - clk_disable_unprepare(i2c->clk); > return -ENODEV; > } > i2c->ip_clock_khz = clock_frequency / 1000; > @@ -678,8 +675,7 @@ static int ocores_i2c_probe(struct platform_device *pdev) > default: > dev_err(&pdev->dev, "Unsupported I/O width (%d)\n", > i2c->reg_io_width); > - ret = -EINVAL; > - goto err_clk; > + return -EINVAL; > } > } > > @@ -710,13 +706,13 @@ static int ocores_i2c_probe(struct platform_device *pdev) > pdev->name, i2c); > if (ret) { > dev_err(&pdev->dev, "Cannot claim IRQ\n"); > - goto err_clk; > + return ret; > } > } > > ret = ocores_init(&pdev->dev, i2c); > if (ret) > - goto err_clk; > + return ret; > > /* hook up driver to tree */ > platform_set_drvdata(pdev, i2c); > @@ -728,7 +724,7 @@ static int ocores_i2c_probe(struct platform_device *pdev) > /* add i2c adapter to i2c tree */ > ret = i2c_add_adapter(&i2c->adap); > if (ret) > - goto err_clk; > + return ret; > > /* add in known devices to the bus */ > if (pdata) { > @@ -737,10 +733,6 @@ static int ocores_i2c_probe(struct platform_device *pdev) > } > > return 0; > - > -err_clk: > - clk_disable_unprepare(i2c->clk); > - return ret; > } > > static int ocores_i2c_remove(struct platform_device *pdev) > @@ -755,9 +747,6 @@ static int ocores_i2c_remove(struct platform_device *pdev) > /* remove adapter & data */ > i2c_del_adapter(&i2c->adap); > > - if (!IS_ERR(i2c->clk)) > - clk_disable_unprepare(i2c->clk); > - > return 0; > } > > @@ -771,8 +760,7 @@ static int ocores_i2c_suspend(struct device *dev) > ctrl &= ~(OCI2C_CTRL_EN | OCI2C_CTRL_IEN); > oc_setreg(i2c, OCI2C_CONTROL, ctrl); > > - if (!IS_ERR(i2c->clk)) > - clk_disable_unprepare(i2c->clk); > + clk_disable_unprepare(i2c->clk); > return 0; > } > > @@ -780,19 +768,18 @@ static int ocores_i2c_resume(struct device *dev) > { > struct ocores_i2c *i2c = dev_get_drvdata(dev); > > - if (!IS_ERR(i2c->clk)) { > - unsigned long rate; > - int ret = clk_prepare_enable(i2c->clk); > + unsigned long rate; > + int ret = clk_prepare_enable(i2c->clk); > > - if (ret) { > - dev_err(dev, > - "clk_prepare_enable failed: %d\n", ret); > - return ret; > - } > - rate = clk_get_rate(i2c->clk) / 1000; > - if (rate) > - i2c->ip_clock_khz = rate; > + if (ret) { > + dev_err(dev, > + "clk_prepare_enable failed: %d\n", ret); > + return ret; > } > + rate = clk_get_rate(i2c->clk) / 1000; Now (because of the devm_clk_get_optional_enabled()), i2c->clk can be NULL, so this would deference a NULL pointer. CJ > + if (rate) > + i2c->ip_clock_khz = rate; > + > return ocores_init(dev, i2c); > } >
Hi Christophe, Thanks for your suggestions. However, both clk_get_rate and clk_prepare_enable will return 0 if i2c->clk is NULL, so I think we may not need to take this issue into account. Regards, Wang Zhang "Christophe JAILLET" <christophe.jaillet@wanadoo.fr>写道： > Le 24/05/2023 à 17:43, Wang Zhang a écrit : > > While it is not entirely clear why the original author implemented this > > behavior, there may have been certain circumstances or issues that were not > > apparent to us. It's possible that they were trying to work around a bug by > > employing an unconventional solution.Using `devm_clk_get_enabled()` rather > > than devm_clk_get() can automatically track the usage of clocks and free > > them when they are no longer needed or an error occurs. > > > > fixing it by changing `ocores_i2c_of_probe` to use > > `devm_clk_get_optional_enabled()` rather than `devm_clk_get()`, changing > > `goto err_clk' to direct return and removing `err_clk`. > > > > Signed-off-by: Wang Zhang <silver_code@hust.edu.cn> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > --- > > v2->v3: use `devm_clk_get_optional_enabled()` to manage clks > > v1->v2: change `ocores_i2c_of_probe` to use `devm_clk_get_enabled()` > > --- > > drivers/i2c/busses/i2c-ocores.c | 57 +++++++++++++-------------------- > > 1 file changed, 22 insertions(+), 35 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c > > index 2e575856c5cd..316d72cde3b9 100644 > > --- a/drivers/i2c/busses/i2c-ocores.c > > +++ b/drivers/i2c/busses/i2c-ocores.c > > @@ -552,16 +552,14 @@ static int ocores_i2c_of_probe(struct platform_device *pdev, > > &clock_frequency); > > i2c->bus_clock_khz = 100; > > > > - i2c->clk = devm_clk_get(&pdev->dev, NULL); > > + i2c->clk = devm_clk_get_optional_enabled(&pdev->dev, NULL); > > > > - if (!IS_ERR(i2c->clk)) { > > - int ret = clk_prepare_enable(i2c->clk); > > - > > - if (ret) { > > - dev_err(&pdev->dev, > > - "clk_prepare_enable failed: %d\n", ret); > > - return ret; > > - } > > + if (IS_ERR(i2c->clk)) { > > + dev_err(&pdev->dev, > > + "devm_clk_get_optional_enabled failed\n"); > > Maybe dev_err_probe() would be nice here. This would log the error code, > and filter -EPROBE_DEFER, should it happen. > > > + return PTR_ERR(i2c->clk); > > + } > > + if (i2c->clk) { > > i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000; > > if (clock_frequency_present) > > i2c->bus_clock_khz = clock_frequency / 1000; > > @@ -573,7 +571,6 @@ static int ocores_i2c_of_probe(struct platform_device *pdev, > > if (!clock_frequency_present) { > > dev_err(&pdev->dev, > > "Missing required parameter 'opencores,ip-clock-frequency'\n"); > > - clk_disable_unprepare(i2c->clk); > > return -ENODEV; > > } > > i2c->ip_clock_khz = clock_frequency / 1000; > > @@ -678,8 +675,7 @@ static int ocores_i2c_probe(struct platform_device *pdev) > > default: > > dev_err(&pdev->dev, "Unsupported I/O width (%d)\n", > > i2c->reg_io_width); > > - ret = -EINVAL; > > - goto err_clk; > > + return -EINVAL; > > } > > } > > > > @@ -710,13 +706,13 @@ static int ocores_i2c_probe(struct platform_device *pdev) > > pdev->name, i2c); > > if (ret) { > > dev_err(&pdev->dev, "Cannot claim IRQ\n"); > > - goto err_clk; > > + return ret; > > } > > } > > > > ret = ocores_init(&pdev->dev, i2c); > > if (ret) > > - goto err_clk; > > + return ret; > > > > /* hook up driver to tree */ > > platform_set_drvdata(pdev, i2c); > > @@ -728,7 +724,7 @@ static int ocores_i2c_probe(struct platform_device *pdev) > > /* add i2c adapter to i2c tree */ > > ret = i2c_add_adapter(&i2c->adap); > > if (ret) > > - goto err_clk; > > + return ret; > > > > /* add in known devices to the bus */ > > if (pdata) { > > @@ -737,10 +733,6 @@ static int ocores_i2c_probe(struct platform_device *pdev) > > } > > > > return 0; > > - > > -err_clk: > > - clk_disable_unprepare(i2c->clk); > > - return ret; > > } > > > > static int ocores_i2c_remove(struct platform_device *pdev) > > @@ -755,9 +747,6 @@ static int ocores_i2c_remove(struct platform_device *pdev) > > /* remove adapter & data */ > > i2c_del_adapter(&i2c->adap); > > > > - if (!IS_ERR(i2c->clk)) > > - clk_disable_unprepare(i2c->clk); > > - > > return 0; > > } > > > > @@ -771,8 +760,7 @@ static int ocores_i2c_suspend(struct device *dev) > > ctrl &= ~(OCI2C_CTRL_EN | OCI2C_CTRL_IEN); > > oc_setreg(i2c, OCI2C_CONTROL, ctrl); > > > > - if (!IS_ERR(i2c->clk)) > > - clk_disable_unprepare(i2c->clk); > > + clk_disable_unprepare(i2c->clk); > > return 0; > > } > > > > @@ -780,19 +768,18 @@ static int ocores_i2c_resume(struct device *dev) > > { > > struct ocores_i2c *i2c = dev_get_drvdata(dev); > > > > - if (!IS_ERR(i2c->clk)) { > > - unsigned long rate; > > - int ret = clk_prepare_enable(i2c->clk); > > + unsigned long rate; > > + int ret = clk_prepare_enable(i2c->clk); > > > > - if (ret) { > > - dev_err(dev, > > - "clk_prepare_enable failed: %d\n", ret); > > - return ret; > > - } > > - rate = clk_get_rate(i2c->clk) / 1000; > > - if (rate) > > - i2c->ip_clock_khz = rate; > > + if (ret) { > > + dev_err(dev, > > + "clk_prepare_enable failed: %d\n", ret); > > + return ret; > > } > > + rate = clk_get_rate(i2c->clk) / 1000; > > Now (because of the devm_clk_get_optional_enabled()), i2c->clk can be > NULL, so this would deference a NULL pointer. > > CJ > > > > + if (rate) > > + i2c->ip_clock_khz = rate; > > + > > return ocores_init(dev, i2c); > > } > >
On Wed, May 24, 2023 at 11:43:18PM +0800, Wang Zhang wrote: > @@ -780,19 +768,18 @@ static int ocores_i2c_resume(struct device *dev) > { > struct ocores_i2c *i2c = dev_get_drvdata(dev); > > - if (!IS_ERR(i2c->clk)) { > - unsigned long rate; > - int ret = clk_prepare_enable(i2c->clk); > + unsigned long rate; > + int ret = clk_prepare_enable(i2c->clk); Don't put functions which can fail in the declaration block. Generally the declaration block is for preliminary stuff, and the important actions should be in the code block. There should not be a blank line before the function call and the error checking. > > - if (ret) { > - dev_err(dev, > - "clk_prepare_enable failed: %d\n", ret); > - return ret; > - } > - rate = clk_get_rate(i2c->clk) / 1000; > - if (rate) > - i2c->ip_clock_khz = rate; > + if (ret) { > + dev_err(dev, > + "clk_prepare_enable failed: %d\n", ret); This can fit on one line now. int ret; ret = clk_prepare_enable(i2c->clk); if (ret) { dev_err(dev, "clk_prepare_enable failed: %d\n", ret); return ret; } regards, dan carpenter > + return ret; > } > + rate = clk_get_rate(i2c->clk) / 1000; > + if (rate) > + i2c->ip_clock_khz = rate; > + > return ocores_init(dev, i2c);
On Wed, May 24, 2023 at 09:21:56PM +0200, Christophe JAILLET wrote: > > + if (ret) { > > + dev_err(dev, > > + "clk_prepare_enable failed: %d\n", ret); > > + return ret; > > } > > + rate = clk_get_rate(i2c->clk) / 1000; > > Now (because of the devm_clk_get_optional_enabled()), i2c->clk can be NULL, > so this would deference a NULL pointer. > No, it's fine. clk_get_rate() checks for NULL. When a function returns a mix of error pointers and NULL, like devm_clk_get_optional_enabled(), then all the functions like this must have NULL checks built it. regards, dan carpenter
On Thu, May 25, 2023 at 08:02:02AM +0300, Dan Carpenter wrote: > On Wed, May 24, 2023 at 11:43:18PM +0800, Wang Zhang wrote: > > @@ -780,19 +768,18 @@ static int ocores_i2c_resume(struct device *dev) > > { > > struct ocores_i2c *i2c = dev_get_drvdata(dev); > > > > - if (!IS_ERR(i2c->clk)) { > > - unsigned long rate; > > - int ret = clk_prepare_enable(i2c->clk); > > + unsigned long rate; > > + int ret = clk_prepare_enable(i2c->clk); > > Don't put functions which can fail in the declaration block. Generally > the declaration block is for preliminary stuff, and the important > actions should be in the code block. There should not be a blank line > before the function call and the error checking. s/before/between/. regards, dan carpenter
Le 25/05/2023 à 05:33, 张网 a écrit : > Hi Christophe, > > Thanks for your suggestions. However, both clk_get_rate and clk_prepare_enable > will return 0 if i2c->clk is NULL, so I think we may not need to take this issue > into account. > > Regards, > Wang Zhang > Ouch! For some reaon, when I read: > + rate = clk_get_rate(i2c->clk) / 1000; I read "if i2c->clk is NULL, then the i2c pointer is NULL as well and is dereferenced". :/ sorry for the noise. CJ (slightly ashamed)
diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c index 2e575856c5cd..316d72cde3b9 100644 --- a/drivers/i2c/busses/i2c-ocores.c +++ b/drivers/i2c/busses/i2c-ocores.c @@ -552,16 +552,14 @@ static int ocores_i2c_of_probe(struct platform_device *pdev, &clock_frequency); i2c->bus_clock_khz = 100; - i2c->clk = devm_clk_get(&pdev->dev, NULL); + i2c->clk = devm_clk_get_optional_enabled(&pdev->dev, NULL); - if (!IS_ERR(i2c->clk)) { - int ret = clk_prepare_enable(i2c->clk); - - if (ret) { - dev_err(&pdev->dev, - "clk_prepare_enable failed: %d\n", ret); - return ret; - } + if (IS_ERR(i2c->clk)) { + dev_err(&pdev->dev, + "devm_clk_get_optional_enabled failed\n"); + return PTR_ERR(i2c->clk); + } + if (i2c->clk) { i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000; if (clock_frequency_present) i2c->bus_clock_khz = clock_frequency / 1000; @@ -573,7 +571,6 @@ static int ocores_i2c_of_probe(struct platform_device *pdev, if (!clock_frequency_present) { dev_err(&pdev->dev, "Missing required parameter 'opencores,ip-clock-frequency'\n"); - clk_disable_unprepare(i2c->clk); return -ENODEV; } i2c->ip_clock_khz = clock_frequency / 1000; @@ -678,8 +675,7 @@ static int ocores_i2c_probe(struct platform_device *pdev) default: dev_err(&pdev->dev, "Unsupported I/O width (%d)\n", i2c->reg_io_width); - ret = -EINVAL; - goto err_clk; + return -EINVAL; } } @@ -710,13 +706,13 @@ static int ocores_i2c_probe(struct platform_device *pdev) pdev->name, i2c); if (ret) { dev_err(&pdev->dev, "Cannot claim IRQ\n"); - goto err_clk; + return ret; } } ret = ocores_init(&pdev->dev, i2c); if (ret) - goto err_clk; + return ret; /* hook up driver to tree */ platform_set_drvdata(pdev, i2c); @@ -728,7 +724,7 @@ static int ocores_i2c_probe(struct platform_device *pdev) /* add i2c adapter to i2c tree */ ret = i2c_add_adapter(&i2c->adap); if (ret) - goto err_clk; + return ret; /* add in known devices to the bus */ if (pdata) { @@ -737,10 +733,6 @@ static int ocores_i2c_probe(struct platform_device *pdev) } return 0; - -err_clk: - clk_disable_unprepare(i2c->clk); - return ret; } static int ocores_i2c_remove(struct platform_device *pdev) @@ -755,9 +747,6 @@ static int ocores_i2c_remove(struct platform_device *pdev) /* remove adapter & data */ i2c_del_adapter(&i2c->adap); - if (!IS_ERR(i2c->clk)) - clk_disable_unprepare(i2c->clk); - return 0; } @@ -771,8 +760,7 @@ static int ocores_i2c_suspend(struct device *dev) ctrl &= ~(OCI2C_CTRL_EN | OCI2C_CTRL_IEN); oc_setreg(i2c, OCI2C_CONTROL, ctrl); - if (!IS_ERR(i2c->clk)) - clk_disable_unprepare(i2c->clk); + clk_disable_unprepare(i2c->clk); return 0; } @@ -780,19 +768,18 @@ static int ocores_i2c_resume(struct device *dev) { struct ocores_i2c *i2c = dev_get_drvdata(dev); - if (!IS_ERR(i2c->clk)) { - unsigned long rate; - int ret = clk_prepare_enable(i2c->clk); + unsigned long rate; + int ret = clk_prepare_enable(i2c->clk); - if (ret) { - dev_err(dev, - "clk_prepare_enable failed: %d\n", ret); - return ret; - } - rate = clk_get_rate(i2c->clk) / 1000; - if (rate) - i2c->ip_clock_khz = rate; + if (ret) { + dev_err(dev, + "clk_prepare_enable failed: %d\n", ret); + return ret; } + rate = clk_get_rate(i2c->clk) / 1000; + if (rate) + i2c->ip_clock_khz = rate; + return ocores_init(dev, i2c); }