[PATCH net v2 0/2] net: bcmasp: Fix issues during driver unbind

[PATCH net v2 0/2] net: bcmasp: Fix issues during driver unbind

[justin.chen]

From: Justin Chen <justin.chen@broadcom.com>

Fix two issues when we unbind the driver. We hit a double free of the
WoL irq and a double disable of the clk.

Justin Chen (2):
  net: bcmasp: fix double free of WoL irq
  net: bcmasp: fix double disable of clk

 drivers/net/ethernet/broadcom/asp2/bcmasp.c | 37 ++++++++++++---------
 1 file changed, 21 insertions(+), 16 deletions(-)

-- 
2.34.1

[PATCH net v2 1/2] net: bcmasp: fix double free of WoL irq

[justin.chen]

From: Justin Chen <justin.chen@broadcom.com>

We do not need to free wol_irq since it was instantiated with
devm_request_irq(). So devres will free for us.

Fixes: a2f0751206b0 ("net: bcmasp: Add support for WoL magic packet")
Signed-off-by: Justin Chen <justin.chen@broadcom.com>
---
v2
- Split into two patches.

 drivers/net/ethernet/broadcom/asp2/bcmasp.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
index aa6d8606849f..2034a1593db7 100644
--- a/drivers/net/ethernet/broadcom/asp2/bcmasp.c
+++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
@@ -1152,12 +1152,6 @@ void bcmasp_enable_wol(struct bcmasp_intf *intf, bool en)
 	}
 }
 
-static void bcmasp_wol_irq_destroy(struct bcmasp_priv *priv)
-{
-	if (priv->wol_irq > 0)
-		free_irq(priv->wol_irq, priv);
-}
-
 static void bcmasp_eee_fixup(struct bcmasp_intf *intf, bool en)
 {
 	u32 reg, phy_lpi_overwrite;
@@ -1363,7 +1357,6 @@ static int bcmasp_probe(struct platform_device *pdev)
 	return ret;
 
 err_cleanup:
-	bcmasp_wol_irq_destroy(priv);
 	bcmasp_remove_intfs(priv);
 
 	return ret;
@@ -1376,7 +1369,6 @@ static void bcmasp_remove(struct platform_device *pdev)
 	if (!priv)
 		return;
 
-	bcmasp_wol_irq_destroy(priv);
 	bcmasp_remove_intfs(priv);
 }
 
-- 
2.34.1

Re: [PATCH net v2 1/2] net: bcmasp: fix double free of WoL irq

[Florian Fainelli]

On 3/16/26 11:02, justin.chen@broadcom.com wrote:
> From: Justin Chen <justin.chen@broadcom.com>
> 
> We do not need to free wol_irq since it was instantiated with
> devm_request_irq(). So devres will free for us.
> 
> Fixes: a2f0751206b0 ("net: bcmasp: Add support for WoL magic packet")
> Signed-off-by: Justin Chen <justin.chen@broadcom.com>

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

[PATCH net v2 2/2] net: bcmasp: fix double disable of clk

[justin.chen]

From: Justin Chen <justin.chen@broadcom.com>

Switch to devm_clk_get_optional() so we can manage the clock ourselves.
We dynamically control the clocks depending on the state of the interface
for power savings. The default state is clock disabled, so unbinding the
driver causes a double disable.

Fixes: 490cb412007d ("net: bcmasp: Add support for ASP2.0 Ethernet controller")
Signed-off-by: Justin Chen <justin.chen@broadcom.com>
---
v2
- Split into two patches.
- Add error path to disable clock if we fail during probe.

 drivers/net/ethernet/broadcom/asp2/bcmasp.c | 29 +++++++++++++++------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
index 2034a1593db7..dda89afb2917 100644
--- a/drivers/net/ethernet/broadcom/asp2/bcmasp.c
+++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
@@ -1249,7 +1249,7 @@ static int bcmasp_probe(struct platform_device *pdev)
 	if (priv->irq <= 0)
 		return -EINVAL;
 
