Re: [PATCH 12/20] net: dsa: xilinx: register per-MAC MDIO buses

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Memory leak due to using `devm_mdiobus_alloc()` inside the DSA `setup()` callback.
--

--- Patch [12]: [PATCH 12/20] net: dsa: xilinx: register per-MAC MDIO buses ---
commit c6fca5c014413b7a2cd4b20eaa39b52cdbb9cc43
Author: Nagadheeraj Rottela <[email protected]>

net: dsa: xilinx: register per-MAC MDIO buses

This commit registers each MAC's MDIO bus as a child of the switch node
and adds a new struct xlnx_tsn_mac to group per-MAC state. The MDIO clock
divisor is derived from the IP-wide s_axi clock.

> diff --git a/drivers/net/dsa/xilinx/xilinx_tsn.c b/drivers/net/dsa/xilinx/xilinx_tsn.c
> index 020b1622670c8..542b74514ed3a 100644
> --- a/drivers/net/dsa/xilinx/xilinx_tsn.c
> +++ b/drivers/net/dsa/xilinx/xilinx_tsn.c

[ ... ]

> +static int xlnx_tsn_mdio_register_one(struct xlnx_tsn *sw, int port,
> +				      const char *child_name,
> +				      unsigned long host_hz)
> +{
> +	struct xlnx_tsn_mac *m = &sw->mac[port];
> +	struct device_node *mdio_np;
> +	struct mii_bus *bus;
> +	int ret;
> +
> +	mdio_np = of_get_child_by_name(sw->dev->of_node, child_name);
> +	if (!mdio_np)
> +		return 0;
> +
> +	bus = devm_mdiobus_alloc(sw->dev);

[Severity: Medium]
Does this leak memory if the DSA tree is unbound and rebound?

This allocation is eventually called from the xlnx_tsn_setup() callback.
The setup() and teardown() callbacks in the DSA framework can be invoked
multiple times during a switch device's lifetime, such as when the master
netdevice is administratively unbound and rebound, or when the DSA tree is
reconfigured.

By using devm_mdiobus_alloc(), the memory lifetime is tied to the physical
switch device (sw->dev) rather than the DSA tree binding, meaning the
allocated memory will accumulate across teardown/setup cycles.

> +	if (!bus) {
> +		of_node_put(mdio_np);
> +		return -ENOMEM;
> +	}

[ ... ]

> +static void xlnx_tsn_mdio_unregister_all(struct xlnx_tsn *sw)
> +{
> +	int port;
> +
> +	for (port = XLNX_TSN_PORT_MAC1; port <= XLNX_TSN_PORT_MAC2; port++) {
> +		struct xlnx_tsn_mac *m = &sw->mac[port];
> +
> +		if (m->mii_bus) {
> +			mdiobus_unregister(m->mii_bus);
> +			m->mii_bus = NULL;
> +		}

[Severity: Medium]
Does this teardown path leave the MDIO bus memory allocated?

Because the bus was allocated with devm_mdiobus_alloc(), unregistering it
here during the DSA teardown path won't actually free the memory.

Should this use mdiobus_alloc() instead so it can be explicitly freed with
mdiobus_free() during teardown, or should the MDIO bus allocation be moved
to the initial driver probe() instead of the DSA setup() phase?

> +
> +		/* clear the enable bit even when no bus was registered (failed probe) */
> +		mac_iow(m, TSN_MDIO_MC_OFFSET, 0);
> +	}
> +}

[ ... ]

> @@ -160,7 +351,7 @@ static int xlnx_tsn_setup(struct dsa_switch *ds)
>  			return ret;
>  	}
>  
> -	return 0;
> +	return xlnx_tsn_mdio_register_all(sw);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12