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

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi Andrew,

On Tue, 8 Jul 2025 16:01:06 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > +static int nsim_phy_state_link_set(void *data, u64 val)
> > +{
> > +	struct nsim_phy_device *ns_phy = (struct nsim_phy_device *)data;
> > +
> > +	ns_phy->link = !!val;
> > +
> > +	phy_trigger_machine(ns_phy->phy);
> > +
> > +	return 0;
> > +}
> > +
> > +static int nsim_phy_state_link_get(void *data, u64 *val)
> > +{
> > +	struct nsim_phy_device *ns_phy = (struct nsim_phy_device *)data;
> > +
> > +	*val = ns_phy->link;
> > +
> > +	return 0;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(nsim_phy_state_link_fops, nsim_phy_state_link_get,
> > +			 nsim_phy_state_link_set, "%llu\n");
> > +
> > +static void nsim_phy_debugfs_create(struct nsim_dev_port *port,
> > +				    struct nsim_phy_device *ns_phy)
> > +{
> > +	char phy_dir_name[sizeof("phy") + 10];
> > +
> > +	sprintf(phy_dir_name, "phy%u", ns_phy->phy->phyindex);
> > +
> > +	/* create debugfs stuff */
> > +	ns_phy->phy_dir = debugfs_create_dir(phy_dir_name, port->ddir);
> > +
> > +	debugfs_create_file("link", 0600, ns_phy->phy_dir, ns_phy, &nsim_phy_state_link_fops);  
> 
> Maybe this can be converted into:
> 
> debugfs_create_bool("link", 0600, ns_phy->phy_dir, &ns_phy->link);
> 
> You loose the phy_trigger_machine(), but that might actually be
> good. PHYs are async to any operation you take on them. It can take up
> to 1 second due to the polling before any change is reported. So any
> user space tools which expect an immediate state change are broken. So
> leaving the PHY state machine to poll the PHY to notice the link has
> changed is a better simulation.

That's true indeed... Simpler and more accurate, I'll add that in V3.

Thanks for the feedback :)

Maxime