* Re: [PATCH] net: bcmasp: fix MDIO child device leaks
2026-09-21 13:09 [PATCH] net: bcmasp: fix MDIO child device leaks Guangshuo Li
@ 2026-09-23 4:10 ` netdev-bot+sashiko
0 siblings, 0 replies; 2+ messages in thread
From: netdev-bot+sashiko @ 2026-09-23 4:10 UTC (permalink / raw)
To: lgs201920130244
Cc: justin.chen, florian.fainelli, andrew+netdev, davem, edumazet,
kuba, pabeni, horms, bcm-kernel-feedback-list, netdev,
linux-kernel, stable
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 0 (1) · Medium: 1 (2) · Low: 0 (1)
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] The newly added of_platform_depopulate(&pdev->dev) in
bcmasp_remove() (bcmasp.c:1388) runs after bcmasp_remove_intfs(), i.e.…
Pre-existing issues:
- [High] priv->intfs is traversed from the hardirq handler bcmasp_isr()
(bcmasp.c:101-122) with list_for_each_entry() and no lock or RCU…
- [Medium] bcmasp_probe() calls of_find_node_by_name(dev->of_node,
"ethernet-ports") without first taking a reference on dev->of_node.
- [Medium] bcmasp_interface_create() stores the reference returned by
of_parse_phandle(ndev_dn, "phy-handle", 0) in intf->phy_dn…
- [Low] bcmasp_init_wol() calls device_set_wakeup_capable(&pdev->dev, 1)
(bcmasp.c:1123), which sets dev->power.can_wakeup and merges the…
--
net: bcmasp: fix MDIO child device leaks
This adds an err_depopulate label to bcmasp_probe() so that failures
occurring after of_platform_populate() call of_platform_depopulate(), and
adds the same call to bcmasp_remove(). A few probe error gotos are
retargeted from err_clock_disable to the new label.
A few questions about the surrounding code below.
> diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
> index 972474893a6bc..c4087408a8228 100644
> --- a/drivers/net/ethernet/broadcom/asp2/bcmasp.c
> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
[ ... ]
> @@ -1327,7 +1327,7 @@ static int bcmasp_probe(struct platform_device *pdev)
> if (!ports_node) {
> dev_warn(dev, "No ports found\n");
> ret = -EINVAL;
> - goto err_clock_disable;
> + goto err_depopulate;
> }
[Severity: Medium]
This isn't a bug introduced by this patch, but since the error handling of
this exact block is being reworked, does the lookup just above it drop a
reference the driver never took?
bcmasp_probe() does:
ports_node = of_find_node_by_name(dev->of_node, "ethernet-ports");
and of_find_node_by_name() ends with an unconditional put on its "from"
argument:
drivers/of/base.c:of_find_node_by_name() {
...
for_each_of_allnodes_from(from, np)
if (of_node_name_eq(np, name) && of_node_get(np))
break;
of_node_put(from);
...
}
So every probe, successful or not, appears to release one reference on the
controller's own OF node. On CONFIG_OF_DYNAMIC=y, repeated bind/unbind would
then reach of_node_release() on a node without OF_DETACHED, which prints
"ERROR: of_node_release() detected bad of_node_put()" with a stack dump.
The same helper also walks the flattened all-nodes list rather than only the
children of dev->of_node, so a match outside this controller's subtree can be
returned. Would of_get_child_by_name() be the right API here, with the
matching of_node_put() as today?
> @@ -1369,6 +1369,8 @@ static int bcmasp_probe(struct platform_device *pdev)
>
> err_cleanup:
> bcmasp_remove_intfs(priv);
> +err_depopulate:
> + of_platform_depopulate(dev);
> err_clock_disable:
> clk_disable_unprepare(priv->clk);
[Severity: Medium]
This is a pre-existing issue, but while this epilogue is being extended, does
the interface teardown it invokes leak the PHY OF node?
bcmasp_interface_create() stores an acquired reference:
intf->phy_dn = of_parse_phandle(ndev_dn, "phy-handle", 0);
if (!intf->phy_dn && of_phy_is_fixed_link(ndev_dn)) {
and the only two exits from the interface lifetime never release it. The
creation failure paths only free the netdev, and normal teardown does:
drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c:bcmasp_interface_destroy() {
if (intf->ndev->reg_state == NETREG_REGISTERED)
unregister_netdev(intf->ndev);
if (of_phy_is_fixed_link(intf->ndev_dn))
of_phy_deregister_fixed_link(intf->ndev_dn);
free_netdev(intf->ndev);
}
of_phy_connect() does not consume the caller's reference, so every
external-PHY port would leak one device_node reference per probe/remove
cycle. Note that in the fixed-link case intf->phy_dn is set to the borrowed
ndev_dn pointer instead of a newly acquired reference, so a fix would have to
skip the put there.
[Severity: Low]
This is a pre-existing issue as well, but is the wakeup capability set
earlier in probe ever reversed?
bcmasp_init_wol() does:
priv->wol_irq = irq;
priv->wol_irq_enabled_mask = 0;
device_set_wakeup_capable(&pdev->dev, 1);
That runs before the ethernet-ports lookup, so both the new err_depopulate
path and err_cleanup leave the platform device advertising wakeup capability
with no driver bound; bcmasp_remove() does not clear it either. Should
device_set_wakeup_capable(&pdev->dev, 0) be paired with this on the error
epilogue and on remove?
> @@ -1383,6 +1385,7 @@ static void bcmasp_remove(struct platform_device *pdev)
> return;
>
> bcmasp_remove_intfs(priv);
> + of_platform_depopulate(&pdev->dev);
> }
[Severity: Medium]
Is this depopulate running with the ASP clock gated? The matching populate in
bcmasp_probe() is deliberately inside the clk_prepare_enable() window:
/* Enable all clocks to ensure successful probing */
...
/* Register mdio child nodes */
of_platform_populate(dev->of_node, bcmasp_mdio_of_match, NULL, dev);
and the new probe-side call added by this patch is likewise above
err_clock_disable. Here, though, bcmasp_remove_intfs() has already run
unregister_netdev() for every port, so the per-interface
clk_prepare_enable()/clk_disable_unprepare() pairs from ndo_open/ndo_close
are balanced and probe ended with clk_disable_unprepare(priv->clk).
The child being destroyed is the UniMAC MDIO block, whose register window is
a sub-range of the ASP block and which has no clocks property of its own
(binding example: mdio@c614 reg = <0xc614 0x8>), so mdio-bcm-unimac's
priv->clk is NULL and all of its MMIO depends on the parent sw_asp clock.
Any MDIO transaction issued during child teardown would then hit a gated
register block, for example:
bcmasp_remove()
of_platform_depopulate()
of_platform_device_destroy()
device_del()
unimac_mdio_remove()
mdiobus_unregister()
phy_device_remove() -> device_del() -> phy_remove()
phy_leds_unregister() -> led_classdev_unregister()
led_set_brightness(led_cdev, LED_OFF)
bcm_phy_led_brightness_set()
unimac_mdio_write()
PHY driver .remove() callbacks (mscc, marvell10g, nxp-c45) are a similar
path. Would bracketing the teardown the same way the populate is bracketed be
appropriate?
clk_prepare_enable(priv->clk);
of_platform_depopulate(&pdev->dev);
clk_disable_unprepare(priv->clk);
[Severity: High]
This isn't a bug introduced by this patch, but the patch extends this
teardown sequence, so: is priv->intfs safe to mutate here while the ASP L2
interrupt handler is still installed?
The hardirq handler walks the list with no lock and no RCU:
drivers/net/ethernet/broadcom/asp2/bcmasp.c:bcmasp_isr() {
...
/* Handle intferfaces */
list_for_each_entry(intf, &priv->intfs, list)
bcmasp_intr2_handling(intf, status);
...
}
and the teardown called just above the new depopulate deletes and frees
entries from process context:
bcmasp_remove_intfs() {
list_for_each_entry_safe(intf, n, &priv->intfs, list) {
list_del(&intf->list);
bcmasp_interface_destroy(intf);
}
}
Neither bcmasp_remove() nor bcmasp_shutdown() calls
bcmasp_intr2_mask_set_all() or frees priv->irq before this, and the
devm_request_irq() resource is only released after remove() returns. On a
multi-port device (or on the register_netdev() failure path, where earlier
ports are already visible to userspace and may have been brought up), port N
can have RX/TX interrupts unmasked via bcmasp_enable_rx_irq() /
bcmasp_enable_tx_irq() while port 0 is being destroyed.
Can a handler that is mid-walk then read LIST_POISON1 from intf->list.next,
or dereference intf->ndev->phydev in bcmasp_intr2_handling() after
free_netdev()? Would masking intr2 and synchronizing the IRQ before touching
the list close that?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921130929.403129-1-lgs201920130244%40gmail.com
^ permalink raw reply [flat|nested] 2+ messages in thread