-	priv->clk = devm_clk_get_optional_enabled(dev, "sw_asp");
+	priv->clk = devm_clk_get_optional(dev, "sw_asp");
 	if (IS_ERR(priv->clk))
 		return dev_err_probe(dev, PTR_ERR(priv->clk),
 				     "failed to request clock\n");
@@ -1277,6 +1277,10 @@ static int bcmasp_probe(struct platform_device *pdev)
 
 	bcmasp_set_pdata(priv, pdata);
 
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to start clock\n");
+
 	/* Enable all clocks to ensure successful probing */
 	bcmasp_core_clock_set(priv, ASP_CTRL_CLOCK_CTRL_ASP_ALL_DISABLE, 0);
 
@@ -1288,8 +1292,10 @@ static int bcmasp_probe(struct platform_device *pdev)
 
 	ret = devm_request_irq(&pdev->dev, priv->irq, bcmasp_isr, 0,
 			       pdev->name, priv);
-	if (ret)
-		return dev_err_probe(dev, ret, "failed to request ASP interrupt: %d", ret);
+	if (ret) {
+		dev_err(dev, "Failed to request ASP interrupt: %d", ret);
+		goto err_clock_disable;
+	}
 
 	/* Register mdio child nodes */
 	of_platform_populate(dev->of_node, bcmasp_mdio_of_match, NULL, dev);
@@ -1301,13 +1307,17 @@ static int bcmasp_probe(struct platform_device *pdev)
 
 	priv->mda_filters = devm_kcalloc(dev, priv->num_mda_filters,
 					 sizeof(*priv->mda_filters), GFP_KERNEL);
-	if (!priv->mda_filters)
-		return -ENOMEM;
+	if (!priv->mda_filters) {
+		ret = -ENOMEM;
+		goto err_clock_disable;
+	}
 
 	priv->net_filters = devm_kcalloc(dev, priv->num_net_filters,
 					 sizeof(*priv->net_filters), GFP_KERNEL);
-	if (!priv->net_filters)
-		return -ENOMEM;
+	if (!priv->net_filters) {
+		ret = -ENOMEM;
+		goto err_clock_disable;
+	}
 
 	bcmasp_core_init_filters(priv);
 
@@ -1316,7 +1326,8 @@ static int bcmasp_probe(struct platform_device *pdev)
 	ports_node = of_find_node_by_name(dev->of_node, "ethernet-ports");
 	if (!ports_node) {
 		dev_warn(dev, "No ports found\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err_clock_disable;
 	}
 
 	i = 0;
@@ -1358,6 +1369,8 @@ static int bcmasp_probe(struct platform_device *pdev)
 
 err_cleanup:
 	bcmasp_remove_intfs(priv);
+err_clock_disable:
+	clk_disable_unprepare(priv->clk);
 
 	return ret;
 }
-- 
2.34.1

Re: [PATCH net v2 2/2] net: bcmasp: fix double disable of clk

[Florian Fainelli]

On 3/16/26 11:02, justin.chen@broadcom.com wrote:
> From: Justin Chen <justin.chen@broadcom.com>
> 
> Switch to devm_clk_get_optional() so we can manage the clock ourselves.
> We dynamically control the clocks depending on the state of the interface
> for power savings. The default state is clock disabled, so unbinding the
> driver causes a double disable.
> 
> Fixes: 490cb412007d ("net: bcmasp: Add support for ASP2.0 Ethernet controller")
> Signed-off-by: Justin Chen <justin.chen@broadcom.com>

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

Re: [PATCH net v2 2/2] net: bcmasp: fix double disable of clk

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: bcmasp: fix double disable of clk

Switch to devm_clk_get_optional() so we can manage the clock ourselves.
We dynamically control the clocks depending on the state of the interface
for power savings. The default state is clock disabled, so unbinding the
driver causes a double disable.

> diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
> index 2034a1593db7..dda89afb2917 100644
> --- a/drivers/net/ethernet/broadcom/asp2/bcmasp.c
> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
> @@ -1249,7 +1249,7 @@ static int bcmasp_probe(struct platform_device *pdev)
>  	if (priv->irq <= 0)
>  		return -EINVAL;
>
> -	priv->clk = devm_clk_get_optional_enabled(dev, "sw_asp");
> +	priv->clk = devm_clk_get_optional(dev, "sw_asp");
>  	if (IS_ERR(priv->clk))
>  		return dev_err_probe(dev, PTR_ERR(priv->clk),
>  				     "failed to request clock\n");
> @@ -1277,6 +1277,10 @@ static int bcmasp_probe(struct platform_device *pdev)
>
>  	bcmasp_set_pdata(priv, pdata);
>
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to start clock\n");
> +
>  	/* Enable all clocks to ensure successful probing */
>  	bcmasp_core_clock_set(priv, ASP_CTRL_CLOCK_CTRL_ASP_ALL_DISABLE, 0);
>
> @@ -1288,8 +1292,10 @@ static int bcmasp_probe(struct platform_device *pdev)
>
>  	ret = devm_request_irq(&pdev->dev, priv->irq, bcmasp_isr, 0,
>  			       pdev->name, priv);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "failed to request ASP interrupt: %d", ret);
> +	if (ret) {
> +		dev_err(dev, "Failed to request ASP interrupt: %d", ret);
> +		goto err_clock_disable;
> +	}
>
>  	/* Register mdio child nodes */
>  	of_platform_populate(dev->of_node, bcmasp_mdio_of_match, NULL, dev);
> @@ -1301,13 +1307,17 @@ static int bcmasp_probe(struct platform_device *pdev)
>
>  	priv->mda_filters = devm_kcalloc(dev, priv->num_mda_filters,
>  					 sizeof(*priv->mda_filters), GFP_KERNEL);
> -	if (!priv->mda_filters)
> -		return -ENOMEM;
> +	if (!priv->mda_filters) {
> +		ret = -ENOMEM;
> +		goto err_clock_disable;
> +	}
>
>  	priv->net_filters = devm_kcalloc(dev, priv->num_net_filters,
>  					 sizeof(*priv->net_filters), GFP_KERNEL);
> -	if (!priv->net_filters)
> -		return -ENOMEM;
> +	if (!priv->net_filters) {
> +		ret = -ENOMEM;
> +		goto err_clock_disable;
> +	}
>
>  	bcmasp_core_init_filters(priv);
>
> @@ -1316,7 +1326,8 @@ static int bcmasp_probe(struct platform_device *pdev)
>  	ports_node = of_find_node_by_name(dev->of_node, "ethernet-ports");
>  	if (!ports_node) {
>  		dev_warn(dev, "No ports found\n");
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto err_clock_disable;
>  	}
>
>  	i = 0;

[ ... ]

> @@ -1358,6 +1369,8 @@ static int bcmasp_probe(struct platform_device *pdev)
>
>  err_cleanup:
>  	bcmasp_remove_intfs(priv);
> +err_clock_disable:
> +	clk_disable_unprepare(priv->clk);
>                  ^^^^

Does this double-disable the clock on the register_netdev() error path?

Looking at bcmasp_probe(), the clock handling sequence is:

    Line 1280: clk_prepare_enable(priv->clk) enables the clock
    Line 1352: clk_disable_unprepare(priv->clk) disables it

Then register_netdev() is called in a loop starting around line 1358.
If register_netdev() fails, it jumps to err_cleanup which falls through
to err_clock_disable and calls clk_disable_unprepare() again.

Since the clock was already disabled at line 1352 before the network
device registration loop, calling clk_disable_unprepare() a second time
in the error path would unbalance the clock reference count.

Should the err_cleanup label return directly without falling through to
err_clock_disable, or should a separate error label be added for paths
after the clock is already disabled?
-- 
pw-bot: cr