[PATCH net-next 00/12] net: Convert using devm_clk_get_enabled()/devm_clk_get_optional_enabled()

[PATCH net-next 00/12] net: Convert using devm_clk_get_enabled()/devm_clk_get_optional_enabled()

[Li Zetao]

There are many examples[1][2] of clk resource leakage in LTS. The
reason is that developers need to maintain the allocation and release
of clk resources themselves, but this will increase the burden on
developers. Using the API related to devm_clk_get_*_enable ensures
that the life cycle of clk is consistent with that of the device,
reducing the risk of unreleased resources like clk.

Several other developers are also working on converting to more
secure interfaces, and this patch set is in principle the same as
theirs.

[1]: https://lore.kernel.org/all/20240812160128.338041191@linuxfoundation.org/
[2]: https://lore.kernel.org/all/20240812160135.992451065@linuxfoundation.org/

Li Zetao (12):
  net: dsa: bcm_sf2: Convert using devm_clk_get_optional_enabled() in
    bcm_sf2_sw_probe()
  net: ethernet: Convert using devm_clk_get_enabled() in emac_probe()
  net: ethernet: arc: Convert using devm_clk_get_enabled() in
    emac_probe()
  net: ethernet: ethoc: Convert using devm_clk_get_enabled() in
    ethoc_probe()
  net: ftgmac100: Convert using devm_clk_get_enabled() in
    ftgmac100_setup_clk()
  net: ethernet: hisilicon: Convert using devm_clk_get_enabled() in
    hisi_femac_drv_probe()
  net: lantiq_xrx200: Convert using devm_clk_get_enabled() in
    xrx200_probe()
  net: stmmac: dwmac-dwc-qos-eth: Convert using devm_clk_get_enabled()
    in dwc_qos_probe()
  net: ethernet: sunplus: Convert using devm_clk_get_enabled() in
    spl2sw_probe()
  net: xilinx: axienet: Convert using devm_clk_get_optional_enabled() in
    axienet_probe()
  wifi: wilc1000: Convert using devm_clk_get_optional_enabled() in
    wilc_sdio_probe()
  wifi: wilc1000: Convert using devm_clk_get_optional_enabled() in
    wilc_bus_probe()

 drivers/net/dsa/bcm_sf2.c                     | 28 ++----
 drivers/net/ethernet/allwinner/sun4i-emac.c   | 13 +--
 drivers/net/ethernet/arc/emac_rockchip.c      | 34 ++-----
 drivers/net/ethernet/ethoc.c                  | 18 ++--
 drivers/net/ethernet/faraday/ftgmac100.c      | 27 ++---
 drivers/net/ethernet/hisilicon/hisi_femac.c   | 17 +---
 drivers/net/ethernet/lantiq_xrx200.c          | 17 +---
 .../stmicro/stmmac/dwmac-dwc-qos-eth.c        | 98 ++++---------------
 drivers/net/ethernet/sunplus/spl2sw_driver.c  | 18 +---
 .../net/ethernet/xilinx/xilinx_axienet_main.c | 15 +--
 .../net/wireless/microchip/wilc1000/sdio.c    | 10 +-
 drivers/net/wireless/microchip/wilc1000/spi.c |  5 +-
 12 files changed, 64 insertions(+), 236 deletions(-)

-- 
2.34.1

[PATCH net-next 01/12] net: dsa: bcm_sf2: Convert using devm_clk_get_optional_enabled() in bcm_sf2_sw_probe()

[Li Zetao]

Use devm_clk_get_optional_enabled() instead of devm_clk_get_optional() +
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 out_clk_mdiv and out_clk labels, and the original error process can
be returned directly.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/net/dsa/bcm_sf2.c | 28 ++++++----------------------
 1 file changed, 6 insertions(+), 22 deletions(-)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 0e663ec0c12a..96c0fdb56601 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -1453,28 +1453,18 @@ static int bcm_sf2_sw_probe(struct platform_device *pdev)
 		base++;
 	}
 
-	priv->clk = devm_clk_get_optional(&pdev->dev, "sw_switch");
+	priv->clk = devm_clk_get_optional_enabled(&pdev->dev, "sw_switch");
 	if (IS_ERR(priv->clk))
 		return PTR_ERR(priv->clk);
 
-	ret = clk_prepare_enable(priv->clk);
-	if (ret)
-		return ret;
-
-	priv->clk_mdiv = devm_clk_get_optional(&pdev->dev, "sw_switch_mdiv");
-	if (IS_ERR(priv->clk_mdiv)) {
-		ret = PTR_ERR(priv->clk_mdiv);
-		goto out_clk;
-	}
-
-	ret = clk_prepare_enable(priv->clk_mdiv);
-	if (ret)
-		goto out_clk;
+	priv->clk_mdiv = devm_clk_get_optional_enabled(&pdev->dev, "sw_switch_mdiv");
+	if (IS_ERR(priv->clk_mdiv))
+		return PTR_ERR(priv->clk_mdiv);
 
 	ret = bcm_sf2_sw_rst(priv);
 	if (ret) {
 		pr_err("unable to software reset switch: %d\n", ret);
-		goto out_clk_mdiv;
+		return ret;
 	}
 
 	bcm_sf2_crossbar_setup(priv);
@@ -1484,7 +1474,7 @@ static int bcm_sf2_sw_probe(struct platform_device *pdev)
 	ret = bcm_sf2_mdio_register(ds);
 	if (ret) {
 		pr_err("failed to register MDIO bus\n");
-		goto out_clk_mdiv;
+		return ret;
 	}
 
 	bcm_sf2_gphy_enable_set(priv->dev->ds, false);
@@ -1551,10 +1541,6 @@ static int bcm_sf2_sw_probe(struct platform_device *pdev)
 
 out_mdio:
 	bcm_sf2_mdio_unregister(priv);
-out_clk_mdiv:
-	clk_disable_unprepare(priv->clk_mdiv);
-out_clk:
-	clk_disable_unprepare(priv->clk);
 	return ret;
 }
 
@@ -1571,8 +1557,6 @@ static void bcm_sf2_sw_remove(struct platform_device *pdev)
 	dsa_unregister_switch(priv->dev->ds);
 	bcm_sf2_cfp_exit(priv->dev->ds);
 	bcm_sf2_mdio_unregister(priv);
-	clk_disable_unprepare(priv->clk_mdiv);
-	clk_disable_unprepare(priv->clk);
 	if (priv->type == BCM7278_DEVICE_ID)
 		reset_control_assert(priv->rcdev);
 }
-- 
2.34.1

Re: [PATCH net-next 01/12] net: dsa: bcm_sf2: Convert using devm_clk_get_optional_enabled() in bcm_sf2_sw_probe()

[Florian Fainelli]

On 8/30/2024 7:13 PM, Li Zetao wrote:
> Use devm_clk_get_optional_enabled() instead of devm_clk_get_optional() +
> 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 out_clk_mdiv and out_clk labels, and the original error process can
> be returned directly.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>

