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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250715080837.1aa0e9dd@fedora.home>
Date: Tue, 15 Jul 2025 08:08:37 +0200
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com, Andrew Lunn
 <andrew@...n.ch>, Eric Dumazet <edumazet@...gle.com>, Paolo Abeni
 <pabeni@...hat.com>, Russell King <linux@...linux.org.uk>, Florian Fainelli
 <f.fainelli@...il.com>, Heiner Kallweit <hkallweit1@...il.com>, Vladimir
 Oltean <vladimir.oltean@....com>, Köry Maincent
 <kory.maincent@...tlin.com>, Oleksij Rempel <o.rempel@...gutronix.de>,
 Simon Horman <horms@...nel.org>, Shuah Khan <shuah@...nel.org>,
 linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v3 1/3] net: netdevsim: Add PHY support in
 netdevsim

On Fri, 11 Jul 2025 16:55:41 -0700
Jakub Kicinski <kuba@...nel.org> 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