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

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.. ?