Acked-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

[PATCH net-next 02/12] net: ethernet: Convert using devm_clk_get_enabled() in emac_probe()

[Li Zetao]

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 out_clk_disable_unprepare label, and the original error process can
changed to the out_dispose_mapping error path.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/net/ethernet/allwinner/sun4i-emac.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.c b/drivers/net/ethernet/allwinner/sun4i-emac.c
index d761c08fe5c1..8f42501729b7 100644
--- a/drivers/net/ethernet/allwinner/sun4i-emac.c
+++ b/drivers/net/ethernet/allwinner/sun4i-emac.c
@@ -1005,22 +1005,16 @@ static int emac_probe(struct platform_device *pdev)
 	if (emac_configure_dma(db))
 		netdev_info(ndev, "configure dma failed. disable dma.\n");
 
-	db->clk = devm_clk_get(&pdev->dev, NULL);
+	db->clk = devm_clk_get_enabled(&pdev->dev, NULL);
 	if (IS_ERR(db->clk)) {
 		ret = PTR_ERR(db->clk);
 		goto out_dispose_mapping;
 	}
 
-	ret = clk_prepare_enable(db->clk);
-	if (ret) {
-		dev_err(&pdev->dev, "Error couldn't enable clock (%d)\n", ret);
-		goto out_dispose_mapping;
-	}
-
 	ret = sunxi_sram_claim(&pdev->dev);
 	if (ret) {
 		dev_err(&pdev->dev, "Error couldn't map SRAM to device\n");
-		goto out_clk_disable_unprepare;
+		goto out_dispose_mapping;
 	}
 
 	db->phy_node = of_parse_phandle(np, "phy-handle", 0);
@@ -1068,8 +1062,6 @@ static int emac_probe(struct platform_device *pdev)
 
 out_release_sram:
 	sunxi_sram_release(&pdev->dev);
-out_clk_disable_unprepare:
-	clk_disable_unprepare(db->clk);
 out_dispose_mapping:
 	irq_dispose_mapping(ndev->irq);
 	dma_release_channel(db->rx_chan);
@@ -1095,7 +1087,6 @@ static void emac_remove(struct platform_device *pdev)
 
 	unregister_netdev(ndev);
 	sunxi_sram_release(&pdev->dev);
-	clk_disable_unprepare(db->clk);
 	irq_dispose_mapping(ndev->irq);
 	iounmap(db->membase);
 	free_netdev(ndev);
-- 
2.34.1

[PATCH net-next 03/12] net: ethernet: arc: Convert using devm_clk_get_enabled() in emac_probe()

[Li Zetao]

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 out_clk_disable_unprepare label, and the original error process can
changed to the out_dispose_mapping error path.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/net/ethernet/arc/emac_rockchip.c | 34 +++++-------------------
 1 file changed, 6 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/arc/emac_rockchip.c b/drivers/net/ethernet/arc/emac_rockchip.c
index 493d6356c8ca..22b3ebe059d9 100644
--- a/drivers/net/ethernet/arc/emac_rockchip.c
+++ b/drivers/net/ethernet/arc/emac_rockchip.c
@@ -144,7 +144,7 @@ static int emac_rockchip_probe(struct platform_device *pdev)
 		goto out_netdev;
 	}
 
-	priv->refclk = devm_clk_get(dev, "macref");
+	priv->refclk = devm_clk_get_enabled(dev, "macref");
 	if (IS_ERR(priv->refclk)) {
 		dev_err(dev, "failed to retrieve reference clock (%ld)\n",
 			PTR_ERR(priv->refclk));
@@ -152,18 +152,12 @@ static int emac_rockchip_probe(struct platform_device *pdev)
 		goto out_netdev;
 	}
 
-	err = clk_prepare_enable(priv->refclk);
-	if (err) {
-		dev_err(dev, "failed to enable reference clock (%d)\n", err);
-		goto out_netdev;
-	}
-
 	/* Optional regulator for PHY */
 	priv->regulator = devm_regulator_get_optional(dev, "phy");
 	if (IS_ERR(priv->regulator)) {
 		if (PTR_ERR(priv->regulator) == -EPROBE_DEFER) {
 			err = -EPROBE_DEFER;
-			goto out_clk_disable;
+			goto out_netdev;
 		}
 		dev_err(dev, "no regulator found\n");
 		priv->regulator = NULL;
@@ -173,7 +167,7 @@ static int emac_rockchip_probe(struct platform_device *pdev)
 		err = regulator_enable(priv->regulator);
 		if (err) {
 			dev_err(dev, "failed to enable phy-supply (%d)\n", err);
-			goto out_clk_disable;
+			goto out_netdev;
 		}
 	}
 
@@ -200,7 +194,7 @@ static int emac_rockchip_probe(struct platform_device *pdev)
 	}
 
 	if (priv->soc_data->need_div_macclk) {
-		priv->macclk = devm_clk_get(dev, "macclk");
+		priv->macclk = devm_clk_get_enabled(dev, "macclk");
 		if (IS_ERR(priv->macclk)) {
 			dev_err(dev, "failed to retrieve mac clock (%ld)\n",
 				PTR_ERR(priv->macclk));
@@ -208,37 +202,26 @@ static int emac_rockchip_probe(struct platform_device *pdev)
 			goto out_regulator_disable;
 		}
 
-		err = clk_prepare_enable(priv->macclk);
-		if (err) {
-			dev_err(dev, "failed to enable mac clock (%d)\n", err);
-			goto out_regulator_disable;
-		}
-
 		/* RMII TX/RX needs always a rate of 25MHz */
 		err = clk_set_rate(priv->macclk, 25000000);
 		if (err) {
 			dev_err(dev,
 				"failed to change mac clock rate (%d)\n", err);
-			goto out_clk_disable_macclk;
+			goto out_regulator_disable;
 		}
 	}
 
 	err = arc_emac_probe(ndev, interface);
 	if (err) {
 		dev_err(dev, "failed to probe arc emac (%d)\n", err);
-		goto out_clk_disable_macclk;
+		goto out_regulator_disable;
 	}
 
 	return 0;
 
-out_clk_disable_macclk:
-	if (priv->soc_data->need_div_macclk)
-		clk_disable_unprepare(priv->macclk);
 out_regulator_disable:
 	if (priv->regulator)
 		regulator_disable(priv->regulator);
-out_clk_disable:
-	clk_disable_unprepare(priv->refclk);
 out_netdev:
 	free_netdev(ndev);
 	return err;
@@ -251,14 +234,9 @@ static void emac_rockchip_remove(struct platform_device *pdev)
 
 	arc_emac_remove(ndev);
 
-	clk_disable_unprepare(priv->refclk);
-
 	if (priv->regulator)
 		regulator_disable(priv->regulator);
 
-	if (priv->soc_data->need_div_macclk)
-		clk_disable_unprepare(priv->macclk);
-
 	free_netdev(ndev);
 }
 
