* [PATCH] i2c: ocores: add missing unwind goto in `ocores_i2c_probe` @ 2023-04-16 7:20 Wang Zhang 2023-04-16 15:33 ` Andrew Lunn 0 siblings, 1 reply; 27+ messages in thread From: Wang Zhang @ 2023-04-16 7:20 UTC (permalink / raw) To: Peter Korsgaard, Andrew Lunn Cc: Wang Zhang, Dongliang Mu, linux-i2c, linux-kernel platform_get_irq_optional is a function used to obtain an IRQ number for a device on a platform. The function returns the IRQ number associated with the specified device, or a negative error code if it fails. The error handling code after the err_clk label should be executed to release any resources that were allocated for the clock if a negative error code returned. Fix this by assigning irq to ret and changing the direct return to err_clk. Signed-off-by: Wang Zhang <silver_code@hust.edu.cn> Reviewed-by: Dongliang Mu <dzm91@hust.edu.cn> --- drivers/i2c/busses/i2c-ocores.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c index a0af027db04c..95efad5a5a28 100644 --- a/drivers/i2c/busses/i2c-ocores.c +++ b/drivers/i2c/busses/i2c-ocores.c @@ -697,8 +697,10 @@ static int ocores_i2c_probe(struct platform_device *pdev) if (irq == -ENXIO) { ocores_algorithm.master_xfer = ocores_xfer_polling; } else { - if (irq < 0) - return irq; + if (irq < 0) { + ret = irq; + goto err_clk; + } } if (ocores_algorithm.master_xfer != ocores_xfer_polling) { -- 2.34.1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH] i2c: ocores: add missing unwind goto in `ocores_i2c_probe` 2023-04-16 7:20 [PATCH] i2c: ocores: add missing unwind goto in `ocores_i2c_probe` Wang Zhang @ 2023-04-16 15:33 ` Andrew Lunn 2023-04-17 13:27 ` [PATCH v2] i2c: ocores: use devm_ managed clks Wang Zhang 2023-04-17 14:05 ` Wang Zhang 0 siblings, 2 replies; 27+ messages in thread From: Andrew Lunn @ 2023-04-16 15:33 UTC (permalink / raw) To: Wang Zhang; +Cc: Peter Korsgaard, Dongliang Mu, linux-i2c, linux-kernel On Sun, Apr 16, 2023 at 03:20:40PM +0800, Wang Zhang wrote: > platform_get_irq_optional is a function used to obtain an IRQ > number for a device on a platform. The function returns the IRQ number > associated with the specified device, or a negative error code if it fails. > > The error handling code after the err_clk label should be executed to > release any resources that were allocated for the clock if a negative > error code returned. > > Fix this by assigning irq to ret and changing the direct return to err_clk. The clock is got in ocores_i2c_of_probe(). That function is not always called. So you need to be careful in the error handling that you are not disabling a clock which does not exist.... But i think a better fix is to change ocores_i2c_of_probe() to use devm_clk_get_enabled(), rather than devm_clk_get() so that the driver core will disable to clock if the probe fails, or when the driver is unloaded. Andrew ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2] i2c: ocores: use devm_ managed clks 2023-04-16 15:33 ` Andrew Lunn @ 2023-04-17 13:27 ` Wang Zhang 2023-04-17 14:05 ` Wang Zhang 1 sibling, 0 replies; 27+ messages in thread From: Wang Zhang @ 2023-04-17 13:27 UTC (permalink / raw) To: Peter Korsgaard, Andrew Lunn Cc: hust-os-kernel-patches, Wang Zhang, linux-i2c, linux-kernel Smatch Warns: drivers/i2c/busses/i2c-ocores.c:701 ocores_i2c_probe() warn: missing unwind goto? If any wrong occurs in ocores_i2c_of_probe, the i2c->clk needs to be released. But the function returns directly in line 701 without freeing the clock. Even though we can fix it by freeing the clock manually if platform_get_irq_optional fails, it may not be following the best practice. The original code for this driver contains if (IS_ERR()) checks throughout, explicitly allowing the driver to continue loading even if devm_clk_get() fails. 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_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> --- v1->v2: change `ocores_i2c_of_probe` to use `devm_clk_get_enabled()` drivers/i2c/busses/i2c-ocores.c | 62 +++++++++++++-------------------- 1 file changed, 24 insertions(+), 38 deletions(-) diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c index a0af027db04c..1dcb1af1ad13 100644 --- a/drivers/i2c/busses/i2c-ocores.c +++ b/drivers/i2c/busses/i2c-ocores.c @@ -549,28 +549,24 @@ 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_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; - } - i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000; - if (clock_frequency_present) - i2c->bus_clock_khz = clock_frequency / 1000; + if (IS_ERR(i2c->clk)) { + dev_err(&pdev->dev, + "devm_clk_get_enabled failed\n"); + return PTR_ERR(i2c->clk); } + i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000; + if (clock_frequency_present) + i2c->bus_clock_khz = clock_frequency / 1000; + if (i2c->ip_clock_khz == 0) { if (of_property_read_u32(np, "opencores,ip-clock-frequency", &val)) { 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; @@ -675,8 +671,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; } } @@ -707,13 +702,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); @@ -725,7 +720,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) { @@ -734,10 +729,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) @@ -752,9 +743,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; } @@ -768,8 +756,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; } @@ -777,19 +764,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); } -- 2.34.1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2] i2c: ocores: use devm_ managed clks 2023-04-16 15:33 ` Andrew Lunn 2023-04-17 13:27 ` [PATCH v2] i2c: ocores: use devm_ managed clks Wang Zhang @ 2023-04-17 14:05 ` Wang Zhang 2023-04-17 14:29 ` Peter Rosin 2023-04-17 15:50 ` Andrew Lunn 1 sibling, 2 replies; 27+ messages in thread From: Wang Zhang @ 2023-04-17 14:05 UTC (permalink / raw) To: Peter Korsgaard, Andrew Lunn Cc: hust-os-kernel-patches, Wang Zhang, linux-i2c, linux-kernel Smatch Warns: drivers/i2c/busses/i2c-ocores.c:701 ocores_i2c_probe() warn: missing unwind goto? If any wrong occurs in ocores_i2c_of_probe, the i2c->clk needs to be released. But the function returns directly in line 701 without freeing the clock. Even though we can fix it by freeing the clock manually if platform_get_irq_optional fails, it may not be following the best practice. The original code for this driver contains if (IS_ERR()) checks throughout, explicitly allowing the driver to continue loading even if devm_clk_get() fails. 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_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> --- v1->v2: change `ocores_i2c_of_probe` to use `devm_clk_get_enabled()` drivers/i2c/busses/i2c-ocores.c | 62 +++++++++++++-------------------- 1 file changed, 24 insertions(+), 38 deletions(-) diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c index a0af027db04c..1dcb1af1ad13 100644 --- a/drivers/i2c/busses/i2c-ocores.c +++ b/drivers/i2c/busses/i2c-ocores.c @@ -549,28 +549,24 @@ 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_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; - } - i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000; - if (clock_frequency_present) - i2c->bus_clock_khz = clock_frequency / 1000; + if (IS_ERR(i2c->clk)) { + dev_err(&pdev->dev, + "devm_clk_get_enabled failed\n"); + return PTR_ERR(i2c->clk); } + i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000; + if (clock_frequency_present) + i2c->bus_clock_khz = clock_frequency / 1000; + if (i2c->ip_clock_khz == 0) { if (of_property_read_u32(np, "opencores,ip-clock-frequency", &val)) { 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; @@ -675,8 +671,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; } } @@ -707,13 +702,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); @@ -725,7 +720,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) { @@ -734,10 +729,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) @@ -752,9 +743,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; } @@ -768,8 +756,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; } @@ -777,19 +764,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); } -- 2.34.1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2] i2c: ocores: use devm_ managed clks 2023-04-17 14:05 ` Wang Zhang @ 2023-04-17 14:29 ` Peter Rosin 2023-04-17 15:50 ` Andrew Lunn 1 sibling, 0 replies; 27+ messages in thread From: Peter Rosin @ 2023-04-17 14:29 UTC (permalink / raw) To: Wang Zhang, Peter Korsgaard, Andrew Lunn Cc: hust-os-kernel-patches, linux-i2c, linux-kernel 2023-04-17 at 16:05, Wang Zhang wrote: > Smatch Warns: > drivers/i2c/busses/i2c-ocores.c:701 ocores_i2c_probe() warn: > missing unwind goto? > > If any wrong occurs in ocores_i2c_of_probe, the i2c->clk needs to be > released. But the function returns directly in line 701 without freeing > the clock. Even though we can fix it by freeing the clock manually if > platform_get_irq_optional fails, it may not be following the best practice. > The original code for this driver contains if (IS_ERR()) checks > throughout, explicitly allowing the driver to continue loading even if > devm_clk_get() fails. > > 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_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> > --- > v1->v2: change `ocores_i2c_of_probe` to use `devm_clk_get_enabled()` > > drivers/i2c/busses/i2c-ocores.c | 62 +++++++++++++-------------------- > 1 file changed, 24 insertions(+), 38 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c > index a0af027db04c..1dcb1af1ad13 100644 > --- a/drivers/i2c/busses/i2c-ocores.c > +++ b/drivers/i2c/busses/i2c-ocores.c > @@ -549,28 +549,24 @@ 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_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; > - } > - i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000; > - if (clock_frequency_present) > - i2c->bus_clock_khz = clock_frequency / 1000; > + if (IS_ERR(i2c->clk)) { > + dev_err(&pdev->dev, > + "devm_clk_get_enabled failed\n"); > + return PTR_ERR(i2c->clk); > } > > + i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000; > + if (clock_frequency_present) > + i2c->bus_clock_khz = clock_frequency / 1000; > + > if (i2c->ip_clock_khz == 0) { This branch seems hard to hit with this patch. Cheers, Peter > if (of_property_read_u32(np, "opencores,ip-clock-frequency", > &val)) { > 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; > @@ -675,8 +671,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; > } > } > > @@ -707,13 +702,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); > @@ -725,7 +720,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) { > @@ -734,10 +729,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) > @@ -752,9 +743,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; > } > > @@ -768,8 +756,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; > } > > @@ -777,19 +764,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); > } > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] i2c: ocores: use devm_ managed clks 2023-04-17 14:05 ` Wang Zhang 2023-04-17 14:29 ` Peter Rosin @ 2023-04-17 15:50 ` Andrew Lunn 2023-04-22 12:32 ` [PATCH v3] " Wang Zhang 1 sibling, 1 reply; 27+ messages in thread From: Andrew Lunn @ 2023-04-17 15:50 UTC (permalink / raw) To: Wang Zhang Cc: Peter Korsgaard, hust-os-kernel-patches, linux-i2c, linux-kernel > drivers/i2c/busses/i2c-ocores.c | 62 +++++++++++++-------------------- > 1 file changed, 24 insertions(+), 38 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c > index a0af027db04c..1dcb1af1ad13 100644 > --- a/drivers/i2c/busses/i2c-ocores.c > +++ b/drivers/i2c/busses/i2c-ocores.c > @@ -549,28 +549,24 @@ 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_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; > - } > - i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000; > - if (clock_frequency_present) > - i2c->bus_clock_khz = clock_frequency / 1000; > + if (IS_ERR(i2c->clk)) { > + dev_err(&pdev->dev, > + "devm_clk_get_enabled failed\n"); > + return PTR_ERR(i2c->clk); I think this is wrong. The old code would not return an error if devm_clk_get() failed, e.g because the clock does not exist in DT. i2c->bus_clock_khz would default to 100, and it keeps going. Now, it appears you have turned the missing clock into a fatal error. devm_clk_get_optional_enabled() seems to do what you want. Andrew ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3] i2c: ocores: use devm_ managed clks 2023-04-17 15:50 ` Andrew Lunn @ 2023-04-22 12:32 ` Wang Zhang 2023-04-25 9:58 ` 张网 2023-04-25 11:57 ` Andrew Lunn 0 siblings, 2 replies; 27+ messages in thread From: Wang Zhang @ 2023-04-22 12:32 UTC (permalink / raw) To: Peter Korsgaard, Andrew Lunn Cc: hust-os-kernel-patches, Wang Zhang, linux-i2c, linux-kernel If any wrong occurs in ocores_i2c_of_probe, the i2c->clk needs to be released. But the function returns directly in line 701 without freeing the clock. Even though we can fix it by freeing the clock manually if platform_get_irq_optional fails, it may not be following the best practice. The original code for this driver contains if (IS_ERR()) checks throughout, explicitly allowing the driver to continue loading even if devm_clk_get() fails. 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> --- 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 | 56 +++++++++++++-------------------- 1 file changed, 22 insertions(+), 34 deletions(-) diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c index 2e575856c5cd..0b225177fdd1 100644 --- a/drivers/i2c/busses/i2c-ocores.c +++ b/drivers/i2c/busses/i2c-ocores.c @@ -552,16 +552,15 @@ 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 (IS_ERR(i2c->clk)) { + dev_err(&pdev->dev, + "devm_clk_get_optional_enabled failed\n"); + return PTR_ERR(i2c->clk); + } - if (ret) { - dev_err(&pdev->dev, - "clk_prepare_enable failed: %d\n", ret); - return ret; - } + 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 +572,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 +676,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 +707,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 +725,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 +734,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 +748,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 +761,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 +769,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); } -- 2.34.1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v3] i2c: ocores: use devm_ managed clks 2023-04-22 12:32 ` [PATCH v3] " Wang Zhang @ 2023-04-25 9:58 ` 张网 2023-04-25 11:57 ` Andrew Lunn 1 sibling, 0 replies; 27+ messages in thread From: 张网 @ 2023-04-25 9:58 UTC (permalink / raw) To: Peter Korsgaard, Andrew Lunn Cc: hust-os-kernel-patches, linux-i2c, linux-kernel ping? "Wang Zhang" <silver_code@hust.edu.cn>写道: > If any wrong occurs in ocores_i2c_of_probe, the i2c->clk needs to be > released. But the function returns directly in line 701 without freeing > the clock. Even though we can fix it by freeing the clock manually if > platform_get_irq_optional fails, it may not be following the best practice. > The original code for this driver contains if (IS_ERR()) checks > throughout, explicitly allowing the driver to continue loading even if > devm_clk_get() fails. > > 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> > --- > 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 | 56 +++++++++++++-------------------- > 1 file changed, 22 insertions(+), 34 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c > index 2e575856c5cd..0b225177fdd1 100644 > --- a/drivers/i2c/busses/i2c-ocores.c > +++ b/drivers/i2c/busses/i2c-ocores.c > @@ -552,16 +552,15 @@ 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 (IS_ERR(i2c->clk)) { > + dev_err(&pdev->dev, > + "devm_clk_get_optional_enabled failed\n"); > + return PTR_ERR(i2c->clk); > + } > > - if (ret) { > - dev_err(&pdev->dev, > - "clk_prepare_enable failed: %d\n", ret); > - return ret; > - } > + 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 +572,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 +676,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 +707,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 +725,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 +734,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 +748,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 +761,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 +769,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); > } > > -- > 2.34.1 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3] i2c: ocores: use devm_ managed clks 2023-04-22 12:32 ` [PATCH v3] " Wang Zhang 2023-04-25 9:58 ` 张网 @ 2023-04-25 11:57 ` Andrew Lunn 2023-04-25 14:47 ` 张网 2023-05-23 4:49 ` Re: [PATCH v3] " 张网 1 sibling, 2 replies; 27+ messages in thread From: Andrew Lunn @ 2023-04-25 11:57 UTC (permalink / raw) To: Wang Zhang Cc: Peter Korsgaard, hust-os-kernel-patches, linux-i2c, linux-kernel On Sat, Apr 22, 2023 at 08:32:53PM +0800, Wang Zhang wrote: > If any wrong occurs in ocores_i2c_of_probe, the i2c->clk needs to be > released. But the function returns directly in line 701 without freeing > the clock. Even though we can fix it by freeing the clock manually if > platform_get_irq_optional fails, it may not be following the best practice. > The original code for this driver contains if (IS_ERR()) checks > throughout, explicitly allowing the driver to continue loading even if > devm_clk_get() fails. > > 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> Andrew ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Re: [PATCH v3] i2c: ocores: use devm_ managed clks 2023-04-25 11:57 ` Andrew Lunn @ 2023-04-25 14:47 ` 张网 2023-04-25 15:06 ` Andrew Lunn 2023-05-23 4:49 ` Re: [PATCH v3] " 张网 1 sibling, 1 reply; 27+ messages in thread From: 张网 @ 2023-04-25 14:47 UTC (permalink / raw) To: Andrew Lunn Cc: Peter Korsgaard, hust-os-kernel-patches, linux-i2c, linux-kernel Hi Andrew, I would like to express my sincere gratitude for taking the time and effort to review my submitted patch. I understand that reviewing can be a time-consuming process and I truly appreciate your dedication. As we move forward, I would like to inquire about the first version[1] of the patch I submitted. As clk_disable_unprepare() has checks for error pointer and NULL already, I think there is no need to add the check. So both the first version of the patch and this one can work on this branch. If there are any further changes or revisions needed, please do not hesitate to let me know. I am committed to learning and improving, and I welcome any feedback you may have. Thank you again for your support and guidance throughout this process. Best regards, Wang Zhang --- [1] http://patchwork.ozlabs.org/project/linux-i2c/patch/20230416071854.58335-1-silver_code@hust.edu.cn/ "Andrew Lunn" <andrew@lunn.ch>写道: > On Sat, Apr 22, 2023 at 08:32:53PM +0800, Wang Zhang wrote: > > If any wrong occurs in ocores_i2c_of_probe, the i2c->clk needs to be > > released. But the function returns directly in line 701 without freeing > > the clock. Even though we can fix it by freeing the clock manually if > > platform_get_irq_optional fails, it may not be following the best practice. > > The original code for this driver contains if (IS_ERR()) checks > > throughout, explicitly allowing the driver to continue loading even if > > devm_clk_get() fails. > > > > 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> > > Andrew ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Re: [PATCH v3] i2c: ocores: use devm_ managed clks 2023-04-25 14:47 ` 张网 @ 2023-04-25 15:06 ` Andrew Lunn 2023-05-26 7:05 ` [PATCH v4] " Wang Zhang 0 siblings, 1 reply; 27+ messages in thread From: Andrew Lunn @ 2023-04-25 15:06 UTC (permalink / raw) To: 张网 Cc: Peter Korsgaard, hust-os-kernel-patches, linux-i2c, linux-kernel On Tue, Apr 25, 2023 at 10:47:29PM +0800, 张网 wrote: > Hi Andrew, > > I would like to express my sincere gratitude for taking the time and effort to review > my submitted patch. I understand that reviewing can be a time-consuming process > and I truly appreciate your dedication. > > As we move forward, I would like to inquire about the first version[1] of the patch I submitted. > As clk_disable_unprepare() has checks for error pointer and NULL already, I think there is no > need to add the check. So both the first version of the patch and this one can work on this > branch. > > If there are any further changes or revisions needed, please do not hesitate to let me know. > I am committed to learning and improving, and I welcome any feedback you may have. > Thank you again for your support and guidance throughout this process. > > Best regards, > Wang Zhang > --- > [1] http://patchwork.ozlabs.org/project/linux-i2c/patch/20230416071854.58335-1-silver_code@hust.edu.cn/ So this patch is about the IRQ being an error code, and doing the correct cleanup. With the change to devm_clk_get_ there is no need to disabled the clock, it will be done automatically. This means there is no cleanup the driver needs to do itself. So the patch is no longer needed. Andrew ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v4] i2c: ocores: use devm_ managed clks 2023-04-25 15:06 ` Andrew Lunn @ 2023-05-26 7:05 ` Wang Zhang 2023-06-13 23:54 ` Andi Shyti 2023-06-23 8:22 ` Wolfram Sang 0 siblings, 2 replies; 27+ messages in thread From: Wang Zhang @ 2023-05-26 7:05 UTC (permalink / raw) To: Peter Korsgaard, Andrew Lunn, Wolfram Sang, Andreas Larsson Cc: hust-os-kernel-patches, Wang Zhang, Peter Korsgaard, linux-i2c, linux-kernel Smatch complains that: drivers/i2c/busses/i2c-ocores.c:704 ocores_i2c_probe() warn: missing unwind goto? If any wrong occurs in ocores_i2c_of_probe, the i2c->clk needs to be released. But the function returns directly without freeing the clock. Fix this by updating the code to use devm_clk_get_optional_enabled() instead. Use dev_err_probe() where appropriate as well since we are changing those statements. Fixes: f5f35a92e44a ("i2c: ocores: Add irq support for sparc") Signed-off-by: Wang Zhang <silver_code@hust.edu.cn> --- v3->v4: use `dev_err_probe` to compact the code and add a fixes tag 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 | 64 +++++++++++---------------------- 1 file changed, 21 insertions(+), 43 deletions(-) diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c index 2e575856c5cd..e30df2b78fdf 100644 --- a/drivers/i2c/busses/i2c-ocores.c +++ b/drivers/i2c/busses/i2c-ocores.c @@ -552,28 +552,20 @@ 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); - - 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; - } - i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000; - if (clock_frequency_present) - i2c->bus_clock_khz = clock_frequency / 1000; - } - + i2c->clk = devm_clk_get_optional_enabled(&pdev->dev, NULL); + if (IS_ERR(i2c->clk)) + return dev_err_probe(&pdev->dev, PTR_ERR(i2c->clk), + "devm_clk_get_optional_enabled failed\n"); + + i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000; + if (clock_frequency_present) + i2c->bus_clock_khz = clock_frequency / 1000; if (i2c->ip_clock_khz == 0) { if (of_property_read_u32(np, "opencores,ip-clock-frequency", &val)) { 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 +670,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 +701,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 +719,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 +728,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 +742,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,28 +755,22 @@ 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; } static int ocores_i2c_resume(struct device *dev) { struct ocores_i2c *i2c = dev_get_drvdata(dev); + unsigned long rate; + int ret; - if (!IS_ERR(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; - } + ret = clk_prepare_enable(i2c->clk); + if (ret) + return dev_err_probe(dev, ret, "clk_prepare_enable failed\n"); + rate = clk_get_rate(i2c->clk) / 1000; + if (rate) + i2c->ip_clock_khz = rate; return ocores_init(dev, i2c); } -- 2.34.1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v4] i2c: ocores: use devm_ managed clks 2023-05-26 7:05 ` [PATCH v4] " Wang Zhang @ 2023-06-13 23:54 ` Andi Shyti 2023-06-23 8:22 ` Wolfram Sang 1 sibling, 0 replies; 27+ messages in thread From: Andi Shyti @ 2023-06-13 23:54 UTC (permalink / raw) To: Wang Zhang Cc: Peter Korsgaard, Andrew Lunn, Wolfram Sang, Andreas Larsson, hust-os-kernel-patches, Peter Korsgaard, linux-i2c, linux-kernel Hi Wang, > - i2c->clk = devm_clk_get(&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; > - } > - i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000; > - if (clock_frequency_present) > - i2c->bus_clock_khz = clock_frequency / 1000; > - } > - > + i2c->clk = devm_clk_get_optional_enabled(&pdev->dev, NULL); > + if (IS_ERR(i2c->clk)) > + return dev_err_probe(&pdev->dev, PTR_ERR(i2c->clk), > + "devm_clk_get_optional_enabled failed\n"); > + > + i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000; if devm_clk_get_optional_enabled() returns NULL, clk_get_rate() returns '0' and op_clk_khz would be '0'... > + if (clock_frequency_present) > + i2c->bus_clock_khz = clock_frequency / 1000; > if (i2c->ip_clock_khz == 0) { ... and we fall inside this 'if', as expected. Looks correct! Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Thanks, Andi ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4] i2c: ocores: use devm_ managed clks 2023-05-26 7:05 ` [PATCH v4] " Wang Zhang 2023-06-13 23:54 ` Andi Shyti @ 2023-06-23 8:22 ` Wolfram Sang 2023-06-23 8:26 ` Wolfram Sang 1 sibling, 1 reply; 27+ messages in thread From: Wolfram Sang @ 2023-06-23 8:22 UTC (permalink / raw) To: Wang Zhang Cc: Peter Korsgaard, Andrew Lunn, Andreas Larsson, hust-os-kernel-patches, Peter Korsgaard, linux-i2c, linux-kernel [-- Attachment #1: Type: text/plain, Size: 802 bytes --] On Fri, May 26, 2023 at 03:05:33PM +0800, Wang Zhang wrote: > Smatch complains that: > drivers/i2c/busses/i2c-ocores.c:704 ocores_i2c_probe() > warn: missing unwind goto? > > If any wrong occurs in ocores_i2c_of_probe, the i2c->clk needs to be > released. But the function returns directly without freeing the clock. > > Fix this by updating the code to use devm_clk_get_optional_enabled() > instead. Use dev_err_probe() where appropriate as well since we are > changing those statements. > > Fixes: f5f35a92e44a ("i2c: ocores: Add irq support for sparc") > Signed-off-by: Wang Zhang <silver_code@hust.edu.cn> Applied to for-next with Andrew's Rev-by from v3 added, thanks! Please send new versions of a patch as a new mail thread. This makes handling easier for maintainers. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4] i2c: ocores: use devm_ managed clks 2023-06-23 8:22 ` Wolfram Sang @ 2023-06-23 8:26 ` Wolfram Sang 0 siblings, 0 replies; 27+ messages in thread From: Wolfram Sang @ 2023-06-23 8:26 UTC (permalink / raw) To: Wang Zhang, Peter Korsgaard, Andrew Lunn, Andreas Larsson, hust-os-kernel-patches, Peter Korsgaard, linux-i2c, linux-kernel [-- Attachment #1: Type: text/plain, Size: 157 bytes --] > Applied to for-next with Andrew's Rev-by from v3 added, thanks! And I needed to rebase it to i2c/for-mergewindow because of the remove-callback rework. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Re: [PATCH v3] i2c: ocores: use devm_ managed clks 2023-04-25 11:57 ` Andrew Lunn 2023-04-25 14:47 ` 张网 @ 2023-05-23 4:49 ` 张网 2023-05-23 12:10 ` Andrew Lunn ` (3 more replies) 1 sibling, 4 replies; 27+ messages in thread From: 张网 @ 2023-05-23 4:49 UTC (permalink / raw) To: andrew lunn Cc: hust-os-kernel-patches, peter korsgaard, linux-i2c, linux-kernel > -----原始邮件----- > 发件人: "Andrew Lunn" <andrew@lunn.ch> > 发送时间: 2023-04-25 19:57:26 (星期二) > 收件人: "Wang Zhang" <silver_code@hust.edu.cn> > 抄送: "Peter Korsgaard" <peter@korsgaard.com>, hust-os-kernel-patches@googlegroups.com, linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org > 主题: Re: [PATCH v3] i2c: ocores: use devm_ managed clks > > On Sat, Apr 22, 2023 at 08:32:53PM +0800, Wang Zhang wrote: > > If any wrong occurs in ocores_i2c_of_probe, the i2c->clk needs to be > > released. But the function returns directly in line 701 without freeing > > the clock. Even though we can fix it by freeing the clock manually if > > platform_get_irq_optional fails, it may not be following the best practice. > > The original code for this driver contains if (IS_ERR()) checks > > throughout, explicitly allowing the driver to continue loading even if > > devm_clk_get() fails. > > > > 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> > > Andrew Hi Andrew, I'm checking in about my patch submission for i2c ocores that was "review'ed" on 4/25, but its status has not been updated yet. I would greatly appreciate it if you could provide me with an update on the status of my submission. Is there any additional information or documentation that I can provide to help expedite the process? Thank you very much for your time and attention. I look forward to hearing from you soon. Regards, Wang Zhang ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Re: [PATCH v3] i2c: ocores: use devm_ managed clks 2023-05-23 4:49 ` Re: [PATCH v3] " 张网 @ 2023-05-23 12:10 ` Andrew Lunn 2023-05-23 14:33 ` Wang Zhang ` (2 subsequent siblings) 3 siblings, 0 replies; 27+ messages in thread From: Andrew Lunn @ 2023-05-23 12:10 UTC (permalink / raw) To: 张网 Cc: hust-os-kernel-patches, peter korsgaard, linux-i2c, linux-kernel > Hi Andrew, > I'm checking in about my patch submission for i2c ocores that was > "review'ed" on 4/25, but its status has not been updated yet. > I would greatly appreciate it if you could provide me with an > update on the status of my submission. Is there any additional > information or documentation that I can provide to help expedite > the process? I think your patch was submitted during the merge window. This is the point in time when subsystems give Linus patches for the next release. During those two weeks new patches are not accepted. https://www.kernel.org/doc/html/latest/process/2.Process.html Now that the merge window is closed, please rebase your patch on git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next and resubmit. Include my Reviewed-by tag. Andrew ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3] i2c: ocores: use devm_ managed clks 2023-05-23 4:49 ` Re: [PATCH v3] " 张网 2023-05-23 12:10 ` Andrew Lunn @ 2023-05-23 14:33 ` Wang Zhang 2023-05-24 15:35 ` Wang Zhang 2023-05-24 15:43 ` Wang Zhang 3 siblings, 0 replies; 27+ messages in thread From: Wang Zhang @ 2023-05-23 14:33 UTC (permalink / raw) To: Peter Korsgaard, Andrew Lunn Cc: os-kernel-patches, Wang Zhang, linux-i2c, linux-kernel 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> --- 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"); + 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); } -- 2.34.1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3] i2c: ocores: use devm_ managed clks 2023-05-23 4:49 ` Re: [PATCH v3] " 张网 2023-05-23 12:10 ` Andrew Lunn 2023-05-23 14:33 ` Wang Zhang @ 2023-05-24 15:35 ` Wang Zhang 2023-05-24 15:43 ` Wang Zhang 3 siblings, 0 replies; 27+ messages in thread From: Wang Zhang @ 2023-05-24 15:35 UTC (permalink / raw) To: Peter Korsgaard, Andrew Lunn Cc: os-kernel-patches, Wang Zhang, linux-i2c, linux-kernel 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"); + 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); } -- 2.34.1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3] i2c: ocores: use devm_ managed clks 2023-05-23 4:49 ` Re: [PATCH v3] " 张网 ` (2 preceding siblings ...) 2023-05-24 15:35 ` Wang Zhang @ 2023-05-24 15:43 ` Wang Zhang 2023-05-24 18:45 ` Dan Carpenter ` (2 more replies) 3 siblings, 3 replies; 27+ messages in thread From: Wang Zhang @ 2023-05-24 15:43 UTC (permalink / raw) To: Peter Korsgaard, Andrew Lunn Cc: hust-os-kernel-patches, Wang Zhang, linux-i2c, linux-kernel 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"); + 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); } -- 2.34.1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v3] i2c: ocores: use devm_ managed clks 2023-05-24 15:43 ` Wang Zhang @ 2023-05-24 18:45 ` Dan Carpenter 2023-05-24 19:21 ` Christophe JAILLET 2023-05-25 5:02 ` Dan Carpenter 2 siblings, 0 replies; 27+ messages in thread From: Dan Carpenter @ 2023-05-24 18:45 UTC (permalink / raw) To: Wang Zhang Cc: Peter Korsgaard, Andrew Lunn, hust-os-kernel-patches, linux-i2c, linux-kernel 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3] i2c: ocores: use devm_ managed clks 2023-05-24 15:43 ` Wang Zhang 2023-05-24 18:45 ` Dan Carpenter @ 2023-05-24 19:21 ` Christophe JAILLET 2023-05-25 3:33 ` 张网 2023-05-25 5:05 ` Dan Carpenter 2023-05-25 5:02 ` Dan Carpenter 2 siblings, 2 replies; 27+ messages in thread From: Christophe JAILLET @ 2023-05-24 19:21 UTC (permalink / raw) To: Wang Zhang, Peter Korsgaard, Andrew Lunn Cc: hust-os-kernel-patches, linux-i2c, linux-kernel, 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); > } > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Re: [PATCH v3] i2c: ocores: use devm_ managed clks 2023-05-24 19:21 ` Christophe JAILLET @ 2023-05-25 3:33 ` 张网 2023-05-25 19:16 ` Christophe JAILLET 2023-05-25 5:05 ` Dan Carpenter 1 sibling, 1 reply; 27+ messages in thread From: 张网 @ 2023-05-25 3:33 UTC (permalink / raw) To: Christophe JAILLET Cc: Peter Korsgaard, Andrew Lunn, hust-os-kernel-patches, linux-i2c, linux-kernel, Dan Carpenter 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); > > } > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3] i2c: ocores: use devm_ managed clks 2023-05-25 3:33 ` 张网 @ 2023-05-25 19:16 ` Christophe JAILLET 0 siblings, 0 replies; 27+ messages in thread From: Christophe JAILLET @ 2023-05-25 19:16 UTC (permalink / raw) To: 张网 Cc: Peter Korsgaard, Andrew Lunn, hust-os-kernel-patches, linux-i2c, linux-kernel, 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) ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3] i2c: ocores: use devm_ managed clks 2023-05-24 19:21 ` Christophe JAILLET 2023-05-25 3:33 ` 张网 @ 2023-05-25 5:05 ` Dan Carpenter 1 sibling, 0 replies; 27+ messages in thread From: Dan Carpenter @ 2023-05-25 5:05 UTC (permalink / raw) To: Christophe JAILLET Cc: Wang Zhang, Peter Korsgaard, Andrew Lunn, hust-os-kernel-patches, linux-i2c, linux-kernel 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3] i2c: ocores: use devm_ managed clks 2023-05-24 15:43 ` Wang Zhang 2023-05-24 18:45 ` Dan Carpenter 2023-05-24 19:21 ` Christophe JAILLET @ 2023-05-25 5:02 ` Dan Carpenter 2023-05-25 9:28 ` Dan Carpenter 2 siblings, 1 reply; 27+ messages in thread From: Dan Carpenter @ 2023-05-25 5:02 UTC (permalink / raw) To: Wang Zhang Cc: Peter Korsgaard, Andrew Lunn, hust-os-kernel-patches, linux-i2c, linux-kernel 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); ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3] i2c: ocores: use devm_ managed clks 2023-05-25 5:02 ` Dan Carpenter @ 2023-05-25 9:28 ` Dan Carpenter 0 siblings, 0 replies; 27+ messages in thread From: Dan Carpenter @ 2023-05-25 9:28 UTC (permalink / raw) To: Wang Zhang Cc: Peter Korsgaard, Andrew Lunn, hust-os-kernel-patches, linux-i2c, linux-kernel 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2023-06-23 8:26 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-04-16 7:20 [PATCH] i2c: ocores: add missing unwind goto in `ocores_i2c_probe` Wang Zhang 2023-04-16 15:33 ` Andrew Lunn 2023-04-17 13:27 ` [PATCH v2] i2c: ocores: use devm_ managed clks Wang Zhang 2023-04-17 14:05 ` Wang Zhang 2023-04-17 14:29 ` Peter Rosin 2023-04-17 15:50 ` Andrew Lunn 2023-04-22 12:32 ` [PATCH v3] " Wang Zhang 2023-04-25 9:58 ` 张网 2023-04-25 11:57 ` Andrew Lunn 2023-04-25 14:47 ` 张网 2023-04-25 15:06 ` Andrew Lunn 2023-05-26 7:05 ` [PATCH v4] " Wang Zhang 2023-06-13 23:54 ` Andi Shyti 2023-06-23 8:22 ` Wolfram Sang 2023-06-23 8:26 ` Wolfram Sang 2023-05-23 4:49 ` Re: [PATCH v3] " 张网 2023-05-23 12:10 ` Andrew Lunn 2023-05-23 14:33 ` Wang Zhang 2023-05-24 15:35 ` Wang Zhang 2023-05-24 15:43 ` Wang Zhang 2023-05-24 18:45 ` Dan Carpenter 2023-05-24 19:21 ` Christophe JAILLET 2023-05-25 3:33 ` 张网 2023-05-25 19:16 ` Christophe JAILLET 2023-05-25 5:05 ` Dan Carpenter 2023-05-25 5:02 ` Dan Carpenter 2023-05-25 9:28 ` Dan Carpenter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).