[PATCH net] net: bcmasp: fix double frees during remove

[PATCH net] net: bcmasp: fix double frees during remove

[justin.chen]

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

We have two double frees during remove.
- We do not need to free wol_irq since it was instantiated with
  devm_request_irq(). So devres will free for us.
- Switch to devm_clk_get_optional() instead of
  devm_clk_get_optional_enabled() so we can manage the clock ourselves.

Fixes: a2f0751206b0 ("net: bcmasp: Add support for WoL magic packet")
Fixes: 490cb412007d ("net: bcmasp: Add support for ASP2.0 Ethernet controller")
Signed-off-by: Justin Chen <justin.chen@broadcom.com>
---
 drivers/net/ethernet/broadcom/asp2/bcmasp.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
index aa6d8606849f..1bfc1f90cd62 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;
@@ -1255,11 +1249,15 @@ 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");
 
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to start clock\n");
+
 	/* Base from parent node */
 	priv->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(priv->base))
@@ -1363,7 +1361,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 +1373,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] net: bcmasp: fix double frees during remove

[Simon Horman]

On Thu, Mar 12, 2026 at 01:08:14PM -0700, justin.chen@broadcom.com wrote:
> From: Justin Chen <justin.chen@broadcom.com>
> 
> We have two double frees during remove.
> - We do not need to free wol_irq since it was instantiated with
>   devm_request_irq(). So devres will free for us.

Hi Justin,

The above seems clear enough.

> - Switch to devm_clk_get_optional() instead of
>   devm_clk_get_optional_enabled() so we can manage the clock ourselves.

But it is unclear to me what problem this is solving.
Or how it relates to the first point - usually the aim
is to have one fix per patch.

Could you elaborate?

> 
> Fixes: a2f0751206b0 ("net: bcmasp: Add support for WoL magic packet")
> Fixes: 490cb412007d ("net: bcmasp: Add support for ASP2.0 Ethernet controller")
> Signed-off-by: Justin Chen <justin.chen@broadcom.com>

Re: [PATCH net] net: bcmasp: fix double frees during remove

[Justin Chen]

On 3/13/26 9:55 AM, Simon Horman wrote:
> On Thu, Mar 12, 2026 at 01:08:14PM -0700, justin.chen@broadcom.com wrote:
>> From: Justin Chen <justin.chen@broadcom.com>
>>
>> We have two double frees during remove.
>> - We do not need to free wol_irq since it was instantiated with
>>    devm_request_irq(). So devres will free for us.
> 
> Hi Justin,
> 
> The above seems clear enough.
> 
>> - Switch to devm_clk_get_optional() instead of
>>    devm_clk_get_optional_enabled() so we can manage the clock ourselves.
> 
> But it is unclear to me what problem this is solving.
> Or how it relates to the first point - usually the aim
> is to have one fix per patch.
> 
> Could you elaborate?
> 
The double free happens because we double disable the clock. Guess it is 
not exactly a double free. We manage the clock at the driver level so we 
disable it when it is unneeded. The default state is clock disabled when
no interfaces are up. After we bring down the interfaces, the clock is 
disabled, then when we unbind the driver, it hits a double disable.

Sure I can turn this into two fixes.

Thanks,
Justin
>>
>> Fixes: a2f0751206b0 ("net: bcmasp: Add support for WoL magic packet")
>> Fixes: 490cb412007d ("net: bcmasp: Add support for ASP2.0 Ethernet controller")
>> Signed-off-by: Justin Chen <justin.chen@broadcom.com>

Re: [PATCH net] net: bcmasp: fix double frees during remove

[Jakub Kicinski]

On Fri, 13 Mar 2026 13:21:13 -0700 Justin Chen wrote:
> >> - Switch to devm_clk_get_optional() instead of
> >>    devm_clk_get_optional_enabled() so we can manage the clock ourselves.  
> > 
> > But it is unclear to me what problem this is solving.
> > Or how it relates to the first point - usually the aim
> > is to have one fix per patch.
> > 
> > Could you elaborate?
> >   
> The double free happens because we double disable the clock. Guess it is 
> not exactly a double free. We manage the clock at the driver level so we 
> disable it when it is unneeded. The default state is clock disabled when
> no interfaces are up. After we bring down the interfaces, the clock is 
> disabled, then when we unbind the driver, it hits a double disable.

AI points out we're not missing a bunch of disable calls if we fail
further down in .probe

> Sure I can turn this into two fixes.

Yes, please
-- 
pw-bot: cr