-- 
2.34.1

[PATCH net-next 04/12] net: ethernet: ethoc: Convert using devm_clk_get_enabled() in ethoc_probe()

[Li Zetao]

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 free2 label, and the meaning of the free3 label is not clear, Changing
it to free_mdiobus will make it more understandable.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/net/ethernet/ethoc.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
index ad41c9019018..1a56e20cb679 100644
--- a/drivers/net/ethernet/ethoc.c
+++ b/drivers/net/ethernet/ethoc.c
@@ -1172,13 +1172,10 @@ static int ethoc_probe(struct platform_device *pdev)
 
 	/* Allow the platform setup code to adjust MII management bus clock. */
 	if (!eth_clkfreq) {
-		struct clk *clk = devm_clk_get(&pdev->dev, NULL);
+		priv->clk = devm_clk_get_enabled(&pdev->dev, NULL);
 
-		if (!IS_ERR(clk)) {
-			priv->clk = clk;
-			clk_prepare_enable(clk);
-			eth_clkfreq = clk_get_rate(clk);
-		}
+		if (!IS_ERR(priv->clk))
+			eth_clkfreq = clk_get_rate(priv->clk);
 	}
 	if (eth_clkfreq) {
 		u32 clkdiv = MIIMODER_CLKDIV(eth_clkfreq / 2500000 + 1);
@@ -1195,7 +1192,7 @@ static int ethoc_probe(struct platform_device *pdev)
 	priv->mdio = mdiobus_alloc();
 	if (!priv->mdio) {
 		ret = -ENOMEM;
-		goto free2;
+		goto free;
 	}
 
 	priv->mdio->name = "ethoc-mdio";
@@ -1208,7 +1205,7 @@ static int ethoc_probe(struct platform_device *pdev)
 	ret = mdiobus_register(priv->mdio);
 	if (ret) {
 		dev_err(&netdev->dev, "failed to register MDIO bus\n");
-		goto free3;
+		goto free_mdiobus;
 	}
 
 	ret = ethoc_mdio_probe(netdev);
@@ -1240,10 +1237,8 @@ static int ethoc_probe(struct platform_device *pdev)
 	netif_napi_del(&priv->napi);
 error:
 	mdiobus_unregister(priv->mdio);
-free3:
+free_mdiobus:
 	mdiobus_free(priv->mdio);
-free2:
-	clk_disable_unprepare(priv->clk);
 free:
 	free_netdev(netdev);
 out:
@@ -1267,7 +1262,6 @@ static void ethoc_remove(struct platform_device *pdev)
 			mdiobus_unregister(priv->mdio);
 			mdiobus_free(priv->mdio);
 		}
-		clk_disable_unprepare(priv->clk);
 		unregister_netdev(netdev);
 		free_netdev(netdev);
 	}
-- 
2.34.1

[PATCH net-next 05/12] net: ftgmac100: Convert using devm_clk_get_enabled() in ftgmac100_setup_clk()

[Li Zetao]

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;
 }
 
 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.
 	 */
-- 
2.34.1

Re: [PATCH net-next 05/12] net: ftgmac100: Convert using devm_clk_get_enabled() in ftgmac100_setup_clk()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3821 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net-next 05/12] net: ftgmac100: Convert using devm_clk_get_enabled() in ftgmac100_setup_clk()

[Li Zetao]

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.

Re: [PATCH net-next 05/12] net: ftgmac100: Convert using devm_clk_get_enabled() in ftgmac100_setup_clk()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3307 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH net-next 06/12] net: ethernet: hisilicon: Convert using devm_clk_get_enabled() in hisi_femac_drv_probe()

[Li Zetao]

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 out_disable_clk label, and the original error process can change to
the out_free_netdev error path.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/net/ethernet/hisilicon/hisi_femac.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hisi_femac.c b/drivers/net/ethernet/hisilicon/hisi_femac.c
index 2406263c9dd3..ffb6f4751d8b 100644
--- a/drivers/net/ethernet/hisilicon/hisi_femac.c
+++ b/drivers/net/ethernet/hisilicon/hisi_femac.c
@@ -797,23 +797,17 @@ static int hisi_femac_drv_probe(struct platform_device *pdev)
 		goto out_free_netdev;
 	}
 
-	priv->clk = devm_clk_get(&pdev->dev, NULL);
+	priv->clk = devm_clk_get_enabled(&pdev->dev, NULL);
 	if (IS_ERR(priv->clk)) {
 		dev_err(dev, "failed to get clk\n");
 		ret = -ENODEV;
 		goto out_free_netdev;
 	}
 
-	ret = clk_prepare_enable(priv->clk);
-	if (ret) {
-		dev_err(dev, "failed to enable clk %d\n", ret);
-		goto out_free_netdev;
-	}
-
 	priv->mac_rst = devm_reset_control_get(dev, "mac");
 	if (IS_ERR(priv->mac_rst)) {
 		ret = PTR_ERR(priv->mac_rst);
-		goto out_disable_clk;
+		goto out_free_netdev;
 	}
 	hisi_femac_core_reset(priv);
 
@@ -826,7 +820,7 @@ static int hisi_femac_drv_probe(struct platform_device *pdev)
 						 priv->phy_reset_delays,
 						 DELAYS_NUM);
 		if (ret)
-			goto out_disable_clk;
+			goto out_free_netdev;
 		hisi_femac_phy_reset(priv);
 	}
 
@@ -834,7 +828,7 @@ static int hisi_femac_drv_probe(struct platform_device *pdev)
 	if (!phy) {
 		dev_err(dev, "connect to PHY failed!\n");
 		ret = -ENODEV;
-		goto out_disable_clk;
+		goto out_free_netdev;
 	}
 
 	phy_attached_print(phy, "phy_id=0x%.8lx, phy_mode=%s\n",
@@ -885,8 +879,6 @@ static int hisi_femac_drv_probe(struct platform_device *pdev)
 out_disconnect_phy:
 	netif_napi_del(&priv->napi);
 	phy_disconnect(phy);
-out_disable_clk:
-	clk_disable_unprepare(priv->clk);
 out_free_netdev:
 	free_netdev(ndev);
 
@@ -902,7 +894,6 @@ static void hisi_femac_drv_remove(struct platform_device *pdev)
 	unregister_netdev(ndev);
 
 	phy_disconnect(ndev->phydev);
-	clk_disable_unprepare(priv->clk);
 	free_netdev(ndev);
 }
 
-- 
2.34.1

[PATCH net-next 07/12] net: lantiq_xrx200: Convert using devm_clk_get_enabled() in xrx200_probe()

[Li Zetao]

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 err_unprepare_clk label, and the original error process can change to
the err_uninit_dma error path. Some comments have also been adjusted.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/net/ethernet/lantiq_xrx200.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/lantiq_xrx200.c b/drivers/net/ethernet/lantiq_xrx200.c
index 07904a528f21..976748551643 100644
--- a/drivers/net/ethernet/lantiq_xrx200.c
+++ b/drivers/net/ethernet/lantiq_xrx200.c
@@ -589,8 +589,8 @@ static int xrx200_probe(struct platform_device *pdev)
 	if (priv->chan_tx.dma.irq < 0)
 		return -ENOENT;
 
