| 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: <0becf4e0-2f66-4c26-b0b3-59ee232eaaef@huawei.com>
Date: Tue, 3 Sep 2024 18:46:48 +0800
From: Li Zetao <lizetao1@...wei.com>
To: Uwe Kleine-König <u.kleine-koenig@...libre.com>
CC: <florian.fainelli@...adcom.com>, <andrew@...n.ch>, <olteanv@...il.com>,
<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
<pabeni@...hat.com>, <wens@...e.org>, <jernej.skrabec@...il.com>,
<samuel@...lland.org>, <heiko@...ech.de>, <yisen.zhuang@...wei.com>,
<salil.mehta@...wei.com>, <hauke@...ke-m.de>, <alexandre.torgue@...s.st.com>,
<joabreu@...opsys.com>, <mcoquelin.stm32@...il.com>, <wellslutw@...il.com>,
<radhey.shyam.pandey@....com>, <michal.simek@....com>,
<ajay.kathat@...rochip.com>, <claudiu.beznea@...on.dev>, <kvalo@...nel.org>,
<jacky_chou@...eedtech.com>, <netdev@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>, <linux-sunxi@...ts.linux.dev>,
<linux-rockchip@...ts.infradead.org>,
<linux-stm32@...md-mailman.stormreply.com>, <linux-wireless@...r.kernel.org>
Subject: Re: [PATCH net-next 05/12] net: ftgmac100: Convert using
devm_clk_get_enabled() in ftgmac100_setup_clk()
Hi,
在 2024/9/3 16:09, Uwe Kleine-König 写道:
> Hello,
>
> On Sat, Aug 31, 2024 at 10:13:27AM +0800, Li Zetao wrote:
>> Use devm_clk_get_enabled() instead of devm_clk_get() +
>> clk_prepare_enable(), which can make the clk consistent with the device
>> life cycle and reduce the risk of unreleased clk resources. Since the
>> device framework has automatically released the clk resource, there is
>> no need to execute clk_disable_unprepare(clk) on the error path, drop
>> the cleanup_clk label, and the original error process can return directly.
>>
>> Signed-off-by: Li Zetao <lizetao1@...wei.com>
>> ---
>> drivers/net/ethernet/faraday/ftgmac100.c | 27 ++++++------------------
>> 1 file changed, 7 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
>> index 4c546c3aef0f..eb57c822c5ac 100644
>> --- a/drivers/net/ethernet/faraday/ftgmac100.c
>> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
>> @@ -1752,13 +1752,10 @@ static int ftgmac100_setup_clk(struct ftgmac100 *priv)
>> struct clk *clk;
>> int rc;
>>
>> - clk = devm_clk_get(priv->dev, NULL /* MACCLK */);
>> + clk = devm_clk_get_enabled(priv->dev, NULL /* MACCLK */);
>> if (IS_ERR(clk))
>> return PTR_ERR(clk);
>> priv->clk = clk;
>> - rc = clk_prepare_enable(priv->clk);
>> - if (rc)
>> - return rc;
>>
>> /* Aspeed specifies a 100MHz clock is required for up to
>> * 1000Mbit link speeds. As NCSI is limited to 100Mbit, 25MHz
>> @@ -1767,21 +1764,17 @@ static int ftgmac100_setup_clk(struct ftgmac100 *priv)
>> rc = clk_set_rate(priv->clk, priv->use_ncsi ? FTGMAC_25MHZ :
>> FTGMAC_100MHZ);
>> if (rc)
>> - goto cleanup_clk;
>> + return rc;
>>
>> /* RCLK is for RMII, typically used for NCSI. Optional because it's not
>> * necessary if it's the AST2400 MAC, or the MAC is configured for
>> * RGMII, or the controller is not an ASPEED-based controller.
>> */
>> - priv->rclk = devm_clk_get_optional(priv->dev, "RCLK");
>> - rc = clk_prepare_enable(priv->rclk);
>> - if (!rc)
>> - return 0;
>> + priv->rclk = devm_clk_get_optional_enabled(priv->dev, "RCLK");
>> + if (IS_ERR(priv->rclk))
>> + return PTR_ERR(priv->rclk);
>>
>> -cleanup_clk:
>> - clk_disable_unprepare(priv->clk);
>> -
>> - return rc;
>> + return 0;
>
> You're changing semantics here. Before your patch ftgmac100_setup_clk()
> was left with priv->clk disabled; now you keep it enabled.
Before my patch, ftgmac100_setup_clk() was only left with priv->clk
disabled when error occurs, and was left with priv->clk enabled when no
error occurs because when enabling priv->rclk successfully, it will
return 0 directly, and when enabling priv->rclk failed, it will disable
priv->clk.
It turns out that the code logic is a bit counter-intuitive, but the
readability has been improved after adjustments.
>
> Further note that there is a bug here, because in ftgmac100_probe()
> (i.e. the caller of ftgmac100_setup_clk())
> clk_disable_unprepare(priv->clk) is called in the error path.
> (I only looked quickly, so I might have missed a detail.)
I have considered the case that clk_disable_unprepare will not be
executed on the wrong path in ftgmac100_probe(). So I understand that
the problem you mentioned should not exist.
>
> So while your patch is an improvement for clock enable/disable
> balancing, it might regress on power consumption.
>
>> }
>>
>> static bool ftgmac100_has_child_node(struct device_node *np, const char *name)
>> @@ -1996,16 +1989,13 @@ static int ftgmac100_probe(struct platform_device *pdev)
>> err = register_netdev(netdev);
>> if (err) {
>> dev_err(&pdev->dev, "Failed to register netdev\n");
>> - goto err_register_netdev;
>> + goto err_phy_connect;
>> }
>>
>> netdev_info(netdev, "irq %d, mapped at %p\n", netdev->irq, priv->base);
>>
>> return 0;
>>
>> -err_register_netdev:
>> - clk_disable_unprepare(priv->rclk);
>> - clk_disable_unprepare(priv->clk);
>> err_phy_connect:
>> ftgmac100_phy_disconnect(netdev);
>> err_ncsi_dev:
>> @@ -2034,9 +2024,6 @@ static void ftgmac100_remove(struct platform_device *pdev)
>> ncsi_unregister_dev(priv->ndev);
>> unregister_netdev(netdev);
>>
>> - clk_disable_unprepare(priv->rclk);
>> - clk_disable_unprepare(priv->clk);
>> -
>> /* There's a small chance the reset task will have been re-queued,
>> * during stop, make sure it's gone before we free the structure.
>> */
>
> Best regards
> Uwe
Thanks,
Li Zetao.
Powered by blists - more mailing lists