Re: [net-next v15 04/12] net: mtip: The L2 switch driver for imx287

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 16 Jul 2025 23:47:23 +0200 Lukasz Majewski wrote:
> +static void mtip_ndev_cleanup(struct switch_enet_private *fep)
> +{
> +	struct mtip_ndev_priv *priv;
> +	int i;
> +
> +	for (i = 0; i < SWITCH_EPORT_NUMBER; i++) {
> +		if (fep->ndev[i]) {

this just checks if netdev is NULL

> +			priv = netdev_priv(fep->ndev[i]);
> +			cancel_work_sync(&priv->tx_timeout_work);
> +
> +			unregister_netdev(fep->ndev[i]);

and if not unregisters

> +			free_netdev(fep->ndev[i]);
> +		}
> +	}
> +}
> +
> +static int mtip_ndev_init(struct switch_enet_private *fep,
> +			  struct platform_device *pdev)
> +{
> +	struct mtip_ndev_priv *priv;
> +	int i, ret = 0;
> +
> +	for (i = 0; i < SWITCH_EPORT_NUMBER; i++) {
> +		fep->ndev[i] = alloc_netdev(sizeof(struct mtip_ndev_priv),

but we assign the pointer immediatelly

> +					    fep->ndev_name[i], NET_NAME_USER,
> +					    ether_setup);
> +		if (!fep->ndev[i]) {
> +			ret = -ENOMEM;
> +			break;
> +		}
> +
> +		fep->ndev[i]->ethtool_ops = &mtip_ethtool_ops;
> +		fep->ndev[i]->netdev_ops = &mtip_netdev_ops;
> +		SET_NETDEV_DEV(fep->ndev[i], &pdev->dev);
> +
> +		priv = netdev_priv(fep->ndev[i]);
> +		priv->dev = fep->ndev[i];
> +		priv->fep = fep;
> +		priv->portnum = i + 1;
> +		fep->ndev[i]->irq = fep->irq;
> +
> +		mtip_setup_mac(fep->ndev[i]);
> +
> +		ret = register_netdev(fep->ndev[i]);

and don't clear it when register fails

> +		if (ret) {
> +			dev_err(&fep->ndev[i]->dev,
> +				"%s: ndev %s register err: %d\n", __func__,
> +				fep->ndev[i]->name, ret);
> +			break;
> +		}
> +
> +		dev_dbg(&fep->ndev[i]->dev, "%s: MTIP eth L2 switch %pM\n",
> +			fep->ndev[i]->name, fep->ndev[i]->dev_addr);
> +	}
> +
> +	if (ret)
> +		mtip_ndev_cleanup(fep);

You're probably better off handling the unwind on error separately from
the full cleanup function, but I guess that's subjective.

> +	return ret;
> +}

> +static int mtip_sw_probe(struct platform_device *pdev)
> +{

> +	ret = mtip_ndev_init(fep, pdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s: Failed to create virtual ndev (%d)\n",
> +			__func__, ret);
> +		goto ndev_init_err;
> +	}
> +
> +	ret = mtip_switch_dma_init(fep);

> +	ret = mtip_mii_init(fep, pdev);

Seems like we're registering the netdevs before fully initializing 
the HW? Is it safe if user (or worse, some other kernel subsystem) 
tries to open the netdevs before the driver finished the init?
 

> +	if (ret) {
> +		dev_err(&pdev->dev, "%s: Cannot init phy bus (%d)!\n", __func__,
> +			ret);
> +		goto mii_init_err;
> +	}
> +	/* setup timer for learning aging function */
> +	timer_setup(&fep->timer_mgnt, mtip_mgnt_timer, 0);
> +	mod_timer(&fep->timer_mgnt,
> +		  jiffies + msecs_to_jiffies(LEARNING_AGING_INTERVAL));
> +
> +	return 0;
> +
> + mii_init_err:
> + dma_init_err:
> +	mtip_ndev_cleanup(fep);

Please name the labels after the action they jump to, not the location
where they jump from.

> + ndev_init_err:
> +
> +	return ret;
-- 
pw-bot: cr