-	/* get the clock */
-	priv->clk = devm_clk_get(dev, NULL);
+	/* get the clock and enable clock gate */
+	priv->clk = devm_clk_get_enabled(dev, NULL);
 	if (IS_ERR(priv->clk)) {
 		dev_err(dev, "failed to get clock\n");
 		return PTR_ERR(priv->clk);
@@ -605,11 +605,6 @@ static int xrx200_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
-	/* enable clock gate */
-	err = clk_prepare_enable(priv->clk);
-	if (err)
-		goto err_uninit_dma;
-
 	/* set IPG to 12 */
 	xrx200_pmac_mask(priv, PMAC_RX_IPG_MASK, 0xb, PMAC_RX_IPG);
 
@@ -628,13 +623,10 @@ static int xrx200_probe(struct platform_device *pdev)
 
 	err = register_netdev(net_dev);
 	if (err)
-		goto err_unprepare_clk;
+		goto err_uninit_dma;
 
 	return 0;
 
-err_unprepare_clk:
-	clk_disable_unprepare(priv->clk);
-
 err_uninit_dma:
 	xrx200_hw_cleanup(priv);
 
@@ -654,9 +646,6 @@ static void xrx200_remove(struct platform_device *pdev)
 	/* remove the actual device */
 	unregister_netdev(net_dev);
 
-	/* release the clock */
-	clk_disable_unprepare(priv->clk);
-
 	/* shut down hardware */
 	xrx200_hw_cleanup(priv);
 }
-- 
2.34.1

[PATCH net-next 08/12] net: stmmac: dwmac-dwc-qos-eth: Convert using devm_clk_get_enabled() in dwc_qos_probe()

[Li Zetao]

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 disable label, and the original error process can return directly.

The tegra_eqos_probe() also has similar modifications.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 .../stmicro/stmmac/dwmac-dwc-qos-eth.c        | 98 ++++---------------
 1 file changed, 17 insertions(+), 81 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
index ec924c6c76c6..d6e9a93771f4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
@@ -123,49 +123,24 @@ static int dwc_qos_probe(struct platform_device *pdev,
 			 struct plat_stmmacenet_data *plat_dat,
 			 struct stmmac_resources *stmmac_res)
 {
-	int err;
-
-	plat_dat->stmmac_clk = devm_clk_get(&pdev->dev, "apb_pclk");
+	plat_dat->stmmac_clk = devm_clk_get_enabled(&pdev->dev, "apb_pclk");
 	if (IS_ERR(plat_dat->stmmac_clk)) {
 		dev_err(&pdev->dev, "apb_pclk clock not found.\n");
 		return PTR_ERR(plat_dat->stmmac_clk);
 	}
 
-	err = clk_prepare_enable(plat_dat->stmmac_clk);
-	if (err < 0) {
-		dev_err(&pdev->dev, "failed to enable apb_pclk clock: %d\n",
-			err);
-		return err;
-	}
-
-	plat_dat->pclk = devm_clk_get(&pdev->dev, "phy_ref_clk");
+	plat_dat->pclk = devm_clk_get_enabled(&pdev->dev, "phy_ref_clk");
 	if (IS_ERR(plat_dat->pclk)) {
 		dev_err(&pdev->dev, "phy_ref_clk clock not found.\n");
-		err = PTR_ERR(plat_dat->pclk);
-		goto disable;
-	}
-
-	err = clk_prepare_enable(plat_dat->pclk);
-	if (err < 0) {
-		dev_err(&pdev->dev, "failed to enable phy_ref clock: %d\n",
-			err);
-		goto disable;
+		return PTR_ERR(plat_dat->pclk);
 	}
 
 	return 0;
-
-disable:
-	clk_disable_unprepare(plat_dat->stmmac_clk);
-	return err;
 }
 
 static void dwc_qos_remove(struct platform_device *pdev)
 {
-	struct net_device *ndev = platform_get_drvdata(pdev);
-	struct stmmac_priv *priv = netdev_priv(ndev);
 
-	clk_disable_unprepare(priv->plat->pclk);
-	clk_disable_unprepare(priv->plat->stmmac_clk);
 }
 
 #define SDMEMCOMPPADCTRL 0x8800
