| Message ID | 20240831021334.1907921-6-lizetao1@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: Convert using devm_clk_get_enabled()/devm_clk_get_optional_enabled() | expand |
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@huawei.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. 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.) 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
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@huawei.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.
Hello, On Tue, Sep 03, 2024 at 06:46:48PM +0800, Li Zetao wrote: > 在 2024/9/3 16:09, Uwe Kleine-König 写道: > > 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@huawei.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. Indeed. This is IMHO worth mentioning in the commit log to prevent the next reviewer stumble over the same code construct. Best regards Uwe
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; } 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. */
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@huawei.com> --- drivers/net/ethernet/faraday/ftgmac100.c | 27 ++++++------------------ 1 file changed, 7 insertions(+), 20 deletions(-)