On Tue, Sep 01, 2026 at 02:43:39PM +0200, Andrew Lunn wrote:
> On Tue, Sep 01, 2026 at 02:39:32PM +0800, Hangbin Liu wrote:
> > On Mon, Aug 31, 2026 at 02:16:46PM +0200, Andrew Lunn wrote:
> > > > static void nsim_ethtool_ring_init(struct netdevsim *ns)
> > > > @@ -250,12 +276,16 @@ void nsim_ethtool_init(struct netdevsim *ns)
> > > > ns->ethtool.fec.active_fec = ETHTOOL_FEC_NONE;
> > > >
> > > > ns->ethtool.channels = ns->nsim_bus_dev->num_queues;
> > > > + ns->ethtool.duplex = DUPLEX_FULL;
> > > > + ns->ethtool.speed = SPEED_5000;
> > > >
> > > > ethtool = debugfs_create_dir("ethtool",
> > > > ns->nsim_dev_port->ddir);
> > > > ns->ethtool_ddir = ethtool;
> > > >
> > > > debugfs_create_u32("get_err", 0600, ethtool,
> > > > &ns->ethtool.get_err);
> > > > debugfs_create_u32("set_err", 0600, ethtool,
> > > > &ns->ethtool.set_err);
> > > > + debugfs_create_u32("speed", 0600, ethtool, &ns->ethtool.speed);
> > > > + debugfs_create_u8("duplex", 0600, ethtool, &ns->ethtool.duplex);
> > >
> > > debugfs_create_bool() ?
> >
> > I'm not sure if anyone want to set nsim duplex to unknown for testing.
> > Do you think that we should disable this behavior?
>
> Ah, i did not think of DUPLEX_UNKNOWN.
>
> We need to consider Jakubs reply, what are you actually testing here,
> do such tests make any sense?
I'm fixing a bug in bonding that need to test with different speed slaves.
So I add this feature for netdevsim.
>
> If they do, i think some validation would be good here. We already
> have:
>
> static inline int ethtool_validate_duplex(__u8 duplex)
> {
> switch (duplex) {
> case DUPLEX_HALF:
> case DUPLEX_FULL:
> case DUPLEX_UNKNOWN:
> return 1;
> }
>
> return 0;
> }
>
> We don't expect a driver to set duplex to 42, so why should the user
> be allowed to do that?
The user interface (ethtool) setting is already handled by
ethtool_virtdev_set_link_ksettings(). Here is the debugfs issue.
Since no one is asking for setting DUPLEX_UNKNOWN, I can use
debugfs_create_bool first.
>
> Also, SPEED_UNKNOWN is -1, so you need debugfs_create_s32().
Ah, yes. Thanks
Hangbin