@@ -283,53 +258,27 @@ static int tegra_eqos_probe(struct platform_device *pdev,
 	if (!is_of_node(dev->fwnode))
 		goto bypass_clk_reset_gpio;
 
-	eqos->clk_master = devm_clk_get(&pdev->dev, "master_bus");
-	if (IS_ERR(eqos->clk_master)) {
-		err = PTR_ERR(eqos->clk_master);
-		goto error;
-	}
+	eqos->clk_master = devm_clk_get_enabled(&pdev->dev, "master_bus");
+	if (IS_ERR(eqos->clk_master))
+		return PTR_ERR(eqos->clk_master);
 
-	err = clk_prepare_enable(eqos->clk_master);
-	if (err < 0)
-		goto error;
-
-	eqos->clk_slave = devm_clk_get(&pdev->dev, "slave_bus");
-	if (IS_ERR(eqos->clk_slave)) {
-		err = PTR_ERR(eqos->clk_slave);
-		goto disable_master;
-	}
+	eqos->clk_slave = devm_clk_get_enabled(&pdev->dev, "slave_bus");
+	if (IS_ERR(eqos->clk_slave))
+		return PTR_ERR(eqos->clk_slave);
 
 	data->stmmac_clk = eqos->clk_slave;
 
-	err = clk_prepare_enable(eqos->clk_slave);
-	if (err < 0)
-		goto disable_master;
-
-	eqos->clk_rx = devm_clk_get(&pdev->dev, "rx");
-	if (IS_ERR(eqos->clk_rx)) {
-		err = PTR_ERR(eqos->clk_rx);
-		goto disable_slave;
-	}
-
-	err = clk_prepare_enable(eqos->clk_rx);
-	if (err < 0)
-		goto disable_slave;
+	eqos->clk_rx = devm_clk_get_enabled(&pdev->dev, "rx");
+	if (IS_ERR(eqos->clk_rx))
+		return PTR_ERR(eqos->clk_rx);
 
-	eqos->clk_tx = devm_clk_get(&pdev->dev, "tx");
-	if (IS_ERR(eqos->clk_tx)) {
-		err = PTR_ERR(eqos->clk_tx);
-		goto disable_rx;
-	}
-
-	err = clk_prepare_enable(eqos->clk_tx);
-	if (err < 0)
-		goto disable_rx;
+	eqos->clk_tx = devm_clk_get_enabled(&pdev->dev, "tx");
+	if (IS_ERR(eqos->clk_tx))
+		return PTR_ERR(eqos->clk_tx);
 
 	eqos->reset = devm_gpiod_get(&pdev->dev, "phy-reset", GPIOD_OUT_HIGH);
-	if (IS_ERR(eqos->reset)) {
-		err = PTR_ERR(eqos->reset);
-		goto disable_tx;
-	}
+	if (IS_ERR(eqos->reset))
+		return PTR_ERR(eqos->reset);
 
 	usleep_range(2000, 4000);
 	gpiod_set_value(eqos->reset, 0);
@@ -370,15 +319,6 @@ static int tegra_eqos_probe(struct platform_device *pdev,
 	reset_control_assert(eqos->rst);
 reset_phy:
 	gpiod_set_value(eqos->reset, 1);
-disable_tx:
-	clk_disable_unprepare(eqos->clk_tx);
-disable_rx:
-	clk_disable_unprepare(eqos->clk_rx);
-disable_slave:
-	clk_disable_unprepare(eqos->clk_slave);
-disable_master:
-	clk_disable_unprepare(eqos->clk_master);
-error:
 	return err;
 }
 
@@ -388,10 +328,6 @@ static void tegra_eqos_remove(struct platform_device *pdev)
 
 	reset_control_assert(eqos->rst);
 	gpiod_set_value(eqos->reset, 1);
-	clk_disable_unprepare(eqos->clk_tx);
-	clk_disable_unprepare(eqos->clk_rx);
-	clk_disable_unprepare(eqos->clk_slave);
-	clk_disable_unprepare(eqos->clk_master);
 }
 
 struct dwc_eth_dwmac_data {
-- 
2.34.1

[PATCH net-next 09/12] net: ethernet: sunplus: Convert using devm_clk_get_enabled() in spl2sw_probe()

[Li Zetao]

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 out_clk_disable label, and the original error process can return
directly. Some comments have also been adjusted.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/net/ethernet/sunplus/spl2sw_driver.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/sunplus/spl2sw_driver.c b/drivers/net/ethernet/sunplus/spl2sw_driver.c
index 391a1bc7f446..5944b96900b2 100644
--- a/drivers/net/ethernet/sunplus/spl2sw_driver.c
+++ b/drivers/net/ethernet/sunplus/spl2sw_driver.c
@@ -355,8 +355,8 @@ static int spl2sw_probe(struct platform_device *pdev)
 		return ret;
 	irq = ret;
 
-	/* Get clock controller. */
-	comm->clk = devm_clk_get(&pdev->dev, NULL);
+	/* Get and enable clock controller. */
+	comm->clk = devm_clk_get_enabled(&pdev->dev, NULL);
 	if (IS_ERR(comm->clk)) {
 		dev_err_probe(&pdev->dev, PTR_ERR(comm->clk),
 			      "Failed to retrieve clock controller!\n");
@@ -371,10 +371,6 @@ static int spl2sw_probe(struct platform_device *pdev)
 		return PTR_ERR(comm->rstc);
 	}
 
-	/* Enable clock. */
-	ret = clk_prepare_enable(comm->clk);
-	if (ret)
-		return ret;
 	udelay(1);
 
 	/* Reset MAC */
@@ -388,7 +384,7 @@ static int spl2sw_probe(struct platform_device *pdev)
 			       dev_name(&pdev->dev), comm);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to request irq #%d!\n", irq);
-		goto out_clk_disable;
+		return ret;
 	}
 
 	/* Initialize TX and RX descriptors. */
@@ -396,7 +392,7 @@ static int spl2sw_probe(struct platform_device *pdev)
 	if (ret) {
 		dev_err(&pdev->dev, "Fail to initialize mac descriptors!\n");
 		spl2sw_descs_free(comm);
-		goto out_clk_disable;
+		return ret;
 	}
 
 	/* Initialize MAC. */
@@ -406,7 +402,7 @@ static int spl2sw_probe(struct platform_device *pdev)
 	ret = spl2sw_mdio_init(comm);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to initialize mdio bus!\n");
-		goto out_clk_disable;
+		return ret;
 	}
 
 	/* Get child node ethernet-ports. */
@@ -506,8 +502,6 @@ static int spl2sw_probe(struct platform_device *pdev)
 out_free_mdio:
 	spl2sw_mdio_remove(comm);
 
-out_clk_disable:
-	clk_disable_unprepare(comm->clk);
 	return ret;
 }
 
@@ -536,8 +530,6 @@ static void spl2sw_remove(struct platform_device *pdev)
 	netif_napi_del(&comm->tx_napi);
 
 	spl2sw_mdio_remove(comm);
-
-	clk_disable_unprepare(comm->clk);
 }
 
 static const struct of_device_id spl2sw_of_match[] = {
-- 
2.34.1

Re: [PATCH net-next 09/12] net: ethernet: sunplus: Convert using devm_clk_get_enabled() in spl2sw_probe()

[Jakub Kicinski]

On Sat, 31 Aug 2024 10:13:31 +0800 Li Zetao wrote:
> +	comm->clk = devm_clk_get_enabled(&pdev->dev, NULL);

You can remove clk from the driver struct now.
Please check if the same applies to other patches.
-- 
pw-bot: cr

Re: [PATCH net-next 09/12] net: ethernet: sunplus: Convert using devm_clk_get_enabled() in spl2sw_probe()

[Li Zetao]

Hi,

在 2024/9/4 6:16, Jakub Kicinski 写道:
> On Sat, 31 Aug 2024 10:13:31 +0800 Li Zetao wrote:
>> +	comm->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> 
> You can remove clk from the driver struct now.
I don’t quite understand why clk can be removed from the driver struct, 
maybe I missed some important discussion information in the community, 
please let me know, thank you.
> Please check if the same applies to other patches.

Thanks,
Li Zetao.

Re: [PATCH net-next 09/12] net: ethernet: sunplus: Convert using devm_clk_get_enabled() in spl2sw_probe()

[Andrew Lunn]

On Wed, Sep 04, 2024 at 09:27:06AM +0800, Li Zetao wrote:
> Hi,
> 
> 在 2024/9/4 6:16, Jakub Kicinski 写道:
> > On Sat, 31 Aug 2024 10:13:31 +0800 Li Zetao wrote:
> > > +	comm->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> > 
> > You can remove clk from the driver struct now.
> I don’t quite understand why clk can be removed from the driver struct,

After you patch, where is it used?

	Andrew

[PATCH net-next 10/12] net: xilinx: axienet: Convert using devm_clk_get_optional_enabled() in axienet_probe()

[Li Zetao]

Use devm_clk_get_optional_enabled() instead of devm_clk_get_optional() +
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.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index fe6a0e2e463f..48b41e95aa74 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -2584,22 +2584,17 @@ static int axienet_probe(struct platform_device *pdev)
 	seqcount_mutex_init(&lp->hw_stats_seqcount, &lp->stats_lock);
 	INIT_DEFERRABLE_WORK(&lp->stats_work, axienet_refresh_stats);
 
-	lp->axi_clk = devm_clk_get_optional(&pdev->dev, "s_axi_lite_clk");
-	if (!lp->axi_clk) {
+	lp->axi_clk = devm_clk_get_optional_enabled(&pdev->dev, "s_axi_lite_clk");
+	if (!lp->axi_clk)
 		/* For backward compatibility, if named AXI clock is not present,
 		 * treat the first clock specified as the AXI clock.
 		 */
-		lp->axi_clk = devm_clk_get_optional(&pdev->dev, NULL);
-	}
+		lp->axi_clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
+
 	if (IS_ERR(lp->axi_clk)) {
 		ret = PTR_ERR(lp->axi_clk);
 		goto free_netdev;
 	}
-	ret = clk_prepare_enable(lp->axi_clk);
-	if (ret) {
-		dev_err(&pdev->dev, "Unable to enable AXI clock: %d\n", ret);
-		goto free_netdev;
-	}
 
 	lp->misc_clks[0].id = "axis_clk";
 	lp->misc_clks[1].id = "ref_clk";
@@ -2915,7 +2910,6 @@ static int axienet_probe(struct platform_device *pdev)
 		axienet_mdio_teardown(lp);
 cleanup_clk:
 	clk_bulk_disable_unprepare(XAE_NUM_MISC_CLOCKS, lp->misc_clks);
-	clk_disable_unprepare(lp->axi_clk);
 
 free_netdev:
 	free_netdev(ndev);
@@ -2939,7 +2933,6 @@ static void axienet_remove(struct platform_device *pdev)
 	axienet_mdio_teardown(lp);
 
 	clk_bulk_disable_unprepare(XAE_NUM_MISC_CLOCKS, lp->misc_clks);
-	clk_disable_unprepare(lp->axi_clk);
 
 	free_netdev(ndev);
 }
