On Fri, 11 Jul 2025 16:55:41 -0700
Jakub Kicinski <[email protected]> wrote:
> 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.
Ah true... Thanks
> > @@ -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
That's totally fine by me, it makes sense :)
> > + 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.. ?
Heh I guess some locking is needed indeed... Let me add that in the
next round...
Maxime
