| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5cceb5b5.c1fe.18850f830e0.Coremail.m202171703@hust.edu.cn>
Date: Thu, 25 May 2023 11:33:42 +0800 (GMT+08:00)
From: 张网 <m202171703@...t.edu.cn>
To: "Christophe JAILLET" <christophe.jaillet@...adoo.fr>
Cc: "Peter Korsgaard" <peter@...sgaard.com>,
"Andrew Lunn" <andrew@...n.ch>,
hust-os-kernel-patches@...glegroups.com, linux-i2c@...r.kernel.org,
linux-kernel@...r.kernel.org,
"Dan Carpenter" <dan.carpenter@...aro.org>
Subject: Re: Re: [PATCH v3] i2c: ocores: use devm_ managed clks
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@...adoo.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@...t.edu.cn>
> > Reviewed-by: Andrew Lunn <andrew@...n.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);
> > }
> >
Powered by blists - more mailing lists