-- 
2.34.1

RE: [PATCH net-next 10/12] net: xilinx: axienet: Convert using devm_clk_get_optional_enabled() in axienet_probe()

[Pandey, Radhey Shyam]

> -----Original Message-----
> From: Li Zetao <lizetao1@huawei.com>
> Sent: Saturday, August 31, 2024 7:44 AM
> To: florian.fainelli@broadcom.com; andrew@lunn.ch; olteanv@gmail.com;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; wens@csie.org; jernej.skrabec@gmail.com;
> samuel@sholland.org; heiko@sntech.de; yisen.zhuang@huawei.com;
> salil.mehta@huawei.com; hauke@hauke-m.de;
> alexandre.torgue@foss.st.com; joabreu@synopsys.com;
> mcoquelin.stm32@gmail.com; wellslutw@gmail.com; Pandey, Radhey
> Shyam <radhey.shyam.pandey@amd.com>; Simek, Michal
> <michal.simek@amd.com>; ajay.kathat@microchip.com;
> claudiu.beznea@tuxon.dev; kvalo@kernel.org; lizetao1@huawei.com;
> u.kleine-koenig@pengutronix.de; jacky_chou@aspeedtech.com
> Cc: netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> sunxi@lists.linux.dev; linux-rockchip@lists.infradead.org; linux-stm32@st-
> md-mailman.stormreply.com; linux-wireless@vger.kernel.org
> Subject: [PATCH net-next 10/12] net: xilinx: axienet: Convert using
> devm_clk_get_optional_enabled() in axienet_probe()
> 
> Use devm_clk_get_optional_enabled() instead of devm_clk_get_optional() +
> 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.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>

Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
Thanks!

> ---
>  drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index fe6a0e2e463f..48b41e95aa74 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -2584,22 +2584,17 @@ static int axienet_probe(struct platform_device
> *pdev)
>  	seqcount_mutex_init(&lp->hw_stats_seqcount, &lp->stats_lock);
>  	INIT_DEFERRABLE_WORK(&lp->stats_work, axienet_refresh_stats);
> 
> -	lp->axi_clk = devm_clk_get_optional(&pdev->dev, "s_axi_lite_clk");
> -	if (!lp->axi_clk) {
> +	lp->axi_clk = devm_clk_get_optional_enabled(&pdev->dev,
> "s_axi_lite_clk");
> +	if (!lp->axi_clk)
>  		/* For backward compatibility, if named AXI clock is not
> present,
>  		 * treat the first clock specified as the AXI clock.
>  		 */
> -		lp->axi_clk = devm_clk_get_optional(&pdev->dev, NULL);
> -	}
> +		lp->axi_clk = devm_clk_get_optional_enabled(&pdev->dev,
> NULL);
> +
>  	if (IS_ERR(lp->axi_clk)) {
>  		ret = PTR_ERR(lp->axi_clk);
>  		goto free_netdev;
>  	}
> -	ret = clk_prepare_enable(lp->axi_clk);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Unable to enable AXI clock: %d\n",
> ret);
> -		goto free_netdev;
> -	}
> 
>  	lp->misc_clks[0].id = "axis_clk";
>  	lp->misc_clks[1].id = "ref_clk";
> @@ -2915,7 +2910,6 @@ static int axienet_probe(struct platform_device
> *pdev)
>  		axienet_mdio_teardown(lp);
>  cleanup_clk:

I also find that there is goto to cleanup_clk when devm_clk_bulk_get_optional/
clk_bulk_prepare_enable fails which is not correct but as it is existing bug it 
can go a separate patch.

>  	clk_bulk_disable_unprepare(XAE_NUM_MISC_CLOCKS, lp-
> >misc_clks);
> -	clk_disable_unprepare(lp->axi_clk);
> 
>  free_netdev:
>  	free_netdev(ndev);
> @@ -2939,7 +2933,6 @@ static void axienet_remove(struct platform_device
> *pdev)
>  	axienet_mdio_teardown(lp);
> 
>  	clk_bulk_disable_unprepare(XAE_NUM_MISC_CLOCKS, lp-
> >misc_clks);
> -	clk_disable_unprepare(lp->axi_clk);
> 
>  	free_netdev(ndev);
>  }
> --
> 2.34.1

Re: [PATCH net-next 10/12] net: xilinx: axienet: Convert using devm_clk_get_optional_enabled() in axienet_probe()

[Li Zetao]

