Re: [net-next v1 3/6] net: stmmac: Add register NCSI device support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Unregistering the NCSI device before the netdevice in `stmmac_dvr_remove` causes a use-after-free.
- [Medium] IRQs requested in `__stmmac_open` are leaked if `ncsi_start_dev()` fails.
- [Medium] Early error return in `stmmac_probe_config_dt` leaks the OF node reference for `phy-handle`.
- [Low] Unbalanced `netif_carrier_on()` when `ncsi_start_dev()` fails.
--

commit cde45e5227e87c535d6fe7fb8ee4f8048e15a5e6
Author: Minda Chen <minda.chen@starfivetech.com>

net: stmmac: Add register NCSI device support

Add stmmac register support. The register NCSI code is almost from faraday
ftgmac100.c

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 0108023897720..b6af537838830 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4167,6 +4168,16 @@ static int __stmmac_open(struct net_device *dev,
>  	if (ret)
>  		goto irq_error;
>  
> +	if (priv->plat->use_ncsi) {
> +		/* If using NC-SI, set our carrier on and start the stack */
> +		netif_carrier_on(priv->dev);
> +
> +		/* Start the NCSI device */
> +		ret = ncsi_start_dev(priv->nsdev);
> +		if (ret)
> +			goto irq_error;

[Severity: Medium]
Are the IRQs leaked if ncsi_start_dev() fails?

The code successfully allocated IRQs right before this via
stmmac_request_irq(). If ncsi_start_dev() fails, it jumps to the irq_error
label:

irq_error:
	stmmac_phylink_stop(priv->phylink);

But stmmac_free_irq() is not called in this error path. Could this leave
the IRQs permanently allocated, causing future attempts to bring the
interface UP to fail with -EBUSY?

[Severity: Low]
Additionally, if ncsi_start_dev() fails, is there a missing call to
netif_carrier_off() in the error path to revert the carrier state?

> +	}
> +
>  	stmmac_enable_all_queues(priv);
>  	netif_tx_start_all_queues(priv->dev);

[ ... ]

> @@ -8141,6 +8175,8 @@ void stmmac_dvr_remove(struct device *dev)
>  
>  	pm_runtime_get_sync(dev);
>  
> +	if (priv->nsdev)
> +		ncsi_unregister_dev(priv->nsdev);
>  	unregister_netdev(ndev);

[Severity: High]
Does unregistering the NCSI device before the netdevice cause a
use-after-free?

If the network interface is UP when the driver is removed,
unregister_netdev() synchronously halts the interface by invoking the
driver's ndo_stop callback (__stmmac_release()).

Inside __stmmac_release(), the code calls ncsi_stop_dev(priv->nsdev), but
priv->nsdev was already freed by ncsi_unregister_dev().

Should unregister_netdev() be called before ncsi_unregister_dev()?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index 5cae2aa729066..483ecf06fc139 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -451,6 +451,14 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
>  	 * they are not converted to phylink. */
>  	plat->phy_node = of_parse_phandle(np, "phy-handle", 0);
>  
> +	if (of_property_read_bool(np, "snps,use-ncsi")) {
> +		if (!IS_ENABLED(CONFIG_NET_NCSI)) {
> +			dev_err(&pdev->dev, "NCSI stack not enabled\n");
> +			return ERR_PTR(-EINVAL);
> +		}

[Severity: Medium]
Could this early return leak the OF node reference acquired just above?

plat->phy_node is set via of_parse_phandle() right before this check.
Returning ERR_PTR(-EINVAL) directly bypasses the error_put_phy cleanup
label, which drops the reference via of_node_put().

Would it be better to use goto error_put_phy instead of returning
directly?

> +		plat->use_ncsi = true;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610072420.64699-1-minda.chen@starfivetech.com?part=3