Re: [PATCH net-next v3 1/3] net: netdevsim: Add PHY support in netdevsim

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 10 Jul 2025 08:22:45 +0200 Maxime Chevallier wrote:
> @@ -1098,6 +1101,10 @@ static int __init nsim_module_init(void)
>  {
>  	int err;
>  
> +	err = nsim_phy_drv_register();
> +	if (err)
> +		return err;
> +
>  	err = nsim_dev_init();
>  	if (err)
>  		return err;

I think you're missing error handling in this function if something
after drv_register fails.

> @@ -1124,6 +1131,7 @@ static void __exit nsim_module_exit(void)
>  	rtnl_link_unregister(&nsim_link_ops);
>  	nsim_bus_exit();
>  	nsim_dev_exit();
> +	nsim_phy_drv_unregister();
>  }

> +free_mdiobus:
> +	atomic_dec(&bus_num);
> +	mdiobus_free(mb->mii);
> +free_pdev:
> +	platform_device_unregister(mb->pdev);
> +free_mb:

Others have added netdevsim code so the entire code base doesn't follow
what I'm about to say, but if you dont mind indulging my personal coding
style - error handling labels on a path disjoint from the success path
should be prefixed with err_$first-undo-action. If the error handling
shares the path with success the label prefix should be exit_$..
You can look at drivers/net/netdevsim/bpf.c for examples

> +	kfree(mb);
> +
> +	return NULL;
> +}
> +
> +static ssize_t
> +nsim_phy_add_write(struct file *file, const char __user *data,
> +		   size_t count, loff_t *ppos)
> +{
> +	struct net_device *dev = file->private_data;
> +	struct netdevsim *ns = netdev_priv(dev);
> +	struct nsim_phy_device *ns_phy;
> +	struct phy_device *pphy;
> +	u32 parent_id;
> +	char buf[10];
> +	ssize_t ret;
> +	int err;
> +
> +	if (*ppos != 0)
> +		return 0;
> +
> +	if (count >= sizeof(buf))
> +		return -ENOSPC;
> +
> +	ret = copy_from_user(buf, data, count);
> +	if (ret)
> +		return -EFAULT;
> +	buf[count] = '\0';
> +
> +	ret = kstrtouint(buf, 10, &parent_id);
> +	if (ret)
> +		return -EINVAL;
> +
> +	ns_phy = nsim_phy_register();
> +	if (IS_ERR(ns_phy))
> +		return PTR_ERR(ns_phy);
> +
> +	if (!parent_id) {
> +		if (!dev->phydev) {
> +			err = phy_connect_direct(dev, ns_phy->phy, nsim_adjust_link,
> +						 PHY_INTERFACE_MODE_NA);
> +			if (err)
> +				return err;
> +
> +			phy_attached_info(ns_phy->phy);
> +
> +			phy_start(ns_phy->phy);
> +		} else {
> +			phy_link_topo_add_phy(dev, ns_phy->phy, PHY_UPSTREAM_MAC, dev);
> +		}
> +	} else {
> +		pphy = phy_link_topo_get_phy(dev, parent_id);
> +		if (!pphy)
> +			return -EINVAL;
> +
> +		phy_link_topo_add_phy(dev, ns_phy->phy, PHY_UPSTREAM_PHY, pphy);
> +	}
> +
> +	nsim_phy_debugfs_create(ns->nsim_dev_port, ns_phy);
> +
> +	list_add(&ns_phy->node, &ns->nsim_dev->phy_list);

No locks needed.. for any of this.. ?
-- 
pw-bot: cr