在 2024/9/1 20:28, Pandey, Radhey Shyam 写道:
>> -----Original Message-----
>> From: Li Zetao <lizetao1@huawei.com>
>> Sent: Saturday, August 31, 2024 7:44 AM
>> To: florian.fainelli@broadcom.com; andrew@lunn.ch; olteanv@gmail.com;
>> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
>> pabeni@redhat.com; wens@csie.org; jernej.skrabec@gmail.com;
>> samuel@sholland.org; heiko@sntech.de; yisen.zhuang@huawei.com;
>> salil.mehta@huawei.com; hauke@hauke-m.de;
>> alexandre.torgue@foss.st.com; joabreu@synopsys.com;
>> mcoquelin.stm32@gmail.com; wellslutw@gmail.com; Pandey, Radhey
>> Shyam <radhey.shyam.pandey@amd.com>; Simek, Michal
>> <michal.simek@amd.com>; ajay.kathat@microchip.com;
>> claudiu.beznea@tuxon.dev; kvalo@kernel.org; lizetao1@huawei.com;
>> u.kleine-koenig@pengutronix.de; jacky_chou@aspeedtech.com
>> Cc: netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>> sunxi@lists.linux.dev; linux-rockchip@lists.infradead.org; linux-stm32@st-
>> md-mailman.stormreply.com; linux-wireless@vger.kernel.org
>> Subject: [PATCH net-next 10/12] net: xilinx: axienet: Convert using
>> devm_clk_get_optional_enabled() in axienet_probe()
>>
>> Use devm_clk_get_optional_enabled() instead of devm_clk_get_optional() +
>> 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.
>>
>> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> 
> Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> Thanks!
> 
>> ---
>>   drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 15 ++++-----------
>>   1 file changed, 4 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> index fe6a0e2e463f..48b41e95aa74 100644
>> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> @@ -2584,22 +2584,17 @@ static int axienet_probe(struct platform_device
>> *pdev)
>>   	seqcount_mutex_init(&lp->hw_stats_seqcount, &lp->stats_lock);
>>   	INIT_DEFERRABLE_WORK(&lp->stats_work, axienet_refresh_stats);
>>
>> -	lp->axi_clk = devm_clk_get_optional(&pdev->dev, "s_axi_lite_clk");
>> -	if (!lp->axi_clk) {
>> +	lp->axi_clk = devm_clk_get_optional_enabled(&pdev->dev,
>> "s_axi_lite_clk");
>> +	if (!lp->axi_clk)
>>   		/* For backward compatibility, if named AXI clock is not
>> present,
>>   		 * treat the first clock specified as the AXI clock.
>>   		 */
>> -		lp->axi_clk = devm_clk_get_optional(&pdev->dev, NULL);
>> -	}
>> +		lp->axi_clk = devm_clk_get_optional_enabled(&pdev->dev,
>> NULL);
>> +
>>   	if (IS_ERR(lp->axi_clk)) {
>>   		ret = PTR_ERR(lp->axi_clk);
>>   		goto free_netdev;
>>   	}
>> -	ret = clk_prepare_enable(lp->axi_clk);
>> -	if (ret) {
>> -		dev_err(&pdev->dev, "Unable to enable AXI clock: %d\n",
>> ret);
>> -		goto free_netdev;
>> -	}
>>
>>   	lp->misc_clks[0].id = "axis_clk";
>>   	lp->misc_clks[1].id = "ref_clk";
>> @@ -2915,7 +2910,6 @@ static int axienet_probe(struct platform_device
>> *pdev)
>>   		axienet_mdio_teardown(lp);
>>   cleanup_clk:
> 
> I also find that there is goto to cleanup_clk when devm_clk_bulk_get_optional/
> clk_bulk_prepare_enable fails which is not correct but as it is existing bug it
> can go a separate patch.
Thanks for the reminder, I considered solving this problem by using 
devm_add_action_or_reset

Thanks,
Li Zetao.
> 
>>   	clk_bulk_disable_unprepare(XAE_NUM_MISC_CLOCKS, lp-
>>> misc_clks);
>> -	clk_disable_unprepare(lp->axi_clk);
>>
>>   free_netdev:
>>   	free_netdev(ndev);
>> @@ -2939,7 +2933,6 @@ static void axienet_remove(struct platform_device
>> *pdev)
>>   	axienet_mdio_teardown(lp);
>>
>>   	clk_bulk_disable_unprepare(XAE_NUM_MISC_CLOCKS, lp-
>>> misc_clks);
>> -	clk_disable_unprepare(lp->axi_clk);
>>
>>   	free_netdev(ndev);
>>   }
>> --
>> 2.34.1
> 

[PATCH net-next 11/12] wifi: wilc1000: Convert using devm_clk_get_optional_enabled() in wilc_sdio_probe()

[Li Zetao]

Use devm_clk_get_optional_enabled() instead of devm_clk_get_optional() +
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 clk_disable_unprepare label, and the original error process can change
to dispose_irq.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/net/wireless/microchip/wilc1000/sdio.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c
index 0043f7a0fdf9..a09ea24074e2 100644
--- a/drivers/net/wireless/microchip/wilc1000/sdio.c
+++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
@@ -174,19 +174,18 @@ static int wilc_sdio_probe(struct sdio_func *func,
 	wilc->bus_data = sdio_priv;
 	wilc->dev = &func->dev;
 
-	wilc->rtc_clk = devm_clk_get_optional(&func->card->dev, "rtc");
+	wilc->rtc_clk = devm_clk_get_optional_enabled(&func->card->dev, "rtc");
 	if (IS_ERR(wilc->rtc_clk)) {
 		ret = PTR_ERR(wilc->rtc_clk);
 		goto dispose_irq;
 	}
-	clk_prepare_enable(wilc->rtc_clk);
 
 	wilc_sdio_init(wilc, false);
 
 	ret = wilc_load_mac_from_nv(wilc);
 	if (ret) {
 		pr_err("Can not retrieve MAC address from chip\n");
-		goto clk_disable_unprepare;
+		goto dispose_irq;
 	}
 
 	wilc_sdio_deinit(wilc);
@@ -195,14 +194,12 @@ static int wilc_sdio_probe(struct sdio_func *func,
 				   NL80211_IFTYPE_STATION, false);
 	if (IS_ERR(vif)) {
 		ret = PTR_ERR(vif);
-		goto clk_disable_unprepare;
+		goto dispose_irq;
 	}
 
 	dev_info(&func->dev, "Driver Initializing success\n");
 	return 0;
 
-clk_disable_unprepare:
-	clk_disable_unprepare(wilc->rtc_clk);
 dispose_irq:
 	irq_dispose_mapping(wilc->dev_irq_num);
 	wilc_netdev_cleanup(wilc);
@@ -217,7 +214,6 @@ static void wilc_sdio_remove(struct sdio_func *func)
 	struct wilc *wilc = sdio_get_drvdata(func);
 	struct wilc_sdio *sdio_priv = wilc->bus_data;
 
-	clk_disable_unprepare(wilc->rtc_clk);
 	wilc_netdev_cleanup(wilc);
 	kfree(sdio_priv->cmd53_buf);
 	kfree(sdio_priv);
-- 
2.34.1

Re: [PATCH net-next 11/12] wifi: wilc1000: Convert using devm_clk_get_optional_enabled() in wilc_sdio_probe()

[Kalle Valo]

Li Zetao <lizetao1@huawei.com> writes:

> Use devm_clk_get_optional_enabled() instead of devm_clk_get_optional() +
> 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 clk_disable_unprepare label, and the original error process can change
> to dispose_irq.
>
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>  drivers/net/wireless/microchip/wilc1000/sdio.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)

wifi patches (patches 11 and 12) go via wireless-next, please submit
those separately.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
https://docs.kernel.org/process/submitting-patches.html

Re: [PATCH net-next 11/12] wifi: wilc1000: Convert using devm_clk_get_optional_enabled() in wilc_sdio_probe()

[Li Zetao]

Hi,

在 2024/9/3 0:38, Kalle Valo 写道:
> Li Zetao <lizetao1@huawei.com> writes:
> 
>> Use devm_clk_get_optional_enabled() instead of devm_clk_get_optional() +
>> 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 clk_disable_unprepare label, and the original error process can change
>> to dispose_irq.
>>
>> Signed-off-by: Li Zetao <lizetao1@huawei.com>
>> ---
>>   drivers/net/wireless/microchip/wilc1000/sdio.c | 10 +++-------
>>   1 file changed, 3 insertions(+), 7 deletions(-)
> 
> wifi patches (patches 11 and 12) go via wireless-next, please submit
> those separately.
Ok, I will resend those separately.
> 

Thanks,
Li Zetao.

[PATCH net-next 12/12] wifi: wilc1000: Convert using devm_clk_get_optional_enabled() in wilc_bus_probe()

[Li Zetao]

Use devm_clk_get_optional_enabled() instead of devm_clk_get_optional() +
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.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/net/wireless/microchip/wilc1000/spi.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
index 5ff940c53ad9..05b577b1068e 100644
--- a/drivers/net/wireless/microchip/wilc1000/spi.c
+++ b/drivers/net/wireless/microchip/wilc1000/spi.c
@@ -228,12 +228,11 @@ static int wilc_bus_probe(struct spi_device *spi)
 	if (ret < 0)
 		goto netdev_cleanup;
 
-	wilc->rtc_clk = devm_clk_get_optional(&spi->dev, "rtc");
+	wilc->rtc_clk = devm_clk_get_optional_enabled(&spi->dev, "rtc");
 	if (IS_ERR(wilc->rtc_clk)) {
 		ret = PTR_ERR(wilc->rtc_clk);
 		goto netdev_cleanup;
 	}
-	clk_prepare_enable(wilc->rtc_clk);
 
 	dev_info(&spi->dev, "Selected CRC config: crc7=%s, crc16=%s\n",
 		 enable_crc7 ? "on" : "off", enable_crc16 ? "on" : "off");
@@ -266,7 +265,6 @@ static int wilc_bus_probe(struct spi_device *spi)
 	return 0;
 
 power_down:
-	clk_disable_unprepare(wilc->rtc_clk);
 	wilc_wlan_power(wilc, false);
 netdev_cleanup:
 	wilc_netdev_cleanup(wilc);
@@ -280,7 +278,6 @@ static void wilc_bus_remove(struct spi_device *spi)
 	struct wilc *wilc = spi_get_drvdata(spi);
 	struct wilc_spi *spi_priv = wilc->bus_data;
 
-	clk_disable_unprepare(wilc->rtc_clk);
 	wilc_netdev_cleanup(wilc);
 	kfree(spi_priv);
 }
-- 
2.34.1

Re: [PATCH net-next 00/12] net: Convert using devm_clk_get_enabled()/devm_clk_get_optional_enabled()

[Jeff Johnson]

On 8/30/2024 7:13 PM, Li Zetao wrote:
> There are many examples[1][2] of clk resource leakage in LTS. The
> reason is that developers need to maintain the allocation and release
> of clk resources themselves, but this will increase the burden on
> developers. Using the API related to devm_clk_get_*_enable ensures
> that the life cycle of clk is consistent with that of the device,
> reducing the risk of unreleased resources like clk.
> 
> Several other developers are also working on converting to more
> secure interfaces, and this patch set is in principle the same as
> theirs.

...

>  drivers/net/dsa/bcm_sf2.c                     | 28 ++----
>  drivers/net/ethernet/allwinner/sun4i-emac.c   | 13 +--
>  drivers/net/ethernet/arc/emac_rockchip.c      | 34 ++-----
>  drivers/net/ethernet/ethoc.c                  | 18 ++--
>  drivers/net/ethernet/faraday/ftgmac100.c      | 27 ++---
>  drivers/net/ethernet/hisilicon/hisi_femac.c   | 17 +---
>  drivers/net/ethernet/lantiq_xrx200.c          | 17 +---
>  .../stmicro/stmmac/dwmac-dwc-qos-eth.c        | 98 ++++---------------
>  drivers/net/ethernet/sunplus/spl2sw_driver.c  | 18 +---
>  .../net/ethernet/xilinx/xilinx_axienet_main.c | 15 +--
>  .../net/wireless/microchip/wilc1000/sdio.c    | 10 +-
>  drivers/net/wireless/microchip/wilc1000/spi.c |  5 +-

note the wifi driver changes go through the wireless tree and not the net tree
so those should be split out separately

Re: [PATCH net-next 00/12] net: Convert using devm_clk_get_enabled()/devm_clk_get_optional_enabled()

[Li Zetao]

Hi,

在 2024/9/7 7:17, Jeff Johnson 写道:
> On 8/30/2024 7:13 PM, Li Zetao wrote:
>> There are many examples[1][2] of clk resource leakage in LTS. The
>> reason is that developers need to maintain the allocation and release
>> of clk resources themselves, but this will increase the burden on
>> developers. Using the API related to devm_clk_get_*_enable ensures
>> that the life cycle of clk is consistent with that of the device,
>> reducing the risk of unreleased resources like clk.
>>
>> Several other developers are also working on converting to more
>> secure interfaces, and this patch set is in principle the same as
>> theirs.
> 
> ...
> 
>>   drivers/net/dsa/bcm_sf2.c                     | 28 ++----
>>   drivers/net/ethernet/allwinner/sun4i-emac.c   | 13 +--
>>   drivers/net/ethernet/arc/emac_rockchip.c      | 34 ++-----
>>   drivers/net/ethernet/ethoc.c                  | 18 ++--
>>   drivers/net/ethernet/faraday/ftgmac100.c      | 27 ++---
>>   drivers/net/ethernet/hisilicon/hisi_femac.c   | 17 +---
>>   drivers/net/ethernet/lantiq_xrx200.c          | 17 +---
>>   .../stmicro/stmmac/dwmac-dwc-qos-eth.c        | 98 ++++---------------
>>   drivers/net/ethernet/sunplus/spl2sw_driver.c  | 18 +---
>>   .../net/ethernet/xilinx/xilinx_axienet_main.c | 15 +--
>>   .../net/wireless/microchip/wilc1000/sdio.c    | 10 +-
>>   drivers/net/wireless/microchip/wilc1000/spi.c |  5 +-
> 
> note the wifi driver changes go through the wireless tree and not the net tree
> so those should be split out separately

I have separated the wifi related patches and sent them to the community:
https://lore.kernel.org/all/20240903110205.4127706-1-lizetao1@huawei.com/
> 
> 
Thanks,
Li Zetao.